[1/2] drm/i915: Optimize HDMI EDID reads

Commit Message

Sharma, Shashank Aug. 12, 2014, 12:38 p.m. UTC

From: Shashank Sharma <[e-mail]>

The current hdmi_detect() function is getting called from
many places, few of these are:
1. HDMI hot plug interrupt bottom half
2. get_resources() IOCTL family
3. drm_helper_probe_single_connector_modes() family
4. output_poll_execute()
5. status_show() etc...

Every time this function is called, it goes and reads HDMI EDID over
DDC channel. Ideally, reading EDID is only required when there is a
real hot plug, and then for all IOCTL and userspace detect functions
can be answered using this same EDID.

The current patch adds EDID caching for a finite duration (1 minute)
This is how it works:
1. With in this caching duration, when usespace get_resource and other
   modeset_detect calls get called, we can use the cached EDID.
2. Even the get_mode function can use the cached EDID.
3. A delayed work will clear the cached EDID after the timeout.
4. If there is a real HDMI hotplug, within the caching duration, the
   cached EDID is updates, and a new delayed work is scheduled.

Signed-off-by: Shashank Sharma <[e-mail]>
---
 drivers/gpu/drm/i915/intel_drv.h  |  4 ++
 drivers/gpu/drm/i915/intel_hdmi.c | 92 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 90 insertions(+), 6 deletions(-)

Comments

Daniel Vetter Aug. 13, 2014, 12:14 p.m. UTC | #1

On Tue, Aug 12, 2014 at 06:08:20PM +0530, [e-mail] wrote:
> From: Shashank Sharma <[e-mail]>
> 
> The current hdmi_detect() function is getting called from
> many places, few of these are:
> 1. HDMI hot plug interrupt bottom half
> 2. get_resources() IOCTL family
> 3. drm_helper_probe_single_connector_modes() family
> 4. output_poll_execute()
> 5. status_show() etc...
> 
> Every time this function is called, it goes and reads HDMI EDID over
> DDC channel. Ideally, reading EDID is only required when there is a
> real hot plug, and then for all IOCTL and userspace detect functions
> can be answered using this same EDID.
> 
> The current patch adds EDID caching for a finite duration (1 minute)
> This is how it works:
> 1. With in this caching duration, when usespace get_resource and other
>    modeset_detect calls get called, we can use the cached EDID.
> 2. Even the get_mode function can use the cached EDID.
> 3. A delayed work will clear the cached EDID after the timeout.
> 4. If there is a real HDMI hotplug, within the caching duration, the
>    cached EDID is updates, and a new delayed work is scheduled.
> 
> Signed-off-by: Shashank Sharma <[e-mail]>

This has a bunch of changes compared to what I've proposed, and so will
not actually work. Also, keying off the source platform (with the gen6
checks) is useless if we're dealing with random brokeness in existing hdmi
sinks here.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_drv.h  |  4 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 92 ++++++++++++++++++++++++++++++++++++---
>  2 files changed, 90 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 28d185d..185a45a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -110,6 +110,8 @@
>  #define INTEL_DSI_VIDEO_MODE	0
>  #define INTEL_DSI_COMMAND_MODE	1
>  
> +#define INTEL_HDMI_EDID_CACHING_SEC 60
> +
>  struct intel_framebuffer {
>  	struct drm_framebuffer base;
>  	struct drm_i915_gem_object *obj;
> @@ -507,6 +509,8 @@ struct intel_hdmi {
>  	enum hdmi_force_audio force_audio;
>  	bool rgb_quant_range_selectable;
>  	enum hdmi_picture_aspect aspect_ratio;
> +	struct edid *edid;
> +	struct delayed_work edid_work;
>  	void (*write_infoframe)(struct drm_encoder *encoder,
>  				enum hdmi_infoframe_type type,
>  				const void *frame, ssize_t len);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 5f47d35..8dc3970 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -962,6 +962,22 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> +/* Work function to invalidate EDID caching */
> +static void intel_hdmi_invalidate_edid(struct work_struct *work)
> +{
> +	struct intel_hdmi *intel_hdmi = container_of(to_delayed_work(work),
> +				struct intel_hdmi, edid_work);
> +	struct drm_device *dev = intel_hdmi_to_dev(intel_hdmi);
> +	struct drm_mode_config *mode_config = &dev->mode_config;
> +
> +	mutex_lock(&mode_config->mutex);
> +	/* Checkpatch says kfree is NULL protected */
> +	kfree(intel_hdmi->edid);
> +	intel_hdmi->edid = NULL;
> +	mutex_unlock(&mode_config->mutex);
> +	DRM_DEBUG_DRIVER("cleaned up cached EDID\n");
> +}
> +
>  static enum drm_connector_status
>  intel_hdmi_detect(struct drm_connector *connector, bool force)
>  {
> @@ -978,15 +994,58 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
>  
> +	/*
> +	* hdmi_detect() gets called from both get_resource()
> +	* and HDMI hpd bottom half work function.
> +	* Its not required to read EDID for every detect call until it's is
> +	* from a hot plug. Lets cache the EDID as soon as we get
> +	* a HPD, and then try to re-use this for all the non hpd detact calls
> +	* coming with in a finite duration.
> +	*/
> +	if (INTEL_INFO(dev)->gen < 6)
> +		/* Do not break old platforms */
> +		goto skip_optimization;
> +
> +	/* If call is from HPD, do check real status by reading EDID */
> +	if (!force)
> +		goto skip_optimization;
> +
> +	/* This is a non-hpd call, lets see if we can optimize this */
> +	if (intel_hdmi->edid) {
> +		/*
> +		* So this is a non-hpd call, within the duration when
> +		* EDID caching is valid. So previous status (valid)
> +		* of connector is re-usable.
> +		*/
> +		if (connector->status != connector_status_unknown) {
> +			DRM_DEBUG_DRIVER("Reporting force status\n");
> +			return connector->status;
> +		}
> +	}
> +
> +skip_optimization:
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
>  	intel_hdmi->has_hdmi_sink = false;
>  	intel_hdmi->has_audio = false;
>  	intel_hdmi->rgb_quant_range_selectable = false;
> +
> +	/*
> +	* You are well deserving, dear code, as you have survived
> +	* all the optimizations. Now go and enjoy reading EDID
> +	*/
>  	edid = drm_get_edid(connector,
> -			    intel_gmbus_get_adapter(dev_priv,
> -						    intel_hdmi->ddc_bus));
> +			intel_gmbus_get_adapter(dev_priv,
> +						intel_hdmi->ddc_bus));
> +	/*
> +	* Now when we have read new EDID, update cached EDID with
> +	* latest (both NULL or non NULL). Cancel the delayed work
> +	* which cleans up the cached EDID. Re-schedule if required.
> +	*/
> +	kfree(intel_hdmi->edid);
> +	intel_hdmi->edid = edid;
> +	cancel_delayed_work_sync(&intel_hdmi->edid_work);
>  
>  	if (edid) {
>  		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> @@ -997,8 +1056,17 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>  			intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
>  			intel_hdmi->rgb_quant_range_selectable =
>  				drm_rgb_quant_range_selectable(edid);
> +			/*
> +			* Allow re-use of cached EDID for 60 sec, as
> +			* userspace modeset should happen within this
> +			* duration, and multiple detect calls will be
> +			* handled using cached EDID.
> +			*/
> +			schedule_delayed_work(&intel_hdmi->edid_work,
> +				msecs_to_jiffies(
> +					INTEL_HDMI_EDID_CACHING_SEC
> +							* 1000));
>  		}
> -		kfree(edid);
>  	}
>  
>  	if (status == connector_status_connected) {
> @@ -1027,13 +1095,22 @@ static int intel_hdmi_get_modes(struct drm_connector *connector)
>  
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
> -
> -	ret = intel_ddc_get_modes(connector,
> +	/*
> +	* GEN6 and + have software support for EDID caching, so
> +	* use cached_edid from detect call, if available.
> +	*/
> +	if (intel_hdmi->edid && (INTEL_INFO(connector->dev)->gen >= 6)) {
> +		ret = intel_connector_update_modes(connector,
> +				intel_hdmi->edid);
> +		DRM_DEBUG_DRIVER("Using cached EDID, got %d modes\n", ret);
> +	} else {
> +		ret = intel_ddc_get_modes(connector,
>  				   intel_gmbus_get_adapter(dev_priv,
>  							   intel_hdmi->ddc_bus));
> +		DRM_DEBUG_DRIVER("Read EDID, got %d modes\n", ret);
> +	}
>  
>  	intel_display_power_put(dev_priv, power_domain);
> -
>  	return ret;
>  }
>  
> @@ -1661,5 +1738,8 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>  	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
>  	intel_dig_port->dp.output_reg = 0;
>  
> +	/* Work function to invalidate cached EDID after timeout */
> +	INIT_DELAYED_WORK(&(intel_dig_port->hdmi.edid_work),
> +				intel_hdmi_invalidate_edid);
>  	intel_hdmi_init_connector(intel_dig_port, intel_connector);
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Sharma, Shashank Aug. 14, 2014, 6:15 a.m. UTC | #2

Regards
Shashank
On 8/13/2014 5:44 PM, Daniel Vetter wrote:
> On Tue, Aug 12, 2014 at 06:08:20PM +0530, [e-mail] wrote:
>> From: Shashank Sharma <[e-mail]>
>>
>> The current hdmi_detect() function is getting called from
>> many places, few of these are:
>> 1. HDMI hot plug interrupt bottom half
>> 2. get_resources() IOCTL family
>> 3. drm_helper_probe_single_connector_modes() family
>> 4. output_poll_execute()
>> 5. status_show() etc...
>>
>> Every time this function is called, it goes and reads HDMI EDID over
>> DDC channel. Ideally, reading EDID is only required when there is a
>> real hot plug, and then for all IOCTL and userspace detect functions
>> can be answered using this same EDID.
>>
>> The current patch adds EDID caching for a finite duration (1 minute)
>> This is how it works:
>> 1. With in this caching duration, when usespace get_resource and other
>>     modeset_detect calls get called, we can use the cached EDID.
>> 2. Even the get_mode function can use the cached EDID.
>> 3. A delayed work will clear the cached EDID after the timeout.
>> 4. If there is a real HDMI hotplug, within the caching duration, the
>>     cached EDID is updates, and a new delayed work is scheduled.
>>
>> Signed-off-by: Shashank Sharma <[e-mail]>
>
> This has a bunch of changes compared to what I've proposed, and so will
> not actually work. Also, keying off the source platform (with the gen6
> checks) is useless if we're dealing with random brokeness in existing hdmi
> sinks here.
> -Daniel
>
Can you please point out what is it, that's unexpected to you ?
I think this is what we (you & Shobhit) agreed on:
1. Timeout based EDID caching
2. Use of cached EDID within caching duration
3. Separate path for HDMI compliance, controllable in kernel, which 
doesn't affect current code flow.

>> ---
>>   drivers/gpu/drm/i915/intel_drv.h  |  4 ++
>>   drivers/gpu/drm/i915/intel_hdmi.c | 92 ++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 90 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 28d185d..185a45a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -110,6 +110,8 @@
>>   #define INTEL_DSI_VIDEO_MODE	0
>>   #define INTEL_DSI_COMMAND_MODE	1
>>
>> +#define INTEL_HDMI_EDID_CACHING_SEC 60
>> +
>>   struct intel_framebuffer {
>>   	struct drm_framebuffer base;
>>   	struct drm_i915_gem_object *obj;
>> @@ -507,6 +509,8 @@ struct intel_hdmi {
>>   	enum hdmi_force_audio force_audio;
>>   	bool rgb_quant_range_selectable;
>>   	enum hdmi_picture_aspect aspect_ratio;
>> +	struct edid *edid;
>> +	struct delayed_work edid_work;
>>   	void (*write_infoframe)(struct drm_encoder *encoder,
>>   				enum hdmi_infoframe_type type,
>>   				const void *frame, ssize_t len);
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 5f47d35..8dc3970 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -962,6 +962,22 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>   	return true;
>>   }
>>
>> +/* Work function to invalidate EDID caching */
>> +static void intel_hdmi_invalidate_edid(struct work_struct *work)
>> +{
>> +	struct intel_hdmi *intel_hdmi = container_of(to_delayed_work(work),
>> +				struct intel_hdmi, edid_work);
>> +	struct drm_device *dev = intel_hdmi_to_dev(intel_hdmi);
>> +	struct drm_mode_config *mode_config = &dev->mode_config;
>> +
>> +	mutex_lock(&mode_config->mutex);
>> +	/* Checkpatch says kfree is NULL protected */
>> +	kfree(intel_hdmi->edid);
>> +	intel_hdmi->edid = NULL;
>> +	mutex_unlock(&mode_config->mutex);
>> +	DRM_DEBUG_DRIVER("cleaned up cached EDID\n");
>> +}
>> +
>>   static enum drm_connector_status
>>   intel_hdmi_detect(struct drm_connector *connector, bool force)
>>   {
>> @@ -978,15 +994,58 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>>   	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>   		      connector->base.id, connector->name);
>>
>> +	/*
>> +	* hdmi_detect() gets called from both get_resource()
>> +	* and HDMI hpd bottom half work function.
>> +	* Its not required to read EDID for every detect call until it's is
>> +	* from a hot plug. Lets cache the EDID as soon as we get
>> +	* a HPD, and then try to re-use this for all the non hpd detact calls
>> +	* coming with in a finite duration.
>> +	*/
>> +	if (INTEL_INFO(dev)->gen < 6)
>> +		/* Do not break old platforms */
>> +		goto skip_optimization;
>> +
>> +	/* If call is from HPD, do check real status by reading EDID */
>> +	if (!force)
>> +		goto skip_optimization;
>> +
>> +	/* This is a non-hpd call, lets see if we can optimize this */
>> +	if (intel_hdmi->edid) {
>> +		/*
>> +		* So this is a non-hpd call, within the duration when
>> +		* EDID caching is valid. So previous status (valid)
>> +		* of connector is re-usable.
>> +		*/
>> +		if (connector->status != connector_status_unknown) {
>> +			DRM_DEBUG_DRIVER("Reporting force status\n");
>> +			return connector->status;
>> +		}
>> +	}
>> +
>> +skip_optimization:
>>   	power_domain = intel_display_port_power_domain(intel_encoder);
>>   	intel_display_power_get(dev_priv, power_domain);
>>
>>   	intel_hdmi->has_hdmi_sink = false;
>>   	intel_hdmi->has_audio = false;
>>   	intel_hdmi->rgb_quant_range_selectable = false;
>> +
>> +	/*
>> +	* You are well deserving, dear code, as you have survived
>> +	* all the optimizations. Now go and enjoy reading EDID
>> +	*/
>>   	edid = drm_get_edid(connector,
>> -			    intel_gmbus_get_adapter(dev_priv,
>> -						    intel_hdmi->ddc_bus));
>> +			intel_gmbus_get_adapter(dev_priv,
>> +						intel_hdmi->ddc_bus));
>> +	/*
>> +	* Now when we have read new EDID, update cached EDID with
>> +	* latest (both NULL or non NULL). Cancel the delayed work
>> +	* which cleans up the cached EDID. Re-schedule if required.
>> +	*/
>> +	kfree(intel_hdmi->edid);
>> +	intel_hdmi->edid = edid;
>> +	cancel_delayed_work_sync(&intel_hdmi->edid_work);
>>
>>   	if (edid) {
>>   		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
>> @@ -997,8 +1056,17 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>>   			intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
>>   			intel_hdmi->rgb_quant_range_selectable =
>>   				drm_rgb_quant_range_selectable(edid);
>> +			/*
>> +			* Allow re-use of cached EDID for 60 sec, as
>> +			* userspace modeset should happen within this
>> +			* duration, and multiple detect calls will be
>> +			* handled using cached EDID.
>> +			*/
>> +			schedule_delayed_work(&intel_hdmi->edid_work,
>> +				msecs_to_jiffies(
>> +					INTEL_HDMI_EDID_CACHING_SEC
>> +							* 1000));
>>   		}
>> -		kfree(edid);
>>   	}
>>
>>   	if (status == connector_status_connected) {
>> @@ -1027,13 +1095,22 @@ static int intel_hdmi_get_modes(struct drm_connector *connector)
>>
>>   	power_domain = intel_display_port_power_domain(intel_encoder);
>>   	intel_display_power_get(dev_priv, power_domain);
>> -
>> -	ret = intel_ddc_get_modes(connector,
>> +	/*
>> +	* GEN6 and + have software support for EDID caching, so
>> +	* use cached_edid from detect call, if available.
>> +	*/
>> +	if (intel_hdmi->edid && (INTEL_INFO(connector->dev)->gen >= 6)) {
>> +		ret = intel_connector_update_modes(connector,
>> +				intel_hdmi->edid);
>> +		DRM_DEBUG_DRIVER("Using cached EDID, got %d modes\n", ret);
>> +	} else {
>> +		ret = intel_ddc_get_modes(connector,
>>   				   intel_gmbus_get_adapter(dev_priv,
>>   							   intel_hdmi->ddc_bus));
>> +		DRM_DEBUG_DRIVER("Read EDID, got %d modes\n", ret);
>> +	}
>>
>>   	intel_display_power_put(dev_priv, power_domain);
>> -
>>   	return ret;
>>   }
>>
>> @@ -1661,5 +1738,8 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>>   	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
>>   	intel_dig_port->dp.output_reg = 0;
>>
>> +	/* Work function to invalidate cached EDID after timeout */
>> +	INIT_DELAYED_WORK(&(intel_dig_port->hdmi.edid_work),
>> +				intel_hdmi_invalidate_edid);
>>   	intel_hdmi_init_connector(intel_dig_port, intel_connector);
>>   }
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Daniel Vetter Aug. 14, 2014, 8:28 a.m. UTC | #3

On Thu, Aug 14, 2014 at 8:15 AM, Sharma, Shashank
<[e-mail]> wrote:
> Can you please point out what is it, that's unexpected to you ?
> I think this is what we (you & Shobhit) agreed on:
> 1. Timeout based EDID caching
> 2. Use of cached EDID within caching duration
> 3. Separate path for HDMI compliance, controllable in kernel, which doesn't
> affect current code flow.

- The timeout is 1 minute instead of 1s. That breaks interactions with
the periodical probing we do when there's a storm.
- There's a generation check in there. This is a generic problem,
restricting platforms only means that fewer people will be able to
test it and find issues with broken hdmi sinks. The problem here are
_sink_ devices, not necessarily platforms. So testing for platforms is
bogus.
- Keying off the force parameter isn't actually precise enough.
There's an encoder->hot_plug callback where you should invalidate the
edid instead.
- Adding the edid caching to the intel_hdmi struct is the wrong place,
we already have an edid pointer in intel_connector, which is used for
panels. Augmenting that to allow caching with time-based invalidation
is the right solution instead of inventing a completely new wheel.

Aside: You commit message is misleading since it's actually not
required to do a full probe cycle for the get_connector ioctl. You can
get at the current cached mode list without any probe calls at all.
Please have a look at SNA for how to do that. And if you have
userspace constantly probing sysfs files and other stuff instead of
listening to uevents then you need to fix your userspace, not cache
the edid in the kernel for a minute.
-Daniel

Sharma, Shashank Aug. 14, 2014, 9:23 a.m. UTC | #4

Hi Daniel,

I can do all the changes accordingly and upload a new patch.

This is what I am going to do:
1. Change the EDID caching time to a second from a minute (probably 
there was a miscommunication).
2. Remove the gen_check
3. Use the connector->edid pointer to cache EDID.

I have only few problems with these two suggestions:
 > Keying off the force parameter isn't actually precise enough.
It is. All the calls to HDMI detect, which come as a result of user 
space interaction keep force = 1, whereas all the hot plug event callers 
keep it force = 0. Please have a look:

IOCTL / Sysfs calls, calling connector->funcs->detect()
1. drm_helper_probe_single_connector_modes_merge_bits => (force = 1)
2. status_show => (force = 1)

Hot plug handlers / hot plug poll
1. drm_helper_hpd_irq_event => (force = 0)
2. output_poll_execute => (force = 0)
So this should work all right.

 > There's an encoder->hot_plug callback where you should invalidate the
 > edid instead.
In MCG branch, we are doing this in encoder->hot_plug only. But there 
the EDID stays, until there is one more next hotplug, and by that time, 
the detect code uses cached EDID only.
As encoder->hot_plug function also gets called every time there is a 
hot_plug, from the hotplug_work_fn, I was afraid this might cause a 
race. Second, I still have to write a delayed_work wrapper, to call 
encoder->hot_plug from, after the timeout.

If you feel that, its better to do it there, I can do changes accordingly.

Regards
Shashank
On 8/14/2014 1:58 PM, Daniel Vetter wrote:
> On Thu, Aug 14, 2014 at 8:15 AM, Sharma, Shashank
> <[e-mail]> wrote:
>> Can you please point out what is it, that's unexpected to you ?
>> I think this is what we (you & Shobhit) agreed on:
>> 1. Timeout based EDID caching
>> 2. Use of cached EDID within caching duration
>> 3. Separate path for HDMI compliance, controllable in kernel, which doesn't
>> affect current code flow.
>
> - The timeout is 1 minute instead of 1s. That breaks interactions with
> the periodical probing we do when there's a storm.
> - There's a generation check in there. This is a generic problem,
> restricting platforms only means that fewer people will be able to
> test it and find issues with broken hdmi sinks. The problem here are
> _sink_ devices, not necessarily platforms. So testing for platforms is
> bogus.
> - Keying off the force parameter isn't actually precise enough.
> There's an encoder->hot_plug callback where you should invalidate the
> edid instead.
> - Adding the edid caching to the intel_hdmi struct is the wrong place,
> we already have an edid pointer in intel_connector, which is used for
> panels. Augmenting that to allow caching with time-based invalidation
> is the right solution instead of inventing a completely new wheel.
>
> Aside: You commit message is misleading since it's actually not
> required to do a full probe cycle for the get_connector ioctl. You can
> get at the current cached mode list without any probe calls at all.
> Please have a look at SNA for how to do that. And if you have
> userspace constantly probing sysfs files and other stuff instead of
> listening to uevents then you need to fix your userspace, not cache
> the edid in the kernel for a minute.
> -Daniel
>

Daniel Vetter Aug. 14, 2014, 11:57 a.m. UTC | #5

On Thu, Aug 14, 2014 at 02:53:44PM +0530, Sharma, Shashank wrote:
> Hi Daniel,
> 
> I can do all the changes accordingly and upload a new patch.
> 
> This is what I am going to do:
> 1. Change the EDID caching time to a second from a minute (probably there
> was a miscommunication).
> 2. Remove the gen_check
> 3. Use the connector->edid pointer to cache EDID.
> 
> I have only few problems with these two suggestions:
> > Keying off the force parameter isn't actually precise enough.
> It is. All the calls to HDMI detect, which come as a result of user space
> interaction keep force = 1, whereas all the hot plug event callers keep it
> force = 0. Please have a look:
> 
> IOCTL / Sysfs calls, calling connector->funcs->detect()
> 1. drm_helper_probe_single_connector_modes_merge_bits => (force = 1)
> 2. status_show => (force = 1)
> 
> Hot plug handlers / hot plug poll
> 1. drm_helper_hpd_irq_event => (force = 0)
> 2. output_poll_execute => (force = 0)
> So this should work all right.

Hm indeed, I've mixed up things with the output poll worker. My concern is
mostly that "force" already has overloaded meaning. But I think if we
polish the code-flow a bit it should work out. Quick draft:

	/* at the top of the detect function before anything else */
	if (!force)
		intel_connector_invalidate_edid(conn);


Then we replace all current calls to drm_get_edid with a new
intel_connector_get_edid_cached, which first checks the conn->edid cache
and only if that's empty call drm_edid_cache.

btw for the edid cache itself I think Chris' idea to use errno pointers is
great, so the state machine would be:

connector->edid == NULL -> cache expired

IS_ERR(connector->edid) -> negative cache entry with errno value, e.g.
-ENXIO. this way we also speed up subsequent detect calls from userspace
when nothing is connected.

everything else -> cached edid

So in code this would look like

void intel_connector_invalidate_edid(conn)
{
	/* locking tbd */

	if (!conn->edid)
		return;

	if (!IS_ERR(conn->edid))
		kfree(conn->edid);

	conn->edid = NULL;
}

struct drm_edid *intel_connector_get_edid_cached(conn, ...)
{
	/* locking tbd */

	if (!conn->edid) {
		conn->edid = drm_get_edid(...);
		if (!conn->edid)
			conn->edid = ERR_PTR(-ENXIO);

		/* rearm invalidate timer */
	}

	return IS_ERR(conn->edid) ? NULL : conn->edid;
}

btw for locking I'd just pick a spinlock, then you can use a simple timer
to free the edid. Currently the drm modeset locking is a bit fuzzy around
connectors (due to the dp mst introduction), so separate locking would
simplify things.

> > There's an encoder->hot_plug callback where you should invalidate the
> > edid instead.
> In MCG branch, we are doing this in encoder->hot_plug only. But there the
> EDID stays, until there is one more next hotplug, and by that time, the
> detect code uses cached EDID only.
> As encoder->hot_plug function also gets called every time there is a
> hot_plug, from the hotplug_work_fn, I was afraid this might cause a race.
> Second, I still have to write a delayed_work wrapper, to call
> encoder->hot_plug from, after the timeout.

I've thought about the hotplug work function when we invalidate the edid,
and I think we can wait with that until there's demand. Currently if the
sink is horribly broken and does only respond after a while we will also
not detect it right away. And as long as we invalidate the edid soonish
(before the user could poke his system to figure out where the screen
went) we'll be good.

> If you feel that, its better to do it there, I can do changes accordingly.

Nah, I think if we wrap the caching/invalidation up into the above
suggested tiny helpers the code will be clear enough and your idea of
using "force" indeed works.

Thanks, Daniel

Sharma, Shashank Aug. 18, 2014, 3 a.m. UTC | #6

Thanks Daniel.
Let me incorporate these changes you suggested, and prepare a new patch set. 

Regards
Shashank 
-----Original Message-----
From: Daniel Vetter [mailto:[e-mail]] On Behalf Of Daniel Vetter
Sent: Thursday, August 14, 2014 5:27 PM
To: Sharma, Shashank
Cc: Daniel Vetter; intel-gfx; Vetter, Daniel; Kumar, Shobhit
Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Optimize HDMI EDID reads

On Thu, Aug 14, 2014 at 02:53:44PM +0530, Sharma, Shashank wrote:
> Hi Daniel,
> 
> I can do all the changes accordingly and upload a new patch.
> 
> This is what I am going to do:
> 1. Change the EDID caching time to a second from a minute (probably 
> there was a miscommunication).
> 2. Remove the gen_check
> 3. Use the connector->edid pointer to cache EDID.
> 
> I have only few problems with these two suggestions:
> > Keying off the force parameter isn't actually precise enough.
> It is. All the calls to HDMI detect, which come as a result of user 
> space interaction keep force = 1, whereas all the hot plug event 
> callers keep it force = 0. Please have a look:
> 
> IOCTL / Sysfs calls, calling connector->funcs->detect() 1. 
> drm_helper_probe_single_connector_modes_merge_bits => (force = 1) 2. 
> status_show => (force = 1)
> 
> Hot plug handlers / hot plug poll
> 1. drm_helper_hpd_irq_event => (force = 0) 2. output_poll_execute => 
> (force = 0) So this should work all right.

Hm indeed, I've mixed up things with the output poll worker. My concern is mostly that "force" already has overloaded meaning. But I think if we polish the code-flow a bit it should work out. Quick draft:

	/* at the top of the detect function before anything else */
	if (!force)
		intel_connector_invalidate_edid(conn);


Then we replace all current calls to drm_get_edid with a new intel_connector_get_edid_cached, which first checks the conn->edid cache and only if that's empty call drm_edid_cache.

btw for the edid cache itself I think Chris' idea to use errno pointers is great, so the state machine would be:

connector->edid == NULL -> cache expired

IS_ERR(connector->edid) -> negative cache entry with errno value, e.g.
-ENXIO. this way we also speed up subsequent detect calls from userspace when nothing is connected.

everything else -> cached edid

So in code this would look like

void intel_connector_invalidate_edid(conn)
{
	/* locking tbd */

	if (!conn->edid)
		return;

	if (!IS_ERR(conn->edid))
		kfree(conn->edid);

	conn->edid = NULL;
}

struct drm_edid *intel_connector_get_edid_cached(conn, ...) {
	/* locking tbd */

	if (!conn->edid) {
		conn->edid = drm_get_edid(...);
		if (!conn->edid)
			conn->edid = ERR_PTR(-ENXIO);

		/* rearm invalidate timer */
	}

	return IS_ERR(conn->edid) ? NULL : conn->edid; }

btw for locking I'd just pick a spinlock, then you can use a simple timer to free the edid. Currently the drm modeset locking is a bit fuzzy around connectors (due to the dp mst introduction), so separate locking would simplify things.

> > There's an encoder->hot_plug callback where you should invalidate 
> > the edid instead.
> In MCG branch, we are doing this in encoder->hot_plug only. But there 
> the EDID stays, until there is one more next hotplug, and by that 
> time, the detect code uses cached EDID only.
> As encoder->hot_plug function also gets called every time there is a 
> hot_plug, from the hotplug_work_fn, I was afraid this might cause a race.
> Second, I still have to write a delayed_work wrapper, to call
> encoder->hot_plug from, after the timeout.

I've thought about the hotplug work function when we invalidate the edid, and I think we can wait with that until there's demand. Currently if the sink is horribly broken and does only respond after a while we will also not detect it right away. And as long as we invalidate the edid soonish (before the user could poke his system to figure out where the screen
went) we'll be good.

> If you feel that, its better to do it there, I can do changes accordingly.

Nah, I think if we wrap the caching/invalidation up into the above suggested tiny helpers the code will be clear enough and your idea of using "force" indeed works.

Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
[phone] - http://blog.ffwll.ch

Patch

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 28d185d..185a45a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -110,6 +110,8 @@ 
 #define INTEL_DSI_VIDEO_MODE	0
 #define INTEL_DSI_COMMAND_MODE	1
 
+#define INTEL_HDMI_EDID_CACHING_SEC 60
+
 struct intel_framebuffer {
 	struct drm_framebuffer base;
 	struct drm_i915_gem_object *obj;
@@ -507,6 +509,8 @@  struct intel_hdmi {
 	enum hdmi_force_audio force_audio;
 	bool rgb_quant_range_selectable;
 	enum hdmi_picture_aspect aspect_ratio;
+	struct edid *edid;
+	struct delayed_work edid_work;
 	void (*write_infoframe)(struct drm_encoder *encoder,
 				enum hdmi_infoframe_type type,
 				const void *frame, ssize_t len);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 5f47d35..8dc3970 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -962,6 +962,22 @@  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	return true;
 }
 
+/* Work function to invalidate EDID caching */
+static void intel_hdmi_invalidate_edid(struct work_struct *work)
+{
+	struct intel_hdmi *intel_hdmi = container_of(to_delayed_work(work),
+				struct intel_hdmi, edid_work);
+	struct drm_device *dev = intel_hdmi_to_dev(intel_hdmi);
+	struct drm_mode_config *mode_config = &dev->mode_config;
+
+	mutex_lock(&mode_config->mutex);
+	/* Checkpatch says kfree is NULL protected */
+	kfree(intel_hdmi->edid);
+	intel_hdmi->edid = NULL;
+	mutex_unlock(&mode_config->mutex);
+	DRM_DEBUG_DRIVER("cleaned up cached EDID\n");
+}
+
 static enum drm_connector_status
 intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
@@ -978,15 +994,58 @@  intel_hdmi_detect(struct drm_connector *connector, bool force)
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
 
+	/*
+	* hdmi_detect() gets called from both get_resource()
+	* and HDMI hpd bottom half work function.
+	* Its not required to read EDID for every detect call until it's is
+	* from a hot plug. Lets cache the EDID as soon as we get
+	* a HPD, and then try to re-use this for all the non hpd detact calls
+	* coming with in a finite duration.
+	*/
+	if (INTEL_INFO(dev)->gen < 6)
+		/* Do not break old platforms */
+		goto skip_optimization;
+
+	/* If call is from HPD, do check real status by reading EDID */
+	if (!force)
+		goto skip_optimization;
+
+	/* This is a non-hpd call, lets see if we can optimize this */
+	if (intel_hdmi->edid) {
+		/*
+		* So this is a non-hpd call, within the duration when
+		* EDID caching is valid. So previous status (valid)
+		* of connector is re-usable.
+		*/
+		if (connector->status != connector_status_unknown) {
+			DRM_DEBUG_DRIVER("Reporting force status\n");
+			return connector->status;
+		}
+	}
+
+skip_optimization:
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
 	intel_hdmi->has_hdmi_sink = false;
 	intel_hdmi->has_audio = false;
 	intel_hdmi->rgb_quant_range_selectable = false;
+
+	/*
+	* You are well deserving, dear code, as you have survived
+	* all the optimizations. Now go and enjoy reading EDID
+	*/
 	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
+			intel_gmbus_get_adapter(dev_priv,
+						intel_hdmi->ddc_bus));
+	/*
+	* Now when we have read new EDID, update cached EDID with
+	* latest (both NULL or non NULL). Cancel the delayed work
+	* which cleans up the cached EDID. Re-schedule if required.
+	*/
+	kfree(intel_hdmi->edid);
+	intel_hdmi->edid = edid;
+	cancel_delayed_work_sync(&intel_hdmi->edid_work);
 
 	if (edid) {
 		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
@@ -997,8 +1056,17 @@  intel_hdmi_detect(struct drm_connector *connector, bool force)
 			intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
 			intel_hdmi->rgb_quant_range_selectable =
 				drm_rgb_quant_range_selectable(edid);
+			/*
+			* Allow re-use of cached EDID for 60 sec, as
+			* userspace modeset should happen within this
+			* duration, and multiple detect calls will be
+			* handled using cached EDID.
+			*/
+			schedule_delayed_work(&intel_hdmi->edid_work,
+				msecs_to_jiffies(
+					INTEL_HDMI_EDID_CACHING_SEC
+							* 1000));
 		}
-		kfree(edid);
 	}
 
 	if (status == connector_status_connected) {
@@ -1027,13 +1095,22 @@  static int intel_hdmi_get_modes(struct drm_connector *connector)
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
-
-	ret = intel_ddc_get_modes(connector,
+	/*
+	* GEN6 and + have software support for EDID caching, so
+	* use cached_edid from detect call, if available.
+	*/
+	if (intel_hdmi->edid && (INTEL_INFO(connector->dev)->gen >= 6)) {
+		ret = intel_connector_update_modes(connector,
+				intel_hdmi->edid);
+		DRM_DEBUG_DRIVER("Using cached EDID, got %d modes\n", ret);
+	} else {
+		ret = intel_ddc_get_modes(connector,
 				   intel_gmbus_get_adapter(dev_priv,
 							   intel_hdmi->ddc_bus));
+		DRM_DEBUG_DRIVER("Read EDID, got %d modes\n", ret);
+	}
 
 	intel_display_power_put(dev_priv, power_domain);
-
 	return ret;
 }
 
@@ -1661,5 +1738,8 @@  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
 	intel_dig_port->dp.output_reg = 0;
 
+	/* Work function to invalidate cached EDID after timeout */
+	INIT_DELAYED_WORK(&(intel_dig_port->hdmi.edid_work),
+				intel_hdmi_invalidate_edid);
 	intel_hdmi_init_connector(intel_dig_port, intel_connector);
 }