Date:   Tue, 30 Mar 2021 17:01:31 +0300
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Sam Ravnborg <sam@...nborg.org>, robdclark@...omium.org,
        Thomas Zimmermann <tzimmermann@...e.de>,
        dri-devel@...ts.freedesktop.org, David Airlie <airlied@...ux.ie>,
        linux-arm-msm@...r.kernel.org, Steev Klimaszewski <steev@...i.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Stanislav Lisovskiy <stanislav.lisovskiy@...el.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 09/14] drm/edid: Use the cached EDID in drm_get_edid()
 if eDP

On Mon, Mar 29, 2021 at 07:53:40PM -0700, Douglas Anderson wrote:
> Each time we call drm_get_edid() we:
> 1. Go out to the bus and ask for the EDID.
> 2. Cache the EDID.
> 
> We can improve this to actually use the cached EDID so that if
> drm_get_edid() is called multiple times then we don't need to go out
> to the bus over and over again.
> 
> In normal DP/HDMI cases reading the EDID over and over again isn't
> _that_ expensive so, presumably, this wasn't that critical in the
> past. However for eDP going out to the bus can be expensive. This is
> because eDP panels might be powered off before the EDID was requested
> so we need to do power sequencing in addition to the transfer.
> 
> In theory we should be able to cache the EDID for all types of
> displays. There is already code throwing the cache away when we detect
> that a display was unplugged. However, it can be noted that it's
> _extra_ safe to cache the EDID for eDP since eDP isn't a hot-pluggable
> interface. If we get the EDID once then we've got the EDID and we
> should never need to read it again. For now we'll only use the cache
> for eDP both because it's more important and extra safe.
> 
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/drm_edid.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c2bbe7bee7b6..fcbf468d73c9 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2049,15 +2049,39 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter)
>  {
>  	struct edid *edid;
> +	size_t old_edid_size;
> +	const struct edid *old_edid;
>  
>  	if (connector->force == DRM_FORCE_OFF)
>  		return NULL;
>  
> -	if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter))
> -		return NULL;
> +	if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
> +	    connector->edid_blob_ptr) {
> +		/*
> +		 * eDP devices are non-removable, or at least not something
> +		 * that's expected to be hot-pluggable. We can freely use
> +		 * the cached EDID.
> +		 *
> +		 * NOTE: technically we could probably even use the cached
> +		 * EDID even for non-eDP because the cached EDID should be
> +		 * cleared if we ever notice a display is not connected, but
> +		 * we'll use an abundance of caution and only do it for eDP.
> +		 * It's more important for eDP anyway because the EDID may not
> +		 * always be readable, like when the panel is powered down.
> +		 */
> +		old_edid = (const struct edid *)connector->edid_blob_ptr->data;
> +		old_edid_size = ksize(old_edid);
> +		edid = kmalloc(old_edid_size, GFP_KERNEL);
> +		if (edid)
> +			memcpy(edid, old_edid, old_edid_size);
> +	} else {
> +		if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter))
> +			return NULL;
> +
> +		edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
> +		drm_connector_update_edid_property(connector, edid);
> +	}

This is a pretty low level function. Too low level for this caching
IMO. So I think better just do it a bit higher up like other drivers.

>  
> -	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
> -	drm_connector_update_edid_property(connector, edid);
>  	return edid;
>  }
>  EXPORT_SYMBOL(drm_get_edid);
> -- 
> 2.31.0.291.g576ba9dcdaf-goog
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

Powered by blists - more mailing lists