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

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

Hi Ville,

Thanks for your time for the review and the suggestions.

Please find my comments inline:

On 2/23/2018 7:52 PM, Ville Syrjälä wrote:

On Thu, Feb 15, 2018 at 05:50:58PM +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 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>
---
 drivers/gpu/drm/drm_atomic.c   | 1 +
 drivers/gpu/drm/drm_property.c | 6 ++++++
 include/drm/drm_property.h     | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 46733d5..86b483e 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_property.c b/drivers/gpu/drm/drm_property.c
index bae50e6..639787c 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -746,6 +746,12 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
 	if (!blob)
 		return -ENOENT;
 
+	if (blob->is_video_mode && !file_priv->aspect_ratio_allowed) {
+		struct drm_mode_modeinfo *mode =
+			(struct drm_mode_modeinfo *) blob->data;
+		mode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);

This is still clobbering the internal state. If another client with
the aspect ratio cap comes in later and asks for the same blob we 
can no longer tell it what the aspect ratio was. So I think we have
to make a temporary copy of the mode here.

Also pointless parens.

Point taken. In that case, will it be alright to just clear the aspect-ratio bits in out_resp->data,
that is being sent to the user?
That way, the blob will retain the aspect ratio information.

Also, as per your comments in subsequent patches of the series, I would use a helper function
to check, if aspect-ratio info is allowed and for clearing the bits if aspect-ratio info is not
allowed.


Regards,
Ankit
 

+	}
+
 	if (out_resp->length == blob->length) {
 		if (copy_to_user(u64_to_user_ptr(out_resp->data),
 				 blob->data,
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