Re: [PATCH v7 5/9] drm: Handle aspect-ratio info in getblob

["Nautiyal, Ankit K" <ankit.k.nautiyal@xxxxxxxxx>]

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

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

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel