From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sharma, Shashank" Subject: Re: [PATCH 1/2] drm/i915: Optimize HDMI EDID reads Date: Thu, 14 Aug 2014 11:45:05 +0530 Message-ID: <53EC53E9.2030809@intel.com> References: <1407847101-21654-1-git-send-email-shashank.sharma@intel.com> <1407847101-21654-2-git-send-email-shashank.sharma@intel.com> <20140813121455.GX10500@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id B41E96E225 for ; Wed, 13 Aug 2014 23:15:55 -0700 (PDT) In-Reply-To: <20140813121455.GX10500@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: daniel.vetter@intel.com, intel-gfx@lists.freedesktop.org, shobhit.kumar@intel.com List-Id: intel-gfx@lists.freedesktop.org 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 >> >> 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 > > 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 >