[PATCH v4 0/4] Picture aspect ratio support in DRM layer

[PATCH v4 0/4] Picture aspect ratio support in DRM layer

[Shashank Sharma]

This patch series adds 4 patches.
- The first two patches add aspect ratio support in DRM layes
- Next two patches add new aspect ratios defined in CEA-861-F
  supported for HDMI 2.0 4k modes.

Adding aspect ratio support in DRM layer:
- The CEA videmodes contain aspect ratio information, which we
  parse when we read the modes from EDID. But while transforming
  user_mode to kernel_mode or viceversa, DRM layer lose this
  information.
- HDMI compliance testing for CEA modes, expects the AVI info frames
  to contain exact VIC no for the 'video mode under test'. Now CEA
  modes have different VIC for same modes but different aspect ratio
  for example:
        VIC 2 = 720x480@60 4:3
        VIC 3 = 720x480@60 16:9
  In this way, lack of aspect ratio information, can cause wrong VIC
  no in AVI IF, causing HDMI complaince test to fail.
- This patch set adds code, which embeds the aspect ratio information
  also in DRM video mode flags, and uses it while comparing two modes.

Adding new aspect ratios for HDMI 2.0
- CEA-861-F defines two new aspect ratios, to be used for 4k HDMI 2.0
  modes.
        - 64:27
        - 256:135
Last two patches in the series, adds code to handle these new
aspect ratios.

V2: Fixed review comments from Sean, Emil, Daniel
V3: Fixed review comments from Jim Bride, got r-b for all patches
V4: Added r-b from Jose for the series, and ack-by from Tomi on patch 3

Shashank Sharma (4):
  drm: add picture aspect ratio flags
  drm: Add aspect ratio parsing in DRM layer
  video: Add new aspect ratios for HDMI 2.0
  drm: Add and handle new aspect ratios in DRM layer

 drivers/gpu/drm/drm_modes.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 drivers/video/hdmi.c        |  4 ++++
 include/linux/hdmi.h        |  2 ++
 include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++-----
 4 files changed, 68 insertions(+), 5 deletions(-)

-- 
1.9.1

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

[PATCH v4 1/4] drm: add picture aspect ratio flags

[Shashank Sharma]

This patch adds drm flag bits for aspect ratio information

Currently drm flag bits don't have field for mode's picture
aspect ratio. This field will help the driver to pick mode with
right aspect ratio, and help in setting right VIC field in avi
infoframes.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Jim Bride <jim.bride@linux.intel.com>
Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>

V2: Addressed review comments from Sean
- Changed PAR-> PIC_AR
V3: Rebase
V3: Added r-b by Jose
---
 include/uapi/drm/drm_mode.h | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index df0e350..5c142b1 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -77,6 +77,19 @@ extern "C" {
 #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
 #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
 
+/* Picture aspect ratio options */
+#define DRM_MODE_PICTURE_ASPECT_NONE		0
+#define DRM_MODE_PICTURE_ASPECT_4_3		1
+#define DRM_MODE_PICTURE_ASPECT_16_9		2
+
+/* Aspect ratio flag bitmask (4 bits 22:19) */
+#define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
+#define  DRM_MODE_FLAG_PIC_AR_NONE \
+			(DRM_MODE_PICTURE_ASPECT_NONE<<19)
+#define  DRM_MODE_FLAG_PIC_AR_4_3 \
+			(DRM_MODE_PICTURE_ASPECT_4_3<<19)
+#define  DRM_MODE_FLAG_PIC_AR_16_9 \
+			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
 
 /* DPMS flags */
 /* bit compatible with the xorg definitions. */
@@ -92,11 +105,6 @@ extern "C" {
 #define DRM_MODE_SCALE_CENTER		2 /* Centered, no scaling */
 #define DRM_MODE_SCALE_ASPECT		3 /* Full screen, preserve aspect */
 
-/* Picture aspect ratio options */
-#define DRM_MODE_PICTURE_ASPECT_NONE	0
-#define DRM_MODE_PICTURE_ASPECT_4_3	1
-#define DRM_MODE_PICTURE_ASPECT_16_9	2
-
 /* Dithering mode options */
 #define DRM_MODE_DITHERING_OFF	0
 #define DRM_MODE_DITHERING_ON	1
-- 
1.9.1

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

[PATCH v4 2/4] drm: Add aspect ratio parsing in DRM layer

[Shashank Sharma]

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

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>
Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>

V2: Addressed review comments from Sean:
- Fix spellings/typo
- No need to handle aspect ratio none
- Add a break, for default case too
V3: Rebase
V4: Added r-b from Jose
---
 drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 53f07ac..fde927a 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 	    mode1->vsync_end == mode2->vsync_end &&
 	    mode1->vtotal == mode2->vtotal &&
 	    mode1->vscan == mode2->vscan &&
+	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
 	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
 	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
 		return true;
@@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 	out->vrefresh = in->vrefresh;
 	out->flags = in->flags;
 	out->type = in->type;
+	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
+
+	switch (in->picture_aspect_ratio) {
+	case HDMI_PICTURE_ASPECT_4_3:
+		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
+		break;
+	case HDMI_PICTURE_ASPECT_16_9:
+		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
+		break;
+	case HDMI_PICTURE_ASPECT_RESERVED:
+	default:
+		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
+		break;
+	}
+
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 }
@@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 
+	/* Clearing picture aspect ratio bits from out flags */
+	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
+
+	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
+	case DRM_MODE_FLAG_PIC_AR_4_3:
+		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
+		break;
+	case DRM_MODE_FLAG_PIC_AR_16_9:
+		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
+		break;
+	default:
+		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+		break;
+	}
+
 	out->status = drm_mode_validate_basic(out);
 	if (out->status != MODE_OK)
 		goto out;
-- 
1.9.1

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

[PATCH v4 3/4] video: Add new aspect ratios for HDMI 2.0

[Shashank Sharma]

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

This patch adds enumeration for the new aspect ratios
in the existing aspect ratio list.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>

V2: rebase
V3: rebase
V4: Added r-b from Jose, Ack by Tomi
---
 drivers/video/hdmi.c | 4 ++++
 include/linux/hdmi.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 1626892..1cf907e 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -533,6 +533,10 @@ hdmi_picture_aspect_get_name(enum hdmi_picture_aspect picture_aspect)
 		return "4:3";
 	case HDMI_PICTURE_ASPECT_16_9:
 		return "16:9";
+	case HDMI_PICTURE_ASPECT_64_27:
+		return "64:27";
+	case HDMI_PICTURE_ASPECT_256_135:
+		return "256:135";
 	case HDMI_PICTURE_ASPECT_RESERVED:
 		return "Reserved";
 	}
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index e974420..edbb4fc 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -78,6 +78,8 @@ enum hdmi_picture_aspect {
 	HDMI_PICTURE_ASPECT_NONE,
 	HDMI_PICTURE_ASPECT_4_3,
 	HDMI_PICTURE_ASPECT_16_9,
+	HDMI_PICTURE_ASPECT_64_27,
+	HDMI_PICTURE_ASPECT_256_135,
 	HDMI_PICTURE_ASPECT_RESERVED,
 };
 
-- 
1.9.1

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

[PATCH v4 4/4] drm: Add and handle new aspect ratios in DRM layer

[Shashank Sharma]

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.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.l.velikov@gmail.com>

V2: Rebase
V3: Align macro for DRM_MODE_PICTURE_ASPECT_256_135 (Jim Bride)
V4: Added r-b from Jose.
---
 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 fde927a..173b7d3 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1509,6 +1509,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 DRM_MODE_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;
@@ -1570,6 +1576,12 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
 	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 5c142b1..084b50a 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -81,6 +81,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)
@@ -90,6 +92,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)
 
 /* DPMS flags */
 /* bit compatible with the xorg definitions. */
-- 
1.9.1

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

Re: [PATCH v4 0/4] Picture aspect ratio support in DRM layer

[Daniel Vetter]

On Mon, Oct 17, 2016 at 05:34:36PM +0530, Shashank Sharma wrote:
> This patch series adds 4 patches.
> - The first two patches add aspect ratio support in DRM layes
> - Next two patches add new aspect ratios defined in CEA-861-F
>   supported for HDMI 2.0 4k modes.
> 
> Adding aspect ratio support in DRM layer:
> - The CEA videmodes contain aspect ratio information, which we
>   parse when we read the modes from EDID. But while transforming
>   user_mode to kernel_mode or viceversa, DRM layer lose this
>   information.
> - HDMI compliance testing for CEA modes, expects the AVI info frames
>   to contain exact VIC no for the 'video mode under test'. Now CEA
>   modes have different VIC for same modes but different aspect ratio
>   for example:
>         VIC 2 = 720x480@60 4:3
>         VIC 3 = 720x480@60 16:9
>   In this way, lack of aspect ratio information, can cause wrong VIC
>   no in AVI IF, causing HDMI complaince test to fail.
> - This patch set adds code, which embeds the aspect ratio information
>   also in DRM video mode flags, and uses it while comparing two modes.
> 
> Adding new aspect ratios for HDMI 2.0
> - CEA-861-F defines two new aspect ratios, to be used for 4k HDMI 2.0
>   modes.
>         - 64:27
>         - 256:135
> Last two patches in the series, adds code to handle these new
> aspect ratios.
> 
> V2: Fixed review comments from Sean, Emil, Daniel
> V3: Fixed review comments from Jim Bride, got r-b for all patches
> V4: Added r-b from Jose for the series, and ack-by from Tomi on patch 3
> 
> Shashank Sharma (4):
>   drm: add picture aspect ratio flags
>   drm: Add aspect ratio parsing in DRM layer
>   video: Add new aspect ratios for HDMI 2.0
>   drm: Add and handle new aspect ratios in DRM layer

Applied to drm-misc, thanks. I guess you'll follow up with i915 patches to
remove the picture aspect ratio for overrides eventually?

Thanks, Daniel

> 
>  drivers/gpu/drm/drm_modes.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/video/hdmi.c        |  4 ++++
>  include/linux/hdmi.h        |  2 ++
>  include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++-----
>  4 files changed, 68 insertions(+), 5 deletions(-)
> 
> -- 
> 1.9.1
> 
> _______________________________________________
> 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

Re: [Intel-gfx] [PATCH v4 2/4] drm: Add aspect ratio parsing in DRM layer

[Ville Syrjälä]

On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote:
> 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).
> 
> 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>
> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> 
> V2: Addressed review comments from Sean:
> - Fix spellings/typo
> - No need to handle aspect ratio none
> - Add a break, for default case too
> V3: Rebase
> V4: Added r-b from Jose
> ---
>  drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 53f07ac..fde927a 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  	    mode1->vsync_end == mode2->vsync_end &&
>  	    mode1->vtotal == mode2->vtotal &&
>  	    mode1->vscan == mode2->vscan &&
> +	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
>  	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
>  	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
>  		return true;
> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>  	out->vrefresh = in->vrefresh;
>  	out->flags = in->flags;
>  	out->type = in->type;
> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> +
> +	switch (in->picture_aspect_ratio) {
> +	case HDMI_PICTURE_ASPECT_4_3:
> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
> +		break;
> +	case HDMI_PICTURE_ASPECT_16_9:
> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> +		break;
> +	case HDMI_PICTURE_ASPECT_RESERVED:
> +	default:
> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
> +		break;
> +	}
> +
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  }
> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  
> +	/* Clearing picture aspect ratio bits from out flags */
> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> +
> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
> +	case DRM_MODE_FLAG_PIC_AR_4_3:
> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
> +		break;
> +	case DRM_MODE_FLAG_PIC_AR_16_9:
> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> +		break;
> +	default:
> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> +		break;
> +	}

Hmm. So now we have the mode aspect ratio infromation duplicated
in two different places. Not sure that won't lead to some confusion.
Although we do want the user to be able to override via the property I
suppose, so we'd have to change that (+ the inforframe code) to
look at the mode flags instead if we would eliminate
'picture_aspect_ratio'.

And that brings me to the other point. At least i915 will simply
override the mode's aspect ration with the property. So this looks like
a big no-op for now on i915. It's looking like we might need a new
propetty value to differentiate between "auto" and "none" for real.

> +
>  	out->status = drm_mode_validate_basic(out);
>  	if (out->status != MODE_OK)
>  		goto out;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v4 3/4] video: Add new aspect ratios for HDMI 2.0

[Ville Syrjälä]

On Mon, Oct 17, 2016 at 05:34:39PM +0530, Shashank Sharma wrote:
> HDMI 2.0/CEA-861-F introduces two new aspect ratios:
> - 64:27
> - 256:135
> 
> This patch adds enumeration for the new aspect ratios
> in the existing aspect ratio list.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
> Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> 
> V2: rebase
> V3: rebase
> V4: Added r-b from Jose, Ack by Tomi
> ---
>  drivers/video/hdmi.c | 4 ++++
>  include/linux/hdmi.h | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 1626892..1cf907e 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -533,6 +533,10 @@ hdmi_picture_aspect_get_name(enum hdmi_picture_aspect picture_aspect)
>  		return "4:3";
>  	case HDMI_PICTURE_ASPECT_16_9:
>  		return "16:9";
> +	case HDMI_PICTURE_ASPECT_64_27:
> +		return "64:27";
> +	case HDMI_PICTURE_ASPECT_256_135:
> +		return "256:135";
>  	case HDMI_PICTURE_ASPECT_RESERVED:
>  		return "Reserved";
>  	}
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index e974420..edbb4fc 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -78,6 +78,8 @@ enum hdmi_picture_aspect {
>  	HDMI_PICTURE_ASPECT_NONE,
>  	HDMI_PICTURE_ASPECT_4_3,
>  	HDMI_PICTURE_ASPECT_16_9,
> +	HDMI_PICTURE_ASPECT_64_27,
> +	HDMI_PICTURE_ASPECT_256_135,

Hmm. How's this gonna work? The AVI infoframe doesn't have these.

>  	HDMI_PICTURE_ASPECT_RESERVED,
>  };
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

✗ Fi.CI.BAT: failure for Picture aspect ratio support in DRM layer

[Patchwork]

== Series Details ==

Series: Picture aspect ratio support in DRM layer
URL   : https://patchwork.freedesktop.org/series/13868/
State : failure

== Summary ==

Series 13868v1 Picture aspect ratio support in DRM layer
https://patchwork.freedesktop.org/api/1.0/series/13868/revisions/1/mbox/

Test gem_ringfill:
        Subgroup basic-default-hang:
                pass       -> TIMEOUT    (fi-ivb-3770)
Test kms_flip:
        Subgroup basic-plain-flip:
                pass       -> INCOMPLETE (fi-ivb-3770)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup force-edid:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup force-load-detect:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup prune-stale-modes:
                pass       -> INCOMPLETE (fi-ivb-3770)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> INCOMPLETE (fi-ivb-3770)
Test kms_pipe_crc_basic:
        Subgroup bad-nb-words-1:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup bad-nb-words-3:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup bad-pipe:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup bad-source:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup hang-read-crc-pipe-a:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup hang-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup hang-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup nonblocking-crc-pipe-a:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup nonblocking-crc-pipe-c:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup read-crc-pipe-a:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-byt-j1900)
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-ivb-3770)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                pass       -> INCOMPLETE (fi-ivb-3770)
Test kms_sink_crc_basic:
                skip       -> INCOMPLETE (fi-ivb-3770)
Test pm_backlight:
        Subgroup basic-brightness:
                skip       -> INCOMPLETE (fi-ivb-3770)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                skip       -> INCOMPLETE (fi-ivb-3770)
        Subgroup basic-rte:
                skip       -> INCOMPLETE (fi-ivb-3770)
Test pm_rps:
        Subgroup basic-api:
                pass       -> INCOMPLETE (fi-ivb-3770)
Test prime_busy:
        Subgroup basic-after-default:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup basic-before-default:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup basic-wait-after-default:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup basic-wait-before-default:
                pass       -> INCOMPLETE (fi-ivb-3770)
Test prime_self_import:
        Subgroup basic-llseek-bad:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup basic-llseek-size:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup basic-with_fd_dup:
                pass       -> INCOMPLETE (fi-ivb-3770)
        Subgroup basic-with_one_bo:
WARNING: Long output truncated

Results at /archive/results/CI_IGT_test/Patchwork_2736/

15dfed2b90e84e7c277f81842fc3f19355293061 drm-intel-nightly: 2016y-10m-16d-23h-15m-00s UTC integration manifest
a34f927 drm: Add and handle new aspect ratios in DRM layer
293026e video: Add new aspect ratios for HDMI 2.0
1711ac2 drm: Add aspect ratio parsing in DRM layer
93ba751 drm: add picture aspect ratio flags

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

Re: [PATCH v4 0/4] Picture aspect ratio support in DRM layer

[Sharma, Shashank]

Regards

Shashank


On 10/17/2016 5:55 PM, Daniel Vetter wrote:
> On Mon, Oct 17, 2016 at 05:34:36PM +0530, Shashank Sharma wrote:
>> This patch series adds 4 patches.
>> - The first two patches add aspect ratio support in DRM layes
>> - Next two patches add new aspect ratios defined in CEA-861-F
>>    supported for HDMI 2.0 4k modes.
>>
>> Adding aspect ratio support in DRM layer:
>> - The CEA videmodes contain aspect ratio information, which we
>>    parse when we read the modes from EDID. But while transforming
>>    user_mode to kernel_mode or viceversa, DRM layer lose this
>>    information.
>> - HDMI compliance testing for CEA modes, expects the AVI info frames
>>    to contain exact VIC no for the 'video mode under test'. Now CEA
>>    modes have different VIC for same modes but different aspect ratio
>>    for example:
>>          VIC 2 = 720x480@60 4:3
>>          VIC 3 = 720x480@60 16:9
>>    In this way, lack of aspect ratio information, can cause wrong VIC
>>    no in AVI IF, causing HDMI complaince test to fail.
>> - This patch set adds code, which embeds the aspect ratio information
>>    also in DRM video mode flags, and uses it while comparing two modes.
>>
>> Adding new aspect ratios for HDMI 2.0
>> - CEA-861-F defines two new aspect ratios, to be used for 4k HDMI 2.0
>>    modes.
>>          - 64:27
>>          - 256:135
>> Last two patches in the series, adds code to handle these new
>> aspect ratios.
>>
>> V2: Fixed review comments from Sean, Emil, Daniel
>> V3: Fixed review comments from Jim Bride, got r-b for all patches
>> V4: Added r-b from Jose for the series, and ack-by from Tomi on patch 3
>>
>> Shashank Sharma (4):
>>    drm: add picture aspect ratio flags
>>    drm: Add aspect ratio parsing in DRM layer
>>    video: Add new aspect ratios for HDMI 2.0
>>    drm: Add and handle new aspect ratios in DRM layer
> Applied to drm-misc, thanks. I guess you'll follow up with i915 patches to
> remove the picture aspect ratio for overrides eventually?
>
> Thanks, Daniel
Thanks Daniel, and yes, its in my to-do list to remove the pic aspect 
ratio property, to override, as suggested by you in our last communication.
I was waiting for this series to be merged, so that I can give reference 
of this.

Will add you for the review of the patch,

Regards
Shashank
>>   drivers/gpu/drm/drm_modes.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>>   drivers/video/hdmi.c        |  4 ++++
>>   include/linux/hdmi.h        |  2 ++
>>   include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++-----
>>   4 files changed, 68 insertions(+), 5 deletions(-)
>>
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: ✗ Fi.CI.BAT: failure for Picture aspect ratio support in DRM layer

[Saarinen, Jani]

> == Series Details ==
> 
> Series: Picture aspect ratio support in DRM layer
> URL   : https://patchwork.freedesktop.org/series/13868/
> State : failure
> 
> == Summary ==
> 
> Series 13868v1 Picture aspect ratio support in DRM layer
> https://patchwork.freedesktop.org/api/1.0/series/13868/revisions/1/mbox/
> 
> Test gem_ringfill:
>         Subgroup basic-default-hang:
>                 pass       -> TIMEOUT    (fi-ivb-3770)
> Test kms_flip:
>         Subgroup basic-plain-flip:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
> Test kms_force_connector_basic:
>         Subgroup force-connector-state:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup force-edid:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup force-load-detect:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup prune-stale-modes:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
> Test kms_frontbuffer_tracking:
>         Subgroup basic:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
> Test kms_pipe_crc_basic:
>         Subgroup bad-nb-words-1:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup bad-nb-words-3:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup bad-pipe:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup bad-source:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup hang-read-crc-pipe-a:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup hang-read-crc-pipe-b:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup hang-read-crc-pipe-c:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup nonblocking-crc-pipe-a:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup nonblocking-crc-pipe-a-frame-sequence:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup nonblocking-crc-pipe-b:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup nonblocking-crc-pipe-b-frame-sequence:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup nonblocking-crc-pipe-c:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup nonblocking-crc-pipe-c-frame-sequence:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup read-crc-pipe-a:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup read-crc-pipe-a-frame-sequence:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup read-crc-pipe-b:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup read-crc-pipe-b-frame-sequence:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup read-crc-pipe-c:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup read-crc-pipe-c-frame-sequence:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup suspend-read-crc-pipe-a:
>                 dmesg-warn -> PASS       (fi-byt-j1900)
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup suspend-read-crc-pipe-b:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup suspend-read-crc-pipe-c:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
> Test kms_setmode:
>         Subgroup basic-clone-single-crtc:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
> Test kms_sink_crc_basic:
>                 skip       -> INCOMPLETE (fi-ivb-3770)
> Test pm_backlight:
>         Subgroup basic-brightness:
>                 skip       -> INCOMPLETE (fi-ivb-3770)
> Test pm_rpm:
>         Subgroup basic-pci-d3-state:
>                 skip       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup basic-rte:
>                 skip       -> INCOMPLETE (fi-ivb-3770)
> Test pm_rps:
>         Subgroup basic-api:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
> Test prime_busy:
>         Subgroup basic-after-default:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup basic-before-default:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup basic-wait-after-default:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup basic-wait-before-default:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
> Test prime_self_import:
>         Subgroup basic-llseek-bad:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup basic-llseek-size:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup basic-with_fd_dup:
>                 pass       -> INCOMPLETE (fi-ivb-3770)
>         Subgroup basic-with_one_bo:
> WARNING: Long output truncated

[182/246] skip: 21, pass: 160, timeout: 1 /
running: igt/kms_flip/basic-plain-flip     

[182/246] skip: 21, pass: 160, timeout: 1 -
Build timed out (after 18 minutes). Marking the build as aborted.

> 
> Results at /archive/results/CI_IGT_test/Patchwork_2736/
> 
> 15dfed2b90e84e7c277f81842fc3f19355293061 drm-intel-nightly: 2016y-10m-
> 16d-23h-15m-00s UTC integration manifest
> a34f927 drm: Add and handle new aspect ratios in DRM layer
> 293026e video: Add new aspect ratios for HDMI 2.0
> 1711ac2 drm: Add aspect ratio parsing in DRM layer
> 93ba751 drm: add picture aspect ratio flags
> 

Jani Saarinen
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo



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

Re: [PATCH v4 2/4] drm: Add aspect ratio parsing in DRM layer

[Sharma, Shashank]

Regards

Shashank


On 10/17/2016 6:01 PM, Ville Syrjälä wrote:
> On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote:
>> 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).
>>
>> 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>
>> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>
>> V2: Addressed review comments from Sean:
>> - Fix spellings/typo
>> - No need to handle aspect ratio none
>> - Add a break, for default case too
>> V3: Rebase
>> V4: Added r-b from Jose
>> ---
>>   drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index 53f07ac..fde927a 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>>   	    mode1->vsync_end == mode2->vsync_end &&
>>   	    mode1->vtotal == mode2->vtotal &&
>>   	    mode1->vscan == mode2->vscan &&
>> +	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
>>   	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
>>   	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
>>   		return true;
>> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>>   	out->vrefresh = in->vrefresh;
>>   	out->flags = in->flags;
>>   	out->type = in->type;
>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>> +
>> +	switch (in->picture_aspect_ratio) {
>> +	case HDMI_PICTURE_ASPECT_4_3:
>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
>> +		break;
>> +	case HDMI_PICTURE_ASPECT_16_9:
>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
>> +		break;
>> +	case HDMI_PICTURE_ASPECT_RESERVED:
>> +	default:
>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
>> +		break;
>> +	}
>> +
>>   	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>   	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>   }
>> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>>   	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>   	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>   
>> +	/* Clearing picture aspect ratio bits from out flags */
>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>> +
>> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
>> +	case DRM_MODE_FLAG_PIC_AR_4_3:
>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
>> +		break;
>> +	case DRM_MODE_FLAG_PIC_AR_16_9:
>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
>> +		break;
>> +	default:
>> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>> +		break;
>> +	}
> Hmm. So now we have the mode aspect ratio infromation duplicated
> in two different places. Not sure that won't lead to some confusion.
In drm layer, this is the only place. Actually till now, DRM layer dint 
have the support for aspect ratio at all. This was causing
HDMI compliance tests like 7-27 fail, which expect a particular unique 
VIC in AVI infoframe on modeset.

I have given some details about this in cover-letter.
> Although we do want the user to be able to override via the property I
> suppose, so we'd have to change that (+ the inforframe code) to
> look at the mode flags instead if we would eliminate
> 'picture_aspect_ratio'.
I had a small discussion on this with Daniel, and we discussed that it 
doesnt make sense to override just the aspect ratio if the monitor 
doesn't even support that aspect ratio.
And currently the was how this property is implemented is, we just 
override the aspect ratio without any validity check.

Now as we have all the supported aspect ratio added properly in the mode 
info itself, we need not to have this property at all, So Daniel 
suggested me to remove that property too.
>
> And that brings me to the other point. At least i915 will simply
> override the mode's aspect ration with the property. So this looks like
> a big no-op for now on i915.
Yes, This is a bug in I915. When I published the first version of this 
series, I had one patch, which was overriding the value only when the 
property is set.
This should be the right case. And then Daniel suggested to remove the 
property all together (and it makes sense as we have proper aspect 
ratios in mode information
itself) So I kept that patch separate, to be merged separately.
>   It's looking like we might need a new
> propetty value to differentiate between "auto" and "none" for real.
Now we have the exact aspect ratio given by monitor EDID, so IMHO it 
would be better if we can just have real aspect.

Regards
Shashank
>> +
>>   	out->status = drm_mode_validate_basic(out);
>>   	if (out->status != MODE_OK)
>>   		goto out;
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v4 2/4] drm: Add aspect ratio parsing in DRM layer

[Ville Syrjälä]

On Mon, Oct 17, 2016 at 07:10:42PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 10/17/2016 6:01 PM, Ville Syrjälä wrote:
> > On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote:
> >> 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).
> >>
> >> 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>
> >> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> >>
> >> V2: Addressed review comments from Sean:
> >> - Fix spellings/typo
> >> - No need to handle aspect ratio none
> >> - Add a break, for default case too
> >> V3: Rebase
> >> V4: Added r-b from Jose
> >> ---
> >>   drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++
> >>   1 file changed, 31 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> >> index 53f07ac..fde927a 100644
> >> --- a/drivers/gpu/drm/drm_modes.c
> >> +++ b/drivers/gpu/drm/drm_modes.c
> >> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> >>   	    mode1->vsync_end == mode2->vsync_end &&
> >>   	    mode1->vtotal == mode2->vtotal &&
> >>   	    mode1->vscan == mode2->vscan &&
> >> +	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
> >>   	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> >>   	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
> >>   		return true;
> >> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> >>   	out->vrefresh = in->vrefresh;
> >>   	out->flags = in->flags;
> >>   	out->type = in->type;
> >> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> >> +
> >> +	switch (in->picture_aspect_ratio) {
> >> +	case HDMI_PICTURE_ASPECT_4_3:
> >> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
> >> +		break;
> >> +	case HDMI_PICTURE_ASPECT_16_9:
> >> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> >> +		break;
> >> +	case HDMI_PICTURE_ASPECT_RESERVED:
> >> +	default:
> >> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
> >> +		break;
> >> +	}
> >> +
> >>   	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >>   	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> >>   }
> >> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
> >>   	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >>   	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> >>   
> >> +	/* Clearing picture aspect ratio bits from out flags */
> >> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> >> +
> >> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
> >> +	case DRM_MODE_FLAG_PIC_AR_4_3:
> >> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
> >> +		break;
> >> +	case DRM_MODE_FLAG_PIC_AR_16_9:
> >> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> >> +		break;
> >> +	default:
> >> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >> +		break;
> >> +	}
> > Hmm. So now we have the mode aspect ratio infromation duplicated
> > in two different places. Not sure that won't lead to some confusion.
> In drm layer, this is the only place. Actually till now, DRM layer dint 
> have the support for aspect ratio at all. This was causing
> HDMI compliance tests like 7-27 fail, which expect a particular unique 
> VIC in AVI infoframe on modeset.
> 
> I have given some details about this in cover-letter.
> > Although we do want the user to be able to override via the property I
> > suppose, so we'd have to change that (+ the inforframe code) to
> > look at the mode flags instead if we would eliminate
> > 'picture_aspect_ratio'.
> I had a small discussion on this with Daniel, and we discussed that it 
> doesnt make sense to override just the aspect ratio if the monitor 
> doesn't even support that aspect ratio.
> And currently the was how this property is implemented is, we just 
> override the aspect ratio without any validity check.
> 
> Now as we have all the supported aspect ratio added properly in the mode 
> info itself, we need not to have this property at all, So Daniel 
> suggested me to remove that property too.
> >
> > And that brings me to the other point. At least i915 will simply
> > override the mode's aspect ration with the property. So this looks like
> > a big no-op for now on i915.
> Yes, This is a bug in I915. When I published the first version of this 
> series, I had one patch, which was overriding the value only when the 
> property is set.
> This should be the right case. And then Daniel suggested to remove the 
> property all together (and it makes sense as we have proper aspect 
> ratios in mode information
> itself) So I kept that patch separate, to be merged separately.

Yeah, removing the property entirely seems like an OKish solution since
userspace can just stuff that information into the mode flags instead.
The only slight concern is that someone's setup might depend on the
property, and now we're removing it.

> >   It's looking like we might need a new
> > propetty value to differentiate between "auto" and "none" for real.
> Now we have the exact aspect ratio given by monitor EDID, so IMHO it 
> would be better if we can just have real aspect.

Well, "real" isn't quite the right term. It's still a user specified
thing, which means the user can ask for somehting the display didn't
even advertise. Which is fine as such, however since the AVI infoframe
lacks the capability to transmit these new aspect ratios we have a bit
of problem on our hands if the user asks for something we can't even
tell the display to do. I guess we would just need to return an error
to the user in that case.

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

Re: [Intel-gfx] [PATCH v4 2/4] drm: Add aspect ratio parsing in DRM layer

[Sharma, Shashank]

Regards

Shashank


On 10/17/2016 7:36 PM, Ville Syrjälä wrote:
> On Mon, Oct 17, 2016 at 07:10:42PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 10/17/2016 6:01 PM, Ville Syrjälä wrote:
>>> On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote:
>>>> 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).
>>>>
>>>> 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>
>>>> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>>>
>>>> V2: Addressed review comments from Sean:
>>>> - Fix spellings/typo
>>>> - No need to handle aspect ratio none
>>>> - Add a break, for default case too
>>>> V3: Rebase
>>>> V4: Added r-b from Jose
>>>> ---
>>>>    drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++
>>>>    1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>>>> index 53f07ac..fde927a 100644
>>>> --- a/drivers/gpu/drm/drm_modes.c
>>>> +++ b/drivers/gpu/drm/drm_modes.c
>>>> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>>>>    	    mode1->vsync_end == mode2->vsync_end &&
>>>>    	    mode1->vtotal == mode2->vtotal &&
>>>>    	    mode1->vscan == mode2->vscan &&
>>>> +	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
>>>>    	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
>>>>    	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
>>>>    		return true;
>>>> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>>>>    	out->vrefresh = in->vrefresh;
>>>>    	out->flags = in->flags;
>>>>    	out->type = in->type;
>>>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>>>> +
>>>> +	switch (in->picture_aspect_ratio) {
>>>> +	case HDMI_PICTURE_ASPECT_4_3:
>>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
>>>> +		break;
>>>> +	case HDMI_PICTURE_ASPECT_16_9:
>>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
>>>> +		break;
>>>> +	case HDMI_PICTURE_ASPECT_RESERVED:
>>>> +	default:
>>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
>>>> +		break;
>>>> +	}
>>>> +
>>>>    	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>>>    	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>>>    }
>>>> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>>>>    	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>>>    	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>>>    
>>>> +	/* Clearing picture aspect ratio bits from out flags */
>>>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>>>> +
>>>> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
>>>> +	case DRM_MODE_FLAG_PIC_AR_4_3:
>>>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
>>>> +		break;
>>>> +	case DRM_MODE_FLAG_PIC_AR_16_9:
>>>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
>>>> +		break;
>>>> +	default:
>>>> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>> +		break;
>>>> +	}
>>> Hmm. So now we have the mode aspect ratio infromation duplicated
>>> in two different places. Not sure that won't lead to some confusion.
>> In drm layer, this is the only place. Actually till now, DRM layer dint
>> have the support for aspect ratio at all. This was causing
>> HDMI compliance tests like 7-27 fail, which expect a particular unique
>> VIC in AVI infoframe on modeset.
>>
>> I have given some details about this in cover-letter.
>>> Although we do want the user to be able to override via the property I
>>> suppose, so we'd have to change that (+ the inforframe code) to
>>> look at the mode flags instead if we would eliminate
>>> 'picture_aspect_ratio'.
>> I had a small discussion on this with Daniel, and we discussed that it
>> doesnt make sense to override just the aspect ratio if the monitor
>> doesn't even support that aspect ratio.
>> And currently the was how this property is implemented is, we just
>> override the aspect ratio without any validity check.
>>
>> Now as we have all the supported aspect ratio added properly in the mode
>> info itself, we need not to have this property at all, So Daniel
>> suggested me to remove that property too.
>>> And that brings me to the other point. At least i915 will simply
>>> override the mode's aspect ration with the property. So this looks like
>>> a big no-op for now on i915.
>> Yes, This is a bug in I915. When I published the first version of this
>> series, I had one patch, which was overriding the value only when the
>> property is set.
>> This should be the right case. And then Daniel suggested to remove the
>> property all together (and it makes sense as we have proper aspect
>> ratios in mode information
>> itself) So I kept that patch separate, to be merged separately.
> Yeah, removing the property entirely seems like an OKish solution since
> userspace can just stuff that information into the mode flags instead.
> The only slight concern is that someone's setup might depend on the
> property, and now we're removing it.
Thanks, will send the patch for it soon, adding you in the mail chain.
>>>    It's looking like we might need a new
>>> propetty value to differentiate between "auto" and "none" for real.
>> Now we have the exact aspect ratio given by monitor EDID, so IMHO it
>> would be better if we can just have real aspect.
> Well, "real" isn't quite the right term. It's still a user specified
> thing, which means the user can ask for somehting the display didn't
> even advertise. Which is fine as such, however since the AVI infoframe
> lacks the capability to transmit these new aspect ratios we have a bit
> of problem on our hands if the user asks for something we can't even
> tell the display to do. I guess we would just need to return an error
> to the user in that case.
The current implementation which we have in andoroid, is hardware 
composer shows the whole mode in the list like:
- 1920x1080@60 16:9    /* From CEA block of EDID */
- 1920x1080@60 0:0        /* From detailed descriptor block of EDID */
- 1280x720@60 4:3          /* From CEA block of EDID */
-etc
These mode is the connector->probed_modes which we populated while 
parsing the EDID.
Now, when user selects the mode, its a complete set of resolution + 
refresh rate + aspect ratio, so user can only pick what
we are showing to it. In this case, there is no option which we cant 
support.

Do you think that there would be some other model of use case, where we 
can mix something among these?

Regards
Shashank


When user picks mode, its picks

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

Re: [PATCH v4 2/4] drm: Add aspect ratio parsing in DRM layer

[Ville Syrjälä]

On Mon, Oct 17, 2016 at 08:21:21PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 10/17/2016 7:36 PM, Ville Syrjälä wrote:
> > On Mon, Oct 17, 2016 at 07:10:42PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 10/17/2016 6:01 PM, Ville Syrjälä wrote:
> >>> On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote:
> >>>> 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).
> >>>>
> >>>> 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>
> >>>> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> >>>>
> >>>> V2: Addressed review comments from Sean:
> >>>> - Fix spellings/typo
> >>>> - No need to handle aspect ratio none
> >>>> - Add a break, for default case too
> >>>> V3: Rebase
> >>>> V4: Added r-b from Jose
> >>>> ---
> >>>>    drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++
> >>>>    1 file changed, 31 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> >>>> index 53f07ac..fde927a 100644
> >>>> --- a/drivers/gpu/drm/drm_modes.c
> >>>> +++ b/drivers/gpu/drm/drm_modes.c
> >>>> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> >>>>    	    mode1->vsync_end == mode2->vsync_end &&
> >>>>    	    mode1->vtotal == mode2->vtotal &&
> >>>>    	    mode1->vscan == mode2->vscan &&
> >>>> +	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
> >>>>    	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> >>>>    	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
> >>>>    		return true;
> >>>> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> >>>>    	out->vrefresh = in->vrefresh;
> >>>>    	out->flags = in->flags;
> >>>>    	out->type = in->type;
> >>>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> >>>> +
> >>>> +	switch (in->picture_aspect_ratio) {
> >>>> +	case HDMI_PICTURE_ASPECT_4_3:
> >>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
> >>>> +		break;
> >>>> +	case HDMI_PICTURE_ASPECT_16_9:
> >>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> >>>> +		break;
> >>>> +	case HDMI_PICTURE_ASPECT_RESERVED:
> >>>> +	default:
> >>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
> >>>> +		break;
> >>>> +	}
> >>>> +
> >>>>    	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >>>>    	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> >>>>    }
> >>>> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
> >>>>    	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >>>>    	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> >>>>    
> >>>> +	/* Clearing picture aspect ratio bits from out flags */
> >>>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> >>>> +
> >>>> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
> >>>> +	case DRM_MODE_FLAG_PIC_AR_4_3:
> >>>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
> >>>> +		break;
> >>>> +	case DRM_MODE_FLAG_PIC_AR_16_9:
> >>>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> >>>> +		break;
> >>>> +	default:
> >>>> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>>> +		break;
> >>>> +	}
> >>> Hmm. So now we have the mode aspect ratio infromation duplicated
> >>> in two different places. Not sure that won't lead to some confusion.
> >> In drm layer, this is the only place. Actually till now, DRM layer dint
> >> have the support for aspect ratio at all. This was causing
> >> HDMI compliance tests like 7-27 fail, which expect a particular unique
> >> VIC in AVI infoframe on modeset.
> >>
> >> I have given some details about this in cover-letter.
> >>> Although we do want the user to be able to override via the property I
> >>> suppose, so we'd have to change that (+ the inforframe code) to
> >>> look at the mode flags instead if we would eliminate
> >>> 'picture_aspect_ratio'.
> >> I had a small discussion on this with Daniel, and we discussed that it
> >> doesnt make sense to override just the aspect ratio if the monitor
> >> doesn't even support that aspect ratio.
> >> And currently the was how this property is implemented is, we just
> >> override the aspect ratio without any validity check.
> >>
> >> Now as we have all the supported aspect ratio added properly in the mode
> >> info itself, we need not to have this property at all, So Daniel
> >> suggested me to remove that property too.
> >>> And that brings me to the other point. At least i915 will simply
> >>> override the mode's aspect ration with the property. So this looks like
> >>> a big no-op for now on i915.
> >> Yes, This is a bug in I915. When I published the first version of this
> >> series, I had one patch, which was overriding the value only when the
> >> property is set.
> >> This should be the right case. And then Daniel suggested to remove the
> >> property all together (and it makes sense as we have proper aspect
> >> ratios in mode information
> >> itself) So I kept that patch separate, to be merged separately.
> > Yeah, removing the property entirely seems like an OKish solution since
> > userspace can just stuff that information into the mode flags instead.
> > The only slight concern is that someone's setup might depend on the
> > property, and now we're removing it.
> Thanks, will send the patch for it soon, adding you in the mail chain.
> >>>    It's looking like we might need a new
> >>> propetty value to differentiate between "auto" and "none" for real.
> >> Now we have the exact aspect ratio given by monitor EDID, so IMHO it
> >> would be better if we can just have real aspect.
> > Well, "real" isn't quite the right term. It's still a user specified
> > thing, which means the user can ask for somehting the display didn't
> > even advertise. Which is fine as such, however since the AVI infoframe
> > lacks the capability to transmit these new aspect ratios we have a bit
> > of problem on our hands if the user asks for something we can't even
> > tell the display to do. I guess we would just need to return an error
> > to the user in that case.
> The current implementation which we have in andoroid, is hardware 
> composer shows the whole mode in the list like:
> - 1920x1080@60 16:9    /* From CEA block of EDID */
> - 1920x1080@60 0:0        /* From detailed descriptor block of EDID */
> - 1280x720@60 4:3          /* From CEA block of EDID */
> -etc
> These mode is the connector->probed_modes which we populated while 
> parsing the EDID.
> Now, when user selects the mode, its a complete set of resolution + 
> refresh rate + aspect ratio, so user can only pick what
> we are showing to it. In this case, there is no option which we cant 
> support.
> 
> Do you think that there would be some other model of use case, where we 
> can mix something among these?

You can't assume the user just picks the mode off the list. They can
stuff whatever random garbage into the mode if they wish.

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

Re: [Intel-gfx] [PATCH v4 2/4] drm: Add aspect ratio parsing in DRM layer

[Sharma, Shashank]

Regards

Shashank


On 10/17/2016 8:30 PM, Ville Syrjälä wrote:
> On Mon, Oct 17, 2016 at 08:21:21PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 10/17/2016 7:36 PM, Ville Syrjälä wrote:
>>> On Mon, Oct 17, 2016 at 07:10:42PM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 10/17/2016 6:01 PM, Ville Syrjälä wrote:
>>>>> On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote:
>>>>>> 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).
>>>>>>
>>>>>> 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>
>>>>>> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>>>>>
>>>>>> V2: Addressed review comments from Sean:
>>>>>> - Fix spellings/typo
>>>>>> - No need to handle aspect ratio none
>>>>>> - Add a break, for default case too
>>>>>> V3: Rebase
>>>>>> V4: Added r-b from Jose
>>>>>> ---
>>>>>>     drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 31 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>>>>>> index 53f07ac..fde927a 100644
>>>>>> --- a/drivers/gpu/drm/drm_modes.c
>>>>>> +++ b/drivers/gpu/drm/drm_modes.c
>>>>>> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>>>>>>     	    mode1->vsync_end == mode2->vsync_end &&
>>>>>>     	    mode1->vtotal == mode2->vtotal &&
>>>>>>     	    mode1->vscan == mode2->vscan &&
>>>>>> +	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
>>>>>>     	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
>>>>>>     	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
>>>>>>     		return true;
>>>>>> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>>>>>>     	out->vrefresh = in->vrefresh;
>>>>>>     	out->flags = in->flags;
>>>>>>     	out->type = in->type;
>>>>>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>>>>>> +
>>>>>> +	switch (in->picture_aspect_ratio) {
>>>>>> +	case HDMI_PICTURE_ASPECT_4_3:
>>>>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
>>>>>> +		break;
>>>>>> +	case HDMI_PICTURE_ASPECT_16_9:
>>>>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
>>>>>> +		break;
>>>>>> +	case HDMI_PICTURE_ASPECT_RESERVED:
>>>>>> +	default:
>>>>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
>>>>>> +		break;
>>>>>> +	}
>>>>>> +
>>>>>>     	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>>>>>     	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>>>>>     }
>>>>>> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>>>>>>     	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>>>>>     	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>>>>>     
>>>>>> +	/* Clearing picture aspect ratio bits from out flags */
>>>>>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>>>>>> +
>>>>>> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
>>>>>> +	case DRM_MODE_FLAG_PIC_AR_4_3:
>>>>>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
>>>>>> +		break;
>>>>>> +	case DRM_MODE_FLAG_PIC_AR_16_9:
>>>>>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
>>>>>> +		break;
>>>>>> +	default:
>>>>>> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>>> +		break;
>>>>>> +	}
>>>>> Hmm. So now we have the mode aspect ratio infromation duplicated
>>>>> in two different places. Not sure that won't lead to some confusion.
>>>> In drm layer, this is the only place. Actually till now, DRM layer dint
>>>> have the support for aspect ratio at all. This was causing
>>>> HDMI compliance tests like 7-27 fail, which expect a particular unique
>>>> VIC in AVI infoframe on modeset.
>>>>
>>>> I have given some details about this in cover-letter.
>>>>> Although we do want the user to be able to override via the property I
>>>>> suppose, so we'd have to change that (+ the inforframe code) to
>>>>> look at the mode flags instead if we would eliminate
>>>>> 'picture_aspect_ratio'.
>>>> I had a small discussion on this with Daniel, and we discussed that it
>>>> doesnt make sense to override just the aspect ratio if the monitor
>>>> doesn't even support that aspect ratio.
>>>> And currently the was how this property is implemented is, we just
>>>> override the aspect ratio without any validity check.
>>>>
>>>> Now as we have all the supported aspect ratio added properly in the mode
>>>> info itself, we need not to have this property at all, So Daniel
>>>> suggested me to remove that property too.
>>>>> And that brings me to the other point. At least i915 will simply
>>>>> override the mode's aspect ration with the property. So this looks like
>>>>> a big no-op for now on i915.
>>>> Yes, This is a bug in I915. When I published the first version of this
>>>> series, I had one patch, which was overriding the value only when the
>>>> property is set.
>>>> This should be the right case. And then Daniel suggested to remove the
>>>> property all together (and it makes sense as we have proper aspect
>>>> ratios in mode information
>>>> itself) So I kept that patch separate, to be merged separately.
>>> Yeah, removing the property entirely seems like an OKish solution since
>>> userspace can just stuff that information into the mode flags instead.
>>> The only slight concern is that someone's setup might depend on the
>>> property, and now we're removing it.
>> Thanks, will send the patch for it soon, adding you in the mail chain.
>>>>>     It's looking like we might need a new
>>>>> propetty value to differentiate between "auto" and "none" for real.
>>>> Now we have the exact aspect ratio given by monitor EDID, so IMHO it
>>>> would be better if we can just have real aspect.
>>> Well, "real" isn't quite the right term. It's still a user specified
>>> thing, which means the user can ask for somehting the display didn't
>>> even advertise. Which is fine as such, however since the AVI infoframe
>>> lacks the capability to transmit these new aspect ratios we have a bit
>>> of problem on our hands if the user asks for something we can't even
>>> tell the display to do. I guess we would just need to return an error
>>> to the user in that case.
>> The current implementation which we have in andoroid, is hardware
>> composer shows the whole mode in the list like:
>> - 1920x1080@60 16:9    /* From CEA block of EDID */
>> - 1920x1080@60 0:0        /* From detailed descriptor block of EDID */
>> - 1280x720@60 4:3          /* From CEA block of EDID */
>> -etc
>> These mode is the connector->probed_modes which we populated while
>> parsing the EDID.
>> Now, when user selects the mode, its a complete set of resolution +
>> refresh rate + aspect ratio, so user can only pick what
>> we are showing to it. In this case, there is no option which we cant
>> support.
>>
>> Do you think that there would be some other model of use case, where we
>> can mix something among these?
> You can't assume the user just picks the mode off the list. They can
> stuff whatever random garbage into the mode if they wish.
>
I agree, there can be a use case model.

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