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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Mar 06, 2018 at 10:33:31AM +0100, Daniel Vetter wrote:
> 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.

Just realized I replied to the wrong patch, this was meant for the forced
clearing of all display_info data in one of the later patches. But it is
also somewhat relevant here ...
-Daniel

> -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

-- 
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