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

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards
Shashank
On 8/13/2014 5:44 PM, Daniel Vetter wrote:
> On Tue, Aug 12, 2014 at 06:08:20PM +0530, shashank.sharma@intel.com wrote:
>> From: Shashank Sharma <shashank.sharma@intel.com>
>>
>> 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 <shashank.sharma@intel.com>
>
> 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
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>