[PATCH v5 0/9] Aspect ratio support in DRM layer

[PATCH v5 0/9] Aspect ratio support in DRM layer

[Nautiyal, Ankit K]

From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

This patch series is a re-attempt to enable aspect ratio support in DRM layer.
Currently the aspect ratio information gets lost in translation during a
user->kernel mode or vice versa.

The old patch series (https://pw-emeril.freedesktop.org/series/10850/) had
4 patches, out of which 2 patches were reverted due to lack of drm client
protection while loading the aspect information.

This patch series also includes 3 patches from Ville Syrjälä's series for
'Info-frame cleanup and fixes' https://patchwork.freedesktop.org/series/33730/
which fixes the mode matching mechanism via flags.

This patch series, adds a DRM client option for aspect ratio, and loads aspect
ratio flags, only when the client sets this cap.

To test this patch, the testdiplay IGT test is modified to have an option to do
a modeset with only aspect ratio modes.
https://patchwork.freedesktop.org/series/37033/

Also, there is a userspace implementation in Wayland/weston layer:
https://patchwork.freedesktop.org/patch/188125/
(Which is already ACK'ed by wayland community.)

This, helps us in passing HDMI compliance test cases like 7-27, where the test
equipment applies a CEA mode, and expects the exact VIC in the AVI infoframes.

Ankit Nautiyal (4):
  drm: Add DRM client cap for aspect-ratio
  drm: Handle aspect-ratio info in getblob
  drm: Handle aspect ratio info in legacy and atomic modeset paths
  drm: Expose modes with aspect ratio, only if requested

Sharma, Shashank (2):
  drm: Add aspect ratio parsing in DRM layer
  drm: Add and handle new aspect ratios in DRM layer

Ville Syrjälä (3):
  drm/modes: Introduce drm_mode_match()
  drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy
  drm/edid: Fix cea mode aspect ratio handling

 drivers/gpu/drm/drm_atomic.c        |  31 +++++--
 drivers/gpu/drm/drm_atomic_helper.c |   6 +-
 drivers/gpu/drm/drm_connector.c     |  45 ++++++++-
 drivers/gpu/drm/drm_crtc.c          |  15 +++
 drivers/gpu/drm/drm_crtc_internal.h |   3 +-
 drivers/gpu/drm/drm_edid.c          |  18 +++-
 drivers/gpu/drm/drm_ioctl.c         |   5 +
 drivers/gpu/drm/drm_mode_object.c   |   9 +-
 drivers/gpu/drm/drm_modes.c         | 178 +++++++++++++++++++++++++++++-------
 drivers/gpu/drm/drm_property.c      |   6 ++
 include/drm/drm_atomic.h            |   5 +-
 include/drm/drm_file.h              |   8 ++
 include/drm/drm_modes.h             |   9 ++
 include/drm/drm_property.h          |   2 +
 include/uapi/drm/drm.h              |   7 ++
 include/uapi/drm/drm_mode.h         |   6 ++
 16 files changed, 296 insertions(+), 57 deletions(-)

-- 
2.7.4

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

[PATCH v5 1/9] drm/modes: Introduce drm_mode_match()

[Nautiyal, Ankit K]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make mode matching less confusing by allowing the caller to specify
which parts of the modes should match via some flags.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_modes.c | 134 ++++++++++++++++++++++++++++++++++----------
 include/drm/drm_modes.h     |   9 +++
 2 files changed, 112 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index c397b52..6ca1f3b 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -940,17 +940,68 @@ struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_mode_duplicate);
 
+static bool drm_mode_match_timings(const struct drm_display_mode *mode1,
+				   const struct drm_display_mode *mode2)
+{
+	return mode1->hdisplay == mode2->hdisplay &&
+		mode1->hsync_start == mode2->hsync_start &&
+		mode1->hsync_end == mode2->hsync_end &&
+		mode1->htotal == mode2->htotal &&
+		mode1->hskew == mode2->hskew &&
+		mode1->vdisplay == mode2->vdisplay &&
+		mode1->vsync_start == mode2->vsync_start &&
+		mode1->vsync_end == mode2->vsync_end &&
+		mode1->vtotal == mode2->vtotal &&
+		mode1->vscan == mode2->vscan;
+}
+
+static bool drm_mode_match_clock(const struct drm_display_mode *mode1,
+				  const struct drm_display_mode *mode2)
+{
+	/*
+	 * do clock check convert to PICOS
+	 * so fb modes get matched the same
+	 */
+	if (mode1->clock && mode2->clock)
+		return KHZ2PICOS(mode1->clock) == KHZ2PICOS(mode2->clock);
+	else
+		return mode1->clock == mode2->clock;
+}
+
+static bool drm_mode_match_flags(const struct drm_display_mode *mode1,
+				 const struct drm_display_mode *mode2)
+{
+	return (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
+		(mode2->flags & ~DRM_MODE_FLAG_3D_MASK);
+}
+
+static bool drm_mode_match_3d_flags(const struct drm_display_mode *mode1,
+				    const struct drm_display_mode *mode2)
+{
+	return (mode1->flags & DRM_MODE_FLAG_3D_MASK) ==
+		(mode2->flags & DRM_MODE_FLAG_3D_MASK);
+}
+
+static bool drm_mode_match_aspect_ratio(const struct drm_display_mode *mode1,
+					const struct drm_display_mode *mode2)
+{
+	return mode1->picture_aspect_ratio == mode2->picture_aspect_ratio;
+}
+
 /**
- * drm_mode_equal - test modes for equality
+ * drm_mode_match - test modes for (partial) equality
  * @mode1: first mode
  * @mode2: second mode
+ * @match_flags: which parts need to match (DRM_MODE_MATCH_*)
  *
  * Check to see if @mode1 and @mode2 are equivalent.
  *
  * Returns:
- * True if the modes are equal, false otherwise.
+ * True if the modes are (partially) equal, false otherwise.
  */
-bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2)
+bool drm_mode_match(const struct drm_display_mode *mode1,
+		    const struct drm_display_mode *mode2,
+		    unsigned int match_flags)
 {
 	if (!mode1 && !mode2)
 		return true;
@@ -958,15 +1009,48 @@ bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_displ
 	if (!mode1 || !mode2)
 		return false;
 
-	/* do clock check convert to PICOS so fb modes get matched
-	 * the same */
-	if (mode1->clock && mode2->clock) {
-		if (KHZ2PICOS(mode1->clock) != KHZ2PICOS(mode2->clock))
-			return false;
-	} else if (mode1->clock != mode2->clock)
+	if (match_flags & DRM_MODE_MATCH_TIMINGS &&
+	    !drm_mode_match_timings(mode1, mode2))
+		return false;
+
+	if (match_flags & DRM_MODE_MATCH_CLOCK &&
+	    !drm_mode_match_clock(mode1, mode2))
+		return false;
+
+	if (match_flags & DRM_MODE_MATCH_FLAGS &&
+	    !drm_mode_match_flags(mode1, mode2))
+		return false;
+
+	if (match_flags & DRM_MODE_MATCH_3D_FLAGS &&
+	    !drm_mode_match_3d_flags(mode1, mode2))
 		return false;
 
-	return drm_mode_equal_no_clocks(mode1, mode2);
+	if (match_flags & DRM_MODE_MATCH_ASPECT_RATIO &&
+	    !drm_mode_match_aspect_ratio(mode1, mode2))
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL(drm_mode_match);
+
+/**
+ * drm_mode_equal - test modes for equality
+ * @mode1: first mode
+ * @mode2: second mode
+ *
+ * Check to see if @mode1 and @mode2 are equivalent.
+ *
+ * Returns:
+ * True if the modes are equal, false otherwise.
+ */
+bool drm_mode_equal(const struct drm_display_mode *mode1,
+		    const struct drm_display_mode *mode2)
+{
+	return drm_mode_match(mode1, mode2,
+			      DRM_MODE_MATCH_TIMINGS |
+			      DRM_MODE_MATCH_CLOCK |
+			      DRM_MODE_MATCH_FLAGS |
+			      DRM_MODE_MATCH_3D_FLAGS);
 }
 EXPORT_SYMBOL(drm_mode_equal);
 
@@ -981,13 +1065,13 @@ EXPORT_SYMBOL(drm_mode_equal);
  * Returns:
  * True if the modes are equal, false otherwise.
  */
-bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2)
+bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1,
+			      const struct drm_display_mode *mode2)
 {
-	if ((mode1->flags & DRM_MODE_FLAG_3D_MASK) !=
-	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
-		return false;
-
-	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
+	return drm_mode_match(mode1, mode2,
+			      DRM_MODE_MATCH_TIMINGS |
+			      DRM_MODE_MATCH_FLAGS |
+			      DRM_MODE_MATCH_3D_FLAGS);
 }
 EXPORT_SYMBOL(drm_mode_equal_no_clocks);
 
@@ -1005,21 +1089,9 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks);
 bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 					const struct drm_display_mode *mode2)
 {
-	if (mode1->hdisplay == mode2->hdisplay &&
-	    mode1->hsync_start == mode2->hsync_start &&
-	    mode1->hsync_end == mode2->hsync_end &&
-	    mode1->htotal == mode2->htotal &&
-	    mode1->hskew == mode2->hskew &&
-	    mode1->vdisplay == mode2->vdisplay &&
-	    mode1->vsync_start == mode2->vsync_start &&
-	    mode1->vsync_end == mode2->vsync_end &&
-	    mode1->vtotal == mode2->vtotal &&
-	    mode1->vscan == mode2->vscan &&
-	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
-	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
-		return true;
-
-	return false;
+	return drm_mode_match(mode1, mode2,
+			      DRM_MODE_MATCH_TIMINGS |
+			      DRM_MODE_MATCH_FLAGS);
 }
 EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
 
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 0d310be..2f78b7e 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -147,6 +147,12 @@ enum drm_mode_status {
 
 #define DRM_MODE_FLAG_3D_MAX	DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF
 
+#define DRM_MODE_MATCH_TIMINGS (1 << 0)
+#define DRM_MODE_MATCH_CLOCK (1 << 1)
+#define DRM_MODE_MATCH_FLAGS (1 << 2)
+#define DRM_MODE_MATCH_3D_FLAGS (1 << 3)
+#define DRM_MODE_MATCH_ASPECT_RATIO (1 << 4)
+
 /**
  * struct drm_display_mode - DRM kernel-internal display mode structure
  * @hdisplay: horizontal display size
@@ -490,6 +496,9 @@ void drm_mode_copy(struct drm_display_mode *dst,
 		   const struct drm_display_mode *src);
 struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev,
 					    const struct drm_display_mode *mode);
+bool drm_mode_match(const struct drm_display_mode *mode1,
+		    const struct drm_display_mode *mode2,
+		    unsigned int match_flags);
 bool drm_mode_equal(const struct drm_display_mode *mode1,
 		    const struct drm_display_mode *mode2);
 bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1,
-- 
2.7.4

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

[PATCH v5 2/9] drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy

[Nautiyal, Ankit K]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use drm_mode_equal_no_clocks_no_stereo() in
drm_match_hdmi_mode_clock_tolerance() for consistency as we
also use it in drm_match_hdmi_mode() and the cea mode matching
functions.

This doesn't actually change anything since the input mode
comes from detailed timings and we match it against
edid_4k_modes[] which. So none of those modes can have stereo
flags set.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index b1cb262..5594292 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3025,7 +3025,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_
 		    abs(to_match->clock - clock2) > clock_tolerance)
 			continue;
 
-		if (drm_mode_equal_no_clocks(to_match, hdmi_mode))
+		if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode))
 			return vic;
 	}
 
-- 
2.7.4

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

[PATCH v5 3/9] drm/edid: Fix cea mode aspect ratio handling

[Nautiyal, Ankit K]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
cause us to not send out any VICs in the AVI infoframes. That commit
was since reverted, but if and when we add aspect ratio handing back
we need to be more careful.

Let's handle this by considering the aspect ratio as a requirement
for cea mode matching only if the passed in mode actually has a
non-zero aspect ratio field. This will keep userspace that doesn't
provide an aspect ratio working as before by matching it to the
first otherwise equal cea mode. And once userspace starts to
provide the aspect ratio it will be considerd a hard requirement
for the match.

Also change the hdmi mode matching to use drm_mode_match() for
consistency, but we don't match on aspect ratio there since the
spec doesn't list a specific aspect ratio for those modes.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: "Lin, Jia" <lin.a.jia@intel.com>
Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
Cc: Jim Bride <jim.bride@linux.intel.com>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 5594292..a4d3919 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2908,11 +2908,15 @@ cea_mode_alternate_timings(u8 vic, struct drm_display_mode *mode)
 static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_match,
 					     unsigned int clock_tolerance)
 {
+	unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS;
 	u8 vic;
 
 	if (!to_match->clock)
 		return 0;
 
+	if (to_match->picture_aspect_ratio)
+		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
+
 	for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
 		struct drm_display_mode cea_mode = edid_cea_modes[vic];
 		unsigned int clock1, clock2;
@@ -2926,7 +2930,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m
 			continue;
 
 		do {
-			if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode))
+			if (drm_mode_match(to_match, &cea_mode, match_flags))
 				return vic;
 		} while (cea_mode_alternate_timings(vic, &cea_mode));
 	}
@@ -2943,11 +2947,15 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m
  */
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
 {
+	unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS;
 	u8 vic;
 
 	if (!to_match->clock)
 		return 0;
 
+	if (to_match->picture_aspect_ratio)
+		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
+
 	for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
 		struct drm_display_mode cea_mode = edid_cea_modes[vic];
 		unsigned int clock1, clock2;
@@ -2961,7 +2969,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
 			continue;
 
 		do {
-			if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode))
+			if (drm_mode_match(to_match, &cea_mode, match_flags))
 				return vic;
 		} while (cea_mode_alternate_timings(vic, &cea_mode));
 	}
@@ -3008,6 +3016,7 @@ hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode)
 static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_match,
 					      unsigned int clock_tolerance)
 {
+	unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS;
 	u8 vic;
 
 	if (!to_match->clock)
@@ -3025,7 +3034,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_
 		    abs(to_match->clock - clock2) > clock_tolerance)
 			continue;
 
-		if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode))
+		if (drm_mode_match(to_match, hdmi_mode, match_flags))
 			return vic;
 	}
 
@@ -3042,6 +3051,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_
  */
 static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
 {
+	unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS;
 	u8 vic;
 
 	if (!to_match->clock)
@@ -3057,7 +3067,7 @@ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
 
 		if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||
 		     KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) &&
-		    drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode))
+		    drm_mode_match(to_match, hdmi_mode, match_flags))
 			return vic;
 	}
 	return 0;
-- 
2.7.4

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

[PATCH v5 4/9] drm: Add DRM client cap for aspect-ratio

[Nautiyal, Ankit K]

From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

To enable aspect-ratio support in DRM, blindly exposing the aspect
ratio information along with mode, can break things in existing
user-spaces which have no intention or support to use this aspect
ratio information.

To avoid this, a new drm client cap is required to enable a
user-space to advertise if it supports modes with aspect-ratio. Based
on this cap value, the kernel will take a call on exposing the aspect
ratio info in modes or not.

This patch adds the client cap for aspect-ratio.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

V3: rebase
V4: As suggested by Marteen Lankhorst modified the commit message
    explaining the need to use the DRM cap for aspect-ratio. Also,
    tweaked the comment lines in the code for better understanding and
    clarity, as recommended by Shashank Sharma.
V5: rebase

Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_ioctl.c | 5 +++++
 include/drm/drm_file.h      | 8 ++++++++
 include/uapi/drm/drm.h      | 7 +++++++
 3 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index af78291..54a98b7 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -325,6 +325,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 		file_priv->atomic = req->value;
 		file_priv->universal_planes = req->value;
 		break;
+	case DRM_CLIENT_CAP_ASPECT_RATIO:
+		if (req->value > 1)
+			return -EINVAL;
+		file_priv->aspect_ratio_allowed = req->value;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 0e0c868..c025301 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -182,6 +182,14 @@ struct drm_file {
 	unsigned atomic:1;
 
 	/**
+	 * @aspect_ratio_allowed:
+	 *
+	 * True, if client can handle picture aspect ratios, and has requested
+	 * to pass this information along with the mode.
+	 */
+	unsigned aspect_ratio_allowed:1;
+
+	/**
 	 * @is_master:
 	 *
 	 * This client is the creator of @master. Protected by struct
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 6fdff59..9c660e1 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -680,6 +680,13 @@ struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_ATOMIC	3
 
+/**
+ * DRM_CLIENT_CAP_ASPECT_RATIO
+ *
+ * If set to 1, the DRM core will provide aspect ratio information in modes.
+ */
+#define DRM_CLIENT_CAP_ASPECT_RATIO    4
+
 /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
-- 
2.7.4

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

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

[Nautiyal, Ankit K]

From: Ankit Nautiyal <ankit.k.nautiyal@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 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@intel.com>
---
 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);
+	}
+
 	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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 6/9] drm: Handle aspect ratio info in legacy and atomic modeset paths

[Nautiyal, Ankit K]

From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

If the user-space does not support aspect-ratio, and requests for a
modeset with mode having aspect ratio bits set, then the given
user-mode must be rejected. Secondly, while preparing a user-mode from
kernel mode, the aspect-ratio info must not be given, if aspect-ratio
is not supported by the user.

This patch:
1. passes the file_priv structure from the drm_mode_atomic_ioctl till
   the drm_mode_crtc_set_mode_prop, to get the user capability.
2. rejects the modes with aspect-ratio info, during modeset, if the
   user does not support aspect ratio.
3. does not load the aspect-ratio info in user-mode structure, if
   aspect ratio is not supported.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

V3: Addressed review comments from Ville:
    Do not corrupt the current crtc state by updating aspect ratio on
    the fly.
V4: rebase
V5: As suggested by Ville, rejected the modeset calls for modes with
    aspect ratio, if the user does not set aspect ratio cap.
---
 drivers/gpu/drm/drm_atomic.c        | 30 ++++++++++++++++++++++--------
 drivers/gpu/drm/drm_atomic_helper.c |  6 +++---
 drivers/gpu/drm/drm_crtc.c          | 15 +++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h |  3 ++-
 drivers/gpu/drm/drm_mode_object.c   |  9 ++++++---
 drivers/gpu/drm/drm_modes.c         |  1 +
 include/drm/drm_atomic.h            |  5 +++--
 7 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 86b483e..7e78305 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -368,6 +368,7 @@ EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
  * drm_atomic_set_mode_prop_for_crtc - set mode for CRTC
  * @state: the CRTC whose incoming state to update
  * @blob: pointer to blob property to use for mode
+ * @file_priv: file priv structure, to get the userspace capabilities
  *
  * Set a mode (originating from a blob property) on the desired CRTC state.
  * This function will take a reference on the blob property for the CRTC state,
@@ -378,7 +379,8 @@ EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
  * Zero on success, error code on failure. Cannot return -EDEADLK.
  */
 int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
-                                      struct drm_property_blob *blob)
+				      struct drm_property_blob *blob,
+				      struct drm_file *file_priv)
 {
 	if (blob == state->mode_blob)
 		return 0;
@@ -389,10 +391,20 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
 	memset(&state->mode, 0, sizeof(state->mode));
 
 	if (blob) {
+		struct drm_mode_modeinfo *u_mode;
+
+		u_mode = (struct drm_mode_modeinfo *) blob->data;
+		if (!file_priv->aspect_ratio_allowed &&
+		    (u_mode->flags & DRM_MODE_FLAG_PIC_AR_MASK) !=
+		    DRM_MODE_FLAG_PIC_AR_NONE) {
+			DRM_DEBUG_ATOMIC("Unexpected aspect-ratio flag bits\n");
+			return -EINVAL;
+		}
+
 		if (blob->length != sizeof(struct drm_mode_modeinfo) ||
 		    drm_mode_convert_umode(state->crtc->dev, &state->mode,
-		                           (const struct drm_mode_modeinfo *)
-		                            blob->data))
+					   (const struct drm_mode_modeinfo *)
+					   u_mode))
 			return -EINVAL;
 
 		state->mode_blob = drm_property_blob_get(blob);
@@ -441,6 +453,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
  * @state: the state object to update with the new property value
  * @property: the property to set
  * @val: the new property value
+ * @file_priv: the file private structure, to get the user capabilities
  *
  * This function handles generic/core properties and calls out to driver's
  * &drm_crtc_funcs.atomic_set_property for driver properties. To ensure
@@ -452,7 +465,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
  */
 int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		struct drm_crtc_state *state, struct drm_property *property,
-		uint64_t val)
+		uint64_t val, struct drm_file *file_priv)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_mode_config *config = &dev->mode_config;
@@ -465,7 +478,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		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);
+		ret = drm_atomic_set_mode_prop_for_crtc(state, mode, file_priv);
 		drm_property_blob_put(mode);
 		return ret;
 	} else if (property == config->degamma_lut_property) {
@@ -1918,7 +1931,8 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
 int drm_atomic_set_property(struct drm_atomic_state *state,
 			    struct drm_mode_object *obj,
 			    struct drm_property *prop,
-			    uint64_t prop_value)
+			    uint64_t prop_value,
+			    struct drm_file *file_priv)
 {
 	struct drm_mode_object *ref;
 	int ret;
@@ -1952,7 +1966,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 		}
 
 		ret = drm_atomic_crtc_set_property(crtc,
-				crtc_state, prop, prop_value);
+				crtc_state, prop, prop_value, file_priv);
 		break;
 	}
 	case DRM_MODE_OBJECT_PLANE: {
@@ -2341,7 +2355,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 			}
 
 			ret = drm_atomic_set_property(state, obj, prop,
-						      prop_value);
+						      prop_value, file_priv);
 			if (ret) {
 				drm_mode_object_put(obj);
 				goto out;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ae3cbfe..781ebd6 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -186,7 +186,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 
 		if (!crtc_state->connector_mask) {
 			ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
-								NULL);
+								NULL, NULL);
 			if (ret < 0)
 				goto out;
 
@@ -2749,7 +2749,7 @@ static int update_output_state(struct drm_atomic_state *state,
 
 		if (!new_crtc_state->connector_mask) {
 			ret = drm_atomic_set_mode_prop_for_crtc(new_crtc_state,
-								NULL);
+								NULL, NULL);
 			if (ret < 0)
 				return ret;
 
@@ -2930,7 +2930,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 
 		crtc_state->active = false;
 
-		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL);
+		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL, NULL);
 		if (ret < 0)
 			goto free;
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 353e24f..e36a971 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -430,6 +430,9 @@ int drm_mode_getcrtc(struct drm_device *dev,
 	if (crtc->state) {
 		if (crtc->state->enable) {
 			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
+			if (!file_priv->aspect_ratio_allowed)
+				crtc->state->mode.flags &=
+				(~DRM_MODE_FLAG_PIC_AR_MASK);
 			crtc_resp->mode_valid = 1;
 
 		} else {
@@ -440,6 +443,9 @@ int drm_mode_getcrtc(struct drm_device *dev,
 		crtc_resp->y = crtc->y;
 		if (crtc->enabled) {
 			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
+			if (!file_priv->aspect_ratio_allowed)
+				crtc->mode.flags &=
+				(~DRM_MODE_FLAG_PIC_AR_MASK);
 			crtc_resp->mode_valid = 1;
 
 		} else {
@@ -614,6 +620,15 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			goto out;
 		}
 
+		if (!file_priv->aspect_ratio_allowed &&
+		    (crtc_req->mode.flags & DRM_MODE_FLAG_PIC_AR_MASK) !=
+		    DRM_MODE_FLAG_PIC_AR_NONE) {
+			DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
+
 		ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
 		if (ret) {
 			DRM_DEBUG_KMS("Invalid mode\n");
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index af00f42..7c3338f 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -186,7 +186,8 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
 int drm_atomic_set_property(struct drm_atomic_state *state,
 			    struct drm_mode_object *obj,
 			    struct drm_property *prop,
-			    uint64_t prop_value);
+			    uint64_t prop_value,
+			    struct drm_file *file_priv);
 int drm_atomic_get_property(struct drm_mode_object *obj,
 			    struct drm_property *property, uint64_t *val);
 int drm_mode_atomic_ioctl(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index ce4d2fb..1c3d498 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -452,7 +452,8 @@ static int set_property_legacy(struct drm_mode_object *obj,
 
 static int set_property_atomic(struct drm_mode_object *obj,
 			       struct drm_property *prop,
-			       uint64_t prop_value)
+			       uint64_t prop_value,
+			       struct drm_file *file_priv)
 {
 	struct drm_device *dev = prop->dev;
 	struct drm_atomic_state *state;
@@ -476,7 +477,8 @@ static int set_property_atomic(struct drm_mode_object *obj,
 						       obj_to_connector(obj),
 						       prop_value);
 	} else {
-		ret = drm_atomic_set_property(state, obj, prop, prop_value);
+		ret = drm_atomic_set_property(state, obj, prop, prop_value,
+					      file_priv);
 		if (ret)
 			goto out;
 		ret = drm_atomic_commit(state);
@@ -519,7 +521,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 		goto out_unref;
 
 	if (drm_drv_uses_atomic_modeset(property->dev))
-		ret = set_property_atomic(arg_obj, property, arg->value);
+		ret = set_property_atomic(arg_obj, property, arg->value,
+					  file_priv);
 	else
 		ret = set_property_legacy(arg_obj, property, arg->value);
 
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 6ca1f3b..ca4c5cc 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1620,6 +1620,7 @@ EXPORT_SYMBOL(drm_mode_create_from_cmdline_mode);
  * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
  * @out: drm_mode_modeinfo struct to return to the user
  * @in: drm_display_mode to use
+ * @file_priv: file_priv structure, to get the user capabilities
  *
  * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to
  * the user.
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index cf13842..d3ad5d1 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -367,7 +367,7 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 			  struct drm_crtc *crtc);
 int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		struct drm_crtc_state *state, struct drm_property *property,
-		uint64_t val);
+		uint64_t val, struct drm_file *file_priv);
 struct drm_plane_state * __must_check
 drm_atomic_get_plane_state(struct drm_atomic_state *state,
 			   struct drm_plane *plane);
@@ -583,7 +583,8 @@ drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
 			     const struct drm_display_mode *mode);
 int __must_check
 drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
-				  struct drm_property_blob *blob);
+				  struct drm_property_blob *blob,
+				  struct drm_file *file_priv);
 int __must_check
 drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
 			      struct drm_crtc *crtc);
-- 
2.7.4

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

[PATCH v5 7/9] drm: Expose modes with aspect ratio, only if requested

[Nautiyal, Ankit K]

From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

We parse the EDID and add all the modes in the connector's modelist.
This adds CEA modes with aspect ratio information too, regadless of
whether user space requested this information or not.

This patch prunes the modes with aspect-ratio information, from a
connector's modelist, if the user-space has not set the aspect ratio
DRM client cap.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Jose Abreu <jose.abreu@synopsys.com>

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

V3: As suggested by Ville, modified the mechanism of pruning of modes
    with aspect-ratio, if the aspect-ratio is not supported. Instead
    of straight away pruning such a mode, the mode is retained with
    aspect ratio bits set to zero, provided it is unique.
V4: rebase
V5: Addressed review comments from Ville:
    -used a pointer to store last valid mode.
    -avoided, modifying of picture_aspect_ratio in kernel mode,
     instead only flags bits of user mode are reset (if aspect-ratio
     is not supported).
---
 drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 16b9c38..49778e8 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1507,8 +1507,10 @@ static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *conne
 	return connector->encoder;
 }
 
-static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
-					 const struct drm_file *file_priv)
+static bool
+drm_mode_expose_to_userspace(const struct drm_display_mode *last_mode,
+			     const struct drm_display_mode *mode,
+			     const struct drm_file *file_priv)
 {
 	/*
 	 * If user-space hasn't configured the driver to expose the stereo 3D
@@ -1516,6 +1518,23 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
 	 */
 	if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode))
 		return false;
+	/*
+	 * If user-space hasn't configured the driver to expose the modes
+	 * with aspect-ratio, don't expose them. But in case of a unique
+	 * mode, let the mode be passed, so that it can be enumerated with
+	 * aspect ratio bits erased.
+	 */
+	if (!file_priv->aspect_ratio_allowed &&
+	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
+		if (last_mode && !drm_mode_match(mode, last_mode,
+						 DRM_MODE_MATCH_TIMINGS |
+						 DRM_MODE_MATCH_CLOCK |
+						 DRM_MODE_MATCH_FLAGS |
+						 DRM_MODE_MATCH_3D_FLAGS))
+			return true;
+		else
+			return false;
+	}
 
 	return true;
 }
@@ -1527,6 +1546,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
 	struct drm_display_mode *mode;
+	struct drm_display_mode *last_valid_mode;
 	int mode_count = 0;
 	int encoders_count = 0;
 	int ret = 0;
@@ -1582,9 +1602,13 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	out_resp->connection = connector->status;
 
 	/* delayed so we get modes regardless of pre-fill_modes state */
+	last_valid_mode = NULL;
 	list_for_each_entry(mode, &connector->modes, head)
-		if (drm_mode_expose_to_userspace(mode, file_priv))
+		if (drm_mode_expose_to_userspace(last_valid_mode, mode,
+						 file_priv)) {
 			mode_count++;
+			last_valid_mode = mode;
+		}
 
 	/*
 	 * This ioctl is called twice, once to determine how much space is
@@ -1593,11 +1617,21 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	if ((out_resp->count_modes >= mode_count) && mode_count) {
 		copied = 0;
 		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
+		last_valid_mode = NULL;
 		list_for_each_entry(mode, &connector->modes, head) {
-			if (!drm_mode_expose_to_userspace(mode, file_priv))
+			if (!drm_mode_expose_to_userspace(last_valid_mode,
+							  mode,
+							  file_priv))
 				continue;
-
 			drm_mode_convert_to_umode(&u_mode, mode);
+
+			/*
+			 * Reset the aspect-ratio flag bits from the user mode,
+			 * if the user mode does have aspect ratio capability.
+			 */
+			if (!file_priv->aspect_ratio_allowed)
+				u_mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
+
 			if (copy_to_user(mode_ptr + copied,
 					 &u_mode, sizeof(u_mode))) {
 				ret = -EFAULT;
@@ -1605,6 +1639,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 
 				goto out;
 			}
+			last_valid_mode = mode;
 			copied++;
 		}
 	}
-- 
2.7.4

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

[PATCH v5 8/9] drm: Add aspect ratio parsing in DRM layer

[Nautiyal, Ankit K]

From: "Sharma, Shashank" <shashank.sharma@intel.com>

Current DRM layer functions don't parse aspect ratio information
while converting a user mode->kernel mode or vice versa. This
causes modeset to pick mode with wrong aspect ratio, eventually
causing failures in HDMI compliance test cases, due to wrong VIC.

This patch adds aspect ratio information in DRM's mode conversion
and mode comparision functions, to make sure kernel picks mode
with right aspect ratio (as per the VIC).

Background:
This patch was once reviewed and merged, and later reverted due to
lack of DRM cap protection. This is a re-spin of this patch, this
time with DRM cap protection, to avoid aspect ratio information, when
the client doesn't request for it.

Review link: https://pw-emeril.freedesktop.org/patch/104068/
Background discussion: https://patchwork.kernel.org/patch/9379057/

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Lin, Jia <lin.a.jia@intel.com>
Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
Reviewed-by: Jim Bride <jim.bride@linux.intel.com> (V2)
Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> (V4)

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Jim Bride <jim.bride@linux.intel.com>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

V3: modified the aspect-ratio check in drm_mode_equal as per new flags
    provided by Ville. https://patchwork.freedesktop.org/patch/188043/
V4: rebase
V5: rebase
---
 drivers/gpu/drm/drm_modes.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index ca4c5cc..25140b9 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1050,7 +1050,8 @@ bool drm_mode_equal(const struct drm_display_mode *mode1,
 			      DRM_MODE_MATCH_TIMINGS |
 			      DRM_MODE_MATCH_CLOCK |
 			      DRM_MODE_MATCH_FLAGS |
-			      DRM_MODE_MATCH_3D_FLAGS);
+			      DRM_MODE_MATCH_3D_FLAGS|
+			      DRM_MODE_MATCH_ASPECT_RATIO);
 }
 EXPORT_SYMBOL(drm_mode_equal);
 
@@ -1649,6 +1650,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 	out->vrefresh = in->vrefresh;
 	out->flags = in->flags;
 	out->type = in->type;
+	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
+
+	switch (in->picture_aspect_ratio) {
+	case HDMI_PICTURE_ASPECT_4_3:
+		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
+		break;
+	case HDMI_PICTURE_ASPECT_16_9:
+		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
+		break;
+	case HDMI_PICTURE_ASPECT_RESERVED:
+	default:
+		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
+		break;
+	}
+
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 }
@@ -1693,6 +1709,21 @@ int drm_mode_convert_umode(struct drm_device *dev,
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 
+	/* Clearing picture aspect ratio bits from out flags */
+	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
+
+	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
+	case DRM_MODE_FLAG_PIC_AR_4_3:
+		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
+		break;
+	case DRM_MODE_FLAG_PIC_AR_16_9:
+		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
+		break;
+	default:
+		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+		break;
+	}
+
 	out->status = drm_mode_validate_driver(dev, out);
 	if (out->status != MODE_OK)
 		goto out;
-- 
2.7.4

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

[PATCH v5 9/9] drm: Add and handle new aspect ratios in DRM layer

[Nautiyal, Ankit K]

From: "Sharma, Shashank" <shashank.sharma@intel.com>

HDMI 2.0/CEA-861-F introduces two new aspect ratios:
- 64:27
- 256:135

This patch:
-  Adds new DRM flags for to represent these new aspect ratios.
-  Adds new cases to handle these aspect ratios while converting
from user->kernel mode or vise versa.

This patch was once reviewed and merged, and later reverted due
to lack of DRM client protection, while adding aspect ratio bits
in user modes. This is a re-spin of the series, with DRM client
cap protection.

The previous series can be found here:
https://pw-emeril.freedesktop.org/series/10850/

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Sean Paul <seanpaul@chromium.org> (V2)
Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> (V2)

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

V3: rebase
V4: rebase
V5: corrected the macro name for an aspect ratio, in a switch case.
---
 drivers/gpu/drm/drm_modes.c | 12 ++++++++++++
 include/uapi/drm/drm_mode.h |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 25140b9..ad907e6 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1659,6 +1659,12 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 	case HDMI_PICTURE_ASPECT_16_9:
 		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
 		break;
+	case HDMI_PICTURE_ASPECT_64_27:
+		out->flags |= DRM_MODE_FLAG_PIC_AR_64_27;
+		break;
+	case HDMI_PICTURE_ASPECT_256_135:
+		out->flags |= DRM_MODE_FLAG_PIC_AR_256_135;
+		break;
 	case HDMI_PICTURE_ASPECT_RESERVED:
 	default:
 		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
@@ -1719,6 +1725,12 @@ int drm_mode_convert_umode(struct drm_device *dev,
 	case DRM_MODE_FLAG_PIC_AR_16_9:
 		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
 		break;
+	case DRM_MODE_FLAG_PIC_AR_64_27:
+		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
+		break;
+	case DRM_MODE_FLAG_PIC_AR_256_135:
+		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
+		break;
 	default:
 		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
 		break;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 2c57579..93c27cd 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -93,6 +93,8 @@ extern "C" {
 #define DRM_MODE_PICTURE_ASPECT_NONE		0
 #define DRM_MODE_PICTURE_ASPECT_4_3		1
 #define DRM_MODE_PICTURE_ASPECT_16_9		2
+#define DRM_MODE_PICTURE_ASPECT_64_27		3
+#define DRM_MODE_PICTURE_ASPECT_256_135		4
 
 /* Aspect ratio flag bitmask (4 bits 22:19) */
 #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
@@ -102,6 +104,10 @@ extern "C" {
 			(DRM_MODE_PICTURE_ASPECT_4_3<<19)
 #define  DRM_MODE_FLAG_PIC_AR_16_9 \
 			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
+#define  DRM_MODE_FLAG_PIC_AR_64_27 \
+			(DRM_MODE_PICTURE_ASPECT_64_27<<19)
+#define  DRM_MODE_FLAG_PIC_AR_256_135 \
+			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
 
 #define  DRM_MODE_FLAG_ALL	(DRM_MODE_FLAG_PHSYNC |		\
 				 DRM_MODE_FLAG_NHSYNC |		\
-- 
2.7.4

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

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

[Ville Syrjälä]

On Thu, Feb 15, 2018 at 05:50:58PM +0530, Nautiyal, Ankit K wrote:
> From: Ankit Nautiyal <ankit.k.nautiyal@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 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@intel.com>
> ---
>  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.

> +	}
> +
>  	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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 6/9] drm: Handle aspect ratio info in legacy and atomic modeset paths

[Ville Syrjälä]

On Thu, Feb 15, 2018 at 05:50:59PM +0530, Nautiyal, Ankit K wrote:
> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> If the user-space does not support aspect-ratio, and requests for a
> modeset with mode having aspect ratio bits set, then the given
> user-mode must be rejected. Secondly, while preparing a user-mode from
> kernel mode, the aspect-ratio info must not be given, if aspect-ratio
> is not supported by the user.
> 
> This patch:
> 1. passes the file_priv structure from the drm_mode_atomic_ioctl till
>    the drm_mode_crtc_set_mode_prop, to get the user capability.
> 2. rejects the modes with aspect-ratio info, during modeset, if the
>    user does not support aspect ratio.
> 3. does not load the aspect-ratio info in user-mode structure, if
>    aspect ratio is not supported.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> V3: Addressed review comments from Ville:
>     Do not corrupt the current crtc state by updating aspect ratio on
>     the fly.
> V4: rebase
> V5: As suggested by Ville, rejected the modeset calls for modes with
>     aspect ratio, if the user does not set aspect ratio cap.
> ---
>  drivers/gpu/drm/drm_atomic.c        | 30 ++++++++++++++++++++++--------
>  drivers/gpu/drm/drm_atomic_helper.c |  6 +++---
>  drivers/gpu/drm/drm_crtc.c          | 15 +++++++++++++++
>  drivers/gpu/drm/drm_crtc_internal.h |  3 ++-
>  drivers/gpu/drm/drm_mode_object.c   |  9 ++++++---
>  drivers/gpu/drm/drm_modes.c         |  1 +
>  include/drm/drm_atomic.h            |  5 +++--
>  7 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 86b483e..7e78305 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -368,6 +368,7 @@ EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
>   * drm_atomic_set_mode_prop_for_crtc - set mode for CRTC
>   * @state: the CRTC whose incoming state to update
>   * @blob: pointer to blob property to use for mode
> + * @file_priv: file priv structure, to get the userspace capabilities
>   *
>   * Set a mode (originating from a blob property) on the desired CRTC state.
>   * This function will take a reference on the blob property for the CRTC state,
> @@ -378,7 +379,8 @@ EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
>   * Zero on success, error code on failure. Cannot return -EDEADLK.
>   */
>  int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
> -                                      struct drm_property_blob *blob)
> +				      struct drm_property_blob *blob,
> +				      struct drm_file *file_priv)
>  {
>  	if (blob == state->mode_blob)
>  		return 0;
> @@ -389,10 +391,20 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
>  	memset(&state->mode, 0, sizeof(state->mode));
>  
>  	if (blob) {
> +		struct drm_mode_modeinfo *u_mode;
> +
> +		u_mode = (struct drm_mode_modeinfo *) blob->data;
> +		if (!file_priv->aspect_ratio_allowed &&
> +		    (u_mode->flags & DRM_MODE_FLAG_PIC_AR_MASK) !=
> +		    DRM_MODE_FLAG_PIC_AR_NONE) {
> +			DRM_DEBUG_ATOMIC("Unexpected aspect-ratio flag bits\n");
> +			return -EINVAL;
> +		}

We should probably pull this logic into a small helper so that we don't
have to duplicate it in both the setprop and setcrtc code paths.

> +
>  		if (blob->length != sizeof(struct drm_mode_modeinfo) ||

The blob length check has to be done before you access the data.

>  		    drm_mode_convert_umode(state->crtc->dev, &state->mode,
> -		                           (const struct drm_mode_modeinfo *)
> -		                            blob->data))
> +					   (const struct drm_mode_modeinfo *)
> +					   u_mode))
>  			return -EINVAL;
>  
>  		state->mode_blob = drm_property_blob_get(blob);
> @@ -441,6 +453,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>   * @state: the state object to update with the new property value
>   * @property: the property to set
>   * @val: the new property value
> + * @file_priv: the file private structure, to get the user capabilities
>   *
>   * This function handles generic/core properties and calls out to driver's
>   * &drm_crtc_funcs.atomic_set_property for driver properties. To ensure
> @@ -452,7 +465,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>   */
>  int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		struct drm_crtc_state *state, struct drm_property *property,
> -		uint64_t val)
> +		uint64_t val, struct drm_file *file_priv)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> @@ -465,7 +478,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		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);
> +		ret = drm_atomic_set_mode_prop_for_crtc(state, mode, file_priv);
>  		drm_property_blob_put(mode);
>  		return ret;
>  	} else if (property == config->degamma_lut_property) {
> @@ -1918,7 +1931,8 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
>  int drm_atomic_set_property(struct drm_atomic_state *state,
>  			    struct drm_mode_object *obj,
>  			    struct drm_property *prop,
> -			    uint64_t prop_value)
> +			    uint64_t prop_value,
> +			    struct drm_file *file_priv)
>  {
>  	struct drm_mode_object *ref;
>  	int ret;
> @@ -1952,7 +1966,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>  		}
>  
>  		ret = drm_atomic_crtc_set_property(crtc,
> -				crtc_state, prop, prop_value);
> +				crtc_state, prop, prop_value, file_priv);
>  		break;
>  	}
>  	case DRM_MODE_OBJECT_PLANE: {
> @@ -2341,7 +2355,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			}
>  
>  			ret = drm_atomic_set_property(state, obj, prop,
> -						      prop_value);
> +						      prop_value, file_priv);
>  			if (ret) {
>  				drm_mode_object_put(obj);
>  				goto out;
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ae3cbfe..781ebd6 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -186,7 +186,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
>  
>  		if (!crtc_state->connector_mask) {
>  			ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
> -								NULL);
> +								NULL, NULL);
>  			if (ret < 0)
>  				goto out;
>  
> @@ -2749,7 +2749,7 @@ static int update_output_state(struct drm_atomic_state *state,
>  
>  		if (!new_crtc_state->connector_mask) {
>  			ret = drm_atomic_set_mode_prop_for_crtc(new_crtc_state,
> -								NULL);
> +								NULL, NULL);
>  			if (ret < 0)
>  				return ret;
>  
> @@ -2930,7 +2930,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>  
>  		crtc_state->active = false;
>  
> -		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL);
> +		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL, NULL);
>  		if (ret < 0)
>  			goto free;
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 353e24f..e36a971 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -430,6 +430,9 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  	if (crtc->state) {
>  		if (crtc->state->enable) {
>  			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
> +			if (!file_priv->aspect_ratio_allowed)
> +				crtc->state->mode.flags &=
> +				(~DRM_MODE_FLAG_PIC_AR_MASK);

Should be manling crtc_resp->mode instead of the internal mode.

>  			crtc_resp->mode_valid = 1;
>  
>  		} else {
> @@ -440,6 +443,9 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  		crtc_resp->y = crtc->y;
>  		if (crtc->enabled) {
>  			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
> +			if (!file_priv->aspect_ratio_allowed)
> +				crtc->mode.flags &=
> +				(~DRM_MODE_FLAG_PIC_AR_MASK);

ditto

Should probably pull this flag filtering logic into a small helper
as well.

>  			crtc_resp->mode_valid = 1;
>  
>  		} else {
> @@ -614,6 +620,15 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			goto out;
>  		}
>  
> +		if (!file_priv->aspect_ratio_allowed &&
> +		    (crtc_req->mode.flags & DRM_MODE_FLAG_PIC_AR_MASK) !=
> +		    DRM_MODE_FLAG_PIC_AR_NONE) {
> +			DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +
>  		ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
>  		if (ret) {
>  			DRM_DEBUG_KMS("Invalid mode\n");
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index af00f42..7c3338f 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -186,7 +186,8 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
>  int drm_atomic_set_property(struct drm_atomic_state *state,
>  			    struct drm_mode_object *obj,
>  			    struct drm_property *prop,
> -			    uint64_t prop_value);
> +			    uint64_t prop_value,
> +			    struct drm_file *file_priv);
>  int drm_atomic_get_property(struct drm_mode_object *obj,
>  			    struct drm_property *property, uint64_t *val);
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index ce4d2fb..1c3d498 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -452,7 +452,8 @@ static int set_property_legacy(struct drm_mode_object *obj,
>  
>  static int set_property_atomic(struct drm_mode_object *obj,
>  			       struct drm_property *prop,
> -			       uint64_t prop_value)
> +			       uint64_t prop_value,
> +			       struct drm_file *file_priv)
>  {
>  	struct drm_device *dev = prop->dev;
>  	struct drm_atomic_state *state;
> @@ -476,7 +477,8 @@ static int set_property_atomic(struct drm_mode_object *obj,
>  						       obj_to_connector(obj),
>  						       prop_value);
>  	} else {
> -		ret = drm_atomic_set_property(state, obj, prop, prop_value);
> +		ret = drm_atomic_set_property(state, obj, prop, prop_value,
> +					      file_priv);
>  		if (ret)
>  			goto out;
>  		ret = drm_atomic_commit(state);
> @@ -519,7 +521,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  		goto out_unref;
>  
>  	if (drm_drv_uses_atomic_modeset(property->dev))
> -		ret = set_property_atomic(arg_obj, property, arg->value);
> +		ret = set_property_atomic(arg_obj, property, arg->value,
> +					  file_priv);
>  	else
>  		ret = set_property_legacy(arg_obj, property, arg->value);
>  
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 6ca1f3b..ca4c5cc 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1620,6 +1620,7 @@ EXPORT_SYMBOL(drm_mode_create_from_cmdline_mode);
>   * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
>   * @out: drm_mode_modeinfo struct to return to the user
>   * @in: drm_display_mode to use
> + * @file_priv: file_priv structure, to get the user capabilities
>   *
>   * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to
>   * the user.
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index cf13842..d3ad5d1 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -367,7 +367,7 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  			  struct drm_crtc *crtc);
>  int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		struct drm_crtc_state *state, struct drm_property *property,
> -		uint64_t val);
> +		uint64_t val, struct drm_file *file_priv);
>  struct drm_plane_state * __must_check
>  drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  			   struct drm_plane *plane);
> @@ -583,7 +583,8 @@ drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  			     const struct drm_display_mode *mode);
>  int __must_check
>  drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
> -				  struct drm_property_blob *blob);
> +				  struct drm_property_blob *blob,
> +				  struct drm_file *file_priv);
>  int __must_check
>  drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>  			      struct drm_crtc *crtc);
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 7/9] drm: Expose modes with aspect ratio, only if requested

[Ville Syrjälä]

On Thu, Feb 15, 2018 at 05:51:00PM +0530, Nautiyal, Ankit K wrote:
> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> We parse the EDID and add all the modes in the connector's modelist.
> This adds CEA modes with aspect ratio information too, regadless of
> whether user space requested this information or not.
> 
> This patch prunes the modes with aspect-ratio information, from a
> connector's modelist, if the user-space has not set the aspect ratio
> DRM client cap.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Jose Abreu <jose.abreu@synopsys.com>
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> V3: As suggested by Ville, modified the mechanism of pruning of modes
>     with aspect-ratio, if the aspect-ratio is not supported. Instead
>     of straight away pruning such a mode, the mode is retained with
>     aspect ratio bits set to zero, provided it is unique.
> V4: rebase
> V5: Addressed review comments from Ville:
>     -used a pointer to store last valid mode.
>     -avoided, modifying of picture_aspect_ratio in kernel mode,
>      instead only flags bits of user mode are reset (if aspect-ratio
>      is not supported).
> ---
>  drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 16b9c38..49778e8 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1507,8 +1507,10 @@ static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *conne
>  	return connector->encoder;
>  }
>  
> -static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
> -					 const struct drm_file *file_priv)
> +static bool
> +drm_mode_expose_to_userspace(const struct drm_display_mode *last_mode,
> +			     const struct drm_display_mode *mode,

I would put the 'mode' argument first since that's the "main" object
we're operating on.

> +			     const struct drm_file *file_priv)
>  {
>  	/*
>  	 * If user-space hasn't configured the driver to expose the stereo 3D
> @@ -1516,6 +1518,23 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
>  	 */
>  	if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode))
>  		return false;
> +	/*
> +	 * If user-space hasn't configured the driver to expose the modes
> +	 * with aspect-ratio, don't expose them. But in case of a unique
> +	 * mode, let the mode be passed, so that it can be enumerated with
> +	 * aspect ratio bits erased.

Might want to note that we expect the list to be sorted such that modes
that have different aspect ratio but are otherwise identical are back to
back.

> +	 */
> +	if (!file_priv->aspect_ratio_allowed &&
> +	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
> +		if (last_mode && !drm_mode_match(mode, last_mode,
> +						 DRM_MODE_MATCH_TIMINGS |
> +						 DRM_MODE_MATCH_CLOCK |
> +						 DRM_MODE_MATCH_FLAGS |
> +						 DRM_MODE_MATCH_3D_FLAGS))
> +			return true;
> +		else
> +			return false;

How does that even work? It'll return false as long as
last_mode==NULL, and last_mode never becomes non-NULL unless
we return true.

To me it looks like the correct logic would be:
if (last_mode && drm_mode_match(...))
	return false;

> +	}
>  
>  	return true;
>  }
> @@ -1527,6 +1546,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	struct drm_connector *connector;
>  	struct drm_encoder *encoder;
>  	struct drm_display_mode *mode;
> +	struct drm_display_mode *last_valid_mode;
>  	int mode_count = 0;
>  	int encoders_count = 0;
>  	int ret = 0;
> @@ -1582,9 +1602,13 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	out_resp->connection = connector->status;
>  
>  	/* delayed so we get modes regardless of pre-fill_modes state */
> +	last_valid_mode = NULL;
>  	list_for_each_entry(mode, &connector->modes, head)
> -		if (drm_mode_expose_to_userspace(mode, file_priv))
> +		if (drm_mode_expose_to_userspace(last_valid_mode, mode,
> +						 file_priv)) {
>  			mode_count++;
> +			last_valid_mode = mode;
> +		}
>  
>  	/*
>  	 * This ioctl is called twice, once to determine how much space is
> @@ -1593,11 +1617,21 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	if ((out_resp->count_modes >= mode_count) && mode_count) {
>  		copied = 0;
>  		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
> +		last_valid_mode = NULL;
>  		list_for_each_entry(mode, &connector->modes, head) {
> -			if (!drm_mode_expose_to_userspace(mode, file_priv))
> +			if (!drm_mode_expose_to_userspace(last_valid_mode,
> +							  mode,
> +							  file_priv))
>  				continue;
> -
>  			drm_mode_convert_to_umode(&u_mode, mode);
> +
> +			/*
> +			 * Reset the aspect-ratio flag bits from the user mode,
> +			 * if the user mode does have aspect ratio capability.
> +			 */
> +			if (!file_priv->aspect_ratio_allowed)
> +				u_mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);

Useless parens. Could use the same helper I suggested we add for
getcrtc() and getblob().

> +
>  			if (copy_to_user(mode_ptr + copied,
>  					 &u_mode, sizeof(u_mode))) {
>  				ret = -EFAULT;
> @@ -1605,6 +1639,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  
>  				goto out;
>  			}
> +			last_valid_mode = mode;
>  			copied++;
>  		}
>  	}
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 8/9] drm: Add aspect ratio parsing in DRM layer

[Ville Syrjälä]

On Thu, Feb 15, 2018 at 05:51:01PM +0530, Nautiyal, Ankit K wrote:
> From: "Sharma, Shashank" <shashank.sharma@intel.com>
> 
> Current DRM layer functions don't parse aspect ratio information
> while converting a user mode->kernel mode or vice versa. This
> causes modeset to pick mode with wrong aspect ratio, eventually
> causing failures in HDMI compliance test cases, due to wrong VIC.
> 
> This patch adds aspect ratio information in DRM's mode conversion
> and mode comparision functions, to make sure kernel picks mode
> with right aspect ratio (as per the VIC).
> 
> Background:
> This patch was once reviewed and merged, and later reverted due to
> lack of DRM cap protection. This is a re-spin of this patch, this
> time with DRM cap protection, to avoid aspect ratio information, when
> the client doesn't request for it.
> 
> Review link: https://pw-emeril.freedesktop.org/patch/104068/
> Background discussion: https://patchwork.kernel.org/patch/9379057/
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Lin, Jia <lin.a.jia@intel.com>
> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
> Reviewed-by: Jim Bride <jim.bride@linux.intel.com> (V2)
> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> (V4)
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Jim Bride <jim.bride@linux.intel.com>
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> V3: modified the aspect-ratio check in drm_mode_equal as per new flags
>     provided by Ville. https://patchwork.freedesktop.org/patch/188043/
> V4: rebase
> V5: rebase
> ---
>  drivers/gpu/drm/drm_modes.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ca4c5cc..25140b9 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1050,7 +1050,8 @@ bool drm_mode_equal(const struct drm_display_mode *mode1,
>  			      DRM_MODE_MATCH_TIMINGS |
>  			      DRM_MODE_MATCH_CLOCK |
>  			      DRM_MODE_MATCH_FLAGS |
> -			      DRM_MODE_MATCH_3D_FLAGS);
> +			      DRM_MODE_MATCH_3D_FLAGS|
> +			      DRM_MODE_MATCH_ASPECT_RATIO);

Hmm. I wonder if all the users are expecting this. Based on a cursory
examination drm_fb_helper might want to ignore the aspect ratio since
it's just looking to clone the same mode on multiple outputs. The other
cases don't look to me like they should suffer badly from this.

>  }
>  EXPORT_SYMBOL(drm_mode_equal);
>  
> @@ -1649,6 +1650,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>  	out->vrefresh = in->vrefresh;
>  	out->flags = in->flags;
>  	out->type = in->type;
> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;

This looks redundant. The internal mode should not have the aspect ratio
flags set. Or are we changing that?

> +
> +	switch (in->picture_aspect_ratio) {
> +	case HDMI_PICTURE_ASPECT_4_3:
> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
> +		break;
> +	case HDMI_PICTURE_ASPECT_16_9:
> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> +		break;
> +	case HDMI_PICTURE_ASPECT_RESERVED:
> +	default:
> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
> +		break;
> +	}
> +
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  }
> @@ -1693,6 +1709,21 @@ int drm_mode_convert_umode(struct drm_device *dev,
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  
> +	/* Clearing picture aspect ratio bits from out flags */

What the code is doing is obvious so this comment is redundant.
The non-obvious part (ie. the "why?") is what the comment
should contain.

> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> +
> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
> +	case DRM_MODE_FLAG_PIC_AR_4_3:
> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
> +		break;
> +	case DRM_MODE_FLAG_PIC_AR_16_9:
> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> +		break;
> +	default:
> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> +		break;
> +	}
> +
>  	out->status = drm_mode_validate_driver(dev, out);
>  	if (out->status != MODE_OK)
>  		goto out;
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Nautiyal, Ankit K]

[-- Attachment #1.1: Type: text/plain, Size: 4050 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 4968 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v5 6/9] drm: Handle aspect ratio info in legacy and atomic modeset paths

[Nautiyal, Ankit K]

[-- Attachment #1.1: Type: text/plain, Size: 13141 bytes --]

Hi Ville,

I agree to all the comments here, and will correct the required things 
in the next patch-set.

On 2/23/2018 7:58 PM, Ville Syrjälä wrote:
> On Thu, Feb 15, 2018 at 05:50:59PM +0530, Nautiyal, Ankit K wrote:
>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> If the user-space does not support aspect-ratio, and requests for a
>> modeset with mode having aspect ratio bits set, then the given
>> user-mode must be rejected. Secondly, while preparing a user-mode from
>> kernel mode, the aspect-ratio info must not be given, if aspect-ratio
>> is not supported by the user.
>>
>> This patch:
>> 1. passes the file_priv structure from the drm_mode_atomic_ioctl till
>>     the drm_mode_crtc_set_mode_prop, to get the user capability.
>> 2. rejects the modes with aspect-ratio info, during modeset, if the
>>     user does not support aspect ratio.
>> 3. does not load the aspect-ratio info in user-mode structure, if
>>     aspect ratio is not supported.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> V3: Addressed review comments from Ville:
>>      Do not corrupt the current crtc state by updating aspect ratio on
>>      the fly.
>> V4: rebase
>> V5: As suggested by Ville, rejected the modeset calls for modes with
>>      aspect ratio, if the user does not set aspect ratio cap.
>> ---
>>   drivers/gpu/drm/drm_atomic.c        | 30 ++++++++++++++++++++++--------
>>   drivers/gpu/drm/drm_atomic_helper.c |  6 +++---
>>   drivers/gpu/drm/drm_crtc.c          | 15 +++++++++++++++
>>   drivers/gpu/drm/drm_crtc_internal.h |  3 ++-
>>   drivers/gpu/drm/drm_mode_object.c   |  9 ++++++---
>>   drivers/gpu/drm/drm_modes.c         |  1 +
>>   include/drm/drm_atomic.h            |  5 +++--
>>   7 files changed, 52 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 86b483e..7e78305 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -368,6 +368,7 @@ EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
>>    * drm_atomic_set_mode_prop_for_crtc - set mode for CRTC
>>    * @state: the CRTC whose incoming state to update
>>    * @blob: pointer to blob property to use for mode
>> + * @file_priv: file priv structure, to get the userspace capabilities
>>    *
>>    * Set a mode (originating from a blob property) on the desired CRTC state.
>>    * This function will take a reference on the blob property for the CRTC state,
>> @@ -378,7 +379,8 @@ EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
>>    * Zero on success, error code on failure. Cannot return -EDEADLK.
>>    */
>>   int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
>> -                                      struct drm_property_blob *blob)
>> +				      struct drm_property_blob *blob,
>> +				      struct drm_file *file_priv)
>>   {
>>   	if (blob == state->mode_blob)
>>   		return 0;
>> @@ -389,10 +391,20 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
>>   	memset(&state->mode, 0, sizeof(state->mode));
>>   
>>   	if (blob) {
>> +		struct drm_mode_modeinfo *u_mode;
>> +
>> +		u_mode = (struct drm_mode_modeinfo *) blob->data;
>> +		if (!file_priv->aspect_ratio_allowed &&
>> +		    (u_mode->flags & DRM_MODE_FLAG_PIC_AR_MASK) !=
>> +		    DRM_MODE_FLAG_PIC_AR_NONE) {
>> +			DRM_DEBUG_ATOMIC("Unexpected aspect-ratio flag bits\n");
>> +			return -EINVAL;
>> +		}
> We should probably pull this logic into a small helper so that we don't
> have to duplicate it in both the setprop and setcrtc code paths.

Agreed. I would add the required helper function, in the next patch-set.

>
>> +
>>   		if (blob->length != sizeof(struct drm_mode_modeinfo) ||
> The blob length check has to be done before you access the data.

Right. Will take care in the next patch-set.

>>   		    drm_mode_convert_umode(state->crtc->dev, &state->mode,
>> -		                           (const struct drm_mode_modeinfo *)
>> -		                            blob->data))
>> +					   (const struct drm_mode_modeinfo *)
>> +					   u_mode))
>>   			return -EINVAL;
>>   
>>   		state->mode_blob = drm_property_blob_get(blob);
>> @@ -441,6 +453,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>>    * @state: the state object to update with the new property value
>>    * @property: the property to set
>>    * @val: the new property value
>> + * @file_priv: the file private structure, to get the user capabilities
>>    *
>>    * This function handles generic/core properties and calls out to driver's
>>    * &drm_crtc_funcs.atomic_set_property for driver properties. To ensure
>> @@ -452,7 +465,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>>    */
>>   int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>   		struct drm_crtc_state *state, struct drm_property *property,
>> -		uint64_t val)
>> +		uint64_t val, struct drm_file *file_priv)
>>   {
>>   	struct drm_device *dev = crtc->dev;
>>   	struct drm_mode_config *config = &dev->mode_config;
>> @@ -465,7 +478,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>   		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);
>> +		ret = drm_atomic_set_mode_prop_for_crtc(state, mode, file_priv);
>>   		drm_property_blob_put(mode);
>>   		return ret;
>>   	} else if (property == config->degamma_lut_property) {
>> @@ -1918,7 +1931,8 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
>>   int drm_atomic_set_property(struct drm_atomic_state *state,
>>   			    struct drm_mode_object *obj,
>>   			    struct drm_property *prop,
>> -			    uint64_t prop_value)
>> +			    uint64_t prop_value,
>> +			    struct drm_file *file_priv)
>>   {
>>   	struct drm_mode_object *ref;
>>   	int ret;
>> @@ -1952,7 +1966,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>>   		}
>>   
>>   		ret = drm_atomic_crtc_set_property(crtc,
>> -				crtc_state, prop, prop_value);
>> +				crtc_state, prop, prop_value, file_priv);
>>   		break;
>>   	}
>>   	case DRM_MODE_OBJECT_PLANE: {
>> @@ -2341,7 +2355,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>   			}
>>   
>>   			ret = drm_atomic_set_property(state, obj, prop,
>> -						      prop_value);
>> +						      prop_value, file_priv);
>>   			if (ret) {
>>   				drm_mode_object_put(obj);
>>   				goto out;
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index ae3cbfe..781ebd6 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -186,7 +186,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
>>   
>>   		if (!crtc_state->connector_mask) {
>>   			ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
>> -								NULL);
>> +								NULL, NULL);
>>   			if (ret < 0)
>>   				goto out;
>>   
>> @@ -2749,7 +2749,7 @@ static int update_output_state(struct drm_atomic_state *state,
>>   
>>   		if (!new_crtc_state->connector_mask) {
>>   			ret = drm_atomic_set_mode_prop_for_crtc(new_crtc_state,
>> -								NULL);
>> +								NULL, NULL);
>>   			if (ret < 0)
>>   				return ret;
>>   
>> @@ -2930,7 +2930,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
>>   
>>   		crtc_state->active = false;
>>   
>> -		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL);
>> +		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL, NULL);
>>   		if (ret < 0)
>>   			goto free;
>>   
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 353e24f..e36a971 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -430,6 +430,9 @@ int drm_mode_getcrtc(struct drm_device *dev,
>>   	if (crtc->state) {
>>   		if (crtc->state->enable) {
>>   			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
>> +			if (!file_priv->aspect_ratio_allowed)
>> +				crtc->state->mode.flags &=
>> +				(~DRM_MODE_FLAG_PIC_AR_MASK);
> Should be manling crtc_resp->mode instead of the internal mode.

Agreed. Will correct this in the next patch-set.

>>   			crtc_resp->mode_valid = 1;
>>   
>>   		} else {
>> @@ -440,6 +443,9 @@ int drm_mode_getcrtc(struct drm_device *dev,
>>   		crtc_resp->y = crtc->y;
>>   		if (crtc->enabled) {
>>   			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
>> +			if (!file_priv->aspect_ratio_allowed)
>> +				crtc->mode.flags &=
>> +				(~DRM_MODE_FLAG_PIC_AR_MASK);
> ditto
>
> Should probably pull this flag filtering logic into a small helper
> as well.

Agreed. Will correct this in the next patch-set.

Regards,
Ankit

>
>>   			crtc_resp->mode_valid = 1;
>>   
>>   		} else {
>> @@ -614,6 +620,15 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>>   			goto out;
>>   		}
>>   
>> +		if (!file_priv->aspect_ratio_allowed &&
>> +		    (crtc_req->mode.flags & DRM_MODE_FLAG_PIC_AR_MASK) !=
>> +		    DRM_MODE_FLAG_PIC_AR_NONE) {
>> +			DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n");
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +
>>   		ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
>>   		if (ret) {
>>   			DRM_DEBUG_KMS("Invalid mode\n");
>> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
>> index af00f42..7c3338f 100644
>> --- a/drivers/gpu/drm/drm_crtc_internal.h
>> +++ b/drivers/gpu/drm/drm_crtc_internal.h
>> @@ -186,7 +186,8 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
>>   int drm_atomic_set_property(struct drm_atomic_state *state,
>>   			    struct drm_mode_object *obj,
>>   			    struct drm_property *prop,
>> -			    uint64_t prop_value);
>> +			    uint64_t prop_value,
>> +			    struct drm_file *file_priv);
>>   int drm_atomic_get_property(struct drm_mode_object *obj,
>>   			    struct drm_property *property, uint64_t *val);
>>   int drm_mode_atomic_ioctl(struct drm_device *dev,
>> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
>> index ce4d2fb..1c3d498 100644
>> --- a/drivers/gpu/drm/drm_mode_object.c
>> +++ b/drivers/gpu/drm/drm_mode_object.c
>> @@ -452,7 +452,8 @@ static int set_property_legacy(struct drm_mode_object *obj,
>>   
>>   static int set_property_atomic(struct drm_mode_object *obj,
>>   			       struct drm_property *prop,
>> -			       uint64_t prop_value)
>> +			       uint64_t prop_value,
>> +			       struct drm_file *file_priv)
>>   {
>>   	struct drm_device *dev = prop->dev;
>>   	struct drm_atomic_state *state;
>> @@ -476,7 +477,8 @@ static int set_property_atomic(struct drm_mode_object *obj,
>>   						       obj_to_connector(obj),
>>   						       prop_value);
>>   	} else {
>> -		ret = drm_atomic_set_property(state, obj, prop, prop_value);
>> +		ret = drm_atomic_set_property(state, obj, prop, prop_value,
>> +					      file_priv);
>>   		if (ret)
>>   			goto out;
>>   		ret = drm_atomic_commit(state);
>> @@ -519,7 +521,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>>   		goto out_unref;
>>   
>>   	if (drm_drv_uses_atomic_modeset(property->dev))
>> -		ret = set_property_atomic(arg_obj, property, arg->value);
>> +		ret = set_property_atomic(arg_obj, property, arg->value,
>> +					  file_priv);
>>   	else
>>   		ret = set_property_legacy(arg_obj, property, arg->value);
>>   
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index 6ca1f3b..ca4c5cc 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -1620,6 +1620,7 @@ EXPORT_SYMBOL(drm_mode_create_from_cmdline_mode);
>>    * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
>>    * @out: drm_mode_modeinfo struct to return to the user
>>    * @in: drm_display_mode to use
>> + * @file_priv: file_priv structure, to get the user capabilities
>>    *
>>    * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to
>>    * the user.
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index cf13842..d3ad5d1 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -367,7 +367,7 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>>   			  struct drm_crtc *crtc);
>>   int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>   		struct drm_crtc_state *state, struct drm_property *property,
>> -		uint64_t val);
>> +		uint64_t val, struct drm_file *file_priv);
>>   struct drm_plane_state * __must_check
>>   drm_atomic_get_plane_state(struct drm_atomic_state *state,
>>   			   struct drm_plane *plane);
>> @@ -583,7 +583,8 @@ drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>>   			     const struct drm_display_mode *mode);
>>   int __must_check
>>   drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
>> -				  struct drm_property_blob *blob);
>> +				  struct drm_property_blob *blob,
>> +				  struct drm_file *file_priv);
>>   int __must_check
>>   drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>>   			      struct drm_crtc *crtc);
>> -- 
>> 2.7.4


[-- Attachment #1.2: Type: text/html, Size: 14139 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v5 7/9] drm: Expose modes with aspect ratio, only if requested

[Nautiyal, Ankit K]

[-- Attachment #1.1: Type: text/plain, Size: 6543 bytes --]


On 2/23/2018 8:06 PM, Ville Syrjälä wrote:
> On Thu, Feb 15, 2018 at 05:51:00PM +0530, Nautiyal, Ankit K wrote:
>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> We parse the EDID and add all the modes in the connector's modelist.
>> This adds CEA modes with aspect ratio information too, regadless of
>> whether user space requested this information or not.
>>
>> This patch prunes the modes with aspect-ratio information, from a
>> connector's modelist, if the user-space has not set the aspect ratio
>> DRM client cap.
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>> Cc: Jose Abreu <jose.abreu@synopsys.com>
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> V3: As suggested by Ville, modified the mechanism of pruning of modes
>>      with aspect-ratio, if the aspect-ratio is not supported. Instead
>>      of straight away pruning such a mode, the mode is retained with
>>      aspect ratio bits set to zero, provided it is unique.
>> V4: rebase
>> V5: Addressed review comments from Ville:
>>      -used a pointer to store last valid mode.
>>      -avoided, modifying of picture_aspect_ratio in kernel mode,
>>       instead only flags bits of user mode are reset (if aspect-ratio
>>       is not supported).
>> ---
>>   drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 40 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 16b9c38..49778e8 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1507,8 +1507,10 @@ static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *conne
>>   	return connector->encoder;
>>   }
>>   
>> -static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
>> -					 const struct drm_file *file_priv)
>> +static bool
>> +drm_mode_expose_to_userspace(const struct drm_display_mode *last_mode,
>> +			     const struct drm_display_mode *mode,
> I would put the 'mode' argument first since that's the "main" object
> we're operating on.
Yes that makes more sense. Will make the mode as first argument in the 
next revision.

>
>> +			     const struct drm_file *file_priv)
>>   {
>>   	/*
>>   	 * If user-space hasn't configured the driver to expose the stereo 3D
>> @@ -1516,6 +1518,23 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
>>   	 */
>>   	if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode))
>>   		return false;
>> +	/*
>> +	 * If user-space hasn't configured the driver to expose the modes
>> +	 * with aspect-ratio, don't expose them. But in case of a unique
>> +	 * mode, let the mode be passed, so that it can be enumerated with
>> +	 * aspect ratio bits erased.
> Might want to note that we expect the list to be sorted such that modes
> that have different aspect ratio but are otherwise identical are back to
> back.

Indeed. The whole idea, is based on the fact that the list of modes are 
sorted.
Will put a note here.

>> +	 */
>> +	if (!file_priv->aspect_ratio_allowed &&
>> +	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
>> +		if (last_mode && !drm_mode_match(mode, last_mode,
>> +						 DRM_MODE_MATCH_TIMINGS |
>> +						 DRM_MODE_MATCH_CLOCK |
>> +						 DRM_MODE_MATCH_FLAGS |
>> +						 DRM_MODE_MATCH_3D_FLAGS))
>> +			return true;
>> +		else
>> +			return false;
> How does that even work? It'll return false as long as
> last_mode==NULL, and last_mode never becomes non-NULL unless
> we return true.
>
> To me it looks like the correct logic would be:
> if (last_mode && drm_mode_match(...))
> 	return false;

This is a mistake, thanks for pointing it out.

While testing with aspect-ratio cap not set, for all the available 
displays, the first mode in the list,
never had a mode with aspect ratio. (or in other words, first mode was 
always with aspect ratio NONE)
This resulted in early exit from the condition and returning true, 
making last_mode as the first mode.

Long story short, couldn't catch the problem while testing as first mode 
was always with aspect ratio NONE :)

Your suggested code is indeed the correct logic, which I intended.

>> +	}
>>   
>>   	return true;
>>   }
>> @@ -1527,6 +1546,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>   	struct drm_connector *connector;
>>   	struct drm_encoder *encoder;
>>   	struct drm_display_mode *mode;
>> +	struct drm_display_mode *last_valid_mode;
>>   	int mode_count = 0;
>>   	int encoders_count = 0;
>>   	int ret = 0;
>> @@ -1582,9 +1602,13 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>   	out_resp->connection = connector->status;
>>   
>>   	/* delayed so we get modes regardless of pre-fill_modes state */
>> +	last_valid_mode = NULL;
>>   	list_for_each_entry(mode, &connector->modes, head)
>> -		if (drm_mode_expose_to_userspace(mode, file_priv))
>> +		if (drm_mode_expose_to_userspace(last_valid_mode, mode,
>> +						 file_priv)) {
>>   			mode_count++;
>> +			last_valid_mode = mode;
>> +		}
>>   
>>   	/*
>>   	 * This ioctl is called twice, once to determine how much space is
>> @@ -1593,11 +1617,21 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>   	if ((out_resp->count_modes >= mode_count) && mode_count) {
>>   		copied = 0;
>>   		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
>> +		last_valid_mode = NULL;
>>   		list_for_each_entry(mode, &connector->modes, head) {
>> -			if (!drm_mode_expose_to_userspace(mode, file_priv))
>> +			if (!drm_mode_expose_to_userspace(last_valid_mode,
>> +							  mode,
>> +							  file_priv))
>>   				continue;
>> -
>>   			drm_mode_convert_to_umode(&u_mode, mode);
>> +
>> +			/*
>> +			 * Reset the aspect-ratio flag bits from the user mode,
>> +			 * if the user mode does have aspect ratio capability.
>> +			 */
>> +			if (!file_priv->aspect_ratio_allowed)
>> +				u_mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> Useless parens. Could use the same helper I suggested we add for
> getcrtc() and getblob().
Yes, the agreed helper function will be used here.

Regards,
Ankit

>
>> +
>>   			if (copy_to_user(mode_ptr + copied,
>>   					 &u_mode, sizeof(u_mode))) {
>>   				ret = -EFAULT;
>> @@ -1605,6 +1639,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>   
>>   				goto out;
>>   			}
>> +			last_valid_mode = mode;
>>   			copied++;
>>   		}
>>   	}
>> -- 
>> 2.7.4


[-- Attachment #1.2: Type: text/html, Size: 8252 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v5 8/9] drm: Add aspect ratio parsing in DRM layer

[Nautiyal, Ankit K]

[-- Attachment #1.1: Type: text/plain, Size: 4963 bytes --]



On 2/23/2018 8:24 PM, Ville Syrjälä wrote:
> On Thu, Feb 15, 2018 at 05:51:01PM +0530, Nautiyal, Ankit K wrote:
>> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>>
>> Current DRM layer functions don't parse aspect ratio information
>> while converting a user mode->kernel mode or vice versa. This
>> causes modeset to pick mode with wrong aspect ratio, eventually
>> causing failures in HDMI compliance test cases, due to wrong VIC.
>>
>> This patch adds aspect ratio information in DRM's mode conversion
>> and mode comparision functions, to make sure kernel picks mode
>> with right aspect ratio (as per the VIC).
>>
>> Background:
>> This patch was once reviewed and merged, and later reverted due to
>> lack of DRM cap protection. This is a re-spin of this patch, this
>> time with DRM cap protection, to avoid aspect ratio information, when
>> the client doesn't request for it.
>>
>> Review link: https://pw-emeril.freedesktop.org/patch/104068/
>> Background discussion: https://patchwork.kernel.org/patch/9379057/
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Lin, Jia <lin.a.jia@intel.com>
>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
>> Reviewed-by: Jim Bride <jim.bride@linux.intel.com> (V2)
>> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> (V4)
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Jim Bride <jim.bride@linux.intel.com>
>> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
>> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> V3: modified the aspect-ratio check in drm_mode_equal as per new flags
>>      provided by Ville. https://patchwork.freedesktop.org/patch/188043/
>> V4: rebase
>> V5: rebase
>> ---
>>   drivers/gpu/drm/drm_modes.c | 33 ++++++++++++++++++++++++++++++++-
>>   1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index ca4c5cc..25140b9 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -1050,7 +1050,8 @@ bool drm_mode_equal(const struct drm_display_mode *mode1,
>>   			      DRM_MODE_MATCH_TIMINGS |
>>   			      DRM_MODE_MATCH_CLOCK |
>>   			      DRM_MODE_MATCH_FLAGS |
>> -			      DRM_MODE_MATCH_3D_FLAGS);
>> +			      DRM_MODE_MATCH_3D_FLAGS|
>> +			      DRM_MODE_MATCH_ASPECT_RATIO);
> Hmm. I wonder if all the users are expecting this. Based on a cursory
> examination drm_fb_helper might want to ignore the aspect ratio since
> it's just looking to clone the same mode on multiple outputs. The other
> cases don't look to me like they should suffer badly from this.

Agreed.
Need to add a function drm_mode_equal_no_aspect_ratio() in drm_modes.c 
which will compare
every field, other than the aspect-ratio.
Next, in the drm_target_cloned( ), where same modes are to be cloned on 
multiple outputs, instead
of drm_mode_equal( ), drm_mode_equal_no_aspect_ratio( ) must be called.

Is this approach correct?

>
>>   }
>>   EXPORT_SYMBOL(drm_mode_equal);
>>   
>> @@ -1649,6 +1650,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>>   	out->vrefresh = in->vrefresh;
>>   	out->flags = in->flags;
>>   	out->type = in->type;
>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> This looks redundant. The internal mode should not have the aspect ratio
> flags set. Or are we changing that?

Yes right, the aspect-ratio flags are never set in the internal-mode flags.
This line can be dropped.

>
>> +
>> +	switch (in->picture_aspect_ratio) {
>> +	case HDMI_PICTURE_ASPECT_4_3:
>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
>> +		break;
>> +	case HDMI_PICTURE_ASPECT_16_9:
>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
>> +		break;
>> +	case HDMI_PICTURE_ASPECT_RESERVED:
>> +	default:
>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
>> +		break;
>> +	}
>> +
>>   	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>   	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>   }
>> @@ -1693,6 +1709,21 @@ int drm_mode_convert_umode(struct drm_device *dev,
>>   	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>   	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>   
>> +	/* Clearing picture aspect ratio bits from out flags */
> What the code is doing is obvious so this comment is redundant.
> The non-obvious part (ie. the "why?") is what the comment
> should contain.

Thanks for pointing that out, will take care of this in future patches.

Regards,
Ankit

>
>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>> +
>> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
>> +	case DRM_MODE_FLAG_PIC_AR_4_3:
>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
>> +		break;
>> +	case DRM_MODE_FLAG_PIC_AR_16_9:
>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
>> +		break;
>> +	default:
>> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>> +		break;
>> +	}
>> +
>>   	out->status = drm_mode_validate_driver(dev, out);
>>   	if (out->status != MODE_OK)
>>   		goto out;
>> -- 
>> 2.7.4


[-- Attachment #1.2: Type: text/html, Size: 7219 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v5 8/9] drm: Add aspect ratio parsing in DRM layer

[Ville Syrjälä]

On Mon, Feb 26, 2018 at 09:18:57PM +0530, Nautiyal, Ankit K wrote:
> 
> 
> On 2/23/2018 8:24 PM, Ville Syrjälä wrote:
> > On Thu, Feb 15, 2018 at 05:51:01PM +0530, Nautiyal, Ankit K wrote:
> >> From: "Sharma, Shashank" <shashank.sharma@intel.com>
> >>
> >> Current DRM layer functions don't parse aspect ratio information
> >> while converting a user mode->kernel mode or vice versa. This
> >> causes modeset to pick mode with wrong aspect ratio, eventually
> >> causing failures in HDMI compliance test cases, due to wrong VIC.
> >>
> >> This patch adds aspect ratio information in DRM's mode conversion
> >> and mode comparision functions, to make sure kernel picks mode
> >> with right aspect ratio (as per the VIC).
> >>
> >> Background:
> >> This patch was once reviewed and merged, and later reverted due to
> >> lack of DRM cap protection. This is a re-spin of this patch, this
> >> time with DRM cap protection, to avoid aspect ratio information, when
> >> the client doesn't request for it.
> >>
> >> Review link: https://pw-emeril.freedesktop.org/patch/104068/
> >> Background discussion: https://patchwork.kernel.org/patch/9379057/
> >>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> Signed-off-by: Lin, Jia <lin.a.jia@intel.com>
> >> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
> >> Reviewed-by: Jim Bride <jim.bride@linux.intel.com> (V2)
> >> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> (V4)
> >>
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Cc: Jim Bride <jim.bride@linux.intel.com>
> >> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> >> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>
> >> V3: modified the aspect-ratio check in drm_mode_equal as per new flags
> >>      provided by Ville. https://patchwork.freedesktop.org/patch/188043/
> >> V4: rebase
> >> V5: rebase
> >> ---
> >>   drivers/gpu/drm/drm_modes.c | 33 ++++++++++++++++++++++++++++++++-
> >>   1 file changed, 32 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> >> index ca4c5cc..25140b9 100644
> >> --- a/drivers/gpu/drm/drm_modes.c
> >> +++ b/drivers/gpu/drm/drm_modes.c
> >> @@ -1050,7 +1050,8 @@ bool drm_mode_equal(const struct drm_display_mode *mode1,
> >>   			      DRM_MODE_MATCH_TIMINGS |
> >>   			      DRM_MODE_MATCH_CLOCK |
> >>   			      DRM_MODE_MATCH_FLAGS |
> >> -			      DRM_MODE_MATCH_3D_FLAGS);
> >> +			      DRM_MODE_MATCH_3D_FLAGS|
> >> +			      DRM_MODE_MATCH_ASPECT_RATIO);
> > Hmm. I wonder if all the users are expecting this. Based on a cursory
> > examination drm_fb_helper might want to ignore the aspect ratio since
> > it's just looking to clone the same mode on multiple outputs. The other
> > cases don't look to me like they should suffer badly from this.
> 
> Agreed.
> Need to add a function drm_mode_equal_no_aspect_ratio() in drm_modes.c 
> which will compare
> every field, other than the aspect-ratio.
> Next, in the drm_target_cloned( ), where same modes are to be cloned on 
> multiple outputs, instead
> of drm_mode_equal( ), drm_mode_equal_no_aspect_ratio( ) must be called.
> 
> Is this approach correct?

Please no more drm_mode_equal_no_whatever(). We should nuke the current
ones. If we think the no_whatever pattern is useful we should probably
add something like drm_mode_match_except() which takes the set of flags
we're going to ignore. Not 100% convinced it's a good pattern to follow
though. Just stating explicitly what we want to match seems generally
better to me.

> 
> >
> >>   }
> >>   EXPORT_SYMBOL(drm_mode_equal);
> >>   
> >> @@ -1649,6 +1650,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> >>   	out->vrefresh = in->vrefresh;
> >>   	out->flags = in->flags;
> >>   	out->type = in->type;
> >> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> > This looks redundant. The internal mode should not have the aspect ratio
> > flags set. Or are we changing that?
> 
> Yes right, the aspect-ratio flags are never set in the internal-mode flags.
> This line can be dropped.
> 
> >
> >> +
> >> +	switch (in->picture_aspect_ratio) {
> >> +	case HDMI_PICTURE_ASPECT_4_3:
> >> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
> >> +		break;
> >> +	case HDMI_PICTURE_ASPECT_16_9:
> >> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> >> +		break;
> >> +	case HDMI_PICTURE_ASPECT_RESERVED:
> >> +	default:
> >> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
> >> +		break;
> >> +	}
> >> +
> >>   	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >>   	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> >>   }
> >> @@ -1693,6 +1709,21 @@ int drm_mode_convert_umode(struct drm_device *dev,
> >>   	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >>   	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> >>   
> >> +	/* Clearing picture aspect ratio bits from out flags */
> > What the code is doing is obvious so this comment is redundant.
> > The non-obvious part (ie. the "why?") is what the comment
> > should contain.
> 
> Thanks for pointing that out, will take care of this in future patches.
> 
> Regards,
> Ankit
> 
> >
> >> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> >> +
> >> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
> >> +	case DRM_MODE_FLAG_PIC_AR_4_3:
> >> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
> >> +		break;
> >> +	case DRM_MODE_FLAG_PIC_AR_16_9:
> >> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> >> +		break;
> >> +	default:
> >> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >> +		break;
> >> +	}
> >> +
> >>   	out->status = drm_mode_validate_driver(dev, out);
> >>   	if (out->status != MODE_OK)
> >>   		goto out;
> >> -- 
> >> 2.7.4
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel