drm/i915: Support HDMI EDID injection

Submitted by marius vlad on Feb. 2, 2016, 1:01 p.m.

Details

Message ID 1454418061-10178-1-git-send-email-marius.c.vlad@intel.com
State New
Headers show
Series "drm/i915: Support HDMI EDID injection" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

marius vlad Feb. 2, 2016, 1:01 p.m.
Use the drm_property_blob data for user-supplied EDID blobs.

Signed-off-by: Marius Vlad <marius.c.vlad@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 8698a64..a10f3d9 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1336,7 +1336,8 @@  intel_hdmi_unset_edid(struct drm_connector *connector)
 	intel_hdmi->has_audio = false;
 	intel_hdmi->rgb_quant_range_selectable = false;
 
-	kfree(to_intel_connector(connector)->detect_edid);
+	if (!connector->override_edid)
+		kfree(to_intel_connector(connector)->detect_edid);
 	to_intel_connector(connector)->detect_edid = NULL;
 }
 
@@ -1355,6 +1356,9 @@  intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 				    intel_gmbus_get_adapter(dev_priv,
 				    intel_hdmi->ddc_bus));
 
+		if (!edid && connector->override_edid)
+			edid = (struct edid *) connector->edid_blob_ptr->data;
+
 		intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
 	}
 

Comments

On Tue, 02 Feb 2016, Marius Vlad <marius.c.vlad@intel.com> wrote:
> Use the drm_property_blob data for user-supplied EDID blobs.
>
> Signed-off-by: Marius Vlad <marius.c.vlad@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 8698a64..a10f3d9 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1336,7 +1336,8 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
>  	intel_hdmi->has_audio = false;
>  	intel_hdmi->rgb_quant_range_selectable = false;
>  
> -	kfree(to_intel_connector(connector)->detect_edid);
> +	if (!connector->override_edid)
> +		kfree(to_intel_connector(connector)->detect_edid);
>  	to_intel_connector(connector)->detect_edid = NULL;
>  }
>  
> @@ -1355,6 +1356,9 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  				    intel_gmbus_get_adapter(dev_priv,
>  				    intel_hdmi->ddc_bus));
>  
> +		if (!edid && connector->override_edid)
> +			edid = (struct edid *) connector->edid_blob_ptr->data;
> +

If the user goes on to update the edid by hand, ->detect_edid will end
up pointing at released memory. You should probably kmemdup the edid
(like some other drivers do, git grep for edid_blob_ptr), even though
that will lead to using a stale edid until intel_hdmi_set_edid is called
again.

The other question is, why do you base the decision to use override edid
on whether we can get the actual edid or not?

/me thinks this is all really messy at the drm level, including the
handling of edid firmware.

BR,
Jani.



>  		intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>  	}
On Tue, Feb 02, 2016 at 05:32:21PM +0200, Jani Nikula wrote:
> On Tue, 02 Feb 2016, Marius Vlad <marius.c.vlad@intel.com> wrote:
> > Use the drm_property_blob data for user-supplied EDID blobs.
> >
> > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 8698a64..a10f3d9 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1336,7 +1336,8 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
> >  	intel_hdmi->has_audio = false;
> >  	intel_hdmi->rgb_quant_range_selectable = false;
> >  
> > -	kfree(to_intel_connector(connector)->detect_edid);
> > +	if (!connector->override_edid)
> > +		kfree(to_intel_connector(connector)->detect_edid);
> >  	to_intel_connector(connector)->detect_edid = NULL;
> >  }
> >  
> > @@ -1355,6 +1356,9 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
> >  				    intel_gmbus_get_adapter(dev_priv,
> >  				    intel_hdmi->ddc_bus));
> >  
> > +		if (!edid && connector->override_edid)
> > +			edid = (struct edid *) connector->edid_blob_ptr->data;
> > +
> 
> If the user goes on to update the edid by hand, ->detect_edid will end
> up pointing at released memory. You should probably kmemdup the edid
> (like some other drivers do, git grep for edid_blob_ptr), even though
> that will lead to using a stale edid until intel_hdmi_set_edid is called
> again.

My initial approach was doing just that. Then I noticed I might get away
without doing any kind of allocations. I've some tests that work
both ways.

In intel_hdmi_unset_edid(), kfree(detect_edid) is only called when
connector->override_edid is not set, in this way it won't deallocate
random data.

> 
> The other question is, why do you base the decision to use override edid
> on whether we can get the actual edid or not?

In case edid is not set (failed to communicate with the display) and
the override_edid is set (which is set by edid_write() in drm_debugfs()
meaing a user supplied a EDID blob) I can assume that the user
did injected a HDMI EDID, and use data from drm_property_blob.

Seems to be the only place before looking for CEA extensions in HDMI
supplied EDID.

> 
> /me thinks this is all really messy at the drm level, including the
> handling of edid firmware.
> 
> BR,
> Jani.
> 
> 
> 
> >  		intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> >  	}
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center