Re: [PATCH v11 07/11] drm: Add helper functions to handle aspect-ratio flag bits

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

On Mon, Apr 23, 2018 at 10:55:54AM +0530, Nautiyal, Ankit K wrote:
> 
> 
> On 4/20/2018 7:42 PM, Ville Syrjälä wrote:
> > On Fri, Apr 20, 2018 at 07:01:47PM +0530, Nautiyal, Ankit K wrote:
> >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>
> >> This patch adds helper functions for determining if aspect-ratio is
> >> expected in user-mode and for allowing/disallowing the aspect-ratio,
> >> if its not expected.
> >>
> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_modes.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
> >>   include/drm/drm_modes.h     |  4 ++++
> >>   2 files changed, 51 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> >> index c395a24..d6133e8 100644
> >> --- a/drivers/gpu/drm/drm_modes.c
> >> +++ b/drivers/gpu/drm/drm_modes.c
> >> @@ -1759,3 +1759,50 @@ bool drm_mode_is_420(const struct drm_display_info *display,
> >>   		drm_mode_is_420_also(display, mode);
> >>   }
> >>   EXPORT_SYMBOL(drm_mode_is_420);
> >> +
> >> +/**
> >> + * drm_mode_aspect_ratio_allowed - checks if the aspect-ratio information
> >> + * is expected from the user-mode.
> >> + *
> >> + * If the user has set aspect-ratio cap, then the flag of the user-mode is
> >> + * allowed to contain aspect-ratio value.
> >> + * If the user does not set aspect-ratio cap, then the only value allowed in the
> >> + * flags bits is aspect-ratio NONE.
> >> + *
> >> + * @file_priv: file private structure to get the user capabilities
> >> + * @umode: drm_mode_modeinfo struct, whose flag carry the aspect ratio
> >> + * information.
> >> + *
> >> + * Returns:
> >> + * true if the aspect-ratio info is allowed in the user-mode flags.
> >> + * false, otherwise.
> >> + */
> >> +bool
> >> +drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
> >> +			      struct drm_mode_modeinfo *umode)
> >> +{
> >> +	return file_priv->aspect_ratio_allowed || (umode->flags &
> >> +		DRM_MODE_FLAG_PIC_AR_MASK) == DRM_MODE_FLAG_PIC_AR_NONE;
> > Odd line split here. Makes this a bit hard to read.
> > I would split after the ||
> 
> Agreed. I wasn't sure how to let it have better readability and less 
> than 80 char length
> at the same time. Will fix this.
> 
> >
> >> +}
> >> +EXPORT_SYMBOL(drm_mode_aspect_ratio_allowed);
> > Do we actually need to export these? I don't think so.
> >
> > But I might be wrong. It's a bit hard to see with the way
> > you split this patch with the actual users in a different patch.
> 
> These helper functions are used in drm_mode_atomic.c, drm_mode_crtc.c, 
> and drm_mode_get_connector.c
> The patches are split as to have modes-set handling separate and the 
> exposing of connector separate.
> Do you suggest this patch to be merged with the patch for handling 
> aspect-ratio in modeset?

Usually if you add a function you should do it in the same patch where
you add the user(s) for said function. Otherwise it's impossibly to
judge whether the new function is any good.

> 
> >> +
> >> +/**
> >> + * drm_mode_filter_aspect_ratio_flags - filters the aspect-ratio bits in the
> >> + * user-mode flags.
> >> + *
> >> + * Checks if the aspect-ratio information is allowed. Resets the aspect-ratio
> >> + * bits in the user-mode flags, if aspect-ratio info is not allowed.
> >> + *
> >> + * @file_priv: file private structure to get the user capabilities.
> >> + * @umode: drm_mode_modeinfo struct, whose flags' aspect-ratio bits needs to
> >> + * be filtered.
> >> + *
> >> + */
> >> +void
> >> +drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
> >> +				   struct drm_mode_modeinfo *umode)
> >> +{
> >> +	if (!drm_mode_aspect_ratio_allowed(file_priv, umode))
> >> +		umode->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> >> +}
> >> +EXPORT_SYMBOL(drm_mode_filter_aspect_ratio_flags);
> >> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> >> index 2f78b7e..e0b060d 100644
> >> --- a/include/drm/drm_modes.h
> >> +++ b/include/drm/drm_modes.h
> >> @@ -461,6 +461,10 @@ bool drm_mode_is_420_also(const struct drm_display_info *display,
> >>   			  const struct drm_display_mode *mode);
> >>   bool drm_mode_is_420(const struct drm_display_info *display,
> >>   		     const struct drm_display_mode *mode);
> >> +bool drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
> >> +				   struct drm_mode_modeinfo *umode);
> >> +void drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
> >> +					struct drm_mode_modeinfo *umode);
> >>   
> >>   struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
> >>   				      int hdisplay, int vdisplay, int vrefresh,
> >> -- 
> >> 2.7.4
> 

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx