Re: [RFC][PATCH 05/11] drm/edid: Clear display info fully

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Feb 27, 2018 at 02:56:54PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Now that we have split the display info into static and dynamic parts,
> we can just zero out the entire dynamic part with memset(). Previously
> we were just clearing parts of it, leaving stale data in other parts
> (eg. HDMI SCDC capabilities).
> 
> Also when the edid is NULL drm_add_edid_modes() bails out early skipping
> the call to drm_add_display_info(). Thus we would again leave stale
> data behind. To avoid that let's clear out the display info at the
> very start of drm_add_edid_modes().
> 
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I like the idea of this, but I think we need to refine it a bit. What
about only doing this for external screens, but not panels? That would
avoid a lot of surprises (there's really no need to reset the display info
for fixed panels ever), and also avoid the need for the first 3 patches in
your series.
-Daniel

> ---
>  drivers/gpu/drm/drm_connector.c |  3 +--
>  drivers/gpu/drm/drm_edid.c      | 23 +++--------------------
>  2 files changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index d73e97ed7dff..ddd7d978f462 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1389,10 +1389,9 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  	 * duplicate it rather than attempt to ensure some arbitrary
>  	 * ordering of calls.
>  	 */
> +	drm_reset_display_info(connector);
>  	if (edid)
>  		drm_add_display_info(connector, edid);
> -	else
> -		drm_reset_display_info(connector);
>  
>  	drm_object_property_set_value(&connector->base,
>  				      dev->mode_config.non_desktop_property,
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 788fee4b4bf9..78c1f37be3db 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4443,37 +4443,18 @@ drm_reset_display_info(struct drm_connector *connector)
>  {
>  	struct drm_display_info *info = &connector->display_info;
>  
> -	info->width_mm = 0;
> -	info->height_mm = 0;
> -
> -	info->bpc = 0;
> -	info->color_formats = 0;
> -	info->cea_rev = 0;
> -	info->max_tmds_clock = 0;
> -	info->dvi_dual = false;
> -	info->has_hdmi_infoframe = false;
> -
> -	info->non_desktop = 0;
> +	memset(info, 0, sizeof(*info));
>  }
>  EXPORT_SYMBOL_GPL(drm_reset_display_info);
>  
>  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid)
>  {
>  	struct drm_display_info *info = &connector->display_info;
> -
>  	u32 quirks = edid_get_quirks(edid);
>  
>  	info->width_mm = edid->width_cm * 10;
>  	info->height_mm = edid->height_cm * 10;
>  
> -	/* driver figures it out in this case */
> -	info->bpc = 0;
> -	info->color_formats = 0;
> -	info->cea_rev = 0;
> -	info->max_tmds_clock = 0;
> -	info->dvi_dual = false;
> -	info->has_hdmi_infoframe = false;
> -
>  	info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
>  
>  	DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
> @@ -4684,6 +4665,8 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>  	int num_modes = 0;
>  	u32 quirks;
>  
> +	drm_reset_display_info(connector);
> +
>  	if (edid == NULL) {
>  		clear_eld(connector);
>  		return 0;
> -- 
> 2.13.6
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel