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

[PATCH 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.

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 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>
---
 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 49a7265..cd66a95 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_PARMASK			(0x0F<<19)
+#define  DRM_MODE_FLAG_PARNONE \
+			(DRM_MODE_PICTURE_ASPECT_NONE << 19)
+#define  DRM_MODE_FLAG_PAR4_3 \
+			(DRM_MODE_PICTURE_ASPECT_4_3 << 19)
+#define  DRM_MODE_FLAG_PAR16_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

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

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

[Shashank Sharma]

Current DRM layer functions dont parse aspect ratio information
while converting a user mode->kernel mode or viceversa. This
causes modeset to pick mode with wrong aspect ratio, eventually
cauing 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>
---
 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 fc5040a..e6029e0 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -969,6 +969,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;
@@ -1471,6 +1472,22 @@ 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_PARMASK;
+
+	switch (in->picture_aspect_ratio) {
+	case HDMI_PICTURE_ASPECT_4_3:
+		out->flags |= DRM_MODE_FLAG_PAR4_3;
+		break;
+	case HDMI_PICTURE_ASPECT_16_9:
+		out->flags |= DRM_MODE_FLAG_PAR16_9;
+		break;
+	case HDMI_PICTURE_ASPECT_NONE:
+	case HDMI_PICTURE_ASPECT_RESERVED:
+	default:
+		out->flags |= DRM_MODE_FLAG_PARNONE;
+		break;
+	}
+
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 }
@@ -1516,6 +1533,20 @@ 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_PARMASK;
+
+	switch (in->flags & DRM_MODE_FLAG_PARMASK) {
+	case DRM_MODE_FLAG_PAR4_3:
+		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
+		break;
+	case DRM_MODE_FLAG_PAR16_9:
+		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
+		break;
+	default:
+		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+	}
+
 	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 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>
---
 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

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

[PATCH 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 viseversa.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 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 e6029e0..bdc353d 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1481,6 +1481,12 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
 	case HDMI_PICTURE_ASPECT_16_9:
 		out->flags |= DRM_MODE_FLAG_PAR16_9;
 		break;
+	case HDMI_PICTURE_ASPECT_64_27:
+		out->flags |= DRM_MODE_FLAG_PAR64_27;
+		break;
+	case DRM_MODE_PICTURE_ASPECT_256_135:
+		out->flags |= DRM_MODE_FLAG_PAR256_135;
+		break;
 	case HDMI_PICTURE_ASPECT_NONE:
 	case HDMI_PICTURE_ASPECT_RESERVED:
 	default:
@@ -1543,6 +1549,12 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
 	case DRM_MODE_FLAG_PAR16_9:
 		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
 		break;
+	case DRM_MODE_FLAG_PAR64_27:
+		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
+		break;
+	case DRM_MODE_FLAG_PAR256_135:
+		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
+		break;
 	default:
 		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
 	}
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index cd66a95..6291eae 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_PARMASK			(0x0F<<19)
@@ -90,6 +92,10 @@ extern "C" {
 			(DRM_MODE_PICTURE_ASPECT_4_3 << 19)
 #define  DRM_MODE_FLAG_PAR16_9 \
 			(DRM_MODE_PICTURE_ASPECT_16_9 << 19)
+#define  DRM_MODE_FLAG_PAR64_27 \
+			(DRM_MODE_PICTURE_ASPECT_64_27 << 19)
+#define  DRM_MODE_FLAG_PAR256_135 \
+			(DRM_MODE_PICTURE_ASPECT_256_135 << 19)
 
 /* DPMS flags */
 /* bit compatible with the xorg definitions. */
-- 
1.9.1

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

✗ Ro.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/10587/
State : failure

== Summary ==

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

Test drv_module_reload_basic:
                pass       -> SKIP       (fi-skl-i5-6260u)
Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-legacy:
                pass       -> FAIL       (ro-hsw-i7-4770r)
                pass       -> FAIL       (ro-bdw-i5-5250u)
                pass       -> FAIL       (fi-skl-i7-6700k)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (ro-hsw-i7-4770r)
                pass       -> FAIL       (ro-bdw-i7-5600u)
                fail       -> PASS       (ro-bdw-i7-5557U)
                fail       -> PASS       (ro-snb-i7-2620M)
                fail       -> PASS       (ro-skl3-i5-6260u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                incomplete -> PASS       (fi-skl-i7-6700k)

fi-hsw-i7-4770k  total:240  pass:218  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-qkkr      total:240  pass:181  dwarn:28  dfail:1   fail:3   skip:27 
fi-skl-i5-6260u  total:240  pass:223  dwarn:0   dfail:0   fail:2   skip:15 
fi-skl-i7-6700k  total:240  pass:208  dwarn:0   dfail:0   fail:4   skip:28 
fi-snb-i7-2600   total:240  pass:198  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:218  dwarn:4   dfail:0   fail:2   skip:16 
ro-bdw-i7-5557U  total:240  pass:224  dwarn:0   dfail:0   fail:0   skip:16 
ro-bdw-i7-5600u  total:240  pass:206  dwarn:0   dfail:0   fail:2   skip:32 
ro-bsw-n3050     total:240  pass:195  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:212  dwarn:0   dfail:0   fail:2   skip:26 
ro-ilk-i7-620lm  total:240  pass:172  dwarn:1   dfail:0   fail:2   skip:65 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:223  dwarn:0   dfail:0   fail:3   skip:14 
ro-snb-i7-2620M  total:240  pass:198  dwarn:0   dfail:0   fail:1   skip:41 
ro-ilk1-i5-650 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1676/

aa8628c drm-intel-nightly: 2016y-08m-02d-14h-10m-12s UTC integration manifest
b1ae19e drm: Add and handle new aspect ratios in DRM layer
6824fd3 video: Add new aspect ratios for HDMI 2.0
5ce6d35 drm: Add aspect ratio parsing in DRM layer
9523625 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 0/4]: Picture aspect ratio support in DRM layer

[Daniel Vetter]

On Wed, Aug 03, 2016 at 04:26:24PM +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.
> 
> 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

Patch series seems to have 0 changelogs anywhere, but I'm pretty sure I've
seen this before. Please try again and state what changed and why you are
resubmitting this.
-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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jose Abreu]

Hi,


On 03-08-2016 12:48, Daniel Vetter wrote:
> On Wed, Aug 03, 2016 at 04:26:24PM +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.
>>
>> 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
> Patch series seems to have 0 changelogs anywhere, but I'm pretty sure I've
> seen this before. Please try again and state what changed and why you are
> resubmitting this.
> -Daniel

I've also seen this before and I am using them in order to pass
HDMI compliance. Without these patches the compliance fails.
Still, I've made some changes which I can submit. I've some
comments to you (Shashank):

First, you add an if condition in
drm_mode_equal_no_clocks_no_stereo() (patch 2) which
unconditionally compares the aspect ratio. But I think that you
have to take into account that some modes handed by the user to
the DRM layer do not initialize this field so I think the best
solution would be to compare the aspect ratios only when the
field is populated (i.e. picture_aspect_ratio !=
HDMI_PICTURE_ASPECT_NONE).

Second, you are expecting that the picture aspect field is
correctly set in the HDMI parsing but, at least in the test
equipment that I am using, this field is not set by the DRM layer
because the mode is coming in the detailed timings section which
does not include a picture aspect field. In my implementation I
add a function which given the mode width and height (fields
->width_mm and ->height_mm of mode) computes the aspect ratio and
populates the field.

Third, I am facing some difficulties when using Xserver and
Xrandr. Using libdrm's modetest application everything works ok
but with xrandr the aspect ratio gets lost between the link DRM
-> Xserver -> DRM. I set the aspect ratio in the flags field when
passing the mode to user level (just like you do in patch 2) but
then when the mode is returned and delivered to the DRM driver
the picture aspect is not present. I think this is due to how
Xserver or xrandr sets the mode but I am not sure.

@Daniel, can you give some comments regarding this?

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

Best regards,
Jose Miguel Abreu
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sharma, Shashank]

> Patch series seems to have 0 changelogs anywhere, but I'm pretty sure I've seen this before. > > Please try again and state what changed and why you are resubmitting this.
> -Daniel

Hi Daniel, 
This is a re-spin of the patch series for aspect ratio support. 
The first four patches dint get reviewed for a long time, so I sent it again. 
https://patchwork.freedesktop.org/patch/78308/ 

You gave review comments on the fifth patch, which was In I915, so I kept that patch away from this series, and made this as DRM layer only.  So this series contains only 4 patches.
https://patchwork.freedesktop.org/patch/78178/ 
 
We can decide for I915, if we want to keep the aspect ratio property or not, internally. 
You are more than welcome to review this series :).
 
Regards
Shashank
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Wednesday, August 3, 2016 5:18 PM
To: Sharma, Shashank <shashank.sharma@intel.com>
Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; ville.syrjala@linux.intel.com; rodrigo.vivi@gmail.com; Vetter, Daniel <daniel.vetter@intel.com>
Subject: Re: [PATCH 0/4]: Picture aspect ratio support in DRM layer

On Wed, Aug 03, 2016 at 04:26:24PM +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.
> 
> 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

Patch series seems to have 0 changelogs anywhere, but I'm pretty sure I've seen this before. Please try again and state what changed and why you are resubmitting this.
-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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

Hello Joes,
> I've also seen this before and I am using them in order to pass HDMI compliance. Without
> these patches the compliance fails. Still, I've made some changes which I can submit. I've
> some comments to you (Shashank):
Thanks for addressing these patches. You are welcome to review the series.
Will it be possible for you, to comment in-line with the patch code, it's easier that way, and kind of conventional too. 

> Second, you are expecting that the picture aspect field is correctly set in the HDMI parsing but, at least in the test equipment that I am using, this field is not set by the DRM layer because the mode is coming in the detailed timings section which does not
> include a picture aspect field. In my implementation I add a function which given the mode width and height (fields
> ->width_mm and ->height_mm of mode) computes the aspect ratio and populates the field.
Please note that we can run the aspect ratio test cases (7-27 and similar) for CEA modes only. For the modes coming from DTDs and VSDBs can be with or without aspect ratio.
But the suggestion to initialize all the drm_modes with ASPECT_RATIO_NONE/DEFAULT is a good one, and it might help for these modes too. 
I think Daniel also had similar suggestion last time, in a different context.

> Third, I am facing some difficulties when using Xserver and Xrandr. Using libdrm's modetest application everything works ok but with xrandr the aspect ratio gets lost between the link DRM
> -> Xserver -> DRM. I set the aspect ratio in the flags field when
> passing the mode to user level (just like you do in patch 2) but then when the mode is returned and delivered to the DRM driver the picture aspect is not present. I think this is due to how Xserver or xrandr sets the mode but I am not sure.
I think while parsing the aspect ratio from libdrm to userspace (X), it's getting lost, and we have to fix your Xserver implementation.
We had added similar support in our HWComposer, and I guess it would be required for X and Wayland too, coz finally these guys issue the modeset.
So May be X server is not handling these flags, ignoring these flags, and sending the flagless modeset back to libdrm.

Regards
Shashank
-----Original Message-----
From: Jose Abreu [mailto:Jose.Abreu@synopsys.com] 
Sent: Wednesday, August 3, 2016 6:38 PM
To: Daniel Vetter <daniel@ffwll.ch>; Sharma, Shashank <shashank.sharma@intel.com>
Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vetter, Daniel <daniel.vetter@intel.com>; Carlos Palminha <CARLOS.PALMINHA@synopsys.com>
Subject: Re: [PATCH 0/4]: Picture aspect ratio support in DRM layer

Hi,


On 03-08-2016 12:48, Daniel Vetter wrote:
> On Wed, Aug 03, 2016 at 04:26:24PM +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.
>>
>> 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
> Patch series seems to have 0 changelogs anywhere, but I'm pretty sure 
> I've seen this before. Please try again and state what changed and why 
> you are resubmitting this.
> -Daniel

I've also seen this before and I am using them in order to pass HDMI compliance. Without these patches the compliance fails.
Still, I've made some changes which I can submit. I've some comments to you (Shashank):

First, you add an if condition in
drm_mode_equal_no_clocks_no_stereo() (patch 2) which unconditionally compares the aspect ratio. But I think that you have to take into account that some modes handed by the user to the DRM layer do not initialize this field so I think the best solution would be to compare the aspect ratios only when the field is populated (i.e. picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE).

Second, you are expecting that the picture aspect field is correctly set in the HDMI parsing but, at least in the test equipment that I am using, this field is not set by the DRM layer because the mode is coming in the detailed timings section which does not include a picture aspect field. In my implementation I add a function which given the mode width and height (fields
->width_mm and ->height_mm of mode) computes the aspect ratio and
populates the field.

Third, I am facing some difficulties when using Xserver and Xrandr. Using libdrm's modetest application everything works ok but with xrandr the aspect ratio gets lost between the link DRM
-> Xserver -> DRM. I set the aspect ratio in the flags field when
passing the mode to user level (just like you do in patch 2) but then when the mode is returned and delivered to the DRM driver the picture aspect is not present. I think this is due to how Xserver or xrandr sets the mode but I am not sure.

@Daniel, can you give some comments regarding this?

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

Best regards,
Jose Miguel Abreu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sean Paul]

On Wed, Aug 3, 2016 at 6:56 AM, Shashank Sharma
<shashank.sharma@intel.com> wrote:
> 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>
> ---
>  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 49a7265..cd66a95 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_PARMASK                  (0x0F<<19)
> +#define  DRM_MODE_FLAG_PARNONE \
> +                       (DRM_MODE_PICTURE_ASPECT_NONE << 19)

While I prefer the spaces in between the << operator, it seems like
convention in this file is to not have them.

> +#define  DRM_MODE_FLAG_PAR4_3 \
> +                       (DRM_MODE_PICTURE_ASPECT_4_3 << 19)
> +#define  DRM_MODE_FLAG_PAR16_9 \
> +                       (DRM_MODE_PICTURE_ASPECT_16_9 << 19)

Not crazy about "PAR". Perhaps DRM_MODE_FLAG_ASPECT_BLAH would be more
descriptive?

>
>  /* 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
>
> _______________________________________________
> 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: [PATCH 2/4] drm: Add aspect ratio parsing in DRM layer

[Sean Paul]

On Wed, Aug 3, 2016 at 6:56 AM, Shashank Sharma
<shashank.sharma@intel.com> wrote:
> Current DRM layer functions dont parse aspect ratio information

s/dont/don't/

> while converting a user mode->kernel mode or viceversa. This

s/viceversa/vice versa/

> causes modeset to pick mode with wrong aspect ratio, eventually
> cauing failures in HDMI compliance test cases, due to wrong VIC.

s/cauing/causing/

Sorry for the spelling nits. I originally just found this one, but
since I was nitpicking one, I figured I should pick at them all.

>
> 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>
> ---
>  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 fc5040a..e6029e0 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -969,6 +969,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;
> @@ -1471,6 +1472,22 @@ 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_PARMASK;
> +
> +       switch (in->picture_aspect_ratio) {
> +       case HDMI_PICTURE_ASPECT_4_3:
> +               out->flags |= DRM_MODE_FLAG_PAR4_3;
> +               break;
> +       case HDMI_PICTURE_ASPECT_16_9:
> +               out->flags |= DRM_MODE_FLAG_PAR16_9;
> +               break;
> +       case HDMI_PICTURE_ASPECT_NONE:
> +       case HDMI_PICTURE_ASPECT_RESERVED:
> +       default:

No need to specify HDMI_PICTURE_ASPECT_NONE &
HDMI_PICTURE_ASPECT_RESERVED if you use default.

> +               out->flags |= DRM_MODE_FLAG_PARNONE;
> +               break;
> +       }
> +
>         strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>         out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  }
> @@ -1516,6 +1533,20 @@ 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_PARMASK;
> +
> +       switch (in->flags & DRM_MODE_FLAG_PARMASK) {
> +       case DRM_MODE_FLAG_PAR4_3:
> +               out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
> +               break;
> +       case DRM_MODE_FLAG_PAR16_9:
> +               out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> +               break;
> +       default:
> +               out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;

For safety,

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sean Paul]

On Wed, Aug 3, 2016 at 6:56 AM, Shashank Sharma
<shashank.sharma@intel.com> 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>

> ---
>  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
>
> _______________________________________________
> 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: [Intel-gfx] [PATCH 4/4] drm: Add and handle new aspect ratios in DRM layer

[Sean Paul]

On Wed, Aug 3, 2016 at 6:56 AM, Shashank Sharma
<shashank.sharma@intel.com> wrote:
> 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 viseversa.
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  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 e6029e0..bdc353d 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1481,6 +1481,12 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>         case HDMI_PICTURE_ASPECT_16_9:
>                 out->flags |= DRM_MODE_FLAG_PAR16_9;
>                 break;
> +       case HDMI_PICTURE_ASPECT_64_27:
> +               out->flags |= DRM_MODE_FLAG_PAR64_27;
> +               break;
> +       case DRM_MODE_PICTURE_ASPECT_256_135:
> +               out->flags |= DRM_MODE_FLAG_PAR256_135;
> +               break;
>         case HDMI_PICTURE_ASPECT_NONE:
>         case HDMI_PICTURE_ASPECT_RESERVED:
>         default:
> @@ -1543,6 +1549,12 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>         case DRM_MODE_FLAG_PAR16_9:
>                 out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
>                 break;
> +       case DRM_MODE_FLAG_PAR64_27:
> +               out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
> +               break;
> +       case DRM_MODE_FLAG_PAR256_135:
> +               out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
> +               break;
>         default:
>                 out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>         }
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index cd66a95..6291eae 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_PARMASK                  (0x0F<<19)
> @@ -90,6 +92,10 @@ extern "C" {
>                         (DRM_MODE_PICTURE_ASPECT_4_3 << 19)
>  #define  DRM_MODE_FLAG_PAR16_9 \
>                         (DRM_MODE_PICTURE_ASPECT_16_9 << 19)
> +#define  DRM_MODE_FLAG_PAR64_27 \
> +                       (DRM_MODE_PICTURE_ASPECT_64_27 << 19)
> +#define  DRM_MODE_FLAG_PAR256_135 \
> +                       (DRM_MODE_PICTURE_ASPECT_256_135 << 19)
>
>  /* DPMS flags */
>  /* bit compatible with the xorg definitions. */
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sharma, Shashank]

Thanks for the review, Sean.

My comments, inline.

Regards
Shashank
On 8/3/2016 11:10 PM, Sean Paul wrote:
> On Wed, Aug 3, 2016 at 6:56 AM, Shashank Sharma
> <shashank.sharma@intel.com> wrote:
>> 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>
>> ---
>>   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 49a7265..cd66a95 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_PARMASK                  (0x0F<<19)
>> +#define  DRM_MODE_FLAG_PARNONE \
>> +                       (DRM_MODE_PICTURE_ASPECT_NONE << 19)
> While I prefer the spaces in between the << operator, it seems like
> convention in this file is to not have them.
I agree, I will remove the spaces.
>
>> +#define  DRM_MODE_FLAG_PAR4_3 \
>> +                       (DRM_MODE_PICTURE_ASPECT_4_3 << 19)
>> +#define  DRM_MODE_FLAG_PAR16_9 \
>> +                       (DRM_MODE_PICTURE_ASPECT_16_9 << 19)
> Not crazy about "PAR". Perhaps DRM_MODE_FLAG_ASPECT_BLAH would be more
> descriptive?
Ok, let me come up with something which rhymes with BLAH :)

Regards
Shashank
>>   /* 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
>>
>> _______________________________________________
>> 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: [PATCH 2/4] drm: Add aspect ratio parsing in DRM layer

[Sharma, Shashank]

Regards

Shashank


On 8/3/2016 11:14 PM, Sean Paul wrote:
> On Wed, Aug 3, 2016 at 6:56 AM, Shashank Sharma
> <shashank.sharma@intel.com> wrote:
>> Current DRM layer functions dont parse aspect ratio information
> s/dont/don't/
Got it.
>
>> while converting a user mode->kernel mode or viceversa. This
> s/viceversa/vice versa/
Got it.
>> causes modeset to pick mode with wrong aspect ratio, eventually
>> cauing failures in HDMI compliance test cases, due to wrong VIC.
> s/cauing/causing/
Ok.
> Sorry for the spelling nits. I originally just found this one, but
> since I was nitpicking one, I figured I should pick at them all.
Thanks for pointing them out. Will fix this.
>> 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>
>> ---
>>   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 fc5040a..e6029e0 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -969,6 +969,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;
>> @@ -1471,6 +1472,22 @@ 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_PARMASK;
>> +
>> +       switch (in->picture_aspect_ratio) {
>> +       case HDMI_PICTURE_ASPECT_4_3:
>> +               out->flags |= DRM_MODE_FLAG_PAR4_3;
>> +               break;
>> +       case HDMI_PICTURE_ASPECT_16_9:
>> +               out->flags |= DRM_MODE_FLAG_PAR16_9;
>> +               break;
>> +       case HDMI_PICTURE_ASPECT_NONE:
>> +       case HDMI_PICTURE_ASPECT_RESERVED:
>> +       default:
> No need to specify HDMI_PICTURE_ASPECT_NONE &
> HDMI_PICTURE_ASPECT_RESERVED if you use default.
I agree on ASPECT_NONE, but I think we should keep ASPECT_RESERVED, to 
maintain the convention, do you think so ?
>> +               out->flags |= DRM_MODE_FLAG_PARNONE;
>> +               break;
>> +       }
>> +
>>          strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>>          out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>>   }
>> @@ -1516,6 +1533,20 @@ 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_PARMASK;
>> +
>> +       switch (in->flags & DRM_MODE_FLAG_PARMASK) {
>> +       case DRM_MODE_FLAG_PAR4_3:
>> +               out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
>> +               break;
>> +       case DRM_MODE_FLAG_PAR16_9:
>> +               out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
>> +               break;
>> +       default:
>> +               out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> For safety,
>
> break;
Sure, Can be done.

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

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

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

[Emil Velikov]

On 3 August 2016 at 11:56, Shashank Sharma <shashank.sharma@intel.com> wrote:

> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h

> +
> +/* Aspect ratio flag bitmask (4 bits 22:19) */
> +#define DRM_MODE_FLAG_PARMASK                  (0x0F<<19)
> +#define  DRM_MODE_FLAG_PARNONE \
> +                       (DRM_MODE_PICTURE_ASPECT_NONE << 19)
> +#define  DRM_MODE_FLAG_PAR4_3 \
> +                       (DRM_MODE_PICTURE_ASPECT_4_3 << 19)
> +#define  DRM_MODE_FLAG_PAR16_9 \
> +                       (DRM_MODE_PICTURE_ASPECT_16_9 << 19)
>
Afaict all these flags are internal/implementation specific thus we
shouldn't expose them to userspace. Right ?

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

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

[Sharma, Shashank]

Hello Emil,

Thanks for your time.

I have got mixed opinion on this.

IMHO we should expose them to userspace too, as UI agents like Hardware 
composer/X/Wayland must know what does these

flags means, so that they can display them on the end user screen (like 
settings menu)

But few people even think thats its too complex to be exposed to 
userspace agents.

So your suggestions are welcome.

Regards
Shashank

On 8/4/2016 3:12 PM, Emil Velikov wrote:
> On 3 August 2016 at 11:56, Shashank Sharma <shashank.sharma@intel.com> wrote:
>
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> +
>> +/* Aspect ratio flag bitmask (4 bits 22:19) */
>> +#define DRM_MODE_FLAG_PARMASK                  (0x0F<<19)
>> +#define  DRM_MODE_FLAG_PARNONE \
>> +                       (DRM_MODE_PICTURE_ASPECT_NONE << 19)
>> +#define  DRM_MODE_FLAG_PAR4_3 \
>> +                       (DRM_MODE_PICTURE_ASPECT_4_3 << 19)
>> +#define  DRM_MODE_FLAG_PAR16_9 \
>> +                       (DRM_MODE_PICTURE_ASPECT_16_9 << 19)
>>
> Afaict all these flags are internal/implementation specific thus we
> shouldn't expose them to userspace. Right ?
>
> -Emil

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

Re: [Intel-gfx] [PATCH 1/4] drm: add picture aspect ratio flags

[Daniel Vetter]

On Thu, Aug 04, 2016 at 03:46:09PM +0530, Sharma, Shashank wrote:
> Hello Emil,
> 
> Thanks for your time.
> 
> I have got mixed opinion on this.
> 
> IMHO we should expose them to userspace too, as UI agents like Hardware
> composer/X/Wayland must know what does these
> 
> flags means, so that they can display them on the end user screen (like
> settings menu)
> 
> But few people even think thats its too complex to be exposed to userspace
> agents.
> 
> So your suggestions are welcome.

These are flags for the internal mode representation, not for the uapi
one. They really don't belong into the uapi header, but instead into
include/drm/drm_modes.h. I.e. I fully concur with Emil.
-Daniel

> 
> Regards
> Shashank
> 
> On 8/4/2016 3:12 PM, Emil Velikov wrote:
> > On 3 August 2016 at 11:56, Shashank Sharma <shashank.sharma@intel.com> wrote:
> > 
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > +
> > > +/* Aspect ratio flag bitmask (4 bits 22:19) */
> > > +#define DRM_MODE_FLAG_PARMASK                  (0x0F<<19)
> > > +#define  DRM_MODE_FLAG_PARNONE \
> > > +                       (DRM_MODE_PICTURE_ASPECT_NONE << 19)
> > > +#define  DRM_MODE_FLAG_PAR4_3 \
> > > +                       (DRM_MODE_PICTURE_ASPECT_4_3 << 19)
> > > +#define  DRM_MODE_FLAG_PAR16_9 \
> > > +                       (DRM_MODE_PICTURE_ASPECT_16_9 << 19)
> > > 
> > Afaict all these flags are internal/implementation specific thus we
> > shouldn't expose them to userspace. Right ?
> > 
> > -Emil
> 
> _______________________________________________
> 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

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

[Emil Velikov]

On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> Hello Emil,
>
> Thanks for your time.
>
> I have got mixed opinion on this.
>
> IMHO we should expose them to userspace too, as UI agents like Hardware
> composer/X/Wayland must know what does these
>
> flags means, so that they can display them on the end user screen (like
> settings menu)
>
> But few people even think thats its too complex to be exposed to userspace
> agents.
>
If we want these/such flags passed between kernel and user space one must:
 - Provide a kernel interface how to do that
 - Provide a userspace (acked by respective developers/maintainers)
that the approach is sane and proves useful.

Since the above can take some time, I'd suggest dropping those from
the UAPI header(s)... for now.

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

Re: [Intel-gfx] [PATCH 1/4] drm: add picture aspect ratio flags

[Sharma, Shashank]

Thanks Daniel.

My comments, inline.

Regards
Shashank
On 8/4/2016 4:06 PM, Daniel Vetter wrote:
> On Thu, Aug 04, 2016 at 03:46:09PM +0530, Sharma, Shashank wrote:
>> Hello Emil,
>>
>> Thanks for your time.
>>
>> I have got mixed opinion on this.
>>
>> IMHO we should expose them to userspace too, as UI agents like Hardware
>> composer/X/Wayland must know what does these
>>
>> flags means, so that they can display them on the end user screen (like
>> settings menu)
>>
>> But few people even think thats its too complex to be exposed to userspace
>> agents.
>>
>> So your suggestions are welcome.
> These are flags for the internal mode representation, not for the uapi
> one. They really don't belong into the uapi header, but instead into
> include/drm/drm_modes.h. I.e. I fully concur with Emil.
> -Daniel
Please correct me if I am missing anything, but In that case, how will 
HWC/X apply a modeset
1920x1080@60 16:9 (Not 1920x1080@60 4:3)

Regards
Shashank
>> Regards
>> Shashank
>>
>> On 8/4/2016 3:12 PM, Emil Velikov wrote:
>>> On 3 August 2016 at 11:56, Shashank Sharma <shashank.sharma@intel.com> wrote:
>>>
>>>> --- a/include/uapi/drm/drm_mode.h
>>>> +++ b/include/uapi/drm/drm_mode.h
>>>> +
>>>> +/* Aspect ratio flag bitmask (4 bits 22:19) */
>>>> +#define DRM_MODE_FLAG_PARMASK                  (0x0F<<19)
>>>> +#define  DRM_MODE_FLAG_PARNONE \
>>>> +                       (DRM_MODE_PICTURE_ASPECT_NONE << 19)
>>>> +#define  DRM_MODE_FLAG_PAR4_3 \
>>>> +                       (DRM_MODE_PICTURE_ASPECT_4_3 << 19)
>>>> +#define  DRM_MODE_FLAG_PAR16_9 \
>>>> +                       (DRM_MODE_PICTURE_ASPECT_16_9 << 19)
>>>>
>>> Afaict all these flags are internal/implementation specific thus we
>>> shouldn't expose them to userspace. Right ?
>>>
>>> -Emil
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Sharma, Shashank]

Regards

Shashank


On 8/4/2016 5:04 PM, Emil Velikov wrote:
> On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com> wrote:
>> Hello Emil,
>>
>> Thanks for your time.
>>
>> I have got mixed opinion on this.
>>
>> IMHO we should expose them to userspace too, as UI agents like Hardware
>> composer/X/Wayland must know what does these
>>
>> flags means, so that they can display them on the end user screen (like
>> settings menu)
>>
>> But few people even think thats its too complex to be exposed to userspace
>> agents.
>>
> If we want these/such flags passed between kernel and user space one must:
>   - Provide a kernel interface how to do that
>   - Provide a userspace (acked by respective developers/maintainers)
> that the approach is sane and proves useful.
>
> Since the above can take some time, I'd suggest dropping those from
> the UAPI header(s)... for now.
>
> -Emil
Please guide me a bit more on this problem, Emil, Daniel.
The reason why I want to pass this to userspace is, so that, HWC/X/any 
other UI manager, can send a modeset
which is accurate upto aspect ratio.

Please think of this complete scenario:
- HDMI compliance testing is going on, and we need to apply a CEA mode 
(for example, VIC=3, mode - 720x480@60Hz aspect 16:9)
- But HWC doesnt pass the aspect ratio  information, and passed only 
720x480@60Hz
- Kernel gets the first matching mode, from edid_cea_modes, which is 
VIC=2 720x480@60Hz (aspect 4:3)
- Kernel does the modeset of VIC = 2, and AVI IF contain VIC=2
- Compliance equipment expects VIC = 3, so fails the test case.

In this case, its essential that the userspace, which is triggering the 
modeset, must know about the aspect ratio field too.
So does this make it a reason to pass this information to usp ?

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

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

[Jose Abreu]

Hi Sharma,


On 03-08-2016 16:47, Sharma, Shashank wrote:
> Hello Joes,
>> I've also seen this before and I am using them in order to pass HDMI compliance. Without
>> these patches the compliance fails. Still, I've made some changes which I can submit. I've
>> some comments to you (Shashank):
> Thanks for addressing these patches. You are welcome to review the series.
> Will it be possible for you, to comment in-line with the patch code, it's easier that way, and kind of conventional too. 
>
>> Second, you are expecting that the picture aspect field is correctly set in the HDMI parsing but, at least in the test equipment that I am using, this field is not set by the DRM layer because the mode is coming in the detailed timings section which does not
>> include a picture aspect field. In my implementation I add a function which given the mode width and height (fields
>> ->width_mm and ->height_mm of mode) computes the aspect ratio and populates the field.
> Please note that we can run the aspect ratio test cases (7-27 and similar) for CEA modes only. For the modes coming from DTDs and VSDBs can be with or without aspect ratio.
> But the suggestion to initialize all the drm_modes with ASPECT_RATIO_NONE/DEFAULT is a good one, and it might help for these modes too. 
> I think Daniel also had similar suggestion last time, in a different context.

In our compliance equipment the modes are coming from DTD and
from the video datablock but the kernel is preferring the DTD
modes so we found a way of determining the picture aspect ratio
from the DTD section. We do not populate the field with
ASPECT_RATIO_NONE/DEFAULT, we populate it given the ratio of
width_mm and height_mm that is sent in the DTD (of course if
these values are zero we set aspect ratio to none). I think it
could be a nice addition to the EDID parsing.

>
>> Third, I am facing some difficulties when using Xserver and Xrandr. Using libdrm's modetest application everything works ok but with xrandr the aspect ratio gets lost between the link DRM
>> -> Xserver -> DRM. I set the aspect ratio in the flags field when
>> passing the mode to user level (just like you do in patch 2) but then when the mode is returned and delivered to the DRM driver the picture aspect is not present. I think this is due to how Xserver or xrandr sets the mode but I am not sure.
> I think while parsing the aspect ratio from libdrm to userspace (X), it's getting lost, and we have to fix your Xserver implementation.
> We had added similar support in our HWComposer, and I guess it would be required for X and Wayland too, coz finally these guys issue the modeset.
> So May be X server is not handling these flags, ignoring these flags, and sending the flagless modeset back to libdrm.

Do you mean Xserver, the X driver that I am using (which is
modesetting), the xrandr or all of them? I guess that if they
don't touch the flags field and return the mode exactly the same
as it was probed to DRM then it will work as expected.

Best regards,
Jose Miguel Abreu

>
> Regards
> Shashank
> -----Original Message-----
> From: Jose Abreu [mailto:Jose.Abreu@synopsys.com] 
> Sent: Wednesday, August 3, 2016 6:38 PM
> To: Daniel Vetter <daniel@ffwll.ch>; Sharma, Shashank <shashank.sharma@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vetter, Daniel <daniel.vetter@intel.com>; Carlos Palminha <CARLOS.PALMINHA@synopsys.com>
> Subject: Re: [PATCH 0/4]: Picture aspect ratio support in DRM layer
>
> Hi,
>
>
> On 03-08-2016 12:48, Daniel Vetter wrote:
>> On Wed, Aug 03, 2016 at 04:26:24PM +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.
>>>
>>> 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
>> Patch series seems to have 0 changelogs anywhere, but I'm pretty sure 
>> I've seen this before. Please try again and state what changed and why 
>> you are resubmitting this.
>> -Daniel
> I've also seen this before and I am using them in order to pass HDMI compliance. Without these patches the compliance fails.
> Still, I've made some changes which I can submit. I've some comments to you (Shashank):
>
> First, you add an if condition in
> drm_mode_equal_no_clocks_no_stereo() (patch 2) which unconditionally compares the aspect ratio. But I think that you have to take into account that some modes handed by the user to the DRM layer do not initialize this field so I think the best solution would be to compare the aspect ratios only when the field is populated (i.e. picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE).
>
> Second, you are expecting that the picture aspect field is correctly set in the HDMI parsing but, at least in the test equipment that I am using, this field is not set by the DRM layer because the mode is coming in the detailed timings section which does not include a picture aspect field. In my implementation I add a function which given the mode width and height (fields
> ->width_mm and ->height_mm of mode) computes the aspect ratio and
> populates the field.
>
> Third, I am facing some difficulties when using Xserver and Xrandr. Using libdrm's modetest application everything works ok but with xrandr the aspect ratio gets lost between the link DRM
> -> Xserver -> DRM. I set the aspect ratio in the flags field when
> passing the mode to user level (just like you do in patch 2) but then when the mode is returned and delivered to the DRM driver the picture aspect is not present. I think this is due to how Xserver or xrandr sets the mode but I am not sure.
>
> @Daniel, can you give some comments regarding this?
>
>>>  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
> Best regards,
> Jose Miguel Abreu

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

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

[Sharma, Shashank]

Regards

Shashank


On 8/4/2016 7:46 PM, Jose Abreu wrote:
> Hi Sharma,
>
>
> On 03-08-2016 16:47, Sharma, Shashank wrote:
>> Hello Joes,
>>> I've also seen this before and I am using them in order to pass HDMI compliance. Without
>>> these patches the compliance fails. Still, I've made some changes which I can submit. I've
>>> some comments to you (Shashank):
>> Thanks for addressing these patches. You are welcome to review the series.
>> Will it be possible for you, to comment in-line with the patch code, it's easier that way, and kind of conventional too.
>>
>>> Second, you are expecting that the picture aspect field is correctly set in the HDMI parsing but, at least in the test equipment that I am using, this field is not set by the DRM layer because the mode is coming in the detailed timings section which does not
>>> include a picture aspect field. In my implementation I add a function which given the mode width and height (fields
>>> ->width_mm and ->height_mm of mode) computes the aspect ratio and populates the field.
>> Please note that we can run the aspect ratio test cases (7-27 and similar) for CEA modes only. For the modes coming from DTDs and VSDBs can be with or without aspect ratio.
>> But the suggestion to initialize all the drm_modes with ASPECT_RATIO_NONE/DEFAULT is a good one, and it might help for these modes too.
>> I think Daniel also had similar suggestion last time, in a different context.
> In our compliance equipment the modes are coming from DTD and
> from the video datablock but the kernel is preferring the DTD
> modes so we found a way of determining the picture aspect ratio
> from the DTD section. We do not populate the field with
> ASPECT_RATIO_NONE/DEFAULT, we populate it given the ratio of
> width_mm and height_mm that is sent in the DTD (of course if
> these values are zero we set aspect ratio to none). I think it
> could be a nice addition to the EDID parsing.
I agree, I will come up with another patch, which does either of this:
- initialize all the DRM_MODE with aspect ratio default while creating 
the new mode itself
- initialize all the DRM_MODES  with aspect ratio default while parsing 
the modes.
>>> Third, I am facing some difficulties when using Xserver and Xrandr. Using libdrm's modetest application everything works ok but with xrandr the aspect ratio gets lost between the link DRM
>>> -> Xserver -> DRM. I set the aspect ratio in the flags field when
>>> passing the mode to user level (just like you do in patch 2) but then when the mode is returned and delivered to the DRM driver the picture aspect is not present. I think this is due to how Xserver or xrandr sets the mode but I am not sure.
>> I think while parsing the aspect ratio from libdrm to userspace (X), it's getting lost, and we have to fix your Xserver implementation.
>> We had added similar support in our HWComposer, and I guess it would be required for X and Wayland too, coz finally these guys issue the modeset.
>> So May be X server is not handling these flags, ignoring these flags, and sending the flagless modeset back to libdrm.
> Do you mean Xserver, the X driver that I am using (which is
> modesetting), the xrandr or all of them? I guess that if they
> don't touch the flags field and return the mode exactly the same
> as it was probed to DRM then it will work as expected.
I agree, in fact, if the userspace is not even touching the flag field, 
we should get the aspect ratio information intact.
But if we are populating the aspect ratio information while reading the 
modes from EDID, and passing the right flags to
userspace, but still we see the modeset doesn't contain the right flags, 
means userspace is clearing the flags or modifying
the flags. So we should check:
- While creating a user_mode from kernel_mode, are we populating the 
aspect ratio flags (If you have my patches, this shoud work)
- These modes are passed to userspace via a get_modes or get_connector 
IOCTLs, so we should check these IOCTLS
- Once user space sends a modeset, does it set the flags properly ?
- while creating a kernel_mode from user_mode, during modeset, are we 
preserving these flags ? (if you have my patches, this should work)

Regards
Shashank
> Best regards,
> Jose Miguel Abreu
>
>> Regards
>> Shashank
>> -----Original Message-----
>> From: Jose Abreu [mailto:Jose.Abreu@synopsys.com]
>> Sent: Wednesday, August 3, 2016 6:38 PM
>> To: Daniel Vetter <daniel@ffwll.ch>; Sharma, Shashank <shashank.sharma@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vetter, Daniel <daniel.vetter@intel.com>; Carlos Palminha <CARLOS.PALMINHA@synopsys.com>
>> Subject: Re: [PATCH 0/4]: Picture aspect ratio support in DRM layer
>>
>> Hi,
>>
>>
>> On 03-08-2016 12:48, Daniel Vetter wrote:
>>> On Wed, Aug 03, 2016 at 04:26:24PM +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.
>>>>
>>>> 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
>>> Patch series seems to have 0 changelogs anywhere, but I'm pretty sure
>>> I've seen this before. Please try again and state what changed and why
>>> you are resubmitting this.
>>> -Daniel
>> I've also seen this before and I am using them in order to pass HDMI compliance. Without these patches the compliance fails.
>> Still, I've made some changes which I can submit. I've some comments to you (Shashank):
>>
>> First, you add an if condition in
>> drm_mode_equal_no_clocks_no_stereo() (patch 2) which unconditionally compares the aspect ratio. But I think that you have to take into account that some modes handed by the user to the DRM layer do not initialize this field so I think the best solution would be to compare the aspect ratios only when the field is populated (i.e. picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE).
>>
>> Second, you are expecting that the picture aspect field is correctly set in the HDMI parsing but, at least in the test equipment that I am using, this field is not set by the DRM layer because the mode is coming in the detailed timings section which does not include a picture aspect field. In my implementation I add a function which given the mode width and height (fields
>> ->width_mm and ->height_mm of mode) computes the aspect ratio and
>> populates the field.
>>
>> Third, I am facing some difficulties when using Xserver and Xrandr. Using libdrm's modetest application everything works ok but with xrandr the aspect ratio gets lost between the link DRM
>> -> Xserver -> DRM. I set the aspect ratio in the flags field when
>> passing the mode to user level (just like you do in patch 2) but then when the mode is returned and delivered to the DRM driver the picture aspect is not present. I think this is due to how Xserver or xrandr sets the mode but I am not sure.
>>
>> @Daniel, can you give some comments regarding this?
>>
>>>>   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
>> Best regards,
>> Jose Miguel Abreu

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

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

[Emil Velikov]

On 4 August 2016 at 14:15, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> On 8/4/2016 5:04 PM, Emil Velikov wrote:
>>
>> On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com>
>> wrote:
>>>
>>> Hello Emil,
>>>
>>> Thanks for your time.
>>>
>>> I have got mixed opinion on this.
>>>
>>> IMHO we should expose them to userspace too, as UI agents like Hardware
>>> composer/X/Wayland must know what does these
>>>
>>> flags means, so that they can display them on the end user screen (like
>>> settings menu)
>>>
>>> But few people even think thats its too complex to be exposed to
>>> userspace
>>> agents.
>>>
>> If we want these/such flags passed between kernel and user space one must:
>>   - Provide a kernel interface how to do that
>>   - Provide a userspace (acked by respective developers/maintainers)
>> that the approach is sane and proves useful.
>>
>> Since the above can take some time, I'd suggest dropping those from
>> the UAPI header(s)... for now.
>>
>> -Emil
>
> Please guide me a bit more on this problem, Emil, Daniel.
> The reason why I want to pass this to userspace is, so that, HWC/X/any other
> UI manager, can send a modeset
> which is accurate upto aspect ratio.
>
Nobody(?) is arguing that you don't to pass such information to/from
userspace :-)
Your series does the internal parsing/management of the attribute and
has no actual UAPI implementation and/or userspace references (to
code/discussions). Thus the UAPI changes should _not_ be part of these
patches.

Daniel's blog [1] has many educational materials why and how things
are done upstream. I would kindly invite you to give them (another?)
courtesy read.

Regards,
Emil

[1] http://blog.ffwll.ch/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jose Abreu]

On 04-08-2016 15:29, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 8/4/2016 7:46 PM, Jose Abreu wrote:
>> Hi Sharma,
>>
>>
>> On 03-08-2016 16:47, Sharma, Shashank wrote:
>>> Hello Joes,
>>>> I've also seen this before and I am using them in order to
>>>> pass HDMI compliance. Without
>>>> these patches the compliance fails. Still, I've made some
>>>> changes which I can submit. I've
>>>> some comments to you (Shashank):
>>> Thanks for addressing these patches. You are welcome to
>>> review the series.
>>> Will it be possible for you, to comment in-line with the
>>> patch code, it's easier that way, and kind of conventional too.
>>>
>>>> Second, you are expecting that the picture aspect field is
>>>> correctly set in the HDMI parsing but, at least in the test
>>>> equipment that I am using, this field is not set by the DRM
>>>> layer because the mode is coming in the detailed timings
>>>> section which does not
>>>> include a picture aspect field. In my implementation I add a
>>>> function which given the mode width and height (fields
>>>> ->width_mm and ->height_mm of mode) computes the aspect
>>>> ratio and populates the field.
>>> Please note that we can run the aspect ratio test cases (7-27
>>> and similar) for CEA modes only. For the modes coming from
>>> DTDs and VSDBs can be with or without aspect ratio.
>>> But the suggestion to initialize all the drm_modes with
>>> ASPECT_RATIO_NONE/DEFAULT is a good one, and it might help
>>> for these modes too.
>>> I think Daniel also had similar suggestion last time, in a
>>> different context.
>> In our compliance equipment the modes are coming from DTD and
>> from the video datablock but the kernel is preferring the DTD
>> modes so we found a way of determining the picture aspect ratio
>> from the DTD section. We do not populate the field with
>> ASPECT_RATIO_NONE/DEFAULT, we populate it given the ratio of
>> width_mm and height_mm that is sent in the DTD (of course if
>> these values are zero we set aspect ratio to none). I think it
>> could be a nice addition to the EDID parsing.
> I agree, I will come up with another patch, which does either
> of this:
> - initialize all the DRM_MODE with aspect ratio default while
> creating the new mode itself
> - initialize all the DRM_MODES  with aspect ratio default while
> parsing the modes.

Ok, please cc me when you send the new patches.

>>>> Third, I am facing some difficulties when using Xserver and
>>>> Xrandr. Using libdrm's modetest application everything works
>>>> ok but with xrandr the aspect ratio gets lost between the
>>>> link DRM
>>>> -> Xserver -> DRM. I set the aspect ratio in the flags field
>>>> when
>>>> passing the mode to user level (just like you do in patch 2)
>>>> but then when the mode is returned and delivered to the DRM
>>>> driver the picture aspect is not present. I think this is
>>>> due to how Xserver or xrandr sets the mode but I am not sure.
>>> I think while parsing the aspect ratio from libdrm to
>>> userspace (X), it's getting lost, and we have to fix your
>>> Xserver implementation.
>>> We had added similar support in our HWComposer, and I guess
>>> it would be required for X and Wayland too, coz finally these
>>> guys issue the modeset.
>>> So May be X server is not handling these flags, ignoring
>>> these flags, and sending the flagless modeset back to libdrm.
>> Do you mean Xserver, the X driver that I am using (which is
>> modesetting), the xrandr or all of them? I guess that if they
>> don't touch the flags field and return the mode exactly the same
>> as it was probed to DRM then it will work as expected.
> I agree, in fact, if the userspace is not even touching the
> flag field, we should get the aspect ratio information intact.
> But if we are populating the aspect ratio information while
> reading the modes from EDID, and passing the right flags to
> userspace, but still we see the modeset doesn't contain the
> right flags, means userspace is clearing the flags or modifying
> the flags. So we should check:
> - While creating a user_mode from kernel_mode, are we
> populating the aspect ratio flags (If you have my patches, this
> shoud work)

Yes, this is correct.

> - These modes are passed to userspace via a get_modes or
> get_connector IOCTLs, so we should check these IOCTLS

I think it is also happening correctly.

> - Once user space sends a modeset, does it set the flags
> properly ?

If it is a mode created by the user then I think not. If it is a
mode that was passed by the kernel then it will if the flags are
not touched. This correctly happens using libdrm's modetest but
it doesn't happen when using xrandr.

> - while creating a kernel_mode from user_mode, during modeset,
> are we preserving these flags ? (if you have my patches, this
> should work)

Again correct if the user does not touch the flags field.

Best regards,
Jose Miguel Abreu

>
> Regards
> Shashank
>> Best regards,
>> Jose Miguel Abreu
>>
>>> Regards
>>> Shashank
>>> -----Original Message-----
>>> From: Jose Abreu [mailto:Jose.Abreu@synopsys.com]
>>> Sent: Wednesday, August 3, 2016 6:38 PM
>>> To: Daniel Vetter <daniel@ffwll.ch>; Sharma, Shashank
>>> <shashank.sharma@intel.com>
>>> Cc: intel-gfx@lists.freedesktop.org;
>>> dri-devel@lists.freedesktop.org; Vetter, Daniel
>>> <daniel.vetter@intel.com>; Carlos Palminha
>>> <CARLOS.PALMINHA@synopsys.com>
>>> Subject: Re: [PATCH 0/4]: Picture aspect ratio support in DRM
>>> layer
>>>
>>> Hi,
>>>
>>>
>>> On 03-08-2016 12:48, Daniel Vetter wrote:
>>>> On Wed, Aug 03, 2016 at 04:26:24PM +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.
>>>>>
>>>>> 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
>>>> Patch series seems to have 0 changelogs anywhere, but I'm
>>>> pretty sure
>>>> I've seen this before. Please try again and state what
>>>> changed and why
>>>> you are resubmitting this.
>>>> -Daniel
>>> I've also seen this before and I am using them in order to
>>> pass HDMI compliance. Without these patches the compliance
>>> fails.
>>> Still, I've made some changes which I can submit. I've some
>>> comments to you (Shashank):
>>>
>>> First, you add an if condition in
>>> drm_mode_equal_no_clocks_no_stereo() (patch 2) which
>>> unconditionally compares the aspect ratio. But I think that
>>> you have to take into account that some modes handed by the
>>> user to the DRM layer do not initialize this field so I think
>>> the best solution would be to compare the aspect ratios only
>>> when the field is populated (i.e. picture_aspect_ratio !=
>>> HDMI_PICTURE_ASPECT_NONE).
>>>
>>> Second, you are expecting that the picture aspect field is
>>> correctly set in the HDMI parsing but, at least in the test
>>> equipment that I am using, this field is not set by the DRM
>>> layer because the mode is coming in the detailed timings
>>> section which does not include a picture aspect field. In my
>>> implementation I add a function which given the mode width
>>> and height (fields
>>> ->width_mm and ->height_mm of mode) computes the aspect ratio
>>> and
>>> populates the field.
>>>
>>> Third, I am facing some difficulties when using Xserver and
>>> Xrandr. Using libdrm's modetest application everything works
>>> ok but with xrandr the aspect ratio gets lost between the
>>> link DRM
>>> -> Xserver -> DRM. I set the aspect ratio in the flags field
>>> when
>>> passing the mode to user level (just like you do in patch 2)
>>> but then when the mode is returned and delivered to the DRM
>>> driver the picture aspect is not present. I think this is due
>>> to how Xserver or xrandr sets the mode but I am not sure.
>>>
>>> @Daniel, can you give some comments regarding this?
>>>
>>>>>   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
>>> Best regards,
>>> Jose Miguel Abreu
>

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

Re: [Intel-gfx] [PATCH 1/4] drm: add picture aspect ratio flags

[Daniel Vetter]

On Thu, Aug 04, 2016 at 06:35:52PM +0530, Sharma, Shashank wrote:
> Thanks Daniel.
> 
> My comments, inline.
> 
> Regards
> Shashank
> On 8/4/2016 4:06 PM, Daniel Vetter wrote:
> > On Thu, Aug 04, 2016 at 03:46:09PM +0530, Sharma, Shashank wrote:
> > > Hello Emil,
> > > 
> > > Thanks for your time.
> > > 
> > > I have got mixed opinion on this.
> > > 
> > > IMHO we should expose them to userspace too, as UI agents like Hardware
> > > composer/X/Wayland must know what does these
> > > 
> > > flags means, so that they can display them on the end user screen (like
> > > settings menu)
> > > 
> > > But few people even think thats its too complex to be exposed to userspace
> > > agents.
> > > 
> > > So your suggestions are welcome.
> > These are flags for the internal mode representation, not for the uapi
> > one. They really don't belong into the uapi header, but instead into
> > include/drm/drm_modes.h. I.e. I fully concur with Emil.
> > -Daniel
> Please correct me if I am missing anything, but In that case, how will HWC/X
> apply a modeset
> 1920x1080@60 16:9 (Not 1920x1080@60 4:3)

Argh sorry, I mixed up the two mode structures again. You're correct.
-Daniel

> 
> Regards
> Shashank
> > > Regards
> > > Shashank
> > > 
> > > On 8/4/2016 3:12 PM, Emil Velikov wrote:
> > > > On 3 August 2016 at 11:56, Shashank Sharma <shashank.sharma@intel.com> wrote:
> > > > 
> > > > > --- a/include/uapi/drm/drm_mode.h
> > > > > +++ b/include/uapi/drm/drm_mode.h
> > > > > +
> > > > > +/* Aspect ratio flag bitmask (4 bits 22:19) */
> > > > > +#define DRM_MODE_FLAG_PARMASK                  (0x0F<<19)
> > > > > +#define  DRM_MODE_FLAG_PARNONE \
> > > > > +                       (DRM_MODE_PICTURE_ASPECT_NONE << 19)
> > > > > +#define  DRM_MODE_FLAG_PAR4_3 \
> > > > > +                       (DRM_MODE_PICTURE_ASPECT_4_3 << 19)
> > > > > +#define  DRM_MODE_FLAG_PAR16_9 \
> > > > > +                       (DRM_MODE_PICTURE_ASPECT_16_9 << 19)
> > > > > 
> > > > Afaict all these flags are internal/implementation specific thus we
> > > > shouldn't expose them to userspace. Right ?
> > > > 
> > > > -Emil
> > > _______________________________________________
> > > 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

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

[Daniel Vetter]

On Thu, Aug 04, 2016 at 03:31:45PM +0100, Emil Velikov wrote:
> On 4 August 2016 at 14:15, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> > On 8/4/2016 5:04 PM, Emil Velikov wrote:
> >>
> >> On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com>
> >> wrote:
> >>>
> >>> Hello Emil,
> >>>
> >>> Thanks for your time.
> >>>
> >>> I have got mixed opinion on this.
> >>>
> >>> IMHO we should expose them to userspace too, as UI agents like Hardware
> >>> composer/X/Wayland must know what does these
> >>>
> >>> flags means, so that they can display them on the end user screen (like
> >>> settings menu)
> >>>
> >>> But few people even think thats its too complex to be exposed to
> >>> userspace
> >>> agents.
> >>>
> >> If we want these/such flags passed between kernel and user space one must:
> >>   - Provide a kernel interface how to do that
> >>   - Provide a userspace (acked by respective developers/maintainers)
> >> that the approach is sane and proves useful.
> >>
> >> Since the above can take some time, I'd suggest dropping those from
> >> the UAPI header(s)... for now.
> >>
> >> -Emil
> >
> > Please guide me a bit more on this problem, Emil, Daniel.
> > The reason why I want to pass this to userspace is, so that, HWC/X/any other
> > UI manager, can send a modeset
> > which is accurate upto aspect ratio.
> >
> Nobody(?) is arguing that you don't to pass such information to/from
> userspace :-)
> Your series does the internal parsing/management of the attribute and
> has no actual UAPI implementation and/or userspace references (to
> code/discussions). Thus the UAPI changes should _not_ be part of these
> patches.
> 
> Daniel's blog [1] has many educational materials why and how things
> are done upstream. I would kindly invite you to give them (another?)
> courtesy read.

It reuses the already existing uapi mode structure. And since it extends
them both on the probe side and on the modeset set this means userspace
can just pass through the right probed mode and it'll all magically work.
At least that's the idea.

Now if you actually care about the different bits then you can select the
right mode, but that's about it. So if you want to compensate for the
non-square pixels in rendering then you need to look at it, but otherwise
it's just a case of select the mode you want. I don't see what new
userspace we'd need for that really, current one should all work nicely
as-is. At least the entire point of doing this was seamless support with
existing userspace.
-Daniel
-- 
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: [Intel-gfx] [PATCH 1/4] drm: add picture aspect ratio flags

[Sharma, Shashank]

Regards

Shashank


On 8/4/2016 9:39 PM, Daniel Vetter wrote:
> On Thu, Aug 04, 2016 at 03:31:45PM +0100, Emil Velikov wrote:
>> On 4 August 2016 at 14:15, Sharma, Shashank <shashank.sharma@intel.com> wrote:
>>> On 8/4/2016 5:04 PM, Emil Velikov wrote:
>>>> On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com>
>>>> wrote:
>>>>> Hello Emil,
>>>>>
>>>>> Thanks for your time.
>>>>>
>>>>> I have got mixed opinion on this.
>>>>>
>>>>> IMHO we should expose them to userspace too, as UI agents like Hardware
>>>>> composer/X/Wayland must know what does these
>>>>>
>>>>> flags means, so that they can display them on the end user screen (like
>>>>> settings menu)
>>>>>
>>>>> But few people even think thats its too complex to be exposed to
>>>>> userspace
>>>>> agents.
>>>>>
>>>> If we want these/such flags passed between kernel and user space one must:
>>>>    - Provide a kernel interface how to do that
>>>>    - Provide a userspace (acked by respective developers/maintainers)
>>>> that the approach is sane and proves useful.
>>>>
>>>> Since the above can take some time, I'd suggest dropping those from
>>>> the UAPI header(s)... for now.
>>>>
>>>> -Emil
>>> Please guide me a bit more on this problem, Emil, Daniel.
>>> The reason why I want to pass this to userspace is, so that, HWC/X/any other
>>> UI manager, can send a modeset
>>> which is accurate upto aspect ratio.
>>>
>> Nobody(?) is arguing that you don't to pass such information to/from
>> userspace :-)
>> Your series does the internal parsing/management of the attribute and
>> has no actual UAPI implementation and/or userspace references (to
>> code/discussions). Thus the UAPI changes should _not_ be part of these
>> patches.
>>
>> Daniel's blog [1] has many educational materials why and how things
>> are done upstream. I would kindly invite you to give them (another?)
>> courtesy read.
> It reuses the already existing uapi mode structure. And since it extends
> them both on the probe side and on the modeset set this means userspace
> can just pass through the right probed mode and it'll all magically work.
> At least that's the idea.
>
> Now if you actually care about the different bits then you can select the
> right mode, but that's about it. So if you want to compensate for the
> non-square pixels in rendering then you need to look at it, but otherwise
> it's just a case of select the mode you want. I don't see what new
> userspace we'd need for that really, current one should all work nicely
> as-is. At least the entire point of doing this was seamless support with
> existing userspace.
> -Daniel
Thanks Daniel, you explained the zest of this series better than me :)

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

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

[Jose Abreu]

Hi Sharma,


On 05-08-2016 04:37, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 8/4/2016 9:39 PM, Daniel Vetter wrote:
>> On Thu, Aug 04, 2016 at 03:31:45PM +0100, Emil Velikov wrote:
>>> On 4 August 2016 at 14:15, Sharma, Shashank
>>> <shashank.sharma@intel.com> wrote:
>>>> On 8/4/2016 5:04 PM, Emil Velikov wrote:
>>>>> On 4 August 2016 at 11:16, Sharma, Shashank
>>>>> <shashank.sharma@intel.com>
>>>>> wrote:
>>>>>> Hello Emil,
>>>>>>
>>>>>> Thanks for your time.
>>>>>>
>>>>>> I have got mixed opinion on this.
>>>>>>
>>>>>> IMHO we should expose them to userspace too, as UI agents
>>>>>> like Hardware
>>>>>> composer/X/Wayland must know what does these
>>>>>>
>>>>>> flags means, so that they can display them on the end user
>>>>>> screen (like
>>>>>> settings menu)
>>>>>>
>>>>>> But few people even think thats its too complex to be
>>>>>> exposed to
>>>>>> userspace
>>>>>> agents.
>>>>>>
>>>>> If we want these/such flags passed between kernel and user
>>>>> space one must:
>>>>>    - Provide a kernel interface how to do that
>>>>>    - Provide a userspace (acked by respective
>>>>> developers/maintainers)
>>>>> that the approach is sane and proves useful.
>>>>>
>>>>> Since the above can take some time, I'd suggest dropping
>>>>> those from
>>>>> the UAPI header(s)... for now.
>>>>>
>>>>> -Emil
>>>> Please guide me a bit more on this problem, Emil, Daniel.
>>>> The reason why I want to pass this to userspace is, so that,
>>>> HWC/X/any other
>>>> UI manager, can send a modeset
>>>> which is accurate upto aspect ratio.
>>>>
>>> Nobody(?) is arguing that you don't to pass such information
>>> to/from
>>> userspace :-)
>>> Your series does the internal parsing/management of the
>>> attribute and
>>> has no actual UAPI implementation and/or userspace references
>>> (to
>>> code/discussions). Thus the UAPI changes should _not_ be part
>>> of these
>>> patches.
>>>
>>> Daniel's blog [1] has many educational materials why and how
>>> things
>>> are done upstream. I would kindly invite you to give them
>>> (another?)
>>> courtesy read.
>> It reuses the already existing uapi mode structure. And since
>> it extends
>> them both on the probe side and on the modeset set this means
>> userspace
>> can just pass through the right probed mode and it'll all
>> magically work.
>> At least that's the idea.
>>
>> Now if you actually care about the different bits then you can
>> select the
>> right mode, but that's about it. So if you want to compensate
>> for the
>> non-square pixels in rendering then you need to look at it,
>> but otherwise
>> it's just a case of select the mode you want. I don't see what
>> new
>> userspace we'd need for that really, current one should all
>> work nicely
>> as-is. At least the entire point of doing this was seamless
>> support with
>> existing userspace.
>> -Daniel
> Thanks Daniel, you explained the zest of this series better
> than me :)
>
> Regards
> Shashank
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Will you send a new version of these patches? I have a patch
ready that adds the new HDMI 2.0 modes to the CEA modes list in
DRM but these modes require the addition of the new picture
aspect ratio flags (64:27, 256:135). I can either wait that your
patches get accepted or I can add to my patch set one that adds
the new PAR flags.

Best regards,
Jose Miguel Abreu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm: add picture aspect ratio flags

[Daniel Vetter]

On Tue, Aug 9, 2016 at 3:12 PM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> Will you send a new version of these patches? I have a patch
> ready that adds the new HDMI 2.0 modes to the CEA modes list in
> DRM but these modes require the addition of the new picture
> aspect ratio flags (64:27, 256:135). I can either wait that your
> patches get accepted or I can add to my patch set one that adds
> the new PAR flags.

Please base your work on top of Sharma's entire patch series - I
suspect that Sharma's patch series already covers all you really need.
With the exception of then correctly encoding the chosen aspect ratio
into the infoframes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sharma, Shashank]

Thanks Daniel.
 
Hi Joes, 
As Daniel mentioned, I have already written a patch which adds all HDMI 2.0 modes in CEA DB, and it also completes all the VICs in the DB from 1-107 (we have 1-64 now).

I was myself waiting for aspect ratio patches to get accepted, as I needed them for the modes. 
Will keep you in CC. 

I am planning to send a new patch set by tonight. 

Regards
Shashank

-----Original Message-----
From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, August 9, 2016 7:14 PM
To: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Sharma, Shashank <shashank.sharma@intel.com>; Emil Velikov <emil.l.velikov@gmail.com>; Vetter, Daniel <daniel.vetter@intel.com>; intel-gfx@lists.freedesktop.org; ML dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 1/4] drm: add picture aspect ratio flags

On Tue, Aug 9, 2016 at 3:12 PM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> Will you send a new version of these patches? I have a patch ready 
> that adds the new HDMI 2.0 modes to the CEA modes list in DRM but 
> these modes require the addition of the new picture aspect ratio flags 
> (64:27, 256:135). I can either wait that your patches get accepted or 
> I can add to my patch set one that adds the new PAR flags.

Please base your work on top of Sharma's entire patch series - I suspect that Sharma's patch series already covers all you really need.
With the exception of then correctly encoding the chosen aspect ratio into the infoframes.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Emil Velikov]

On 4 August 2016 at 17:09, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Aug 04, 2016 at 03:31:45PM +0100, Emil Velikov wrote:
>> On 4 August 2016 at 14:15, Sharma, Shashank <shashank.sharma@intel.com> wrote:
>> > On 8/4/2016 5:04 PM, Emil Velikov wrote:
>> >>
>> >> On 4 August 2016 at 11:16, Sharma, Shashank <shashank.sharma@intel.com>
>> >> wrote:
>> >>>
>> >>> Hello Emil,
>> >>>
>> >>> Thanks for your time.
>> >>>
>> >>> I have got mixed opinion on this.
>> >>>
>> >>> IMHO we should expose them to userspace too, as UI agents like Hardware
>> >>> composer/X/Wayland must know what does these
>> >>>
>> >>> flags means, so that they can display them on the end user screen (like
>> >>> settings menu)
>> >>>
>> >>> But few people even think thats its too complex to be exposed to
>> >>> userspace
>> >>> agents.
>> >>>
>> >> If we want these/such flags passed between kernel and user space one must:
>> >>   - Provide a kernel interface how to do that
>> >>   - Provide a userspace (acked by respective developers/maintainers)
>> >> that the approach is sane and proves useful.
>> >>
>> >> Since the above can take some time, I'd suggest dropping those from
>> >> the UAPI header(s)... for now.
>> >>
>> >> -Emil
>> >
>> > Please guide me a bit more on this problem, Emil, Daniel.
>> > The reason why I want to pass this to userspace is, so that, HWC/X/any other
>> > UI manager, can send a modeset
>> > which is accurate upto aspect ratio.
>> >
>> Nobody(?) is arguing that you don't to pass such information to/from
>> userspace :-)
>> Your series does the internal parsing/management of the attribute and
>> has no actual UAPI implementation and/or userspace references (to
>> code/discussions). Thus the UAPI changes should _not_ be part of these
>> patches.
>>
>> Daniel's blog [1] has many educational materials why and how things
>> are done upstream. I would kindly invite you to give them (another?)
>> courtesy read.
>
> It reuses the already existing uapi mode structure. And since it extends
> them both on the probe side and on the modeset set this means userspace
> can just pass through the right probed mode and it'll all magically work.
> At least that's the idea.
>
> Now if you actually care about the different bits then you can select the
> right mode, but that's about it. So if you want to compensate for the
> non-square pixels in rendering then you need to look at it, but otherwise
> it's just a case of select the mode you want. I don't see what new
> userspace we'd need for that really, current one should all work nicely
> as-is. At least the entire point of doing this was seamless support with
> existing userspace.
I failed to click that drm_mode_convert_umode does input validation,
apart from the actual conversion.

Thanks a lot gents and sorry for the noise.
Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx