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

[Nautiyal, Ankit K]

On 4/23/2018 3:43 PM, Ville Syrjälä wrote:
> 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 at 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 at 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.

That makes sense. I will merge this patch with the patch for handling 
aspect ratio in modeset path,
where its actually used.

>
>>>> +
>>>> +/**
>>>> + * 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