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

[Daniel Vetter]

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ä 
> > 
> > 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 
> > Cc: Daniel Vetter 
> > Cc: Shashank Sharma 
> > Signed-off-by: Ville Syrjälä 
> 
> 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(>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 = >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 = >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

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

[Daniel Vetter]

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ä 
> > 
> > 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 
> > Cc: Daniel Vetter 
> > Cc: Shashank Sharma 
> > Signed-off-by: Ville Syrjälä 
> 
> 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.

Probably best to extract a drm_connector_is_panel function into
drm_connector.c for that, and also use it in
drm_helper_move_panel_connectors_to_head. Just to make sure we only have 1
definition of  what a panel is.
-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(>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 = >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 = >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

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

[Daniel Vetter]

On Tue, Feb 27, 2018 at 02:56:54PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> 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 
> Cc: Daniel Vetter 
> Cc: Shashank Sharma 
> Signed-off-by: Ville Syrjälä 

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

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

[Sharma, Shashank]

Regards

Shashank


On 2/27/2018 6:26 PM, Ville Syrjala wrote:

From: Ville Syrjälä 

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 
Cc: Daniel Vetter 
Cc: Shashank Sharma 
Signed-off-by: Ville Syrjälä 
---
  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(>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 = >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 = >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);
How about if we move the call to drm_reset_display_info() within 
drm_add_display_info() ?

something like:
drm_add_display_info()
{
struct drm_display_info *info = >display_info;
u32 quirks = edid_get_quirks(edid);

drm_reset_display_info();

info->width_mm = edid->width_cm * 10;
info->height_mm = edid->height_cm * 10;
info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);

..
}

- Shashank

+
if (edid == NULL) {
clear_eld(connector);
return 0;


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel