[Intel-gfx] [PATCH 1/2] drm/i915: Optimize HDMI EDID reads
Sharma, Shashank
shashank.sharma at intel.com
Thu Aug 14 08:15:05 CEST 2014
Regards
Shashank
On 8/13/2014 5:44 PM, Daniel Vetter wrote:
> On Tue, Aug 12, 2014 at 06:08:20PM +0530, shashank.sharma at intel.com wrote:
>> From: Shashank Sharma <shashank.sharma at 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 at 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list