Re: [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, May 04, 2018 at 11:19:45PM +0300, Ville Syrjälä wrote:
> On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
> > On Wed, May 02, 2018 at 12:20:20PM +0530, Nautiyal, Ankit K wrote:
> > > From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > > 
> > > We parse the EDID and add all the modes in the connector's modelist.
> > > This adds CEA modes with aspect ratio information too, regardless of
> > > whether user space requested this information or not.
> > > 
> > > This patch:
> > > -prunes the modes with aspect-ratio information, from a
> > >  connector's modelist, if the user-space has not set the aspect ratio
> > >  DRM client cap. However if such a mode is unique in the list, it is
> > >  kept in the list, with aspect-ratio flags reset.
> > > -prepares a list of exposed modes, which is used to find unique modes
> > >  if aspect-ratio is not allowed.
> > > -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
> > >  the list of exposed modes.
> > > 
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > > Cc: Jose Abreu <jose.abreu@synopsys.com>
> > > 
> > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > > 
> > > V3: As suggested by Ville, modified the mechanism of pruning of modes
> > >     with aspect-ratio, if the aspect-ratio is not supported. Instead
> > >     of straight away pruning such a mode, the mode is retained with
> > >     aspect ratio bits set to zero, provided it is unique.
> > > V4: rebase
> > > V5: Addressed review comments from Ville:
> > >     -used a pointer to store last valid mode.
> > >     -avoided, modifying of picture_aspect_ratio in kernel mode,
> > >      instead only flags bits of user mode are reset (if aspect-ratio
> > >      is not supported).
> > > V6: As suggested by Ville, corrected the mode pruning logic and
> > >     elaborated the mode pruning logic and the assumptions taken.
> > > V7: rebase
> > > V8: rebase
> > > V9: rebase
> > > V10: rebase
> > > V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
> > >      logic to correctly identify and prune modes with aspect-ratio,
> > >      if aspect-ratio cap is not set.
> > > V12: As suggested by Ville, added another list_head in
> > >      drm_mode_display to traverse the list of exposed modes and
> > >      avoided duplication of modes.
> > > V13: Minor modifications, as suggested by Ville.
> > > ---
> > >  drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++-------
> > >  include/drm/drm_modes.h         | 13 ++++++++++++
> > >  2 files changed, 51 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index dfc8ca1..8ca1149 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -1531,15 +1531,36 @@ static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *conne
> > >  	return connector->encoder;
> > >  }
> > >  
> > > -static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
> > > -					 const struct drm_file *file_priv)
> > > +static bool
> > > +drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
> > > +			     const struct list_head *export_list,
> > > +			     const struct drm_file *file_priv)
> > >  {
> > >  	/*
> > >  	 * If user-space hasn't configured the driver to expose the stereo 3D
> > >  	 * modes, don't expose them.
> > >  	 */
> > > +
> > >  	if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode))
> > >  		return false;
> > > +	/*
> > > +	 * If user-space hasn't configured the driver to expose the modes
> > > +	 * with aspect-ratio, don't expose them. However if such a mode
> > > +	 * is unique, let it be exposed, but reset the aspect-ratio flags
> > > +	 * while preparing the list of user-modes.
> > > +	 */
> > > +	if (!file_priv->aspect_ratio_allowed &&
> > > +	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
> > > +		struct drm_display_mode *mode_itr;
> > > +
> > > +		list_for_each_entry(mode_itr, export_list, export_head)
> > 
> > By walking the list of only the modes already added to the export list we
> > rely on ASPECT_NONE being first if present. That seems to be a bit
> > fragile. If we instead walk over all present modes (i.e. connector->modes)
> > then I think that's avoided.
> 
> I don't think that would work. If we just walk over all the modes then
> wouldn't we always find a duplicate when the same mode with two different
> aspect ratios is on the list? And then we'd export neither? Or maybe I
> misunderstood what you mean here.

We'd export both. Let's assume we have 2 modes, only difference is aspect
ratio.

1. We check the first mode, it has aspect_ratio != NONE. But because
there's no other exported mode yet on export_list, we export it (and
filter out the aspect_ratio by setting it to NONE).

2. 2nd mode has aspect_ratio == NONE, so we export it.

Results in duped modes exported. If you always guarantee that aspect_ratio
== NONE is before the same mode with aspect ratio, then this algo here
works. But not for the more general case I think.

> To me it seems like the correct option is to check against the
> export_list unconditionally, no matter whether the current mode has the
> aspect ratio set or not. If an identical mode is already on the list
> we don't export again, if it's not there then we export.

That might be another option to fix the bug.
-Daniel

> 
> > With that changed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > > +			if (drm_mode_match(mode_itr, mode,
> > > +					   DRM_MODE_MATCH_TIMINGS |
> > > +					   DRM_MODE_MATCH_CLOCK |
> > > +					   DRM_MODE_MATCH_FLAGS |
> > > +					   DRM_MODE_MATCH_3D_FLAGS))
> > > +				return false;
> > > +	}
> > >  
> > >  	return true;
> > >  }
> > > @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> > >  	struct drm_mode_modeinfo u_mode;
> > >  	struct drm_mode_modeinfo __user *mode_ptr;
> > >  	uint32_t __user *encoder_ptr;
> > > +	LIST_HEAD(export_list);
> > >  
> > >  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > >  		return -EINVAL;
> > > @@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> > >  
> > >  	/* delayed so we get modes regardless of pre-fill_modes state */
> > >  	list_for_each_entry(mode, &connector->modes, head)
> > > -		if (drm_mode_expose_to_userspace(mode, file_priv))
> > > +		if (drm_mode_expose_to_userspace(mode, &export_list,
> > > +						 file_priv)) {
> > > +			list_add_tail(&mode->export_head, &export_list);
> > >  			mode_count++;
> > > +		}
> > >  
> > >  	/*
> > >  	 * This ioctl is called twice, once to determine how much space is
> > >  	 * needed, and the 2nd time to fill it.
> > > +	 * The modes that need to be exposed to the user are maintained in the
> > > +	 * 'export_list'. When the ioctl is called first time to determine the,
> > > +	 * space, the export_list gets filled, to find the no.of modes. In the
> > > +	 * 2nd time, the user modes are filled, one by one from the export_list.
> > >  	 */
> > >  	if ((out_resp->count_modes >= mode_count) && mode_count) {
> > >  		copied = 0;
> > >  		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
> > > -		list_for_each_entry(mode, &connector->modes, head) {
> > > -			if (!drm_mode_expose_to_userspace(mode, file_priv))
> > > -				continue;
> > > -
> > > +		list_for_each_entry(mode, &export_list, export_head) {
> > >  			drm_mode_convert_to_umode(&u_mode, mode);
> > > +			/*
> > > +			 * Reset aspect ratio flags of user-mode, if modes with
> > > +			 * aspect-ratio are not supported.
> > > +			 */
> > > +			drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode);
> > >  			if (copy_to_user(mode_ptr + copied,
> > >  					 &u_mode, sizeof(u_mode))) {
> > >  				ret = -EFAULT;
> > > diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> > > index 2f78b7e..b159fe0 100644
> > > --- a/include/drm/drm_modes.h
> > > +++ b/include/drm/drm_modes.h
> > > @@ -411,6 +411,19 @@ struct drm_display_mode {
> > >  	 * Field for setting the HDMI picture aspect ratio of a mode.
> > >  	 */
> > >  	enum hdmi_picture_aspect picture_aspect_ratio;
> > > +
> > > +	/**
> > > +	 * @export_head:
> > > +	 *
> > > +	 * struct list_head for modes to be exposed to the userspace.
> > > +	 * This is to maintain a list of exposed modes while preparing
> > > +	 * user-mode's list in drm_mode_getconnector ioctl. The purpose of this
> > > +	 * list_head only lies in the ioctl function, and is not expected to be
> > > +	 * used outside the function.
> > > +	 * Once used, the stale pointers are not reset, but left as it is, to
> > > +	 * avoid overhead of protecting it by mode_config.mutex.
> > > +	 */
> > > +	struct list_head export_head;
> > >  };
> > >  
> > >  /**
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > 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
> 
> -- 
> Ville Syrjälä
> Intel

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