Re: [PATCH v2 13/18] DRM/KMS/EDID: Cache EDID blobs with extensions (v2)

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 22, 2012 at 05:23:03AM -0500, Egbert Eich wrote:
> According the the VESA specs there can be up to 254 EEDID extension blocks.
> Since we may read the EDID (including extensions) in 10 second intervals to
> probe for display hotplugging (at least in cases where no hardware hotplug
> detection exists) and I2C transfer is rather slow we may end up consuming
> a considerable amount on CPU time for just that.
> This patch caches the EDID block if it contains at least one extension.
> To determine if the blocks match we only tranfer the base block, on a match
> we use the cached data.
> 
> V2: Use kmemdup() instead of a kmalloc()/memcpy() combo,
>     erase cache when reading a 'firmware'-supplied EDID or on error.
> 
> Signed-off-by: Egbert Eich <eich@suse.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |    1 +
>  drivers/gpu/drm/drm_edid.c |   57 +++++++++++++++++++++++++++++++++++++------
>  include/drm/drm_crtc.h     |    1 +
>  include/drm/drm_edid.h     |    1 +
>  4 files changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 3533609..e283355 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		drm_mode_remove(connector, mode);
>  
>  	mutex_lock(&dev->mode_config.mutex);
> +	drm_cache_edid(connector, NULL);
>  	drm_mode_object_put(dev, &connector->base);
>  	list_del(&connector->head);
>  	dev->mode_config.num_connector--;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a47fa7f..28b877c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -419,6 +419,38 @@ fixup_edid(u8 **blockp, int valid_extensions)
>  	}
>  }
>  
> +static bool
> +compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp)
> +{
> +	if (connector->edid_cache &&
> +	    connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] &&
> +	    connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] &&
> +	    connector->edid_cache->serial == (*edidp)->serial &&
> +	    connector->edid_cache->input == (*edidp)->input) {
> +		int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH;
> +		struct edid *new = kmemdup(connector->edid_cache, size, GFP_KERNEL);
> +		if (!new)
> +			return false;
> +		DRM_DEBUG_KMS("Got EDID for %s from cache.\n", drm_get_connector_name(connector));
> +		kfree(*edidp);
> +		*edidp = new;
> +		return true;
> +	}
> +	return false;
> +}
> +
> +void
> +drm_cache_edid(struct drm_connector *connector, struct edid *edid)
> +{
> +	struct edid *new = NULL;
> +	kfree(connector->edid_cache);
> +	if (edid) {
> +		int size = (edid->extensions + 1) * EDID_LENGTH;
> +		new = kmemdup(edid, size, GFP_KERNEL);
> +	}
> +	connector->edid_cache = new;
> +}
> +
>  static u8 *
>  drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  {
> @@ -429,28 +461,30 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>  	/* check if the user has specified a 'firmware' EDID file */
>  	block = (u8 *)drm_load_edid_firmware(connector);
> -	if (block)
> +	if (block) {
> +		drm_cache_edid(connector, NULL);
>  		return block;
> +	}
>  #endif
>  
>  	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> -		return NULL;
> +		goto error;
>  
>  	/* base block fetch */
>  	for (i = 0; i < 4; i++) {
>  		if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
> -			goto out;
> +			goto error_free;
>  		if (drm_edid_block_valid(block, 0, print_bad_edid))
>  			break;
>  		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
>  			connector->null_edid_counter++;
> -			goto carp;
> +			goto error_carp;
>  		}
>  	}
>  	if (i == 4)
> -		goto carp;
> +		goto error_carp;
>  
> -	/* if there's no extensions, we're done */
> +	/* if there are no extensions, we're done - don't bother caching */
>  	if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
>  		return block;

Shouldn't you erase the cache here too?

-- 
Ville Syrjälä
Intel OTC