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

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

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ä <ville.syrjala@linux.intel.com>
> > 
> > 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 <keithp@keithp.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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(&dev->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 = &connector->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 &drm_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 = &connector->display_info;
> > -
> > -	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;
> > @@ -4665,7 +4653,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
> >  	int num_modes = 0;
> >  	u32 quirks;
> >  
> > -	drm_reset_display_info(connector);
> > +	drm_connector_reset_display_info(connector);
> >  
> >  	if (edid == NULL) {
> >  		clear_eld(connector);
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 18cb63b30e33..f3eddbbd0616 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2027,7 +2027,7 @@ static int drm_fb_helper_probe_connector_modes(struct drm_fb_helper *fb_helper,
> >  
> >  	drm_fb_helper_for_each_connector(fb_helper, i) {
> >  		connector = fb_helper->connector_info[i]->connector;
> > -		count += connector->funcs->fill_modes(connector, maxX, maxY);
> > +		count += drm_connector_fill_modes(connector, maxX, maxY);
> >  	}
> >  
> >  	return count;
> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > index 1c5b5ce1fd7f..3c6e800b66a0 100644
> > --- a/drivers/gpu/drm/drm_sysfs.c
> > +++ b/drivers/gpu/drm/drm_sysfs.c
> > @@ -130,9 +130,9 @@ static ssize_t status_store(struct device *device,
> >  			      connector->name,
> >  			      old_force, connector->force);
> >  
> > -		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);
> >  	}
> >  
> >  	mutex_unlock(&dev->mode_config.mutex);
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 8815ef1ce429..bf14474c83f5 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -1124,6 +1124,9 @@ void drm_mode_connector_set_link_status_property(struct drm_connector *connector
> >  						 uint64_t link_status);
> >  int drm_connector_init_panel_orientation_property(
> >  	struct drm_connector *connector, int width, int height);
> > +void drm_connector_reset_display_info(struct drm_connector *connector);
> > +int drm_connector_fill_modes(struct drm_connector *connector,
> > +			     unsigned int max_width, unsigned int max_height);
> >  
> >  /**
> >   * struct drm_tile_group - Tile group metadata
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 8d89a9c3748d..db5e6a990c2d 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -465,7 +465,6 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >  struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
> >  				     struct i2c_adapter *adapter);
> >  struct edid *drm_edid_duplicate(const struct edid *edid);
> > -void drm_reset_display_info(struct drm_connector *connector);
> >  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid);
> >  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
> >  
> > -- 
> > 2.13.6
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel