Re: [RFC][PATCH 08/11] drm: Add drm_connector_fill_modes()

[Ville Syrjälä]

On Tue, Mar 06, 2018 at 11:00:30AM +0100, Daniel Vetter wrote:
> On Tue, Feb 27, 2018 at 02:56:57PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > Wrap the ->fill_modes() call in a small helper that first clears out the
> > stale data from connector->display_info. This should guarantee that we
> > get consistent display_info whether or not the drivers use the EDID
> > based stuff to clear and fill it.
> > 
> > TODO: what about just after init, before anyone has called
> > ->fill_modes()? In that case userspace could see stale data if they do
> > the cheap getconnector ioctl. Not sure if that's a valid concern though.
> > 
> > Cc: Keith Packard 
> > Cc: Daniel Vetter 
> > Signed-off-by: Ville Syrjälä 
> 
> Some thoughts:
> - I think unconditionally resetting for panels is the wrong thing to do.a

I think we do fill it dynamically at least for eDP. I suppose not
resetting and just overwriting with the same data could work, but in
theory it could also lead to inconsistent behaviour if the code that
fills the info assumes that things stay at 0 until filled.

> - We're not resetting in even more places, can't we just condense them all
>   down to 1?
> - I'm undecided on whether this should be in the core, or in the helpers.
>   Atm the core is the one that implements the "just give me the current
>   mode list, don't reprobe" logic, but then we punt everything else to
>   ->fill_modes (including setting all modes to stale and all that stuff).
>   I'm slightly leaning towards doing this in the helper code, not the core
>   code. Any reasons for doing this in core?

I was pretty much just hoping to force everyone down one path. Having
multiple ways of doing things can lead to inconsistent behaviour,
especially when people are unsure which one to choose. And at least
I don't particularly enjoy having to remind myself about the internal
vs. external differences all the time.

But I must admit to not having really thought this thing through, so
I might be on the wrong track here.

> 
> Cheers, Daniel
> > ---
> >  drivers/gpu/drm/drm_connector.c | 44 
> > +
> >  drivers/gpu/drm/drm_edid.c  | 14 +
> >  drivers/gpu/drm/drm_fb_helper.c |  2 +-
> >  drivers/gpu/drm/drm_sysfs.c |  6 +++---
> >  include/drm/drm_connector.h |  3 +++
> >  include/drm/drm_edid.h  |  1 -
> >  6 files changed, 48 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_connector.c 
> > b/drivers/gpu/drm/drm_connector.c
> > index d8c3ef4f17da..2bf19a37dbac 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1389,7 +1389,7 @@ 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);
> > +   drm_connector_reset_display_info(connector);
> > if (edid && drm_edid_is_valid(edid))
> > drm_add_display_info(connector, edid);
> >  
> > @@ -1594,9 +1594,9 @@ int drm_mode_getconnector(struct drm_device *dev, 
> > void *data,
> >  
> > mutex_lock(>mode_config.mutex);
> > if (out_resp->count_modes == 0) {
> > -   connector->funcs->fill_modes(connector,
> > -dev->mode_config.max_width,
> > -dev->mode_config.max_height);
> > +   drm_connector_fill_modes(connector,
> > +dev->mode_config.max_width,
> > +dev->mode_config.max_height);
> > }
> >  
> > out_resp->mm_width = connector->display_info.width_mm;
> > @@ -1759,3 +1759,39 @@ struct drm_tile_group 
> > *drm_mode_create_tile_group(struct drm_device *dev,
> > return tg;
> >  }
> >  EXPORT_SYMBOL(drm_mode_create_tile_group);
> > +
> > +/**
> > + * drm_connector_reset_display_info - reset the connector's display info
> > + * @connector: DRM connector
> > + *
> > + * Clear the old display info for @connector allowing the driver to
> > + * repopulate it based on fresh data.
> > + */
> > +void drm_connector_reset_display_info(struct drm_connector *connector)
> > +{
> > +   struct drm_display_info *info = >display_info;
> > +
> > +   memset(info, 0, sizeof(*info));
> > +}
> > +EXPORT_SYMBOL_GPL(drm_connector_reset_display_info);
> > +
> > +/**
> > + * drm_connector_fill_modes - fill connector mode list and dynamic display 
> > info
> > + * @connector: DRM connector
> > + * @max_width: max width for modes
> > + * @max_height: max height for modes
> > + *
> > + * Reset the display info and calls _connector_funcs.fill_modes() vfunc
> > + * repopulate it and and the mode list.
> > + *
> > + * RETURNS:
> > + * The number of modes found on @connector.
> > + */
> > +int 

Re: [RFC][PATCH 08/11] drm: Add drm_connector_fill_modes()

[Daniel Vetter]

On Tue, Feb 27, 2018 at 02:56:57PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Wrap the ->fill_modes() call in a small helper that first clears out the
> stale data from connector->display_info. This should guarantee that we
> get consistent display_info whether or not the drivers use the EDID
> based stuff to clear and fill it.
> 
> TODO: what about just after init, before anyone has called
> ->fill_modes()? In that case userspace could see stale data if they do
> the cheap getconnector ioctl. Not sure if that's a valid concern though.
> 
> Cc: Keith Packard 
> Cc: Daniel Vetter 
> Signed-off-by: Ville Syrjälä 

Some thoughts:
- I think unconditionally resetting for panels is the wrong thing to do.
- We're not resetting in even more places, can't we just condense them all
  down to 1?
- I'm undecided on whether this should be in the core, or in the helpers.
  Atm the core is the one that implements the "just give me the current
  mode list, don't reprobe" logic, but then we punt everything else to
  ->fill_modes (including setting all modes to stale and all that stuff).
  I'm slightly leaning towards doing this in the helper code, not the core
  code. Any reasons for doing this in core?

Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_connector.c | 44 
> +
>  drivers/gpu/drm/drm_edid.c  | 14 +
>  drivers/gpu/drm/drm_fb_helper.c |  2 +-
>  drivers/gpu/drm/drm_sysfs.c |  6 +++---
>  include/drm/drm_connector.h |  3 +++
>  include/drm/drm_edid.h  |  1 -
>  6 files changed, 48 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index d8c3ef4f17da..2bf19a37dbac 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1389,7 +1389,7 @@ 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);
> + drm_connector_reset_display_info(connector);
>   if (edid && drm_edid_is_valid(edid))
>   drm_add_display_info(connector, edid);
>  
> @@ -1594,9 +1594,9 @@ int drm_mode_getconnector(struct drm_device *dev, void 
> *data,
>  
>   mutex_lock(>mode_config.mutex);
>   if (out_resp->count_modes == 0) {
> - connector->funcs->fill_modes(connector,
> -  dev->mode_config.max_width,
> -  dev->mode_config.max_height);
> + drm_connector_fill_modes(connector,
> +  dev->mode_config.max_width,
> +  dev->mode_config.max_height);
>   }
>  
>   out_resp->mm_width = connector->display_info.width_mm;
> @@ -1759,3 +1759,39 @@ struct drm_tile_group 
> *drm_mode_create_tile_group(struct drm_device *dev,
>   return tg;
>  }
>  EXPORT_SYMBOL(drm_mode_create_tile_group);
> +
> +/**
> + * drm_connector_reset_display_info - reset the connector's display info
> + * @connector: DRM connector
> + *
> + * Clear the old display info for @connector allowing the driver to
> + * repopulate it based on fresh data.
> + */
> +void drm_connector_reset_display_info(struct drm_connector *connector)
> +{
> + struct drm_display_info *info = >display_info;
> +
> + memset(info, 0, sizeof(*info));
> +}
> +EXPORT_SYMBOL_GPL(drm_connector_reset_display_info);
> +
> +/**
> + * drm_connector_fill_modes - fill connector mode list and dynamic display 
> info
> + * @connector: DRM connector
> + * @max_width: max width for modes
> + * @max_height: max height for modes
> + *
> + * Reset the display info and calls _connector_funcs.fill_modes() vfunc
> + * repopulate it and and the mode list.
> + *
> + * RETURNS:
> + * The number of modes found on @connector.
> + */
> +int drm_connector_fill_modes(struct drm_connector *connector,
> +  unsigned int max_width, unsigned int max_height)
> +{
> + drm_connector_reset_display_info(connector);
> +
> + return connector->funcs->fill_modes(connector, max_width, max_height);
> +}
> +EXPORT_SYMBOL(drm_connector_fill_modes);
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 78c1f37be3db..618093c4a039 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4435,18 +4435,6 @@ static void drm_parse_cea_ext(struct drm_connector 
> *connector,
>   }
>  }
>  
> -/* A connector has no EDID information, so we've got no EDID to compute 
> quirks from. Reset
> - * all of the values which would have been set from EDID
> - */
> -void
> -drm_reset_display_info(struct drm_connector *connector)
> -{
> - struct drm_display_info *info = >display_info;
> -
> - memset(info, 0,