On Fri, Mar 09, 2018 at 11:07:14AM +0530, Nautiyal, Ankit K wrote:
> From: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
>
> 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@xxxxxxxxx>
>
> 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.
> +{
> + 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?
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?
> 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
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
