[PATCH v13 00/10] Aspect ratio support in DRM layer

[PATCH v13 00/10] 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 5 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, and also ensures that no bogus aspect-ratios
are sent in the AVI infoframes.

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.
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 (3):
  drm: Add DRM client cap for aspect-ratio
  drm: Handle aspect ratio info in legacy modeset path
  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ä (5):
  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
  drm/edid: Don't send bogus aspect ratios in AVI infoframes
  video/hdmi: Reject illegal picture aspect ratios

 drivers/gpu/drm/drm_connector.c     |  45 +++++--
 drivers/gpu/drm/drm_crtc.c          |   8 ++
 drivers/gpu/drm/drm_crtc_internal.h |   6 +
 drivers/gpu/drm/drm_edid.c          |  41 +++++--
 drivers/gpu/drm/drm_fb_helper.c     |  12 +-
 drivers/gpu/drm/drm_ioctl.c         |   9 ++
 drivers/gpu/drm/drm_modes.c         | 226 +++++++++++++++++++++++++++++++-----
 drivers/video/hdmi.c                |   3 +
 include/drm/drm_file.h              |   8 ++
 include/drm/drm_modes.h             |  22 ++++
 include/uapi/drm/drm.h              |   7 ++
 include/uapi/drm/drm_mode.h         |   6 +
 12 files changed, 343 insertions(+), 50 deletions(-)

-- 
2.7.4

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

[PATCH v13 01/10] 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 e82b61e..c395a24 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -939,17 +939,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;
@@ -957,15 +1008,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;
 
-	return drm_mode_equal_no_clocks(mode1, mode2);
+	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;
+
+	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);
 
@@ -980,13 +1064,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);
 
@@ -1004,21 +1088,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 v13 02/10] 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 08d33b4..5475f31 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3047,7 +3047,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 v13 03/10] 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 5475f31..3f157c8 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2930,11 +2930,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;
@@ -2948,7 +2952,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));
 	}
@@ -2965,11 +2969,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;
@@ -2983,7 +2991,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));
 	}
@@ -3030,6 +3038,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)
@@ -3047,7 +3056,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;
 	}
 
@@ -3064,6 +3073,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)
@@ -3079,7 +3089,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 v13 04/10] drm/edid: Don't send bogus aspect ratios in AVI infoframes

[Nautiyal, Ankit K]

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

If the user mode would specify an aspect ratio other than 4:3 or 16:9
we now silently ignore it. Maybe a better apporoach is to return an
error? Let's try that.

Also we must be careful that we don't try to send illegal picture
aspect in the infoframe as it's only capable of signalling none,
4:3, and 16:9. Currently we're sending these bogus infoframes
whenever the cea mode specifies some other aspect ratio.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
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 | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3f157c8..40e1e24 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4833,6 +4833,7 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 					 const struct drm_display_mode *mode,
 					 bool is_hdmi2_sink)
 {
+	enum hdmi_picture_aspect picture_aspect;
 	int err;
 
 	if (!frame || !mode)
@@ -4875,13 +4876,23 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 	 * Populate picture aspect ratio from either
 	 * user input (if specified) or from the CEA mode list.
 	 */
-	if (mode->picture_aspect_ratio == HDMI_PICTURE_ASPECT_4_3 ||
-		mode->picture_aspect_ratio == HDMI_PICTURE_ASPECT_16_9)
-		frame->picture_aspect = mode->picture_aspect_ratio;
-	else if (frame->video_code > 0)
-		frame->picture_aspect = drm_get_cea_aspect_ratio(
-						frame->video_code);
+	picture_aspect = mode->picture_aspect_ratio;
+	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE)
+		picture_aspect = drm_get_cea_aspect_ratio(frame->video_code);
 
+	/*
+	 * The infoframe can't convey anything but none, 4:3
+	 * and 16:9, so if the user has asked for anything else
+	 * we can only satisfy it by specifying the right VIC.
+	 */
+	if (picture_aspect > HDMI_PICTURE_ASPECT_16_9) {
+		if (picture_aspect !=
+		    drm_get_cea_aspect_ratio(frame->video_code))
+			return -EINVAL;
+		picture_aspect = HDMI_PICTURE_ASPECT_NONE;
+	}
+
+	frame->picture_aspect = picture_aspect;
 	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
 	frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v13 05/10] video/hdmi: Reject illegal picture aspect ratios

[Nautiyal, Ankit K]

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

AVI infoframe can only carry none, 4:3, or 16:9 picture aspect
ratios. Return an error if the user asked for something different.

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>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: linux-media@vger.kernel.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Jose Abreu <joabreu@synopsys.com>
---
 drivers/video/hdmi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 111a0ab..38716eb5 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -93,6 +93,9 @@ ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer,
 	if (size < length)
 		return -ENOSPC;
 
+	if (frame->picture_aspect > HDMI_PICTURE_ASPECT_16_9)
+		return -EINVAL;
+
 	memset(buffer, 0, size);
 
 	ptr[0] = frame->type;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v13 06/10] 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
non-atomic 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 non-atomic
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.

Since no atomic-userspaces blow up on receiving aspect-ratio
information, the client cap for aspect-ratio is always enabled
for atomic clients.

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
V6: rebase
V7: rebase
V8: rebase
V9: rebase
V10: rebase
V11: rebase
V12: As suggested by Daniel Vetter and Ville Syrjala,
     always enable aspect-ratio client cap for atomic userspaces,
     if no atomic userspace breaks on aspect-ratio bits.
V13: rebase

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

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index af78291..0379983 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -324,6 +324,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 			return -EINVAL;
 		file_priv->atomic = req->value;
 		file_priv->universal_planes = req->value;
+		/*
+		 * No atomic user-space blows up on aspect ratio mode bits.
+		 */
+		file_priv->aspect_ratio_allowed = 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 5176c37..02b7dde 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

Re: [Intel-gfx] [PATCH v13 06/10] drm: Add DRM client cap for aspect-ratio

[Daniel Vetter]

On Wed, May 02, 2018 at 12:20:18PM +0530, Nautiyal, Ankit K wrote:
> 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
> non-atomic 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 non-atomic
> 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.
> 
> Since no atomic-userspaces blow up on receiving aspect-ratio
> information, the client cap for aspect-ratio is always enabled
> for atomic clients.
> 
> 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
> V6: rebase
> V7: rebase
> V8: rebase
> V9: rebase
> V10: rebase
> V11: rebase
> V12: As suggested by Daniel Vetter and Ville Syrjala,
>      always enable aspect-ratio client cap for atomic userspaces,
>      if no atomic userspace breaks on aspect-ratio bits.
> V13: rebase
> 
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

Yeah this is what I had in mind with auto-enabling this for atomic
clients.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_ioctl.c | 9 +++++++++
>  include/drm/drm_file.h      | 8 ++++++++
>  include/uapi/drm/drm.h      | 7 +++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index af78291..0379983 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -324,6 +324,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  			return -EINVAL;
>  		file_priv->atomic = req->value;
>  		file_priv->universal_planes = req->value;
> +		/*
> +		 * No atomic user-space blows up on aspect ratio mode bits.
> +		 */
> +		file_priv->aspect_ratio_allowed = 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 5176c37..02b7dde 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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v13 07/10] drm: Handle aspect ratio info in legacy modeset path

[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. rejects the modes with aspect-ratio info, during modeset, if the
   user does not support aspect ratio.
2. does not load the aspect-ratio info in user-mode structure, if
   aspect ratio is not supported.
3. adds helper functions for determining if aspect-ratio is expected
   in user-mode and for allowing/disallowing the aspect-ratio, if its
   not expected.

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.
V6: Used the helper functions for determining if aspect-ratio is
    expected in the user-mode.
V7: rebase
V8: rebase
V9: rebase
V10: Modified the commit-message
V11: rebase
V12: Merged the patch for adding aspect-ratio helper functions
     with this patch.
V13: Minor modifications as suggested by Ville.
---
 drivers/gpu/drm/drm_crtc.c          |  8 +++++++
 drivers/gpu/drm/drm_crtc_internal.h |  6 +++++
 drivers/gpu/drm/drm_modes.c         | 47 +++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a231dd5..98323f4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -449,6 +449,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
 			crtc_resp->mode_valid = 0;
 		}
 	}
+	drm_mode_filter_aspect_ratio_flags(file_priv, &crtc_resp->mode);
 	drm_modeset_unlock(&crtc->mutex);
 
 	return 0;
@@ -628,6 +629,13 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			ret = -ENOMEM;
 			goto out;
 		}
+		if (!drm_mode_aspect_ratio_allowed(file_priv,
+						   &crtc_req->mode)) {
+			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) {
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 5d307b2..31d6c77 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -222,3 +222,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
 void drm_reset_display_info(struct drm_connector *connector);
 u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid);
+
+/* drm_modes.c */
+bool drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
+				   struct drm_mode_modeinfo *umode);
+void drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
+					struct drm_mode_modeinfo *umode);
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index c395a24..2bf2f0b 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1759,3 +1759,50 @@ bool drm_mode_is_420(const struct drm_display_info *display,
 		drm_mode_is_420_also(display, mode);
 }
 EXPORT_SYMBOL(drm_mode_is_420);
+
+/**
+ * drm_mode_aspect_ratio_allowed - checks if the aspect-ratio information
+ * is expected from the user-mode.
+ *
+ * If the user has set aspect-ratio cap, then the flag of the user-mode is
+ * allowed to contain aspect-ratio value.
+ * If the user does not set aspect-ratio cap, then the only value allowed in the
+ * flags bits is aspect-ratio NONE.
+ *
+ * @file_priv: file private structure to get the user capabilities
+ * @umode: drm_mode_modeinfo struct, whose flag carry the aspect ratio
+ * information.
+ *
+ * Returns:
+ * true if the aspect-ratio info is allowed in the user-mode flags.
+ * false, otherwise.
+ */
+bool
+drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
+			      struct drm_mode_modeinfo *umode)
+{
+	if (file_priv->aspect_ratio_allowed ||
+	    (umode->flags & DRM_MODE_FLAG_PIC_AR_MASK) == DRM_MODE_FLAG_PIC_AR_NONE)
+		return true;
+	return false;
+}
+
+/**
+ * drm_mode_filter_aspect_ratio_flags - filters the aspect-ratio bits in the
+ * user-mode flags.
+ *
+ * Checks if the aspect-ratio information is allowed. Resets the aspect-ratio
+ * bits in the user-mode flags, if aspect-ratio info is not allowed.
+ *
+ * @file_priv: file private structure to get the user capabilities.
+ * @umode: drm_mode_modeinfo struct, whose flags' aspect-ratio bits needs to
+ * be filtered.
+ *
+ */
+void
+drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
+				   struct drm_mode_modeinfo *umode)
+{
+	if (!drm_mode_aspect_ratio_allowed(file_priv, umode))
+		umode->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
+}
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v13 07/10] drm: Handle aspect ratio info in legacy modeset path

[Daniel Vetter]

On Wed, May 02, 2018 at 12:20:19PM +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. rejects the modes with aspect-ratio info, during modeset, if the
>    user does not support aspect ratio.
> 2. does not load the aspect-ratio info in user-mode structure, if
>    aspect ratio is not supported.
> 3. adds helper functions for determining if aspect-ratio is expected
>    in user-mode and for allowing/disallowing the aspect-ratio, if its
>    not expected.
> 
> 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.
> V6: Used the helper functions for determining if aspect-ratio is
>     expected in the user-mode.
> V7: rebase
> V8: rebase
> V9: rebase
> V10: Modified the commit-message
> V11: rebase
> V12: Merged the patch for adding aspect-ratio helper functions
>      with this patch.
> V13: Minor modifications as suggested by Ville.
> ---
>  drivers/gpu/drm/drm_crtc.c          |  8 +++++++
>  drivers/gpu/drm/drm_crtc_internal.h |  6 +++++
>  drivers/gpu/drm/drm_modes.c         | 47 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index a231dd5..98323f4 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -449,6 +449,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  			crtc_resp->mode_valid = 0;
>  		}
>  	}
> +	drm_mode_filter_aspect_ratio_flags(file_priv, &crtc_resp->mode);
>  	drm_modeset_unlock(&crtc->mutex);
>  
>  	return 0;
> @@ -628,6 +629,13 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			ret = -ENOMEM;
>  			goto out;
>  		}
> +		if (!drm_mode_aspect_ratio_allowed(file_priv,
> +						   &crtc_req->mode)) {
> +			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) {
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 5d307b2..31d6c77 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -222,3 +222,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
>  void drm_reset_display_info(struct drm_connector *connector);
>  u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid);
> +
> +/* drm_modes.c */
> +bool drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
> +				   struct drm_mode_modeinfo *umode);
> +void drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
> +					struct drm_mode_modeinfo *umode);
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index c395a24..2bf2f0b 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1759,3 +1759,50 @@ bool drm_mode_is_420(const struct drm_display_info *display,
>  		drm_mode_is_420_also(display, mode);
>  }
>  EXPORT_SYMBOL(drm_mode_is_420);
> +
> +/**
> + * drm_mode_aspect_ratio_allowed - checks if the aspect-ratio information
> + * is expected from the user-mode.
> + *
> + * If the user has set aspect-ratio cap, then the flag of the user-mode is
> + * allowed to contain aspect-ratio value.
> + * If the user does not set aspect-ratio cap, then the only value allowed in the
> + * flags bits is aspect-ratio NONE.
> + *
> + * @file_priv: file private structure to get the user capabilities
> + * @umode: drm_mode_modeinfo struct, whose flag carry the aspect ratio
> + * information.
> + *
> + * Returns:
> + * true if the aspect-ratio info is allowed in the user-mode flags.
> + * false, otherwise.
> + */

We generally don't do full kerneldoc for drm.ko internal functions (which
these both are), only for driver functions. I'd remove them both because
they don't add a hole lot really.

> +bool
> +drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
> +			      struct drm_mode_modeinfo *umode)

I don't really see the point of this helper. It has a bit a confusing
name, and open-code it looks simpler to me in setcrtc.

> +{
> +	if (file_priv->aspect_ratio_allowed ||
> +	    (umode->flags & DRM_MODE_FLAG_PIC_AR_MASK) == DRM_MODE_FLAG_PIC_AR_NONE)
> +		return true;
> +	return false;
> +}
> +
> +/**
> + * drm_mode_filter_aspect_ratio_flags - filters the aspect-ratio bits in the
> + * user-mode flags.
> + *
> + * Checks if the aspect-ratio information is allowed. Resets the aspect-ratio
> + * bits in the user-mode flags, if aspect-ratio info is not allowed.
> + *
> + * @file_priv: file private structure to get the user capabilities.
> + * @umode: drm_mode_modeinfo struct, whose flags' aspect-ratio bits needs to
> + * be filtered.
> + *
> + */
> +void
> +drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
> +				   struct drm_mode_modeinfo *umode)
> +{
> +	if (!drm_mode_aspect_ratio_allowed(file_priv, umode))

You only need to check for file_priv->aspect_ratio_allowed here, then
clear unconditionally.
-Daniel

> +		umode->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> +}
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v13 08/10] 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, regardless 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. However if such a mode is unique in the list, it is
 kept in the list, with aspect-ratio flags reset.
-prepares a list of exposed modes, which is used to find unique modes
 if aspect-ratio is not allowed.
-adds a new list_head 'exposed_head' in drm_mode_display, to traverse
 the list of exposed modes.

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).
V6: As suggested by Ville, corrected the mode pruning logic and
    elaborated the mode pruning logic and the assumptions taken.
V7: rebase
V8: rebase
V9: rebase
V10: rebase
V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
     logic to correctly identify and prune modes with aspect-ratio,
     if aspect-ratio cap is not set.
V12: As suggested by Ville, added another list_head in
     drm_mode_display to traverse the list of exposed modes and
     avoided duplication of modes.
V13: Minor modifications, as suggested by Ville.
---
 drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++-------
 include/drm/drm_modes.h         | 13 ++++++++++++
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index dfc8ca1..8ca1149 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1531,15 +1531,36 @@ 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 *mode,
+			     const struct list_head *export_list,
+			     const struct drm_file *file_priv)
 {
 	/*
 	 * If user-space hasn't configured the driver to expose the stereo 3D
 	 * modes, don't expose them.
 	 */
+
 	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. However if such a mode
+	 * is unique, let it be exposed, but reset the aspect-ratio flags
+	 * while preparing the list of user-modes.
+	 */
+	if (!file_priv->aspect_ratio_allowed &&
+	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
+		struct drm_display_mode *mode_itr;
+
+		list_for_each_entry(mode_itr, export_list, export_head)
+			if (drm_mode_match(mode_itr, mode,
+					   DRM_MODE_MATCH_TIMINGS |
+					   DRM_MODE_MATCH_CLOCK |
+					   DRM_MODE_MATCH_FLAGS |
+					   DRM_MODE_MATCH_3D_FLAGS))
+				return false;
+	}
 
 	return true;
 }
@@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	struct drm_mode_modeinfo u_mode;
 	struct drm_mode_modeinfo __user *mode_ptr;
 	uint32_t __user *encoder_ptr;
+	LIST_HEAD(export_list);
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 
 	/* delayed so we get modes regardless of pre-fill_modes state */
 	list_for_each_entry(mode, &connector->modes, head)
-		if (drm_mode_expose_to_userspace(mode, file_priv))
+		if (drm_mode_expose_to_userspace(mode, &export_list,
+						 file_priv)) {
+			list_add_tail(&mode->export_head, &export_list);
 			mode_count++;
+		}
 
 	/*
 	 * This ioctl is called twice, once to determine how much space is
 	 * needed, and the 2nd time to fill it.
+	 * The modes that need to be exposed to the user are maintained in the
+	 * 'export_list'. When the ioctl is called first time to determine the,
+	 * space, the export_list gets filled, to find the no.of modes. In the
+	 * 2nd time, the user modes are filled, one by one from the export_list.
 	 */
 	if ((out_resp->count_modes >= mode_count) && mode_count) {
 		copied = 0;
 		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
-		list_for_each_entry(mode, &connector->modes, head) {
-			if (!drm_mode_expose_to_userspace(mode, file_priv))
-				continue;
-
+		list_for_each_entry(mode, &export_list, export_head) {
 			drm_mode_convert_to_umode(&u_mode, mode);
+			/*
+			 * Reset aspect ratio flags of user-mode, if modes with
+			 * aspect-ratio are not supported.
+			 */
+			drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode);
 			if (copy_to_user(mode_ptr + copied,
 					 &u_mode, sizeof(u_mode))) {
 				ret = -EFAULT;
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 2f78b7e..b159fe0 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -411,6 +411,19 @@ struct drm_display_mode {
 	 * Field for setting the HDMI picture aspect ratio of a mode.
 	 */
 	enum hdmi_picture_aspect picture_aspect_ratio;
+
+	/**
+	 * @export_head:
+	 *
+	 * struct list_head for modes to be exposed to the userspace.
+	 * This is to maintain a list of exposed modes while preparing
+	 * user-mode's list in drm_mode_getconnector ioctl. The purpose of this
+	 * list_head only lies in the ioctl function, and is not expected to be
+	 * used outside the function.
+	 * Once used, the stale pointers are not reset, but left as it is, to
+	 * avoid overhead of protecting it by mode_config.mutex.
+	 */
+	struct list_head export_head;
 };
 
 /**
-- 
2.7.4

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

Re: [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Daniel Vetter]

On Wed, May 02, 2018 at 12:20:20PM +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, regardless 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. However if such a mode is unique in the list, it is
>  kept in the list, with aspect-ratio flags reset.
> -prepares a list of exposed modes, which is used to find unique modes
>  if aspect-ratio is not allowed.
> -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
>  the list of exposed modes.
> 
> 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).
> V6: As suggested by Ville, corrected the mode pruning logic and
>     elaborated the mode pruning logic and the assumptions taken.
> V7: rebase
> V8: rebase
> V9: rebase
> V10: rebase
> V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
>      logic to correctly identify and prune modes with aspect-ratio,
>      if aspect-ratio cap is not set.
> V12: As suggested by Ville, added another list_head in
>      drm_mode_display to traverse the list of exposed modes and
>      avoided duplication of modes.
> V13: Minor modifications, as suggested by Ville.
> ---
>  drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++-------
>  include/drm/drm_modes.h         | 13 ++++++++++++
>  2 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index dfc8ca1..8ca1149 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1531,15 +1531,36 @@ 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 *mode,
> +			     const struct list_head *export_list,
> +			     const struct drm_file *file_priv)
>  {
>  	/*
>  	 * If user-space hasn't configured the driver to expose the stereo 3D
>  	 * modes, don't expose them.
>  	 */
> +
>  	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. However if such a mode
> +	 * is unique, let it be exposed, but reset the aspect-ratio flags
> +	 * while preparing the list of user-modes.
> +	 */
> +	if (!file_priv->aspect_ratio_allowed &&
> +	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
> +		struct drm_display_mode *mode_itr;
> +
> +		list_for_each_entry(mode_itr, export_list, export_head)

By walking the list of only the modes already added to the export list we
rely on ASPECT_NONE being first if present. That seems to be a bit
fragile. If we instead walk over all present modes (i.e. connector->modes)
then I think that's avoided. With that changed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +			if (drm_mode_match(mode_itr, mode,
> +					   DRM_MODE_MATCH_TIMINGS |
> +					   DRM_MODE_MATCH_CLOCK |
> +					   DRM_MODE_MATCH_FLAGS |
> +					   DRM_MODE_MATCH_3D_FLAGS))
> +				return false;
> +	}
>  
>  	return true;
>  }
> @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	struct drm_mode_modeinfo u_mode;
>  	struct drm_mode_modeinfo __user *mode_ptr;
>  	uint32_t __user *encoder_ptr;
> +	LIST_HEAD(export_list);
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  
>  	/* delayed so we get modes regardless of pre-fill_modes state */
>  	list_for_each_entry(mode, &connector->modes, head)
> -		if (drm_mode_expose_to_userspace(mode, file_priv))
> +		if (drm_mode_expose_to_userspace(mode, &export_list,
> +						 file_priv)) {
> +			list_add_tail(&mode->export_head, &export_list);
>  			mode_count++;
> +		}
>  
>  	/*
>  	 * This ioctl is called twice, once to determine how much space is
>  	 * needed, and the 2nd time to fill it.
> +	 * The modes that need to be exposed to the user are maintained in the
> +	 * 'export_list'. When the ioctl is called first time to determine the,
> +	 * space, the export_list gets filled, to find the no.of modes. In the
> +	 * 2nd time, the user modes are filled, one by one from the export_list.
>  	 */
>  	if ((out_resp->count_modes >= mode_count) && mode_count) {
>  		copied = 0;
>  		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
> -		list_for_each_entry(mode, &connector->modes, head) {
> -			if (!drm_mode_expose_to_userspace(mode, file_priv))
> -				continue;
> -
> +		list_for_each_entry(mode, &export_list, export_head) {
>  			drm_mode_convert_to_umode(&u_mode, mode);
> +			/*
> +			 * Reset aspect ratio flags of user-mode, if modes with
> +			 * aspect-ratio are not supported.
> +			 */
> +			drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode);
>  			if (copy_to_user(mode_ptr + copied,
>  					 &u_mode, sizeof(u_mode))) {
>  				ret = -EFAULT;
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 2f78b7e..b159fe0 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -411,6 +411,19 @@ struct drm_display_mode {
>  	 * Field for setting the HDMI picture aspect ratio of a mode.
>  	 */
>  	enum hdmi_picture_aspect picture_aspect_ratio;
> +
> +	/**
> +	 * @export_head:
> +	 *
> +	 * struct list_head for modes to be exposed to the userspace.
> +	 * This is to maintain a list of exposed modes while preparing
> +	 * user-mode's list in drm_mode_getconnector ioctl. The purpose of this
> +	 * list_head only lies in the ioctl function, and is not expected to be
> +	 * used outside the function.
> +	 * Once used, the stale pointers are not reset, but left as it is, to
> +	 * avoid overhead of protecting it by mode_config.mutex.
> +	 */
> +	struct list_head export_head;
>  };
>  
>  /**
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Ville Syrjälä]

On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
> On Wed, May 02, 2018 at 12:20:20PM +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, regardless 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. However if such a mode is unique in the list, it is
> >  kept in the list, with aspect-ratio flags reset.
> > -prepares a list of exposed modes, which is used to find unique modes
> >  if aspect-ratio is not allowed.
> > -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
> >  the list of exposed modes.
> > 
> > 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).
> > V6: As suggested by Ville, corrected the mode pruning logic and
> >     elaborated the mode pruning logic and the assumptions taken.
> > V7: rebase
> > V8: rebase
> > V9: rebase
> > V10: rebase
> > V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
> >      logic to correctly identify and prune modes with aspect-ratio,
> >      if aspect-ratio cap is not set.
> > V12: As suggested by Ville, added another list_head in
> >      drm_mode_display to traverse the list of exposed modes and
> >      avoided duplication of modes.
> > V13: Minor modifications, as suggested by Ville.
> > ---
> >  drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++-------
> >  include/drm/drm_modes.h         | 13 ++++++++++++
> >  2 files changed, 51 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index dfc8ca1..8ca1149 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1531,15 +1531,36 @@ 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 *mode,
> > +			     const struct list_head *export_list,
> > +			     const struct drm_file *file_priv)
> >  {
> >  	/*
> >  	 * If user-space hasn't configured the driver to expose the stereo 3D
> >  	 * modes, don't expose them.
> >  	 */
> > +
> >  	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. However if such a mode
> > +	 * is unique, let it be exposed, but reset the aspect-ratio flags
> > +	 * while preparing the list of user-modes.
> > +	 */
> > +	if (!file_priv->aspect_ratio_allowed &&
> > +	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
> > +		struct drm_display_mode *mode_itr;
> > +
> > +		list_for_each_entry(mode_itr, export_list, export_head)
> 
> By walking the list of only the modes already added to the export list we
> rely on ASPECT_NONE being first if present. That seems to be a bit
> fragile. If we instead walk over all present modes (i.e. connector->modes)
> then I think that's avoided.

I don't think that would work. If we just walk over all the modes then
wouldn't we always find a duplicate when the same mode with two different
aspect ratios is on the list? And then we'd export neither? Or maybe I
misunderstood what you mean here.

To me it seems like the correct option is to check against the
export_list unconditionally, no matter whether the current mode has the
aspect ratio set or not. If an identical mode is already on the list
we don't export again, if it's not there then we export.

> With that changed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > +			if (drm_mode_match(mode_itr, mode,
> > +					   DRM_MODE_MATCH_TIMINGS |
> > +					   DRM_MODE_MATCH_CLOCK |
> > +					   DRM_MODE_MATCH_FLAGS |
> > +					   DRM_MODE_MATCH_3D_FLAGS))
> > +				return false;
> > +	}
> >  
> >  	return true;
> >  }
> > @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> >  	struct drm_mode_modeinfo u_mode;
> >  	struct drm_mode_modeinfo __user *mode_ptr;
> >  	uint32_t __user *encoder_ptr;
> > +	LIST_HEAD(export_list);
> >  
> >  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >  		return -EINVAL;
> > @@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> >  
> >  	/* delayed so we get modes regardless of pre-fill_modes state */
> >  	list_for_each_entry(mode, &connector->modes, head)
> > -		if (drm_mode_expose_to_userspace(mode, file_priv))
> > +		if (drm_mode_expose_to_userspace(mode, &export_list,
> > +						 file_priv)) {
> > +			list_add_tail(&mode->export_head, &export_list);
> >  			mode_count++;
> > +		}
> >  
> >  	/*
> >  	 * This ioctl is called twice, once to determine how much space is
> >  	 * needed, and the 2nd time to fill it.
> > +	 * The modes that need to be exposed to the user are maintained in the
> > +	 * 'export_list'. When the ioctl is called first time to determine the,
> > +	 * space, the export_list gets filled, to find the no.of modes. In the
> > +	 * 2nd time, the user modes are filled, one by one from the export_list.
> >  	 */
> >  	if ((out_resp->count_modes >= mode_count) && mode_count) {
> >  		copied = 0;
> >  		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
> > -		list_for_each_entry(mode, &connector->modes, head) {
> > -			if (!drm_mode_expose_to_userspace(mode, file_priv))
> > -				continue;
> > -
> > +		list_for_each_entry(mode, &export_list, export_head) {
> >  			drm_mode_convert_to_umode(&u_mode, mode);
> > +			/*
> > +			 * Reset aspect ratio flags of user-mode, if modes with
> > +			 * aspect-ratio are not supported.
> > +			 */
> > +			drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode);
> >  			if (copy_to_user(mode_ptr + copied,
> >  					 &u_mode, sizeof(u_mode))) {
> >  				ret = -EFAULT;
> > diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> > index 2f78b7e..b159fe0 100644
> > --- a/include/drm/drm_modes.h
> > +++ b/include/drm/drm_modes.h
> > @@ -411,6 +411,19 @@ struct drm_display_mode {
> >  	 * Field for setting the HDMI picture aspect ratio of a mode.
> >  	 */
> >  	enum hdmi_picture_aspect picture_aspect_ratio;
> > +
> > +	/**
> > +	 * @export_head:
> > +	 *
> > +	 * struct list_head for modes to be exposed to the userspace.
> > +	 * This is to maintain a list of exposed modes while preparing
> > +	 * user-mode's list in drm_mode_getconnector ioctl. The purpose of this
> > +	 * list_head only lies in the ioctl function, and is not expected to be
> > +	 * used outside the function.
> > +	 * Once used, the stale pointers are not reset, but left as it is, to
> > +	 * avoid overhead of protecting it by mode_config.mutex.
> > +	 */
> > +	struct list_head export_head;
> >  };
> >  
> >  /**
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Nautiyal, Ankit K]

On 5/5/2018 1:49 AM, Ville Syrjälä wrote:
> On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
>> On Wed, May 02, 2018 at 12:20:20PM +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, regardless 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. However if such a mode is unique in the list, it is
>>>   kept in the list, with aspect-ratio flags reset.
>>> -prepares a list of exposed modes, which is used to find unique modes
>>>   if aspect-ratio is not allowed.
>>> -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
>>>   the list of exposed modes.
>>>
>>> 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).
>>> V6: As suggested by Ville, corrected the mode pruning logic and
>>>      elaborated the mode pruning logic and the assumptions taken.
>>> V7: rebase
>>> V8: rebase
>>> V9: rebase
>>> V10: rebase
>>> V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
>>>       logic to correctly identify and prune modes with aspect-ratio,
>>>       if aspect-ratio cap is not set.
>>> V12: As suggested by Ville, added another list_head in
>>>       drm_mode_display to traverse the list of exposed modes and
>>>       avoided duplication of modes.
>>> V13: Minor modifications, as suggested by Ville.
>>> ---
>>>   drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++-------
>>>   include/drm/drm_modes.h         | 13 ++++++++++++
>>>   2 files changed, 51 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>> index dfc8ca1..8ca1149 100644
>>> --- a/drivers/gpu/drm/drm_connector.c
>>> +++ b/drivers/gpu/drm/drm_connector.c
>>> @@ -1531,15 +1531,36 @@ 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 *mode,
>>> +			     const struct list_head *export_list,
>>> +			     const struct drm_file *file_priv)
>>>   {
>>>   	/*
>>>   	 * If user-space hasn't configured the driver to expose the stereo 3D
>>>   	 * modes, don't expose them.
>>>   	 */
>>> +
>>>   	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. However if such a mode
>>> +	 * is unique, let it be exposed, but reset the aspect-ratio flags
>>> +	 * while preparing the list of user-modes.
>>> +	 */
>>> +	if (!file_priv->aspect_ratio_allowed &&
>>> +	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
>>> +		struct drm_display_mode *mode_itr;
>>> +
>>> +		list_for_each_entry(mode_itr, export_list, export_head)
>> By walking the list of only the modes already added to the export list we
>> rely on ASPECT_NONE being first if present. That seems to be a bit
>> fragile. If we instead walk over all present modes (i.e. connector->modes)
>> then I think that's avoided.
> I don't think that would work. If we just walk over all the modes then
> wouldn't we always find a duplicate when the same mode with two different
> aspect ratios is on the list? And then we'd export neither? Or maybe I
> misunderstood what you mean here.
>
> To me it seems like the correct option is to check against the
> export_list unconditionally, no matter whether the current mode has the
> aspect ratio set or not. If an identical mode is already on the list
> we don't export again, if it's not there then we export.

Agreed. The current code does have a problem rightly pointed by Daniel 
Vetter and the main reason
for that is - we are checking for duplicates only if the mode has some 
aspect-ratio.
As you have suggested, If we can do away with the condition to check for 
duplicates for only
"modes having aspect ratio", it will solve the problem.
In that case, if the aspect-ratio cap is not set, we will be removing 
the duplicates for all modes, irrespective
of having aspect-ratio information or not, but if aspect-ratio is 
allowed, we would do no such checking.

Or do you suggest that we remove duplicates in any case, whether 
aspect-ratio cap is set by the user or not?

>
>> With that changed:
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>>> +			if (drm_mode_match(mode_itr, mode,
>>> +					   DRM_MODE_MATCH_TIMINGS |
>>> +					   DRM_MODE_MATCH_CLOCK |
>>> +					   DRM_MODE_MATCH_FLAGS |
>>> +					   DRM_MODE_MATCH_3D_FLAGS))
>>> +				return false;
>>> +	}
>>>   
>>>   	return true;
>>>   }
>>> @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>>   	struct drm_mode_modeinfo u_mode;
>>>   	struct drm_mode_modeinfo __user *mode_ptr;
>>>   	uint32_t __user *encoder_ptr;
>>> +	LIST_HEAD(export_list);
>>>   
>>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>   		return -EINVAL;
>>> @@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>>   
>>>   	/* delayed so we get modes regardless of pre-fill_modes state */
>>>   	list_for_each_entry(mode, &connector->modes, head)
>>> -		if (drm_mode_expose_to_userspace(mode, file_priv))
>>> +		if (drm_mode_expose_to_userspace(mode, &export_list,
>>> +						 file_priv)) {
>>> +			list_add_tail(&mode->export_head, &export_list);
>>>   			mode_count++;
>>> +		}
>>>   
>>>   	/*
>>>   	 * This ioctl is called twice, once to determine how much space is
>>>   	 * needed, and the 2nd time to fill it.
>>> +	 * The modes that need to be exposed to the user are maintained in the
>>> +	 * 'export_list'. When the ioctl is called first time to determine the,
>>> +	 * space, the export_list gets filled, to find the no.of modes. In the
>>> +	 * 2nd time, the user modes are filled, one by one from the export_list.
>>>   	 */
>>>   	if ((out_resp->count_modes >= mode_count) && mode_count) {
>>>   		copied = 0;
>>>   		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
>>> -		list_for_each_entry(mode, &connector->modes, head) {
>>> -			if (!drm_mode_expose_to_userspace(mode, file_priv))
>>> -				continue;
>>> -
>>> +		list_for_each_entry(mode, &export_list, export_head) {
>>>   			drm_mode_convert_to_umode(&u_mode, mode);
>>> +			/*
>>> +			 * Reset aspect ratio flags of user-mode, if modes with
>>> +			 * aspect-ratio are not supported.
>>> +			 */
>>> +			drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode);
>>>   			if (copy_to_user(mode_ptr + copied,
>>>   					 &u_mode, sizeof(u_mode))) {
>>>   				ret = -EFAULT;
>>> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
>>> index 2f78b7e..b159fe0 100644
>>> --- a/include/drm/drm_modes.h
>>> +++ b/include/drm/drm_modes.h
>>> @@ -411,6 +411,19 @@ struct drm_display_mode {
>>>   	 * Field for setting the HDMI picture aspect ratio of a mode.
>>>   	 */
>>>   	enum hdmi_picture_aspect picture_aspect_ratio;
>>> +
>>> +	/**
>>> +	 * @export_head:
>>> +	 *
>>> +	 * struct list_head for modes to be exposed to the userspace.
>>> +	 * This is to maintain a list of exposed modes while preparing
>>> +	 * user-mode's list in drm_mode_getconnector ioctl. The purpose of this
>>> +	 * list_head only lies in the ioctl function, and is not expected to be
>>> +	 * used outside the function.
>>> +	 * Once used, the stale pointers are not reset, but left as it is, to
>>> +	 * avoid overhead of protecting it by mode_config.mutex.
>>> +	 */
>>> +	struct list_head export_head;
>>>   };
>>>   
>>>   /**
>>> -- 
>>> 2.7.4
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Ville Syrjälä]

On Mon, May 07, 2018 at 10:34:53AM +0530, Nautiyal, Ankit K wrote:
> 
> 
> On 5/5/2018 1:49 AM, Ville Syrjälä wrote:
> > On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
> >> On Wed, May 02, 2018 at 12:20:20PM +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, regardless 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. However if such a mode is unique in the list, it is
> >>>   kept in the list, with aspect-ratio flags reset.
> >>> -prepares a list of exposed modes, which is used to find unique modes
> >>>   if aspect-ratio is not allowed.
> >>> -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
> >>>   the list of exposed modes.
> >>>
> >>> 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).
> >>> V6: As suggested by Ville, corrected the mode pruning logic and
> >>>      elaborated the mode pruning logic and the assumptions taken.
> >>> V7: rebase
> >>> V8: rebase
> >>> V9: rebase
> >>> V10: rebase
> >>> V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
> >>>       logic to correctly identify and prune modes with aspect-ratio,
> >>>       if aspect-ratio cap is not set.
> >>> V12: As suggested by Ville, added another list_head in
> >>>       drm_mode_display to traverse the list of exposed modes and
> >>>       avoided duplication of modes.
> >>> V13: Minor modifications, as suggested by Ville.
> >>> ---
> >>>   drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++-------
> >>>   include/drm/drm_modes.h         | 13 ++++++++++++
> >>>   2 files changed, 51 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >>> index dfc8ca1..8ca1149 100644
> >>> --- a/drivers/gpu/drm/drm_connector.c
> >>> +++ b/drivers/gpu/drm/drm_connector.c
> >>> @@ -1531,15 +1531,36 @@ 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 *mode,
> >>> +			     const struct list_head *export_list,
> >>> +			     const struct drm_file *file_priv)
> >>>   {
> >>>   	/*
> >>>   	 * If user-space hasn't configured the driver to expose the stereo 3D
> >>>   	 * modes, don't expose them.
> >>>   	 */
> >>> +
> >>>   	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. However if such a mode
> >>> +	 * is unique, let it be exposed, but reset the aspect-ratio flags
> >>> +	 * while preparing the list of user-modes.
> >>> +	 */
> >>> +	if (!file_priv->aspect_ratio_allowed &&
> >>> +	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
> >>> +		struct drm_display_mode *mode_itr;
> >>> +
> >>> +		list_for_each_entry(mode_itr, export_list, export_head)
> >> By walking the list of only the modes already added to the export list we
> >> rely on ASPECT_NONE being first if present. That seems to be a bit
> >> fragile. If we instead walk over all present modes (i.e. connector->modes)
> >> then I think that's avoided.
> > I don't think that would work. If we just walk over all the modes then
> > wouldn't we always find a duplicate when the same mode with two different
> > aspect ratios is on the list? And then we'd export neither? Or maybe I
> > misunderstood what you mean here.
> >
> > To me it seems like the correct option is to check against the
> > export_list unconditionally, no matter whether the current mode has the
> > aspect ratio set or not. If an identical mode is already on the list
> > we don't export again, if it's not there then we export.
> 
> Agreed. The current code does have a problem rightly pointed by Daniel 
> Vetter and the main reason
> for that is - we are checking for duplicates only if the mode has some 
> aspect-ratio.
> As you have suggested, If we can do away with the condition to check for 
> duplicates for only
> "modes having aspect ratio", it will solve the problem.
> In that case, if the aspect-ratio cap is not set, we will be removing 
> the duplicates for all modes, irrespective
> of having aspect-ratio information or not, but if aspect-ratio is 
> allowed, we would do no such checking.
> 
> Or do you suggest that we remove duplicates in any case, whether 
> aspect-ratio cap is set by the user or not?

If there are duplicates on the connector mode list we've done something
wrong when populating the list. So no, we should not have to check for
duplicates in that case.

> 
> >
> >> With that changed:
> >>
> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >>> +			if (drm_mode_match(mode_itr, mode,
> >>> +					   DRM_MODE_MATCH_TIMINGS |
> >>> +					   DRM_MODE_MATCH_CLOCK |
> >>> +					   DRM_MODE_MATCH_FLAGS |
> >>> +					   DRM_MODE_MATCH_3D_FLAGS))
> >>> +				return false;
> >>> +	}
> >>>   
> >>>   	return true;
> >>>   }
> >>> @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> >>>   	struct drm_mode_modeinfo u_mode;
> >>>   	struct drm_mode_modeinfo __user *mode_ptr;
> >>>   	uint32_t __user *encoder_ptr;
> >>> +	LIST_HEAD(export_list);
> >>>   
> >>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>>   		return -EINVAL;
> >>> @@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> >>>   
> >>>   	/* delayed so we get modes regardless of pre-fill_modes state */
> >>>   	list_for_each_entry(mode, &connector->modes, head)
> >>> -		if (drm_mode_expose_to_userspace(mode, file_priv))
> >>> +		if (drm_mode_expose_to_userspace(mode, &export_list,
> >>> +						 file_priv)) {
> >>> +			list_add_tail(&mode->export_head, &export_list);
> >>>   			mode_count++;
> >>> +		}
> >>>   
> >>>   	/*
> >>>   	 * This ioctl is called twice, once to determine how much space is
> >>>   	 * needed, and the 2nd time to fill it.
> >>> +	 * The modes that need to be exposed to the user are maintained in the
> >>> +	 * 'export_list'. When the ioctl is called first time to determine the,
> >>> +	 * space, the export_list gets filled, to find the no.of modes. In the
> >>> +	 * 2nd time, the user modes are filled, one by one from the export_list.
> >>>   	 */
> >>>   	if ((out_resp->count_modes >= mode_count) && mode_count) {
> >>>   		copied = 0;
> >>>   		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
> >>> -		list_for_each_entry(mode, &connector->modes, head) {
> >>> -			if (!drm_mode_expose_to_userspace(mode, file_priv))
> >>> -				continue;
> >>> -
> >>> +		list_for_each_entry(mode, &export_list, export_head) {
> >>>   			drm_mode_convert_to_umode(&u_mode, mode);
> >>> +			/*
> >>> +			 * Reset aspect ratio flags of user-mode, if modes with
> >>> +			 * aspect-ratio are not supported.
> >>> +			 */
> >>> +			drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode);
> >>>   			if (copy_to_user(mode_ptr + copied,
> >>>   					 &u_mode, sizeof(u_mode))) {
> >>>   				ret = -EFAULT;
> >>> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> >>> index 2f78b7e..b159fe0 100644
> >>> --- a/include/drm/drm_modes.h
> >>> +++ b/include/drm/drm_modes.h
> >>> @@ -411,6 +411,19 @@ struct drm_display_mode {
> >>>   	 * Field for setting the HDMI picture aspect ratio of a mode.
> >>>   	 */
> >>>   	enum hdmi_picture_aspect picture_aspect_ratio;
> >>> +
> >>> +	/**
> >>> +	 * @export_head:
> >>> +	 *
> >>> +	 * struct list_head for modes to be exposed to the userspace.
> >>> +	 * This is to maintain a list of exposed modes while preparing
> >>> +	 * user-mode's list in drm_mode_getconnector ioctl. The purpose of this
> >>> +	 * list_head only lies in the ioctl function, and is not expected to be
> >>> +	 * used outside the function.
> >>> +	 * Once used, the stale pointers are not reset, but left as it is, to
> >>> +	 * avoid overhead of protecting it by mode_config.mutex.
> >>> +	 */
> >>> +	struct list_head export_head;
> >>>   };
> >>>   
> >>>   /**
> >>> -- 
> >>> 2.7.4
> >>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >> -- 
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Nautiyal, Ankit K]

On 5/7/2018 5:58 PM, Ville Syrjälä wrote:
> On Mon, May 07, 2018 at 10:34:53AM +0530, Nautiyal, Ankit K wrote:
>>
>> On 5/5/2018 1:49 AM, Ville Syrjälä wrote:
>>> On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
>>>> On Wed, May 02, 2018 at 12:20:20PM +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, regardless 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. However if such a mode is unique in the list, it is
>>>>>    kept in the list, with aspect-ratio flags reset.
>>>>> -prepares a list of exposed modes, which is used to find unique modes
>>>>>    if aspect-ratio is not allowed.
>>>>> -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
>>>>>    the list of exposed modes.
>>>>>
>>>>> 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).
>>>>> V6: As suggested by Ville, corrected the mode pruning logic and
>>>>>       elaborated the mode pruning logic and the assumptions taken.
>>>>> V7: rebase
>>>>> V8: rebase
>>>>> V9: rebase
>>>>> V10: rebase
>>>>> V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
>>>>>        logic to correctly identify and prune modes with aspect-ratio,
>>>>>        if aspect-ratio cap is not set.
>>>>> V12: As suggested by Ville, added another list_head in
>>>>>        drm_mode_display to traverse the list of exposed modes and
>>>>>        avoided duplication of modes.
>>>>> V13: Minor modifications, as suggested by Ville.
>>>>> ---
>>>>>    drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++-------
>>>>>    include/drm/drm_modes.h         | 13 ++++++++++++
>>>>>    2 files changed, 51 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>>> index dfc8ca1..8ca1149 100644
>>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>>> @@ -1531,15 +1531,36 @@ 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 *mode,
>>>>> +			     const struct list_head *export_list,
>>>>> +			     const struct drm_file *file_priv)
>>>>>    {
>>>>>    	/*
>>>>>    	 * If user-space hasn't configured the driver to expose the stereo 3D
>>>>>    	 * modes, don't expose them.
>>>>>    	 */
>>>>> +
>>>>>    	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. However if such a mode
>>>>> +	 * is unique, let it be exposed, but reset the aspect-ratio flags
>>>>> +	 * while preparing the list of user-modes.
>>>>> +	 */
>>>>> +	if (!file_priv->aspect_ratio_allowed &&
>>>>> +	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
>>>>> +		struct drm_display_mode *mode_itr;
>>>>> +
>>>>> +		list_for_each_entry(mode_itr, export_list, export_head)
>>>> By walking the list of only the modes already added to the export list we
>>>> rely on ASPECT_NONE being first if present. That seems to be a bit
>>>> fragile. If we instead walk over all present modes (i.e. connector->modes)
>>>> then I think that's avoided.
>>> I don't think that would work. If we just walk over all the modes then
>>> wouldn't we always find a duplicate when the same mode with two different
>>> aspect ratios is on the list? And then we'd export neither? Or maybe I
>>> misunderstood what you mean here.
>>>
>>> To me it seems like the correct option is to check against the
>>> export_list unconditionally, no matter whether the current mode has the
>>> aspect ratio set or not. If an identical mode is already on the list
>>> we don't export again, if it's not there then we export.
>> Agreed. The current code does have a problem rightly pointed by Daniel
>> Vetter and the main reason
>> for that is - we are checking for duplicates only if the mode has some
>> aspect-ratio.
>> As you have suggested, If we can do away with the condition to check for
>> duplicates for only
>> "modes having aspect ratio", it will solve the problem.
>> In that case, if the aspect-ratio cap is not set, we will be removing
>> the duplicates for all modes, irrespective
>> of having aspect-ratio information or not, but if aspect-ratio is
>> allowed, we would do no such checking.
>>
>> Or do you suggest that we remove duplicates in any case, whether
>> aspect-ratio cap is set by the user or not?
> If there are duplicates on the connector mode list we've done something
> wrong when populating the list. So no, we should not have to check for
> duplicates in that case.

Alright. So I'll just drop the condition to check the mode's 
aspect-ratio and so the export_list will be checked unconditionally, if 
aspect-ratio cap is not set.
However if the aspect-ratio cap is set, we just move the mode to 
export-list without checking for its duplicates.

I will send the next-patch set with this change, along with the removal 
of helper functions as discussed earlier.

>
>>>> With that changed:
>>>>
>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>
>>>>> +			if (drm_mode_match(mode_itr, mode,
>>>>> +					   DRM_MODE_MATCH_TIMINGS |
>>>>> +					   DRM_MODE_MATCH_CLOCK |
>>>>> +					   DRM_MODE_MATCH_FLAGS |
>>>>> +					   DRM_MODE_MATCH_3D_FLAGS))
>>>>> +				return false;
>>>>> +	}
>>>>>    
>>>>>    	return true;
>>>>>    }
>>>>> @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>>>>    	struct drm_mode_modeinfo u_mode;
>>>>>    	struct drm_mode_modeinfo __user *mode_ptr;
>>>>>    	uint32_t __user *encoder_ptr;
>>>>> +	LIST_HEAD(export_list);
>>>>>    
>>>>>    	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>>    		return -EINVAL;
>>>>> @@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>>>>    
>>>>>    	/* delayed so we get modes regardless of pre-fill_modes state */
>>>>>    	list_for_each_entry(mode, &connector->modes, head)
>>>>> -		if (drm_mode_expose_to_userspace(mode, file_priv))
>>>>> +		if (drm_mode_expose_to_userspace(mode, &export_list,
>>>>> +						 file_priv)) {
>>>>> +			list_add_tail(&mode->export_head, &export_list);
>>>>>    			mode_count++;
>>>>> +		}
>>>>>    
>>>>>    	/*
>>>>>    	 * This ioctl is called twice, once to determine how much space is
>>>>>    	 * needed, and the 2nd time to fill it.
>>>>> +	 * The modes that need to be exposed to the user are maintained in the
>>>>> +	 * 'export_list'. When the ioctl is called first time to determine the,
>>>>> +	 * space, the export_list gets filled, to find the no.of modes. In the
>>>>> +	 * 2nd time, the user modes are filled, one by one from the export_list.
>>>>>    	 */
>>>>>    	if ((out_resp->count_modes >= mode_count) && mode_count) {
>>>>>    		copied = 0;
>>>>>    		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
>>>>> -		list_for_each_entry(mode, &connector->modes, head) {
>>>>> -			if (!drm_mode_expose_to_userspace(mode, file_priv))
>>>>> -				continue;
>>>>> -
>>>>> +		list_for_each_entry(mode, &export_list, export_head) {
>>>>>    			drm_mode_convert_to_umode(&u_mode, mode);
>>>>> +			/*
>>>>> +			 * Reset aspect ratio flags of user-mode, if modes with
>>>>> +			 * aspect-ratio are not supported.
>>>>> +			 */
>>>>> +			drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode);
>>>>>    			if (copy_to_user(mode_ptr + copied,
>>>>>    					 &u_mode, sizeof(u_mode))) {
>>>>>    				ret = -EFAULT;
>>>>> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
>>>>> index 2f78b7e..b159fe0 100644
>>>>> --- a/include/drm/drm_modes.h
>>>>> +++ b/include/drm/drm_modes.h
>>>>> @@ -411,6 +411,19 @@ struct drm_display_mode {
>>>>>    	 * Field for setting the HDMI picture aspect ratio of a mode.
>>>>>    	 */
>>>>>    	enum hdmi_picture_aspect picture_aspect_ratio;
>>>>> +
>>>>> +	/**
>>>>> +	 * @export_head:
>>>>> +	 *
>>>>> +	 * struct list_head for modes to be exposed to the userspace.
>>>>> +	 * This is to maintain a list of exposed modes while preparing
>>>>> +	 * user-mode's list in drm_mode_getconnector ioctl. The purpose of this
>>>>> +	 * list_head only lies in the ioctl function, and is not expected to be
>>>>> +	 * used outside the function.
>>>>> +	 * Once used, the stale pointers are not reset, but left as it is, to
>>>>> +	 * avoid overhead of protecting it by mode_config.mutex.
>>>>> +	 */
>>>>> +	struct list_head export_head;
>>>>>    };
>>>>>    
>>>>>    /**
>>>>> -- 
>>>>> 2.7.4
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>> -- 
>>>> Daniel Vetter
>>>> Software Engineer, Intel Corporation
>>>> http://blog.ffwll.ch
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Daniel Vetter]

On Fri, May 04, 2018 at 11:19:45PM +0300, Ville Syrjälä wrote:
> On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
> > On Wed, May 02, 2018 at 12:20:20PM +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, regardless 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. However if such a mode is unique in the list, it is
> > >  kept in the list, with aspect-ratio flags reset.
> > > -prepares a list of exposed modes, which is used to find unique modes
> > >  if aspect-ratio is not allowed.
> > > -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
> > >  the list of exposed modes.
> > > 
> > > 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).
> > > V6: As suggested by Ville, corrected the mode pruning logic and
> > >     elaborated the mode pruning logic and the assumptions taken.
> > > V7: rebase
> > > V8: rebase
> > > V9: rebase
> > > V10: rebase
> > > V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
> > >      logic to correctly identify and prune modes with aspect-ratio,
> > >      if aspect-ratio cap is not set.
> > > V12: As suggested by Ville, added another list_head in
> > >      drm_mode_display to traverse the list of exposed modes and
> > >      avoided duplication of modes.
> > > V13: Minor modifications, as suggested by Ville.
> > > ---
> > >  drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++-------
> > >  include/drm/drm_modes.h         | 13 ++++++++++++
> > >  2 files changed, 51 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index dfc8ca1..8ca1149 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -1531,15 +1531,36 @@ 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 *mode,
> > > +			     const struct list_head *export_list,
> > > +			     const struct drm_file *file_priv)
> > >  {
> > >  	/*
> > >  	 * If user-space hasn't configured the driver to expose the stereo 3D
> > >  	 * modes, don't expose them.
> > >  	 */
> > > +
> > >  	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. However if such a mode
> > > +	 * is unique, let it be exposed, but reset the aspect-ratio flags
> > > +	 * while preparing the list of user-modes.
> > > +	 */
> > > +	if (!file_priv->aspect_ratio_allowed &&
> > > +	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
> > > +		struct drm_display_mode *mode_itr;
> > > +
> > > +		list_for_each_entry(mode_itr, export_list, export_head)
> > 
> > By walking the list of only the modes already added to the export list we
> > rely on ASPECT_NONE being first if present. That seems to be a bit
> > fragile. If we instead walk over all present modes (i.e. connector->modes)
> > then I think that's avoided.
> 
> I don't think that would work. If we just walk over all the modes then
> wouldn't we always find a duplicate when the same mode with two different
> aspect ratios is on the list? And then we'd export neither? Or maybe I
> misunderstood what you mean here.

We'd export both. Let's assume we have 2 modes, only difference is aspect
ratio.

1. We check the first mode, it has aspect_ratio != NONE. But because
there's no other exported mode yet on export_list, we export it (and
filter out the aspect_ratio by setting it to NONE).

2. 2nd mode has aspect_ratio == NONE, so we export it.

Results in duped modes exported. If you always guarantee that aspect_ratio
== NONE is before the same mode with aspect ratio, then this algo here
works. But not for the more general case I think.

> To me it seems like the correct option is to check against the
> export_list unconditionally, no matter whether the current mode has the
> aspect ratio set or not. If an identical mode is already on the list
> we don't export again, if it's not there then we export.

That might be another option to fix the bug.
-Daniel

> 
> > With that changed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > > +			if (drm_mode_match(mode_itr, mode,
> > > +					   DRM_MODE_MATCH_TIMINGS |
> > > +					   DRM_MODE_MATCH_CLOCK |
> > > +					   DRM_MODE_MATCH_FLAGS |
> > > +					   DRM_MODE_MATCH_3D_FLAGS))
> > > +				return false;
> > > +	}
> > >  
> > >  	return true;
> > >  }
> > > @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> > >  	struct drm_mode_modeinfo u_mode;
> > >  	struct drm_mode_modeinfo __user *mode_ptr;
> > >  	uint32_t __user *encoder_ptr;
> > > +	LIST_HEAD(export_list);
> > >  
> > >  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > >  		return -EINVAL;
> > > @@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> > >  
> > >  	/* delayed so we get modes regardless of pre-fill_modes state */
> > >  	list_for_each_entry(mode, &connector->modes, head)
> > > -		if (drm_mode_expose_to_userspace(mode, file_priv))
> > > +		if (drm_mode_expose_to_userspace(mode, &export_list,
> > > +						 file_priv)) {
> > > +			list_add_tail(&mode->export_head, &export_list);
> > >  			mode_count++;
> > > +		}
> > >  
> > >  	/*
> > >  	 * This ioctl is called twice, once to determine how much space is
> > >  	 * needed, and the 2nd time to fill it.
> > > +	 * The modes that need to be exposed to the user are maintained in the
> > > +	 * 'export_list'. When the ioctl is called first time to determine the,
> > > +	 * space, the export_list gets filled, to find the no.of modes. In the
> > > +	 * 2nd time, the user modes are filled, one by one from the export_list.
> > >  	 */
> > >  	if ((out_resp->count_modes >= mode_count) && mode_count) {
> > >  		copied = 0;
> > >  		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
> > > -		list_for_each_entry(mode, &connector->modes, head) {
> > > -			if (!drm_mode_expose_to_userspace(mode, file_priv))
> > > -				continue;
> > > -
> > > +		list_for_each_entry(mode, &export_list, export_head) {
> > >  			drm_mode_convert_to_umode(&u_mode, mode);
> > > +			/*
> > > +			 * Reset aspect ratio flags of user-mode, if modes with
> > > +			 * aspect-ratio are not supported.
> > > +			 */
> > > +			drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode);
> > >  			if (copy_to_user(mode_ptr + copied,
> > >  					 &u_mode, sizeof(u_mode))) {
> > >  				ret = -EFAULT;
> > > diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> > > index 2f78b7e..b159fe0 100644
> > > --- a/include/drm/drm_modes.h
> > > +++ b/include/drm/drm_modes.h
> > > @@ -411,6 +411,19 @@ struct drm_display_mode {
> > >  	 * Field for setting the HDMI picture aspect ratio of a mode.
> > >  	 */
> > >  	enum hdmi_picture_aspect picture_aspect_ratio;
> > > +
> > > +	/**
> > > +	 * @export_head:
> > > +	 *
> > > +	 * struct list_head for modes to be exposed to the userspace.
> > > +	 * This is to maintain a list of exposed modes while preparing
> > > +	 * user-mode's list in drm_mode_getconnector ioctl. The purpose of this
> > > +	 * list_head only lies in the ioctl function, and is not expected to be
> > > +	 * used outside the function.
> > > +	 * Once used, the stale pointers are not reset, but left as it is, to
> > > +	 * avoid overhead of protecting it by mode_config.mutex.
> > > +	 */
> > > +	struct list_head export_head;
> > >  };
> > >  
> > >  /**
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v13 09/10] 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
V6: As recommended by Ville, avoided matching of aspect-ratio in
    drm_fb_helper, while trying to find a common mode among connectors
    for the target clone mode.
V7: rebase
V8: rebase
V9: rebase
V10: rebase
V11: rebase
V12: rebase
V13: rebase
---
 drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++--
 drivers/gpu/drm/drm_modes.c     | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0646b10..2ee1eaa 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2183,7 +2183,11 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
 		for (j = 0; j < i; j++) {
 			if (!enabled[j])
 				continue;
-			if (!drm_mode_equal(modes[j], modes[i]))
+			if (!drm_mode_match(modes[j], modes[i],
+					    DRM_MODE_MATCH_TIMINGS |
+					    DRM_MODE_MATCH_CLOCK |
+					    DRM_MODE_MATCH_FLAGS |
+					    DRM_MODE_MATCH_3D_FLAGS))
 				can_clone = false;
 		}
 	}
@@ -2203,7 +2207,11 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
 
 		fb_helper_conn = fb_helper->connector_info[i];
 		list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) {
-			if (drm_mode_equal(mode, dmt_mode))
+			if (drm_mode_match(mode, dmt_mode,
+					   DRM_MODE_MATCH_TIMINGS |
+					   DRM_MODE_MATCH_CLOCK |
+					   DRM_MODE_MATCH_FLAGS |
+					   DRM_MODE_MATCH_3D_FLAGS))
 				modes[i] = mode;
 		}
 		if (!modes[i])
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 2bf2f0b..c91aec5 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1049,7 +1049,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);
 
@@ -1647,6 +1648,20 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 	out->vrefresh = in->vrefresh;
 	out->flags = in->flags;
 	out->type = in->type;
+
+	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 +1708,24 @@ 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,
+	 * as the aspect-ratio information is not stored in
+	 * flags for kernel-mode, but in picture_aspect_ratio.
+	 */
+	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)
 		return -EINVAL;
-- 
2.7.4

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

[PATCH v13 10/10] 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.
V6: rebase
V7: rebase
V8: rebase
V9: rebase
V10: rebase
V11: rebase
V12: rebase
V13: rebase
---
 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 c91aec5..9d70bc0 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1656,6 +1656,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;
@@ -1721,6 +1727,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 50bcf42..4b3a1bb 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for Aspect ratio support in DRM layer

[Patchwork]

== Series Details ==

Series: Aspect ratio support in DRM layer
URL   : https://patchwork.freedesktop.org/series/42546/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
10a7bea36941 drm/modes: Introduce drm_mode_match()
-:39: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#39: FILE: drivers/gpu/drm/drm_modes.c:958:
+static bool drm_mode_match_clock(const struct drm_display_mode *mode1,
+				  const struct drm_display_mode *mode2)

total: 0 errors, 0 warnings, 1 checks, 190 lines checked
2f25a53751d3 drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy
7746cbe0b7a5 drm/edid: Fix cea mode aspect ratio handling
023012e2f0d0 drm/edid: Don't send bogus aspect ratios in AVI infoframes
67936f9d93c9 video/hdmi: Reject illegal picture aspect ratios
4c1ac075fbaf drm: Add DRM client cap for aspect-ratio
7188f4f6cd36 drm: Handle aspect ratio info in legacy modeset path
3e6df2a97a1a drm: Expose modes with aspect ratio, only if requested
e65cee05eba3 drm: Add aspect ratio parsing in DRM layer
-:89: CHECK:SPACING: space preferred before that '|' (ctx:VxE)
#89: FILE: drivers/gpu/drm/drm_modes.c:1052:
+			      DRM_MODE_MATCH_3D_FLAGS|
 			                             ^

total: 0 errors, 0 warnings, 1 checks, 77 lines checked
199615dff0c9 drm: Add and handle new aspect ratios in DRM layer
-:92: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#92: FILE: include/uapi/drm/drm_mode.h:108:
+			(DRM_MODE_PICTURE_ASPECT_64_27<<19)
 			                              ^

-:94: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#94: FILE: include/uapi/drm/drm_mode.h:110:
+			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
 			                                ^

total: 0 errors, 0 warnings, 2 checks, 42 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Aspect ratio support in DRM layer

[Patchwork]

== Series Details ==

Series: Aspect ratio support in DRM layer
URL   : https://patchwork.freedesktop.org/series/42546/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4118 -> Patchwork_8865 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8865 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8865, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42546/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8865:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_8865 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         PASS -> FAIL (fdo#102575)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@read-crc-pipe-c-frame-sequence:
      fi-skl-6770hq:      FAIL (fdo#103481) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-skl-6700k2:      FAIL (fdo#103191) -> PASS

    
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (39 -> 36) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4118 -> Patchwork_8865

  CI_DRM_4118: 56959c011ac38f9cfd820b991fde970ae38653f4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8865: 199615dff0c96acadfe52d9ccee63a50ad2c5376 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

199615dff0c9 drm: Add and handle new aspect ratios in DRM layer
e65cee05eba3 drm: Add aspect ratio parsing in DRM layer
3e6df2a97a1a drm: Expose modes with aspect ratio, only if requested
7188f4f6cd36 drm: Handle aspect ratio info in legacy modeset path
4c1ac075fbaf drm: Add DRM client cap for aspect-ratio
67936f9d93c9 video/hdmi: Reject illegal picture aspect ratios
023012e2f0d0 drm/edid: Don't send bogus aspect ratios in AVI infoframes
7746cbe0b7a5 drm/edid: Fix cea mode aspect ratio handling
2f25a53751d3 drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy
10a7bea36941 drm/modes: Introduce drm_mode_match()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8865/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Aspect ratio support in DRM layer

[Patchwork]

== Series Details ==

Series: Aspect ratio support in DRM layer
URL   : https://patchwork.freedesktop.org/series/42546/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4118_full -> Patchwork_8865_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8865_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8865_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42546/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8865_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_8865_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_eio@hibernate:
      shard-kbl:          PASS -> DMESG-WARN (fdo#104238) +1

    igt@gem_exec_suspend@basic-s3:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-apl:          PASS -> FAIL (fdo#103375)

    igt@kms_cursor_legacy@flip-vs-cursor-atomic:
      shard-glk:          PASS -> FAIL (fdo#102670)

    igt@kms_flip@2x-plain-flip-ts-check:
      shard-hsw:          PASS -> FAIL (fdo#103928)

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368) +2

    
    ==== Possible fixes ====

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105707) -> PASS

    igt@kms_flip@flip-vs-wf_vblank-interruptible:
      shard-glk:          FAIL (fdo#105312) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#104238 https://bugs.freedesktop.org/show_bug.cgi?id=104238
  fdo#105312 https://bugs.freedesktop.org/show_bug.cgi?id=105312
  fdo#105707 https://bugs.freedesktop.org/show_bug.cgi?id=105707


== Participating hosts (8 -> 7) ==

  Missing    (1): shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4118 -> Patchwork_8865

  CI_DRM_4118: 56959c011ac38f9cfd820b991fde970ae38653f4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8865: 199615dff0c96acadfe52d9ccee63a50ad2c5376 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8865/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx