[PATCH v7 5/9] drm: Handle aspect-ratio info in getblob
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Mon Mar 12 08:02:48 UTC 2018
On 3/9/2018 6:40 PM, Ville Syrjälä wrote:
> On Fri, Mar 09, 2018 at 11:07:14AM +0530, Nautiyal, Ankit K wrote:
>> From: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>>
>> If the user-space does not support aspect-ratio, then getblob called
>> with the blob id of a user-mode, should clear the aspect-ratio
>> information in the blob data.
>>
>> Currently for a given blob id, there is no way to determine if the
>> blob stores user-mode or not. This can only be ascertained when the
>> blob is used for an atomic modeset call.
>>
>> This patch:
>> -adds a new field 'is_video_mode' in drm_property_blob to
>> differentiate between the video mode blobs and the other blobs.
>> -sets the field 'is_video_mode' when the blob is used for modeset.
>> -removes the aspect-ratio info from the user-mode data if aspect-ratio
>> is not supported by the user, while returning the blob to the user,
>> in getblob ioctl.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>>
>> V6: As suggested by Ville:
>> -added helper functions for determining if aspect-ratio is
>> expected in user-mode and for allowing/disallowing the
>> aspect-ratio, if its not expected.
>> -avoided clobbering of blob-data, instead cleared the aspect-ratio
>> in the user-mode only, so that another client with aspect-ratio
>> cap, can still get the aspect-ratio information from getblob.
>> V7: Fixed warning [Wint-to-pointer-cast] for 32 bit platforms.
>> ---
>> drivers/gpu/drm/drm_atomic.c | 1 +
>> drivers/gpu/drm/drm_modes.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/drm_property.c | 5 +++++
>> include/drm/drm_modes.h | 4 ++++
>> include/drm/drm_property.h | 2 ++
>> 5 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 34b7d42..2b1c88a 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -464,6 +464,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>> else if (property == config->prop_mode_id) {
>> struct drm_property_blob *mode =
>> drm_property_lookup_blob(dev, val);
>> + mode->is_video_mode = true;
>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>> drm_property_blob_put(mode);
>> return ret;
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index a48672c..4430294 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -1761,3 +1761,47 @@ 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
>> + * @flags: 32 bit flag that contains 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, uint32_t flags)
> I think we should pass the entire umode to these functions. That way the
> compiler will make sure we're passing in the correct thing instead of
> some other mode flags or random integers.
Agreed, that makes sense.
Will change parameter to the umode.
>
>> +{
>> + return file_priv->aspect_ratio_allowed ||
>> + (flags & DRM_MODE_FLAG_PIC_AR_MASK) == DRM_MODE_FLAG_PIC_AR_NONE;
>> +}
>> +EXPORT_SYMBOL(drm_mode_aspect_ratio_allowed);
>> +
>> +/**
>> + * drm_mode_handle_aspect_ratio - handles 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 flag, if aspect-ratio info is not allowed.
>> + *
>> + * @file_priv: file private structure to get the user capabilities.
>> + * @flags: 32 bit flag that is to be modified, in case the aspect
>> + * ratio info is not allowed.
>> + *
>> + */
>> +void
>> +drm_mode_handle_aspect_ratio(const struct drm_file *file_priv, uint32_t *flags)
>> +{
>> + if (!drm_mode_aspect_ratio_allowed(file_priv, *flags))
>> + *flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>> +}
>> +EXPORT_SYMBOL(drm_mode_handle_aspect_ratio);
> The function name could be more descriptive.
> drm_mode_filter_aspect_ratio_flags() or someting similar perhaps?
Point taken. I was a little worried about the length of the function name.
But having a meaningful name is important.
Will change this also, in the next patch set.
> Otherwise I think this is looking good.
>
> IIRC I did have a few more patches in my last inforame series to make
> sure we don't send invalid aspect ratio values in the infoframe. Unless
> something has changed I believe we still need those as well. Or am I
> wrong?
You are right, There are two patches:
https://patchwork.freedesktop.org/patch/188049/
and https://patchwork.freedesktop.org/patch/188051/
from the info-frame clean up series, both of which have already received
r-b.
These patches need a little change, as they reject picture aspect ratio
> 16:9
Since we are adding support of 64:27 and 256:135, then we need to update
the reject condition.
I can make the required changes and add these two patches as part of
this series.
What do you suggest? Is it fine to make these changes?
Regards,
Ankit
>
>> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
>> index c37ac41..0a9c879 100644
>> --- a/drivers/gpu/drm/drm_property.c
>> +++ b/drivers/gpu/drm/drm_property.c
>> @@ -757,6 +757,11 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
>> ret = -EFAULT;
>> goto unref;
>> }
>> + if (blob->is_video_mode) {
>> + struct drm_mode_modeinfo __user *mode =
>> + u64_to_user_ptr(out_resp->data);
>> + drm_mode_handle_aspect_ratio(file_priv, &mode->flags);
>> + }
>> }
>> out_resp->length = blob->length;
>> unref:
>> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
>> index 2f78b7e..51d1188 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,
>> + uint32_t flags);
>> +void drm_mode_handle_aspect_ratio(const struct drm_file *file_priv,
>> + uint32_t *flags);
>>
>> struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
>> int hdisplay, int vdisplay, int vrefresh,
>> diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
>> index 8a522b4..95e6e32 100644
>> --- a/include/drm/drm_property.h
>> +++ b/include/drm/drm_property.h
>> @@ -194,6 +194,7 @@ struct drm_property {
>> * @head_global: entry on the global blob list in
>> * &drm_mode_config.property_blob_list.
>> * @head_file: entry on the per-file blob list in &drm_file.blobs list.
>> + * @is_video_mode: flag to mark the blobs that contain drm_mode_modeinfo.
>> * @length: size of the blob in bytes, invariant over the lifetime of the object
>> * @data: actual data, embedded at the end of this structure
>> *
>> @@ -208,6 +209,7 @@ struct drm_property_blob {
>> struct drm_device *dev;
>> struct list_head head_global;
>> struct list_head head_file;
>> + bool is_video_mode;
>> size_t length;
>> unsigned char data[];
>> };
>> --
>> 2.7.4
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180312/996dc5e6/attachment-0001.html>
More information about the dri-devel
mailing list