From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Nautiyal, Ankit K" Subject: Re: [PATCH v10 10/11] drm: Add aspect ratio parsing in DRM layer Date: Fri, 6 Apr 2018 22:55:14 +0530 Message-ID: <73defe1e-9bb6-dd9b-577f-a4fb382cf1e4@intel.com> References: <1523034252-5275-1-git-send-email-ankit.k.nautiyal@intel.com> <1523034252-5275-11-git-send-email-ankit.k.nautiyal@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2075141974==" Return-path: In-Reply-To: <1523034252-5275-11-git-send-email-ankit.k.nautiyal@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org Cc: daniel.vetter@ffwll.ch List-Id: dri-devel@lists.freedesktop.org This is a multi-part message in MIME format. --===============2075141974== Content-Type: multipart/alternative; boundary="------------85C44F121BC59BA4107536EF" This is a multi-part message in MIME format. --------------85C44F121BC59BA4107536EF Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit This patch is causing failure of IGT test kms_3d. The kms_3d test expects the no. of 3d modes to be 13. (The test has hard-coded value for expected no. of 3d modes as 13) But due to the addition of "matching aspect_ratio" in drm_mode_equal in this patch, the total no. of modes in the connector modelist is increased by 2, resulting in failure of assertion 'mode_count==13'. Perhaps this need to be handled in the test. -Regards, Ankit On 4/6/2018 10:34 PM, Nautiyal, Ankit K wrote: > From: "Sharma, Shashank" > > 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 > Signed-off-by: Lin, Jia > Signed-off-by: Akashdeep Sharma > Reviewed-by: Jim Bride (V2) > Reviewed-by: Jose Abreu (V4) > > Cc: Ville Syrjala > Cc: Jim Bride > Cc: Jose Abreu > Cc: Ankit Nautiyal > > 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 > --- > 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 d6133e8..454f2ff 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; --------------85C44F121BC59BA4107536EF Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit

This patch is causing failure of IGT test kms_3d. The kms_3d test expects the no. of 3d modes to be 13.

(The test has hard-coded value for expected no. of 3d modes as 13)

But due to the addition of "matching aspect_ratio" in drm_mode_equal in this patch, the total no. of

modes in the connector modelist is increased by 2, resulting in failure of assertion 'mode_count==13'.

Perhaps this need to be handled in the test.

-Regards,

Ankit


On 4/6/2018 10:34 PM, Nautiyal, Ankit K wrote:
From: "Sharma, Shashank" <shashank.sharma@intel.com>

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

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

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

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

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

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

V3: modified the aspect-ratio check in drm_mode_equal as per new flags
    provided by Ville. https://patchwork.freedesktop.org/patch/188043/
V4: rebase
V5: rebase
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
---
 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 d6133e8..454f2ff 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;

--------------85C44F121BC59BA4107536EF-- --===============2075141974== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============2075141974==--