[RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
According to the spec the EDID may contain two blocks that
signal this sampling mode:
	- YCbCr 4:2:0 Video Data Block
	- YCbCr 4:2:0 Video Capability Map Data Block

The video data block contains the list of vic's were
only YCbCr 4:2:0 sampling mode shall be used while the
video capability map data block contains a mask were
YCbCr 4:2:0 sampling mode may be used.

This RFC patch adds support for parsing these two new blocks
and introduces new flags to signal the drivers if the
mode is 4:2:0'only or 4:2:0'able.

The reason this is still a RFC is because there is no
reference in kernel for this new sampling mode (specially in
AVI infoframe part), so, I was hoping to hear some feedback
first.

Tested in a HDMI 2.0 compliance scenario.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_modes.c |  10 +++-
 include/uapi/drm/drm_mode.h |   6 ++
 3 files changed, 151 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 67d6a73..6ce1a38 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
 #define VENDOR_BLOCK    0x03
 #define SPEAKER_BLOCK	0x04
 #define VIDEO_CAPABILITY_BLOCK	0x07
+#define VIDEO_DATA_BLOCK_420	0x0E
+#define VIDEO_CAP_BLOCK_420	0x0F
 #define EDID_BASIC_AUDIO	(1 << 6)
 #define EDID_CEA_YCRCB444	(1 << 5)
 #define EDID_CEA_YCRCB422	(1 << 4)
@@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
 	return modes;
 }
 
+static int add_420_mode(struct drm_connector *connector, u8 vic)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_display_mode *newmode;
+
+	if (!drm_valid_cea_vic(vic))
+		return 0;
+
+	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
+	if (!newmode)
+		return 0;
+
+	newmode->flags |= DRM_MODE_FLAG_420_ONLY;
+	drm_mode_probed_add(connector, newmode);
+
+	return 1;
+}
+
+static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
+		u8 svds_len)
+{
+	int modes = 0, i;
+
+	for (i = 0; i < svds_len; i++)
+		modes += add_420_mode(connector, svds[i]);
+
+	return modes;
+}
+
+static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
+		u8 svds_len, const u8 *video_db, u8 video_len)
+{
+	struct drm_display_mode *newmode = NULL;
+	int modes = 0, i, j;
+
+	for (i = 0; i < svds_len; i++) {
+		u8 mask = svds[i];
+		for (j = 0; j < 8; j++) {
+			if (mask & (1 << j)) {
+				newmode = drm_display_mode_from_vic_index(
+						connector, video_db, video_len,
+						i * 8 + j);
+				if (newmode) {
+					newmode->flags |= DRM_MODE_FLAG_420;
+					drm_mode_probed_add(connector, newmode);
+					modes++;
+				}
+			}
+		}
+	}
+
+	return modes;
+}
+
+static int add_420_vcb_modes_all(struct drm_connector *connector,
+		const u8 *video_db, u8 video_len)
+{
+	struct drm_display_mode *newmode = NULL;
+	int modes = 0, i;
+
+	for (i = 0; i < video_len; i++) {
+		newmode = drm_display_mode_from_vic_index(connector, video_db,
+				video_len, i);
+		if (newmode) {
+			newmode->flags |= DRM_MODE_FLAG_420;
+			drm_mode_probed_add(connector, newmode);
+			modes++;
+		}
+	}
+
+	return modes;
+}
+
+static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
+		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
+		u8 video_len)
+{
+	int modes = 0;
+
+	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
+		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
+
+	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
+		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
+				video_db, video_len);
+	else if (vcb) /* All modes support 4:2:0 mode */
+		modes += add_420_vcb_modes_all(connector, video_db, video_len);
+
+	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
+	return modes;
+}
+
 /*
  * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
  * @connector: connector corresponding to the HDMI sink
@@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
 }
 
 static int
+cea_db_extended_tag(const u8 *db)
+{
+	return db[1];
+}
+
+static int
 cea_revision(const u8 *cea)
 {
 	return cea[1];
@@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
 	return hdmi_id == HDMI_IEEE_OUI;
 }
 
+static bool cea_db_is_hdmi_vdb420(const u8 *db)
+{
+	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
+		return false;
+
+	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
+		return false;
+
+	return true;
+}
+
+static bool cea_db_is_hdmi_vcb420(const u8 *db)
+{
+	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
+		return false;
+
+	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
+		return false;
+
+	return true;
+}
+
 #define for_each_cea_db(cea, i, start, end) \
 	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
 
@@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
 add_cea_modes(struct drm_connector *connector, struct edid *edid)
 {
 	const u8 *cea = drm_find_cea_extension(edid);
-	const u8 *db, *hdmi = NULL, *video = NULL;
-	u8 dbl, hdmi_len, video_len = 0;
+	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
+	      *vcb420 = NULL;
+	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
 	int modes = 0;
 
 	if (cea && cea_revision(cea) >= 3) {
@@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
 				hdmi = db;
 				hdmi_len = dbl;
 			}
+			else if (cea_db_is_hdmi_vdb420(db)) {
+				vdb420 = db;
+				vdb420_len = dbl;
+			}
+			else if (cea_db_is_hdmi_vcb420(db)) {
+				vcb420 = db;
+				vcb420_len = dbl;
+			}
 		}
 	}
 
@@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
 		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
 					    video_len);
 
+	if (vdb420 || vcb420)
+		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
+				vcb420, vcb420_len, video, video_len);
+
 	return modes;
 }
 
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index ac6a352..53c65f6 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
 	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
 		return false;
 
+	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
+	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
+		return false;
+
 	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
 }
 EXPORT_SYMBOL(drm_mode_equal_no_clocks);
@@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
 bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 					const struct drm_display_mode *mode2)
 {
+	unsigned int flags_mask =
+		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
+
 	if (mode1->hdisplay == mode2->hdisplay &&
 	    mode1->hsync_start == mode2->hsync_start &&
 	    mode1->hsync_end == mode2->hsync_end &&
@@ -995,8 +1002,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->flags & ~DRM_MODE_FLAG_3D_MASK) ==
-	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
+	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
 		return true;
 
 	return false;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index ce7efe2..dc8e285 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -84,6 +84,12 @@
 #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
 #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
 #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
+/*
+ * HDMI 2.0
+ */
+#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
+#define  DRM_MODE_FLAG_420			(1<<19)
+#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
 
 /* Picture aspect ratio options */
 #define DRM_MODE_PICTURE_ASPECT_NONE		0
-- 
1.9.1

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Daniel Vetter]

On Fri, Dec 30, 2016 at 04:53:16PM +0000, Jose Abreu wrote:
> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
> According to the spec the EDID may contain two blocks that
> signal this sampling mode:
> 	- YCbCr 4:2:0 Video Data Block
> 	- YCbCr 4:2:0 Video Capability Map Data Block
> 
> The video data block contains the list of vic's were
> only YCbCr 4:2:0 sampling mode shall be used while the
> video capability map data block contains a mask were
> YCbCr 4:2:0 sampling mode may be used.
> 
> This RFC patch adds support for parsing these two new blocks
> and introduces new flags to signal the drivers if the
> mode is 4:2:0'only or 4:2:0'able.
> 
> The reason this is still a RFC is because there is no
> reference in kernel for this new sampling mode (specially in
> AVI infoframe part), so, I was hoping to hear some feedback
> first.
> 
> Tested in a HDMI 2.0 compliance scenario.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org

Thanks for the patch.

Is there driver code to go along with this? Also, since this extends uapi
we need the userspace changes too, and that will probably highlight the
need to hide these fancy special modes behind an explicit opt-in knob that
userspace sets when it understands these flags. Similar to how we handle
3d modes.
-Daniel
> ---
>  drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_modes.c |  10 +++-
>  include/uapi/drm/drm_mode.h |   6 ++
>  3 files changed, 151 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 67d6a73..6ce1a38 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
>  #define VENDOR_BLOCK    0x03
>  #define SPEAKER_BLOCK	0x04
>  #define VIDEO_CAPABILITY_BLOCK	0x07
> +#define VIDEO_DATA_BLOCK_420	0x0E
> +#define VIDEO_CAP_BLOCK_420	0x0F
>  #define EDID_BASIC_AUDIO	(1 << 6)
>  #define EDID_CEA_YCRCB444	(1 << 5)
>  #define EDID_CEA_YCRCB422	(1 << 4)
> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>  	return modes;
>  }
>  
> +static int add_420_mode(struct drm_connector *connector, u8 vic)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_mode *newmode;
> +
> +	if (!drm_valid_cea_vic(vic))
> +		return 0;
> +
> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> +	if (!newmode)
> +		return 0;
> +
> +	newmode->flags |= DRM_MODE_FLAG_420_ONLY;
> +	drm_mode_probed_add(connector, newmode);
> +
> +	return 1;
> +}
> +
> +static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
> +		u8 svds_len)
> +{
> +	int modes = 0, i;
> +
> +	for (i = 0; i < svds_len; i++)
> +		modes += add_420_mode(connector, svds[i]);
> +
> +	return modes;
> +}
> +
> +static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
> +		u8 svds_len, const u8 *video_db, u8 video_len)
> +{
> +	struct drm_display_mode *newmode = NULL;
> +	int modes = 0, i, j;
> +
> +	for (i = 0; i < svds_len; i++) {
> +		u8 mask = svds[i];
> +		for (j = 0; j < 8; j++) {
> +			if (mask & (1 << j)) {
> +				newmode = drm_display_mode_from_vic_index(
> +						connector, video_db, video_len,
> +						i * 8 + j);
> +				if (newmode) {
> +					newmode->flags |= DRM_MODE_FLAG_420;
> +					drm_mode_probed_add(connector, newmode);
> +					modes++;
> +				}
> +			}
> +		}
> +	}
> +
> +	return modes;
> +}
> +
> +static int add_420_vcb_modes_all(struct drm_connector *connector,
> +		const u8 *video_db, u8 video_len)
> +{
> +	struct drm_display_mode *newmode = NULL;
> +	int modes = 0, i;
> +
> +	for (i = 0; i < video_len; i++) {
> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
> +				video_len, i);
> +		if (newmode) {
> +			newmode->flags |= DRM_MODE_FLAG_420;
> +			drm_mode_probed_add(connector, newmode);
> +			modes++;
> +		}
> +	}
> +
> +	return modes;
> +}
> +
> +static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
> +		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
> +		u8 video_len)
> +{
> +	int modes = 0;
> +
> +	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
> +		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
> +
> +	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
> +		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
> +				video_db, video_len);
> +	else if (vcb) /* All modes support 4:2:0 mode */
> +		modes += add_420_vcb_modes_all(connector, video_db, video_len);
> +
> +	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
> +	return modes;
> +}
> +
>  /*
>   * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
>   * @connector: connector corresponding to the HDMI sink
> @@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>  }
>  
>  static int
> +cea_db_extended_tag(const u8 *db)
> +{
> +	return db[1];
> +}
> +
> +static int
>  cea_revision(const u8 *cea)
>  {
>  	return cea[1];
> @@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  	return hdmi_id == HDMI_IEEE_OUI;
>  }
>  
> +static bool cea_db_is_hdmi_vdb420(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
> +		return false;
> +
> +	return true;
> +}
> +
>  #define for_each_cea_db(cea, i, start, end) \
>  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
>  
> @@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  {
>  	const u8 *cea = drm_find_cea_extension(edid);
> -	const u8 *db, *hdmi = NULL, *video = NULL;
> -	u8 dbl, hdmi_len, video_len = 0;
> +	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
> +	      *vcb420 = NULL;
> +	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
>  	int modes = 0;
>  
>  	if (cea && cea_revision(cea) >= 3) {
> @@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  				hdmi = db;
>  				hdmi_len = dbl;
>  			}
> +			else if (cea_db_is_hdmi_vdb420(db)) {
> +				vdb420 = db;
> +				vdb420_len = dbl;
> +			}
> +			else if (cea_db_is_hdmi_vcb420(db)) {
> +				vcb420 = db;
> +				vcb420_len = dbl;
> +			}
>  		}
>  	}
>  
> @@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
>  					    video_len);
>  
> +	if (vdb420 || vcb420)
> +		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
> +				vcb420, vcb420_len, video, video_len);
> +
>  	return modes;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac6a352..53c65f6 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>  	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
>  		return false;
>  
> +	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
> +	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
> +		return false;
> +
>  	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
>  }
>  EXPORT_SYMBOL(drm_mode_equal_no_clocks);
> @@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>  bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  					const struct drm_display_mode *mode2)
>  {
> +	unsigned int flags_mask =
> +		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
> +
>  	if (mode1->hdisplay == mode2->hdisplay &&
>  	    mode1->hsync_start == mode2->hsync_start &&
>  	    mode1->hsync_end == mode2->hsync_end &&
> @@ -995,8 +1002,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->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> -	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
> +	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
>  		return true;
>  
>  	return false;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2..dc8e285 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -84,6 +84,12 @@
>  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
>  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
> +/*
> + * HDMI 2.0
> + */
> +#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
> +#define  DRM_MODE_FLAG_420			(1<<19)
> +#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
>  
>  /* Picture aspect ratio options */
>  #define DRM_MODE_PICTURE_ASPECT_NONE		0
> -- 
> 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

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

Hi Daniel,


On 04-01-2017 08:34, Daniel Vetter wrote:
> On Fri, Dec 30, 2016 at 04:53:16PM +0000, Jose Abreu wrote:
>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>> According to the spec the EDID may contain two blocks that
>> signal this sampling mode:
>> 	- YCbCr 4:2:0 Video Data Block
>> 	- YCbCr 4:2:0 Video Capability Map Data Block
>>
>> The video data block contains the list of vic's were
>> only YCbCr 4:2:0 sampling mode shall be used while the
>> video capability map data block contains a mask were
>> YCbCr 4:2:0 sampling mode may be used.
>>
>> This RFC patch adds support for parsing these two new blocks
>> and introduces new flags to signal the drivers if the
>> mode is 4:2:0'only or 4:2:0'able.
>>
>> The reason this is still a RFC is because there is no
>> reference in kernel for this new sampling mode (specially in
>> AVI infoframe part), so, I was hoping to hear some feedback
>> first.
>>
>> Tested in a HDMI 2.0 compliance scenario.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
> Thanks for the patch.
>
> Is there driver code to go along with this? Also, since this extends uapi
> we need the userspace changes too, and that will probably highlight the
> need to hide these fancy special modes behind an explicit opt-in knob that
> userspace sets when it understands these flags. Similar to how we handle
> 3d modes.
> -Daniel

Yes, we have internal support for this sampling mode in bridge
dw-hdmi and we are planning to submit it. We also have to add
support for this in the AVI infoframe but I think that doesn't
break userspace neither drivers.

Regarding the uapi changes it's the thing that I am concerned and
I will definitely need some guidance to do that. You mean by
opt-in knob a change to drm_mode_expose_to_userspace() function?

Best regards,
Jose Miguel Abreu

>> ---
>>  drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/drm_modes.c |  10 +++-
>>  include/uapi/drm/drm_mode.h |   6 ++
>>  3 files changed, 151 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 67d6a73..6ce1a38 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
>>  #define VENDOR_BLOCK    0x03
>>  #define SPEAKER_BLOCK	0x04
>>  #define VIDEO_CAPABILITY_BLOCK	0x07
>> +#define VIDEO_DATA_BLOCK_420	0x0E
>> +#define VIDEO_CAP_BLOCK_420	0x0F
>>  #define EDID_BASIC_AUDIO	(1 << 6)
>>  #define EDID_CEA_YCRCB444	(1 << 5)
>>  #define EDID_CEA_YCRCB422	(1 << 4)
>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>>  	return modes;
>>  }
>>  
>> +static int add_420_mode(struct drm_connector *connector, u8 vic)
>> +{
>> +	struct drm_device *dev = connector->dev;
>> +	struct drm_display_mode *newmode;
>> +
>> +	if (!drm_valid_cea_vic(vic))
>> +		return 0;
>> +
>> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>> +	if (!newmode)
>> +		return 0;
>> +
>> +	newmode->flags |= DRM_MODE_FLAG_420_ONLY;
>> +	drm_mode_probed_add(connector, newmode);
>> +
>> +	return 1;
>> +}
>> +
>> +static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
>> +		u8 svds_len)
>> +{
>> +	int modes = 0, i;
>> +
>> +	for (i = 0; i < svds_len; i++)
>> +		modes += add_420_mode(connector, svds[i]);
>> +
>> +	return modes;
>> +}
>> +
>> +static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
>> +		u8 svds_len, const u8 *video_db, u8 video_len)
>> +{
>> +	struct drm_display_mode *newmode = NULL;
>> +	int modes = 0, i, j;
>> +
>> +	for (i = 0; i < svds_len; i++) {
>> +		u8 mask = svds[i];
>> +		for (j = 0; j < 8; j++) {
>> +			if (mask & (1 << j)) {
>> +				newmode = drm_display_mode_from_vic_index(
>> +						connector, video_db, video_len,
>> +						i * 8 + j);
>> +				if (newmode) {
>> +					newmode->flags |= DRM_MODE_FLAG_420;
>> +					drm_mode_probed_add(connector, newmode);
>> +					modes++;
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	return modes;
>> +}
>> +
>> +static int add_420_vcb_modes_all(struct drm_connector *connector,
>> +		const u8 *video_db, u8 video_len)
>> +{
>> +	struct drm_display_mode *newmode = NULL;
>> +	int modes = 0, i;
>> +
>> +	for (i = 0; i < video_len; i++) {
>> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
>> +				video_len, i);
>> +		if (newmode) {
>> +			newmode->flags |= DRM_MODE_FLAG_420;
>> +			drm_mode_probed_add(connector, newmode);
>> +			modes++;
>> +		}
>> +	}
>> +
>> +	return modes;
>> +}
>> +
>> +static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
>> +		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
>> +		u8 video_len)
>> +{
>> +	int modes = 0;
>> +
>> +	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
>> +		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
>> +
>> +	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
>> +		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
>> +				video_db, video_len);
>> +	else if (vcb) /* All modes support 4:2:0 mode */
>> +		modes += add_420_vcb_modes_all(connector, video_db, video_len);
>> +
>> +	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
>> +	return modes;
>> +}
>> +
>>  /*
>>   * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
>>   * @connector: connector corresponding to the HDMI sink
>> @@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>>  }
>>  
>>  static int
>> +cea_db_extended_tag(const u8 *db)
>> +{
>> +	return db[1];
>> +}
>> +
>> +static int
>>  cea_revision(const u8 *cea)
>>  {
>>  	return cea[1];
>> @@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  	return hdmi_id == HDMI_IEEE_OUI;
>>  }
>>  
>> +static bool cea_db_is_hdmi_vdb420(const u8 *db)
>> +{
>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
>> +		return false;
>> +
>> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
>> +{
>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
>> +		return false;
>> +
>> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>>  #define for_each_cea_db(cea, i, start, end) \
>>  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
>>  
>> @@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>>  {
>>  	const u8 *cea = drm_find_cea_extension(edid);
>> -	const u8 *db, *hdmi = NULL, *video = NULL;
>> -	u8 dbl, hdmi_len, video_len = 0;
>> +	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
>> +	      *vcb420 = NULL;
>> +	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
>>  	int modes = 0;
>>  
>>  	if (cea && cea_revision(cea) >= 3) {
>> @@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  				hdmi = db;
>>  				hdmi_len = dbl;
>>  			}
>> +			else if (cea_db_is_hdmi_vdb420(db)) {
>> +				vdb420 = db;
>> +				vdb420_len = dbl;
>> +			}
>> +			else if (cea_db_is_hdmi_vcb420(db)) {
>> +				vcb420 = db;
>> +				vcb420_len = dbl;
>> +			}
>>  		}
>>  	}
>>  
>> @@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
>>  					    video_len);
>>  
>> +	if (vdb420 || vcb420)
>> +		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
>> +				vcb420, vcb420_len, video, video_len);
>> +
>>  	return modes;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index ac6a352..53c65f6 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>>  	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
>>  		return false;
>>  
>> +	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
>> +	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
>> +		return false;
>> +
>>  	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
>>  }
>>  EXPORT_SYMBOL(drm_mode_equal_no_clocks);
>> @@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>>  bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>>  					const struct drm_display_mode *mode2)
>>  {
>> +	unsigned int flags_mask =
>> +		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
>> +
>>  	if (mode1->hdisplay == mode2->hdisplay &&
>>  	    mode1->hsync_start == mode2->hsync_start &&
>>  	    mode1->hsync_end == mode2->hsync_end &&
>> @@ -995,8 +1002,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->flags & ~DRM_MODE_FLAG_3D_MASK) ==
>> -	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
>> +	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
>>  		return true;
>>  
>>  	return false;
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index ce7efe2..dc8e285 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -84,6 +84,12 @@
>>  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
>>  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>>  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
>> +/*
>> + * HDMI 2.0
>> + */
>> +#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
>> +#define  DRM_MODE_FLAG_420			(1<<19)
>> +#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
>>  
>>  /* Picture aspect ratio options */
>>  #define DRM_MODE_PICTURE_ASPECT_NONE		0
>> -- 
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DgIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=j5oWw19iFLzjhpxGIA8Ol-5gnLIwpyvDez5j5h7ioUU&s=29t3YOta9uR8eHxWLs24sNu4FkfDav9WYUCI5sDNahw&e= 

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Ville Syrjälä]

On Fri, Dec 30, 2016 at 04:53:16PM +0000, Jose Abreu wrote:
> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
> According to the spec the EDID may contain two blocks that
> signal this sampling mode:
> 	- YCbCr 4:2:0 Video Data Block
> 	- YCbCr 4:2:0 Video Capability Map Data Block
> 
> The video data block contains the list of vic's were
> only YCbCr 4:2:0 sampling mode shall be used while the
> video capability map data block contains a mask were
> YCbCr 4:2:0 sampling mode may be used.
> 
> This RFC patch adds support for parsing these two new blocks
> and introduces new flags to signal the drivers if the
> mode is 4:2:0'only or 4:2:0'able.
> 
> The reason this is still a RFC is because there is no
> reference in kernel for this new sampling mode (specially in
> AVI infoframe part), so, I was hoping to hear some feedback
> first.
> 
> Tested in a HDMI 2.0 compliance scenario.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_modes.c |  10 +++-
>  include/uapi/drm/drm_mode.h |   6 ++
>  3 files changed, 151 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 67d6a73..6ce1a38 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
>  #define VENDOR_BLOCK    0x03
>  #define SPEAKER_BLOCK	0x04
>  #define VIDEO_CAPABILITY_BLOCK	0x07
> +#define VIDEO_DATA_BLOCK_420	0x0E
> +#define VIDEO_CAP_BLOCK_420	0x0F
>  #define EDID_BASIC_AUDIO	(1 << 6)
>  #define EDID_CEA_YCRCB444	(1 << 5)
>  #define EDID_CEA_YCRCB422	(1 << 4)
> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>  	return modes;
>  }
>  
> +static int add_420_mode(struct drm_connector *connector, u8 vic)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_mode *newmode;
> +
> +	if (!drm_valid_cea_vic(vic))
> +		return 0;
> +
> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> +	if (!newmode)
> +		return 0;
> +
> +	newmode->flags |= DRM_MODE_FLAG_420_ONLY;

Why does userspace need to know this? My thinking has been that the
driver would do the right thing automagically.

We do probably want some kind of output colorspace property to allow the
user to select between RGB vs. YCbCr etc. But I think even with that we
should still allow the driver to automagically select YCbCr 4:2:0 output
since that's the only way the mode will work.

> +	drm_mode_probed_add(connector, newmode);
> +
> +	return 1;
> +}
> +
> +static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
> +		u8 svds_len)
> +{
> +	int modes = 0, i;
> +
> +	for (i = 0; i < svds_len; i++)
> +		modes += add_420_mode(connector, svds[i]);
> +
> +	return modes;
> +}
> +
> +static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
> +		u8 svds_len, const u8 *video_db, u8 video_len)
> +{
> +	struct drm_display_mode *newmode = NULL;
> +	int modes = 0, i, j;
> +
> +	for (i = 0; i < svds_len; i++) {
> +		u8 mask = svds[i];
> +		for (j = 0; j < 8; j++) {
> +			if (mask & (1 << j)) {
> +				newmode = drm_display_mode_from_vic_index(
> +						connector, video_db, video_len,
> +						i * 8 + j);
> +				if (newmode) {
> +					newmode->flags |= DRM_MODE_FLAG_420;
> +					drm_mode_probed_add(connector, newmode);
> +					modes++;
> +				}
> +			}
> +		}
> +	}
> +
> +	return modes;
> +}
> +
> +static int add_420_vcb_modes_all(struct drm_connector *connector,
> +		const u8 *video_db, u8 video_len)
> +{
> +	struct drm_display_mode *newmode = NULL;
> +	int modes = 0, i;
> +
> +	for (i = 0; i < video_len; i++) {
> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
> +				video_len, i);
> +		if (newmode) {
> +			newmode->flags |= DRM_MODE_FLAG_420;
> +			drm_mode_probed_add(connector, newmode);
> +			modes++;
> +		}
> +	}
> +
> +	return modes;
> +}
> +
> +static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
> +		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
> +		u8 video_len)
> +{
> +	int modes = 0;
> +
> +	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
> +		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
> +
> +	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
> +		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
> +				video_db, video_len);
> +	else if (vcb) /* All modes support 4:2:0 mode */
> +		modes += add_420_vcb_modes_all(connector, video_db, video_len);
> +
> +	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
> +	return modes;
> +}
> +
>  /*
>   * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
>   * @connector: connector corresponding to the HDMI sink
> @@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>  }
>  
>  static int
> +cea_db_extended_tag(const u8 *db)
> +{
> +	return db[1];
> +}
> +
> +static int
>  cea_revision(const u8 *cea)
>  {
>  	return cea[1];
> @@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  	return hdmi_id == HDMI_IEEE_OUI;
>  }
>  
> +static bool cea_db_is_hdmi_vdb420(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
> +		return false;
> +
> +	return true;
> +}
> +
>  #define for_each_cea_db(cea, i, start, end) \
>  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
>  
> @@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  {
>  	const u8 *cea = drm_find_cea_extension(edid);
> -	const u8 *db, *hdmi = NULL, *video = NULL;
> -	u8 dbl, hdmi_len, video_len = 0;
> +	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
> +	      *vcb420 = NULL;
> +	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
>  	int modes = 0;
>  
>  	if (cea && cea_revision(cea) >= 3) {
> @@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  				hdmi = db;
>  				hdmi_len = dbl;
>  			}
> +			else if (cea_db_is_hdmi_vdb420(db)) {
> +				vdb420 = db;
> +				vdb420_len = dbl;
> +			}
> +			else if (cea_db_is_hdmi_vcb420(db)) {
> +				vcb420 = db;
> +				vcb420_len = dbl;
> +			}
>  		}
>  	}
>  
> @@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
>  					    video_len);
>  
> +	if (vdb420 || vcb420)
> +		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
> +				vcb420, vcb420_len, video, video_len);
> +
>  	return modes;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac6a352..53c65f6 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>  	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
>  		return false;
>  
> +	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
> +	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
> +		return false;
> +
>  	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
>  }
>  EXPORT_SYMBOL(drm_mode_equal_no_clocks);
> @@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>  bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  					const struct drm_display_mode *mode2)
>  {
> +	unsigned int flags_mask =
> +		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
> +
>  	if (mode1->hdisplay == mode2->hdisplay &&
>  	    mode1->hsync_start == mode2->hsync_start &&
>  	    mode1->hsync_end == mode2->hsync_end &&
> @@ -995,8 +1002,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->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> -	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
> +	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
>  		return true;
>  
>  	return false;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2..dc8e285 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -84,6 +84,12 @@
>  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
>  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
> +/*
> + * HDMI 2.0
> + */
> +#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
> +#define  DRM_MODE_FLAG_420			(1<<19)
> +#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
>  
>  /* Picture aspect ratio options */
>  #define DRM_MODE_PICTURE_ASPECT_NONE		0
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

Hi Ville,


On 04-01-2017 13:22, Ville Syrjälä wrote:
> On Fri, Dec 30, 2016 at 04:53:16PM +0000, Jose Abreu wrote:
>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>> According to the spec the EDID may contain two blocks that
>> signal this sampling mode:
>> 	- YCbCr 4:2:0 Video Data Block
>> 	- YCbCr 4:2:0 Video Capability Map Data Block
>>
>> The video data block contains the list of vic's were
>> only YCbCr 4:2:0 sampling mode shall be used while the
>> video capability map data block contains a mask were
>> YCbCr 4:2:0 sampling mode may be used.
>>
>> This RFC patch adds support for parsing these two new blocks
>> and introduces new flags to signal the drivers if the
>> mode is 4:2:0'only or 4:2:0'able.
>>
>> The reason this is still a RFC is because there is no
>> reference in kernel for this new sampling mode (specially in
>> AVI infoframe part), so, I was hoping to hear some feedback
>> first.
>>
>> Tested in a HDMI 2.0 compliance scenario.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/drm_modes.c |  10 +++-
>>  include/uapi/drm/drm_mode.h |   6 ++
>>  3 files changed, 151 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 67d6a73..6ce1a38 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
>>  #define VENDOR_BLOCK    0x03
>>  #define SPEAKER_BLOCK	0x04
>>  #define VIDEO_CAPABILITY_BLOCK	0x07
>> +#define VIDEO_DATA_BLOCK_420	0x0E
>> +#define VIDEO_CAP_BLOCK_420	0x0F
>>  #define EDID_BASIC_AUDIO	(1 << 6)
>>  #define EDID_CEA_YCRCB444	(1 << 5)
>>  #define EDID_CEA_YCRCB422	(1 << 4)
>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>>  	return modes;
>>  }
>>  
>> +static int add_420_mode(struct drm_connector *connector, u8 vic)
>> +{
>> +	struct drm_device *dev = connector->dev;
>> +	struct drm_display_mode *newmode;
>> +
>> +	if (!drm_valid_cea_vic(vic))
>> +		return 0;
>> +
>> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>> +	if (!newmode)
>> +		return 0;
>> +
>> +	newmode->flags |= DRM_MODE_FLAG_420_ONLY;
> Why does userspace need to know this? My thinking has been that the
> driver would do the right thing automagically.
>
> We do probably want some kind of output colorspace property to allow the
> user to select between RGB vs. YCbCr etc. But I think even with that we
> should still allow the driver to automagically select YCbCr 4:2:0 output
> since that's the only way the mode will work.

I agree. When only 4:2:0 is supported there is no need to expose
the flag to userspace. How shall then I signal drivers for this
4:2:0'only sampling mode?

So, for the remaining modes, you propose a new field in the mode
structure called 'colorspace' which contains the list of
supported sampling modes for the given mode? I think it would be
a nice addition. This way if a mode supports only RGB we only
passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
flag, ... And then user could select. We also have to inform user
which one is being actually used.

Best regards,
Jose Miguel Abreu

>
>> +	drm_mode_probed_add(connector, newmode);
>> +
>> +	return 1;
>> +}
>> +
>> +static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
>> +		u8 svds_len)
>> +{
>> +	int modes = 0, i;
>> +
>> +	for (i = 0; i < svds_len; i++)
>> +		modes += add_420_mode(connector, svds[i]);
>> +
>> +	return modes;
>> +}
>> +
>> +static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
>> +		u8 svds_len, const u8 *video_db, u8 video_len)
>> +{
>> +	struct drm_display_mode *newmode = NULL;
>> +	int modes = 0, i, j;
>> +
>> +	for (i = 0; i < svds_len; i++) {
>> +		u8 mask = svds[i];
>> +		for (j = 0; j < 8; j++) {
>> +			if (mask & (1 << j)) {
>> +				newmode = drm_display_mode_from_vic_index(
>> +						connector, video_db, video_len,
>> +						i * 8 + j);
>> +				if (newmode) {
>> +					newmode->flags |= DRM_MODE_FLAG_420;
>> +					drm_mode_probed_add(connector, newmode);
>> +					modes++;
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	return modes;
>> +}
>> +
>> +static int add_420_vcb_modes_all(struct drm_connector *connector,
>> +		const u8 *video_db, u8 video_len)
>> +{
>> +	struct drm_display_mode *newmode = NULL;
>> +	int modes = 0, i;
>> +
>> +	for (i = 0; i < video_len; i++) {
>> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
>> +				video_len, i);
>> +		if (newmode) {
>> +			newmode->flags |= DRM_MODE_FLAG_420;
>> +			drm_mode_probed_add(connector, newmode);
>> +			modes++;
>> +		}
>> +	}
>> +
>> +	return modes;
>> +}
>> +
>> +static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
>> +		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
>> +		u8 video_len)
>> +{
>> +	int modes = 0;
>> +
>> +	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
>> +		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
>> +
>> +	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
>> +		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
>> +				video_db, video_len);
>> +	else if (vcb) /* All modes support 4:2:0 mode */
>> +		modes += add_420_vcb_modes_all(connector, video_db, video_len);
>> +
>> +	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
>> +	return modes;
>> +}
>> +
>>  /*
>>   * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
>>   * @connector: connector corresponding to the HDMI sink
>> @@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>>  }
>>  
>>  static int
>> +cea_db_extended_tag(const u8 *db)
>> +{
>> +	return db[1];
>> +}
>> +
>> +static int
>>  cea_revision(const u8 *cea)
>>  {
>>  	return cea[1];
>> @@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  	return hdmi_id == HDMI_IEEE_OUI;
>>  }
>>  
>> +static bool cea_db_is_hdmi_vdb420(const u8 *db)
>> +{
>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
>> +		return false;
>> +
>> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
>> +{
>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
>> +		return false;
>> +
>> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>>  #define for_each_cea_db(cea, i, start, end) \
>>  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
>>  
>> @@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>>  {
>>  	const u8 *cea = drm_find_cea_extension(edid);
>> -	const u8 *db, *hdmi = NULL, *video = NULL;
>> -	u8 dbl, hdmi_len, video_len = 0;
>> +	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
>> +	      *vcb420 = NULL;
>> +	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
>>  	int modes = 0;
>>  
>>  	if (cea && cea_revision(cea) >= 3) {
>> @@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  				hdmi = db;
>>  				hdmi_len = dbl;
>>  			}
>> +			else if (cea_db_is_hdmi_vdb420(db)) {
>> +				vdb420 = db;
>> +				vdb420_len = dbl;
>> +			}
>> +			else if (cea_db_is_hdmi_vcb420(db)) {
>> +				vcb420 = db;
>> +				vcb420_len = dbl;
>> +			}
>>  		}
>>  	}
>>  
>> @@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
>>  					    video_len);
>>  
>> +	if (vdb420 || vcb420)
>> +		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
>> +				vcb420, vcb420_len, video, video_len);
>> +
>>  	return modes;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index ac6a352..53c65f6 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>>  	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
>>  		return false;
>>  
>> +	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
>> +	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
>> +		return false;
>> +
>>  	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
>>  }
>>  EXPORT_SYMBOL(drm_mode_equal_no_clocks);
>> @@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>>  bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>>  					const struct drm_display_mode *mode2)
>>  {
>> +	unsigned int flags_mask =
>> +		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
>> +
>>  	if (mode1->hdisplay == mode2->hdisplay &&
>>  	    mode1->hsync_start == mode2->hsync_start &&
>>  	    mode1->hsync_end == mode2->hsync_end &&
>> @@ -995,8 +1002,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->flags & ~DRM_MODE_FLAG_3D_MASK) ==
>> -	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
>> +	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
>>  		return true;
>>  
>>  	return false;
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index ce7efe2..dc8e285 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -84,6 +84,12 @@
>>  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
>>  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>>  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
>> +/*
>> + * HDMI 2.0
>> + */
>> +#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
>> +#define  DRM_MODE_FLAG_420			(1<<19)
>> +#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
>>  
>>  /* Picture aspect ratio options */
>>  #define DRM_MODE_PICTURE_ASPECT_NONE		0
>> -- 
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DgIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=wrEITML_IgrIDSea7Q5Fi_ybz9_TVdQZ4aIpgjsRuvo&s=zbMeMXxRIWLaSbyyljhlMOS74zM106xHxld21xu4kxU&e= 

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Ville Syrjälä]

On Wed, Jan 04, 2017 at 04:15:01PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 04-01-2017 13:22, Ville Syrjälä wrote:
> > On Fri, Dec 30, 2016 at 04:53:16PM +0000, Jose Abreu wrote:
> >> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
> >> According to the spec the EDID may contain two blocks that
> >> signal this sampling mode:
> >> 	- YCbCr 4:2:0 Video Data Block
> >> 	- YCbCr 4:2:0 Video Capability Map Data Block
> >>
> >> The video data block contains the list of vic's were
> >> only YCbCr 4:2:0 sampling mode shall be used while the
> >> video capability map data block contains a mask were
> >> YCbCr 4:2:0 sampling mode may be used.
> >>
> >> This RFC patch adds support for parsing these two new blocks
> >> and introduces new flags to signal the drivers if the
> >> mode is 4:2:0'only or 4:2:0'able.
> >>
> >> The reason this is still a RFC is because there is no
> >> reference in kernel for this new sampling mode (specially in
> >> AVI infoframe part), so, I was hoping to hear some feedback
> >> first.
> >>
> >> Tested in a HDMI 2.0 compliance scenario.
> >>
> >> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> >> Cc: Carlos Palminha <palminha@synopsys.com>
> >> Cc: Daniel Vetter <daniel.vetter@intel.com>
> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> Cc: Sean Paul <seanpaul@chromium.org>
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Cc: linux-kernel@vger.kernel.org
> >> ---
> >>  drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
> >>  drivers/gpu/drm/drm_modes.c |  10 +++-
> >>  include/uapi/drm/drm_mode.h |   6 ++
> >>  3 files changed, 151 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index 67d6a73..6ce1a38 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
> >>  #define VENDOR_BLOCK    0x03
> >>  #define SPEAKER_BLOCK	0x04
> >>  #define VIDEO_CAPABILITY_BLOCK	0x07
> >> +#define VIDEO_DATA_BLOCK_420	0x0E
> >> +#define VIDEO_CAP_BLOCK_420	0x0F
> >>  #define EDID_BASIC_AUDIO	(1 << 6)
> >>  #define EDID_CEA_YCRCB444	(1 << 5)
> >>  #define EDID_CEA_YCRCB422	(1 << 4)
> >> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
> >>  	return modes;
> >>  }
> >>  
> >> +static int add_420_mode(struct drm_connector *connector, u8 vic)
> >> +{
> >> +	struct drm_device *dev = connector->dev;
> >> +	struct drm_display_mode *newmode;
> >> +
> >> +	if (!drm_valid_cea_vic(vic))
> >> +		return 0;
> >> +
> >> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> >> +	if (!newmode)
> >> +		return 0;
> >> +
> >> +	newmode->flags |= DRM_MODE_FLAG_420_ONLY;
> > Why does userspace need to know this? My thinking has been that the
> > driver would do the right thing automagically.
> >
> > We do probably want some kind of output colorspace property to allow the
> > user to select between RGB vs. YCbCr etc. But I think even with that we
> > should still allow the driver to automagically select YCbCr 4:2:0 output
> > since that's the only way the mode will work.
> 
> I agree. When only 4:2:0 is supported there is no need to expose
> the flag to userspace. How shall then I signal drivers for this
> 4:2:0'only sampling mode?
> 
> So, for the remaining modes, you propose a new field in the mode
> structure called 'colorspace' which contains the list of
> supported sampling modes for the given mode? I think it would be
> a nice addition. This way if a mode supports only RGB we only
> passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
> flag, ... And then user could select. We also have to inform user
> which one is being actually used.

IIRC there aren't any "RGB only" modes or anything like that. So
YCbCr 4:2:0 is the special case here. We could just add something to the
mode struct for it, or do we already have some other flags thing that's
not exposed to userspace? And I guess drivers should be able to opt into
supporting these 4:2:0 modes in similar way they opt into
interlaced/stereo/whatever.

> 
> Best regards,
> Jose Miguel Abreu
> 
> >
> >> +	drm_mode_probed_add(connector, newmode);
> >> +
> >> +	return 1;
> >> +}
> >> +
> >> +static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
> >> +		u8 svds_len)
> >> +{
> >> +	int modes = 0, i;
> >> +
> >> +	for (i = 0; i < svds_len; i++)
> >> +		modes += add_420_mode(connector, svds[i]);
> >> +
> >> +	return modes;
> >> +}
> >> +
> >> +static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
> >> +		u8 svds_len, const u8 *video_db, u8 video_len)
> >> +{
> >> +	struct drm_display_mode *newmode = NULL;
> >> +	int modes = 0, i, j;
> >> +
> >> +	for (i = 0; i < svds_len; i++) {
> >> +		u8 mask = svds[i];
> >> +		for (j = 0; j < 8; j++) {
> >> +			if (mask & (1 << j)) {
> >> +				newmode = drm_display_mode_from_vic_index(
> >> +						connector, video_db, video_len,
> >> +						i * 8 + j);
> >> +				if (newmode) {
> >> +					newmode->flags |= DRM_MODE_FLAG_420;
> >> +					drm_mode_probed_add(connector, newmode);
> >> +					modes++;
> >> +				}
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return modes;
> >> +}
> >> +
> >> +static int add_420_vcb_modes_all(struct drm_connector *connector,
> >> +		const u8 *video_db, u8 video_len)
> >> +{
> >> +	struct drm_display_mode *newmode = NULL;
> >> +	int modes = 0, i;
> >> +
> >> +	for (i = 0; i < video_len; i++) {
> >> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
> >> +				video_len, i);
> >> +		if (newmode) {
> >> +			newmode->flags |= DRM_MODE_FLAG_420;
> >> +			drm_mode_probed_add(connector, newmode);
> >> +			modes++;
> >> +		}
> >> +	}
> >> +
> >> +	return modes;
> >> +}
> >> +
> >> +static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
> >> +		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
> >> +		u8 video_len)
> >> +{
> >> +	int modes = 0;
> >> +
> >> +	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
> >> +		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
> >> +
> >> +	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
> >> +		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
> >> +				video_db, video_len);
> >> +	else if (vcb) /* All modes support 4:2:0 mode */
> >> +		modes += add_420_vcb_modes_all(connector, video_db, video_len);
> >> +
> >> +	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
> >> +	return modes;
> >> +}
> >> +
> >>  /*
> >>   * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
> >>   * @connector: connector corresponding to the HDMI sink
> >> @@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
> >>  }
> >>  
> >>  static int
> >> +cea_db_extended_tag(const u8 *db)
> >> +{
> >> +	return db[1];
> >> +}
> >> +
> >> +static int
> >>  cea_revision(const u8 *cea)
> >>  {
> >>  	return cea[1];
> >> @@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >>  	return hdmi_id == HDMI_IEEE_OUI;
> >>  }
> >>  
> >> +static bool cea_db_is_hdmi_vdb420(const u8 *db)
> >> +{
> >> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> >> +		return false;
> >> +
> >> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
> >> +		return false;
> >> +
> >> +	return true;
> >> +}
> >> +
> >> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
> >> +{
> >> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> >> +		return false;
> >> +
> >> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
> >> +		return false;
> >> +
> >> +	return true;
> >> +}
> >> +
> >>  #define for_each_cea_db(cea, i, start, end) \
> >>  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
> >>  
> >> @@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
> >>  {
> >>  	const u8 *cea = drm_find_cea_extension(edid);
> >> -	const u8 *db, *hdmi = NULL, *video = NULL;
> >> -	u8 dbl, hdmi_len, video_len = 0;
> >> +	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
> >> +	      *vcb420 = NULL;
> >> +	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
> >>  	int modes = 0;
> >>  
> >>  	if (cea && cea_revision(cea) >= 3) {
> >> @@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >>  				hdmi = db;
> >>  				hdmi_len = dbl;
> >>  			}
> >> +			else if (cea_db_is_hdmi_vdb420(db)) {
> >> +				vdb420 = db;
> >> +				vdb420_len = dbl;
> >> +			}
> >> +			else if (cea_db_is_hdmi_vcb420(db)) {
> >> +				vcb420 = db;
> >> +				vcb420_len = dbl;
> >> +			}
> >>  		}
> >>  	}
> >>  
> >> @@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >>  		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
> >>  					    video_len);
> >>  
> >> +	if (vdb420 || vcb420)
> >> +		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
> >> +				vcb420, vcb420_len, video, video_len);
> >> +
> >>  	return modes;
> >>  }
> >>  
> >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> >> index ac6a352..53c65f6 100644
> >> --- a/drivers/gpu/drm/drm_modes.c
> >> +++ b/drivers/gpu/drm/drm_modes.c
> >> @@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
> >>  	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
> >>  		return false;
> >>  
> >> +	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
> >> +	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
> >> +		return false;
> >> +
> >>  	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
> >>  }
> >>  EXPORT_SYMBOL(drm_mode_equal_no_clocks);
> >> @@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
> >>  bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> >>  					const struct drm_display_mode *mode2)
> >>  {
> >> +	unsigned int flags_mask =
> >> +		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
> >> +
> >>  	if (mode1->hdisplay == mode2->hdisplay &&
> >>  	    mode1->hsync_start == mode2->hsync_start &&
> >>  	    mode1->hsync_end == mode2->hsync_end &&
> >> @@ -995,8 +1002,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->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> >> -	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
> >> +	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
> >>  		return true;
> >>  
> >>  	return false;
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> index ce7efe2..dc8e285 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -84,6 +84,12 @@
> >>  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
> >>  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
> >>  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
> >> +/*
> >> + * HDMI 2.0
> >> + */
> >> +#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
> >> +#define  DRM_MODE_FLAG_420			(1<<19)
> >> +#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
> >>  
> >>  /* Picture aspect ratio options */
> >>  #define DRM_MODE_PICTURE_ASPECT_NONE		0
> >> -- 
> >> 1.9.1
> >>
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DgIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=wrEITML_IgrIDSea7Q5Fi_ybz9_TVdQZ4aIpgjsRuvo&s=zbMeMXxRIWLaSbyyljhlMOS74zM106xHxld21xu4kxU&e= 

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

Hi Ville,


On 04-01-2017 16:36, Ville Syrjälä wrote:
> On Wed, Jan 04, 2017 at 04:15:01PM +0000, Jose Abreu wrote:
>>
 
[snip]

>>> Why does userspace need to know this? My thinking has been that the
>>> driver would do the right thing automagically.
>>>
>>> We do probably want some kind of output colorspace property to allow the
>>> user to select between RGB vs. YCbCr etc. But I think even with that we
>>> should still allow the driver to automagically select YCbCr 4:2:0 output
>>> since that's the only way the mode will work.
>> I agree. When only 4:2:0 is supported there is no need to expose
>> the flag to userspace. How shall then I signal drivers for this
>> 4:2:0'only sampling mode?
>>
>> So, for the remaining modes, you propose a new field in the mode
>> structure called 'colorspace' which contains the list of
>> supported sampling modes for the given mode? I think it would be
>> a nice addition. This way if a mode supports only RGB we only
>> passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
>> flag, ... And then user could select. We also have to inform user
>> which one is being actually used.
> IIRC there aren't any "RGB only" modes or anything like that. So
> YCbCr 4:2:0 is the special case here. We could just add something to the
> mode struct for it, or do we already have some other flags thing that's
> not exposed to userspace? And I guess drivers should be able to opt into
> supporting these 4:2:0 modes in similar way they opt into
> interlaced/stereo/whatever.

I mean, if a source EDID does not declare support for YCbCr modes
(4:2:2 and 4:4:4 [i think they have to be both supported if sink
supports != RGB]) then only RGB can be used. Or is any YCbCr that
is pre-required? Still, I see your point. When EDID declares
support for YCbCr then all modes can use it, and not only some of
them.

I think for stereo modes the flags can be opt in/out in userspace
exposing. There is a function called
drm_mode_expose_to_userspace() which only exposes stereo modes if
user asks to. We could do something similar for 4:2:0 modes (or
even for all pixel encoding). i.e. expose which encoding can be
used in current video mode. What do you think?

About drivers opting in for 4:2:0 modes, then you propose a new
field in drm_connector (called for example ycbcr_420_allowed)
which only does the parsing of the 4:2:0 modes and adds them to
the list when set to true?

Best regards,
Jose Miguel Abreu

>
>> Best regards,
>> Jose Miguel Abreu
>>
>>>

[snip]

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Ville Syrjälä]

On Thu, Jan 05, 2017 at 10:07:45AM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 04-01-2017 16:36, Ville Syrjälä wrote:
> > On Wed, Jan 04, 2017 at 04:15:01PM +0000, Jose Abreu wrote:
> >>
>  
> [snip]
> 
> >>> Why does userspace need to know this? My thinking has been that the
> >>> driver would do the right thing automagically.
> >>>
> >>> We do probably want some kind of output colorspace property to allow the
> >>> user to select between RGB vs. YCbCr etc. But I think even with that we
> >>> should still allow the driver to automagically select YCbCr 4:2:0 output
> >>> since that's the only way the mode will work.
> >> I agree. When only 4:2:0 is supported there is no need to expose
> >> the flag to userspace. How shall then I signal drivers for this
> >> 4:2:0'only sampling mode?
> >>
> >> So, for the remaining modes, you propose a new field in the mode
> >> structure called 'colorspace' which contains the list of
> >> supported sampling modes for the given mode? I think it would be
> >> a nice addition. This way if a mode supports only RGB we only
> >> passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
> >> flag, ... And then user could select. We also have to inform user
> >> which one is being actually used.
> > IIRC there aren't any "RGB only" modes or anything like that. So
> > YCbCr 4:2:0 is the special case here. We could just add something to the
> > mode struct for it, or do we already have some other flags thing that's
> > not exposed to userspace? And I guess drivers should be able to opt into
> > supporting these 4:2:0 modes in similar way they opt into
> > interlaced/stereo/whatever.
> 
> I mean, if a source EDID does not declare support for YCbCr modes
> (4:2:2 and 4:4:4 [i think they have to be both supported if sink
> supports != RGB]) then only RGB can be used. Or is any YCbCr that
> is pre-required? Still, I see your point. When EDID declares
> support for YCbCr then all modes can use it, and not only some of
> them.
> 
> I think for stereo modes the flags can be opt in/out in userspace
> exposing. There is a function called
> drm_mode_expose_to_userspace() which only exposes stereo modes if
> user asks to. We could do something similar for 4:2:0 modes (or
> even for all pixel encoding). i.e. expose which encoding can be
> used in current video mode. What do you think?
> 
> About drivers opting in for 4:2:0 modes, then you propose a new
> field in drm_connector (called for example ycbcr_420_allowed)
> which only does the parsing of the 4:2:0 modes and adds them to
> the list when set to true?

Thinking a bit more about this, we do have a slight problem with not
exposing this information to userspace. Namely we can't actually tell
whether any user provided mode would need to output as 4:2:0 or not.
With the new flag userspace could at least inherit that from the kernel
and pass it back in. But still, expanding the uapi for something like
this feels quite wrong to me. Can we simply look at a particular user
supplied mode and tell whether it needs to be output as 4:2:0 (based
on eg. dotclock)?

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

Hi Ville,


On 05-01-2017 11:46, Ville Syrjälä wrote:
> On Thu, Jan 05, 2017 at 10:07:45AM +0000, Jose Abreu wrote:
>> Hi Ville,
>>
>>
>> On 04-01-2017 16:36, Ville Syrjälä wrote:
>>> On Wed, Jan 04, 2017 at 04:15:01PM +0000, Jose Abreu wrote:
>>  
>> [snip]
>>
>>>>> Why does userspace need to know this? My thinking has been that the
>>>>> driver would do the right thing automagically.
>>>>>
>>>>> We do probably want some kind of output colorspace property to allow the
>>>>> user to select between RGB vs. YCbCr etc. But I think even with that we
>>>>> should still allow the driver to automagically select YCbCr 4:2:0 output
>>>>> since that's the only way the mode will work.
>>>> I agree. When only 4:2:0 is supported there is no need to expose
>>>> the flag to userspace. How shall then I signal drivers for this
>>>> 4:2:0'only sampling mode?
>>>>
>>>> So, for the remaining modes, you propose a new field in the mode
>>>> structure called 'colorspace' which contains the list of
>>>> supported sampling modes for the given mode? I think it would be
>>>> a nice addition. This way if a mode supports only RGB we only
>>>> passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
>>>> flag, ... And then user could select. We also have to inform user
>>>> which one is being actually used.
>>> IIRC there aren't any "RGB only" modes or anything like that. So
>>> YCbCr 4:2:0 is the special case here. We could just add something to the
>>> mode struct for it, or do we already have some other flags thing that's
>>> not exposed to userspace? And I guess drivers should be able to opt into
>>> supporting these 4:2:0 modes in similar way they opt into
>>> interlaced/stereo/whatever.
>> I mean, if a source EDID does not declare support for YCbCr modes
>> (4:2:2 and 4:4:4 [i think they have to be both supported if sink
>> supports != RGB]) then only RGB can be used. Or is any YCbCr that
>> is pre-required? Still, I see your point. When EDID declares
>> support for YCbCr then all modes can use it, and not only some of
>> them.
>>
>> I think for stereo modes the flags can be opt in/out in userspace
>> exposing. There is a function called
>> drm_mode_expose_to_userspace() which only exposes stereo modes if
>> user asks to. We could do something similar for 4:2:0 modes (or
>> even for all pixel encoding). i.e. expose which encoding can be
>> used in current video mode. What do you think?
>>
>> About drivers opting in for 4:2:0 modes, then you propose a new
>> field in drm_connector (called for example ycbcr_420_allowed)
>> which only does the parsing of the 4:2:0 modes and adds them to
>> the list when set to true?
> Thinking a bit more about this, we do have a slight problem with not
> exposing this information to userspace. Namely we can't actually tell
> whether any user provided mode would need to output as 4:2:0 or not.
> With the new flag userspace could at least inherit that from the kernel
> and pass it back in. But still, expanding the uapi for something like
> this feels quite wrong to me. Can we simply look at a particular user
> supplied mode and tell whether it needs to be output as 4:2:0 (based
> on eg. dotclock)?
>

The pixel clock rate is half of the TMDS character rate in 4:2:0
(in 24 bit), but for example in deep color 48 bit it will be the
same rate. There is also a reduction to half of htotal, hactive,
hblank, hfront, hsync and hback but I don't think it's a good
solution to guide us from there. Why does it feel wrong to you
expanding the uapi?

I think its important to say that the chosen colorspace can
improve performance in systems: for example, as I said, 4:2:0
24-bit uses half the rate that RGB 24-bit uses so we have less
trafic in the bus. I am recently working with a FPGA connected
trough pcie and I can definitely say that this is true. But, as
expected, less traffic means less quality in final image so its
not a matter of letting kernel decide, I think its a matter of
user choosing between performance vs. quality.

Best regards,
Jose Miguel Abreu

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Sharma, Shashank]

Regards

Shashank


On 12/30/2016 10:23 PM, Jose Abreu wrote:
> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
> According to the spec the EDID may contain two blocks that
> signal this sampling mode:
> 	- YCbCr 4:2:0 Video Data Block
> 	- YCbCr 4:2:0 Video Capability Map Data Block
>
> The video data block contains the list of vic's were
> only YCbCr 4:2:0 sampling mode shall be used while the
> video capability map data block contains a mask were
> YCbCr 4:2:0 sampling mode may be used.
>
> This RFC patch adds support for parsing these two new blocks
> and introduces new flags to signal the drivers if the
> mode is 4:2:0'only or 4:2:0'able.
>
> The reason this is still a RFC is because there is no
> reference in kernel for this new sampling mode (specially in
> AVI infoframe part), so, I was hoping to hear some feedback
> first.
>
> Tested in a HDMI 2.0 compliance scenario.
>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/drm_modes.c |  10 +++-
>   include/uapi/drm/drm_mode.h |   6 ++
>   3 files changed, 151 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 67d6a73..6ce1a38 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
>   #define VENDOR_BLOCK    0x03
>   #define SPEAKER_BLOCK	0x04
>   #define VIDEO_CAPABILITY_BLOCK	0x07
> +#define VIDEO_DATA_BLOCK_420	0x0E
> +#define VIDEO_CAP_BLOCK_420	0x0F
>   #define EDID_BASIC_AUDIO	(1 << 6)
>   #define EDID_CEA_YCRCB444	(1 << 5)
>   #define EDID_CEA_YCRCB422	(1 << 4)
> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>   	return modes;
>   }
>   
> +static int add_420_mode(struct drm_connector *connector, u8 vic)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_mode *newmode;
> +
> +	if (!drm_valid_cea_vic(vic))
> +		return 0;
> +
> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
Sorry to start the review late, I missed this mail chain. It would be 
great if you can please keep me in CC for this chain.

Practically, YUV420 modes are being used for 4k and UHD video modes. Now 
here, when we want to
add these modes from edid_cea_db, using the VIC index, we should have 
full list of cea_modes from 1 - 107
Particularly 93-107 ( which is for new 38x21 and 40x21 modes, added in 
CEA-861-F). right now, edid_cea_modes
cant index 4k modes, so practically this this patch series will do 
nothing (even though its doing everything right)

To handle this scenario, I had added a patch series 
https://patchwork.freedesktop.org/patch/119627/
(complete the cea modedb (VIC=65 onwards)) Now, this patch series had 
dependency on new aspect ratios
being added in CEA-861-F which I tried to add in series 
(https://patchwork.freedesktop.org/patch/116095/)
Which was added and later reverted by Ville 
(https://patchwork.freedesktop.org/patch/119808/).

In short, the method/sequence for effective development would be:
- Add aspect ratio support in DRM
- Add HDMI 2.0 (CEA-861-F) aspect ratios 
(https://patchwork.freedesktop.org/patch/116095/)
- Complete edid_cea_modes, adding new modes as per 4k VICs 
(https://patchwork.freedesktop.org/patch/119627/ )
- Parse these modes from 420_vdb and 420_vcb using edid_cea_modes db[]  
(This patch series)

And that we should re-prioritize the aspect ratio handling to target YUV 
420 handling from CEA blocks.
Shashank
> +	if (!newmode)
> +		return 0;
> +
> +	newmode->flags |= DRM_MODE_FLAG_420_ONLY;
> +	drm_mode_probed_add(connector, newmode);
> +
> +	return 1;
> +}
> +
> +static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
> +		u8 svds_len)
> +{
> +	int modes = 0, i;
> +
> +	for (i = 0; i < svds_len; i++)
> +		modes += add_420_mode(connector, svds[i]);
> +
> +	return modes;
> +}
> +
> +static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
> +		u8 svds_len, const u8 *video_db, u8 video_len)
> +{
> +	struct drm_display_mode *newmode = NULL;
> +	int modes = 0, i, j;
> +
> +	for (i = 0; i < svds_len; i++) {
> +		u8 mask = svds[i];
> +		for (j = 0; j < 8; j++) {
> +			if (mask & (1 << j)) {
> +				newmode = drm_display_mode_from_vic_index(
> +						connector, video_db, video_len,
> +						i * 8 + j);
> +				if (newmode) {
> +					newmode->flags |= DRM_MODE_FLAG_420;
> +					drm_mode_probed_add(connector, newmode);
> +					modes++;
> +				}
> +			}
> +		}
> +	}
> +
> +	return modes;
> +}
> +
> +static int add_420_vcb_modes_all(struct drm_connector *connector,
> +		const u8 *video_db, u8 video_len)
> +{
> +	struct drm_display_mode *newmode = NULL;
> +	int modes = 0, i;
> +
> +	for (i = 0; i < video_len; i++) {
> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
> +				video_len, i);
> +		if (newmode) {
> +			newmode->flags |= DRM_MODE_FLAG_420;
> +			drm_mode_probed_add(connector, newmode);
> +			modes++;
> +		}
> +	}
> +
> +	return modes;
> +}
> +
> +static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
> +		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
> +		u8 video_len)
> +{
> +	int modes = 0;
> +
> +	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
> +		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
> +
> +	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
> +		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
> +				video_db, video_len);
> +	else if (vcb) /* All modes support 4:2:0 mode */
> +		modes += add_420_vcb_modes_all(connector, video_db, video_len);
> +
> +	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
> +	return modes;
> +}
> +
>   /*
>    * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
>    * @connector: connector corresponding to the HDMI sink
> @@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>   }
>   
>   static int
> +cea_db_extended_tag(const u8 *db)
> +{
> +	return db[1];
> +}
> +
> +static int
>   cea_revision(const u8 *cea)
>   {
>   	return cea[1];
> @@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>   	return hdmi_id == HDMI_IEEE_OUI;
>   }
>   
> +static bool cea_db_is_hdmi_vdb420(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
> +		return false;
> +
> +	return true;
> +}
> +
>   #define for_each_cea_db(cea, i, start, end) \
>   	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
>   
> @@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>   add_cea_modes(struct drm_connector *connector, struct edid *edid)
>   {
>   	const u8 *cea = drm_find_cea_extension(edid);
> -	const u8 *db, *hdmi = NULL, *video = NULL;
> -	u8 dbl, hdmi_len, video_len = 0;
> +	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
> +	      *vcb420 = NULL;
> +	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
>   	int modes = 0;
>   
>   	if (cea && cea_revision(cea) >= 3) {
> @@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>   				hdmi = db;
>   				hdmi_len = dbl;
>   			}
> +			else if (cea_db_is_hdmi_vdb420(db)) {
> +				vdb420 = db;
> +				vdb420_len = dbl;
> +			}
> +			else if (cea_db_is_hdmi_vcb420(db)) {
> +				vcb420 = db;
> +				vcb420_len = dbl;
> +			}
>   		}
>   	}
>   
> @@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>   		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
>   					    video_len);
>   
> +	if (vdb420 || vcb420)
> +		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
> +				vcb420, vcb420_len, video, video_len);
> +
>   	return modes;
>   }
>   
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac6a352..53c65f6 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>   	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
>   		return false;
>   
> +	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
> +	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
> +		return false;
> +
>   	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
>   }
>   EXPORT_SYMBOL(drm_mode_equal_no_clocks);
> @@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>   bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>   					const struct drm_display_mode *mode2)
>   {
> +	unsigned int flags_mask =
> +		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
> +
>   	if (mode1->hdisplay == mode2->hdisplay &&
>   	    mode1->hsync_start == mode2->hsync_start &&
>   	    mode1->hsync_end == mode2->hsync_end &&
> @@ -995,8 +1002,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->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> -	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
> +	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
>   		return true;
>   
>   	return false;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2..dc8e285 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -84,6 +84,12 @@
>   #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
>   #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>   #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
> +/*
> + * HDMI 2.0
> + */
> +#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
> +#define  DRM_MODE_FLAG_420			(1<<19)
> +#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
>   
>   /* Picture aspect ratio options */
>   #define DRM_MODE_PICTURE_ASPECT_NONE		0

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

Hi Shashank,


Thanks for the review.


On 09-01-2017 05:22, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 12/30/2016 10:23 PM, Jose Abreu wrote:
>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>> According to the spec the EDID may contain two blocks that
>> signal this sampling mode:
>>     - YCbCr 4:2:0 Video Data Block
>>     - YCbCr 4:2:0 Video Capability Map Data Block
>>
>> The video data block contains the list of vic's were
>> only YCbCr 4:2:0 sampling mode shall be used while the
>> video capability map data block contains a mask were
>> YCbCr 4:2:0 sampling mode may be used.
>>
>> This RFC patch adds support for parsing these two new blocks
>> and introduces new flags to signal the drivers if the
>> mode is 4:2:0'only or 4:2:0'able.
>>
>> The reason this is still a RFC is because there is no
>> reference in kernel for this new sampling mode (specially in
>> AVI infoframe part), so, I was hoping to hear some feedback
>> first.
>>
>> Tested in a HDMI 2.0 compliance scenario.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 139
>> +++++++++++++++++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/drm_modes.c |  10 +++-
>>   include/uapi/drm/drm_mode.h |   6 ++
>>   3 files changed, 151 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c
>> b/drivers/gpu/drm/drm_edid.c
>> index 67d6a73..6ce1a38 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct
>> drm_connector *connector,
>>   #define VENDOR_BLOCK    0x03
>>   #define SPEAKER_BLOCK    0x04
>>   #define VIDEO_CAPABILITY_BLOCK    0x07
>> +#define VIDEO_DATA_BLOCK_420    0x0E
>> +#define VIDEO_CAP_BLOCK_420    0x0F
>>   #define EDID_BASIC_AUDIO    (1 << 6)
>>   #define EDID_CEA_YCRCB444    (1 << 5)
>>   #define EDID_CEA_YCRCB422    (1 << 4)
>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct
>> drm_connector *connector, u16 structure,
>>       return modes;
>>   }
>>   +static int add_420_mode(struct drm_connector *connector, u8
>> vic)
>> +{
>> +    struct drm_device *dev = connector->dev;
>> +    struct drm_display_mode *newmode;
>> +
>> +    if (!drm_valid_cea_vic(vic))
>> +        return 0;
>> +
>> +    newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> Sorry to start the review late, I missed this mail chain. It
> would be great if you can please keep me in CC for this chain.

Sure. Will do that next time.

>
> Practically, YUV420 modes are being used for 4k and UHD video
> modes. Now here, when we want to
> add these modes from edid_cea_db, using the VIC index, we
> should have full list of cea_modes from 1 - 107
> Particularly 93-107 ( which is for new 38x21 and 40x21 modes,
> added in CEA-861-F). right now, edid_cea_modes
> cant index 4k modes, so practically this this patch series will
> do nothing (even though its doing everything right)

This is correct but not entirely true. I realize 4:2:0 is mostly
used in 4k modes but it can also be used in any other video mode,
as long as it is declared in the VCB.

>
> To handle this scenario, I had added a patch series
> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
> (complete the cea modedb (VIC=65 onwards)) Now, this patch
> series had dependency on new aspect ratios
> being added in CEA-861-F which I tried to add in series
> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
> )
> Which was added and later reverted by Ville
> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119808_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=FAa6aHQ_HjlaVRzDm282p9bSY_tBiN1PngZBhsTqYdI&e=
> ).

Yes, I remember that. If it was breaking userspace then there was
nothing left to do, revert was needed. I thing we should take
your patch and rework/extend it so that userspace does not break
as this is a most welcome feature. The new HDMI spec is almost
ready, and yet, 2.0 features are still missing from the kernel.
We should take advantage from our capability of accessing these
specs, test equipment, compliance equipment ... and submit
patches for these new features :)

>
> In short, the method/sequence for effective development would be:
> - Add aspect ratio support in DRM
> - Add HDMI 2.0 (CEA-861-F) aspect ratios
> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
> )
> - Complete edid_cea_modes, adding new modes as per 4k VICs
> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e= 
> )
> - Parse these modes from 420_vdb and 420_vcb using
> edid_cea_modes db[]  (This patch series)
>

I agree but this rfc does not depend (in terms of code) of any
other patches. The vcb parsing part can be used right now, as for
the vdb part we will have to wait until vic's list is completed.
Thats one of the reasons i sent it in RFC: So that i could ear
some comments before submitting a "real" patch.

Best regards,
Jose Miguel Abreu

> And that we should re-prioritize the aspect ratio handling to
> target YUV 420 handling from CEA blocks.
> Shashank

[snip]

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Sharma, Shashank]

Regards

Shashank


On 1/9/2017 4:41 PM, Jose Abreu wrote:
> Hi Shashank,
>
>
> Thanks for the review.
>
>
> On 09-01-2017 05:22, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 12/30/2016 10:23 PM, Jose Abreu wrote:
>>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>>> According to the spec the EDID may contain two blocks that
>>> signal this sampling mode:
>>>      - YCbCr 4:2:0 Video Data Block
>>>      - YCbCr 4:2:0 Video Capability Map Data Block
>>>
>>> The video data block contains the list of vic's were
>>> only YCbCr 4:2:0 sampling mode shall be used while the
>>> video capability map data block contains a mask were
>>> YCbCr 4:2:0 sampling mode may be used.
>>>
>>> This RFC patch adds support for parsing these two new blocks
>>> and introduces new flags to signal the drivers if the
>>> mode is 4:2:0'only or 4:2:0'able.
>>>
>>> The reason this is still a RFC is because there is no
>>> reference in kernel for this new sampling mode (specially in
>>> AVI infoframe part), so, I was hoping to hear some feedback
>>> first.
>>>
>>> Tested in a HDMI 2.0 compliance scenario.
>>>
>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>> Cc: Sean Paul <seanpaul@chromium.org>
>>> Cc: David Airlie <airlied@linux.ie>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>>    drivers/gpu/drm/drm_edid.c  | 139
>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>    drivers/gpu/drm/drm_modes.c |  10 +++-
>>>    include/uapi/drm/drm_mode.h |   6 ++
>>>    3 files changed, 151 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c
>>> b/drivers/gpu/drm/drm_edid.c
>>> index 67d6a73..6ce1a38 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct
>>> drm_connector *connector,
>>>    #define VENDOR_BLOCK    0x03
>>>    #define SPEAKER_BLOCK    0x04
>>>    #define VIDEO_CAPABILITY_BLOCK    0x07
>>> +#define VIDEO_DATA_BLOCK_420    0x0E
>>> +#define VIDEO_CAP_BLOCK_420    0x0F
>>>    #define EDID_BASIC_AUDIO    (1 << 6)
>>>    #define EDID_CEA_YCRCB444    (1 << 5)
>>>    #define EDID_CEA_YCRCB422    (1 << 4)
>>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct
>>> drm_connector *connector, u16 structure,
>>>        return modes;
>>>    }
>>>    +static int add_420_mode(struct drm_connector *connector, u8
>>> vic)
>>> +{
>>> +    struct drm_device *dev = connector->dev;
>>> +    struct drm_display_mode *newmode;
>>> +
>>> +    if (!drm_valid_cea_vic(vic))
>>> +        return 0;
>>> +
>>> +    newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>> Sorry to start the review late, I missed this mail chain. It
>> would be great if you can please keep me in CC for this chain.
> Sure. Will do that next time.
>
>> Practically, YUV420 modes are being used for 4k and UHD video
>> modes. Now here, when we want to
>> add these modes from edid_cea_db, using the VIC index, we
>> should have full list of cea_modes from 1 - 107
>> Particularly 93-107 ( which is for new 38x21 and 40x21 modes,
>> added in CEA-861-F). right now, edid_cea_modes
>> cant index 4k modes, so practically this this patch series will
>> do nothing (even though its doing everything right)
> This is correct but not entirely true. I realize 4:2:0 is mostly
> used in 4k modes but it can also be used in any other video mode,
> as long as it is declared in the VCB.
I agree (that's why I called it practically).
As such I doubt we will find anything less than a 4k here, coz HDMI 1.4b 
itself can driver 4k@30.
So the biggest benefit of YUV 420, which is half the clock, is mostly 
useful in 4k@60 and above.
I guess we will see more cases of deep-color pixels at non-4k modes soon.
>> To handle this scenario, I had added a patch series
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>> (complete the cea modedb (VIC=65 onwards)) Now, this patch
>> series had dependency on new aspect ratios
>> being added in CEA-861-F which I tried to add in series
>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>> )
>> Which was added and later reverted by Ville
>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119808_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=FAa6aHQ_HjlaVRzDm282p9bSY_tBiN1PngZBhsTqYdI&e=
>> ).
> Yes, I remember that. If it was breaking userspace then there was
> nothing left to do, revert was needed.
As we discovered over the discussions, It dint break anything as such :)
But it made the behavior change for some SW's (which was expected), 
Anyways its gone now.
> I thing we should take
> your patch and rework/extend it so that userspace does not break
> as this is a most welcome feature. The new HDMI spec is almost
> ready, and yet, 2.0 features are still missing from the kernel.
> We should take advantage from our capability of accessing these
> specs, test equipment, compliance equipment ... and submit
> patches for these new features :)
I know. Unfortunately, last time when we spoke about it,  we were 
required to write a full stack code across kernel, drm, libdrm and X 
level, as keeping it
under a cap was not accepted. This seems to be a long term plan to me.
>> In short, the method/sequence for effective development would be:
>> - Add aspect ratio support in DRM
>> - Add HDMI 2.0 (CEA-861-F) aspect ratios
>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>> )
>> - Complete edid_cea_modes, adding new modes as per 4k VICs
>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>> )
>> - Parse these modes from 420_vdb and 420_vcb using
>> edid_cea_modes db[]  (This patch series)
>>
> I agree but this rfc does not depend (in terms of code) of any
> other patches. The vcb parsing part can be used right now, as for
> the vdb part we will have to wait until vic's list is completed.
> Thats one of the reasons i sent it in RFC: So that i could ear
> some comments before submitting a "real" patch.
Right, its so real code, that I forget almost every-time that its a RFC :-)
But I thought its the right place to call, that, we wont be able to test 
4k YUV 420 yet, until we finish the modedb.

- Shashank
>
> Best regards,
> Jose Miguel Abreu
>
>> And that we should re-prioritize the aspect ratio handling to
>> target YUV 420 handling from CEA blocks.
>> Shashank
> [snip]
>

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

Hi Shashank,


On 09-01-2017 12:45, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 1/9/2017 4:41 PM, Jose Abreu wrote:
>> Hi Shashank,
>>
>>
>> Thanks for the review.
>>
>>
>> On 09-01-2017 05:22, Sharma, Shashank wrote:
>>> Regards
>>>
>>> Shashank
>>>
>>>
>>> On 12/30/2016 10:23 PM, Jose Abreu wrote:
>>>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>>>> According to the spec the EDID may contain two blocks that
>>>> signal this sampling mode:
>>>>      - YCbCr 4:2:0 Video Data Block
>>>>      - YCbCr 4:2:0 Video Capability Map Data Block
>>>>
>>>> The video data block contains the list of vic's were
>>>> only YCbCr 4:2:0 sampling mode shall be used while the
>>>> video capability map data block contains a mask were
>>>> YCbCr 4:2:0 sampling mode may be used.
>>>>
>>>> This RFC patch adds support for parsing these two new blocks
>>>> and introduces new flags to signal the drivers if the
>>>> mode is 4:2:0'only or 4:2:0'able.
>>>>
>>>> The reason this is still a RFC is because there is no
>>>> reference in kernel for this new sampling mode (specially in
>>>> AVI infoframe part), so, I was hoping to hear some feedback
>>>> first.
>>>>
>>>> Tested in a HDMI 2.0 compliance scenario.
>>>>
>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>>> Cc: Sean Paul <seanpaul@chromium.org>
>>>> Cc: David Airlie <airlied@linux.ie>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> ---
>>>>    drivers/gpu/drm/drm_edid.c  | 139
>>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>>    drivers/gpu/drm/drm_modes.c |  10 +++-
>>>>    include/uapi/drm/drm_mode.h |   6 ++
>>>>    3 files changed, 151 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c
>>>> b/drivers/gpu/drm/drm_edid.c
>>>> index 67d6a73..6ce1a38 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct
>>>> drm_connector *connector,
>>>>    #define VENDOR_BLOCK    0x03
>>>>    #define SPEAKER_BLOCK    0x04
>>>>    #define VIDEO_CAPABILITY_BLOCK    0x07
>>>> +#define VIDEO_DATA_BLOCK_420    0x0E
>>>> +#define VIDEO_CAP_BLOCK_420    0x0F
>>>>    #define EDID_BASIC_AUDIO    (1 << 6)
>>>>    #define EDID_CEA_YCRCB444    (1 << 5)
>>>>    #define EDID_CEA_YCRCB422    (1 << 4)
>>>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct
>>>> drm_connector *connector, u16 structure,
>>>>        return modes;
>>>>    }
>>>>    +static int add_420_mode(struct drm_connector *connector, u8
>>>> vic)
>>>> +{
>>>> +    struct drm_device *dev = connector->dev;
>>>> +    struct drm_display_mode *newmode;
>>>> +
>>>> +    if (!drm_valid_cea_vic(vic))
>>>> +        return 0;
>>>> +
>>>> +    newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>>> Sorry to start the review late, I missed this mail chain. It
>>> would be great if you can please keep me in CC for this chain.
>> Sure. Will do that next time.
>>
>>> Practically, YUV420 modes are being used for 4k and UHD video
>>> modes. Now here, when we want to
>>> add these modes from edid_cea_db, using the VIC index, we
>>> should have full list of cea_modes from 1 - 107
>>> Particularly 93-107 ( which is for new 38x21 and 40x21 modes,
>>> added in CEA-861-F). right now, edid_cea_modes
>>> cant index 4k modes, so practically this this patch series will
>>> do nothing (even though its doing everything right)
>> This is correct but not entirely true. I realize 4:2:0 is mostly
>> used in 4k modes but it can also be used in any other video mode,
>> as long as it is declared in the VCB.
> I agree (that's why I called it practically).
> As such I doubt we will find anything less than a 4k here, coz
> HDMI 1.4b itself can driver 4k@30.
> So the biggest benefit of YUV 420, which is half the clock, is
> mostly useful in 4k@60 and above.
> I guess we will see more cases of deep-color pixels at non-4k
> modes soon.
>>> To handle this scenario, I had added a patch series
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>>>
>>> (complete the cea modedb (VIC=65 onwards)) Now, this patch
>>> series had dependency on new aspect ratios
>>> being added in CEA-861-F which I tried to add in series
>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>>>
>>> )
>>> Which was added and later reverted by Ville
>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119808_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=FAa6aHQ_HjlaVRzDm282p9bSY_tBiN1PngZBhsTqYdI&e=
>>>
>>> ).
>> Yes, I remember that. If it was breaking userspace then there was
>> nothing left to do, revert was needed.
> As we discovered over the discussions, It dint break anything
> as such :)
> But it made the behavior change for some SW's (which was
> expected), Anyways its gone now.
>> I thing we should take
>> your patch and rework/extend it so that userspace does not break
>> as this is a most welcome feature. The new HDMI spec is almost
>> ready, and yet, 2.0 features are still missing from the kernel.
>> We should take advantage from our capability of accessing these
>> specs, test equipment, compliance equipment ... and submit
>> patches for these new features :)
> I know. Unfortunately, last time when we spoke about it,  we
> were required to write a full stack code across kernel, drm,
> libdrm and X level, as keeping it
> under a cap was not accepted. This seems to be a long term plan
> to me.

I really think we should make the exposure of this new 2.0
features optional. Change the drivers and drm core first and then
move to userland. We can't expect user to deploy the changes at
the same time we apply them to kernel.

>>> In short, the method/sequence for effective development would
>>> be:
>>> - Add aspect ratio support in DRM
>>> - Add HDMI 2.0 (CEA-861-F) aspect ratios
>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>>>
>>> )
>>> - Complete edid_cea_modes, adding new modes as per 4k VICs
>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>>>
>>> )
>>> - Parse these modes from 420_vdb and 420_vcb using
>>> edid_cea_modes db[]  (This patch series)
>>>
>> I agree but this rfc does not depend (in terms of code) of any
>> other patches. The vcb parsing part can be used right now, as for
>> the vdb part we will have to wait until vic's list is completed.
>> Thats one of the reasons i sent it in RFC: So that i could ear
>> some comments before submitting a "real" patch.
> Right, its so real code, that I forget almost every-time that
> its a RFC :-)
> But I thought its the right place to call, that, we wont be
> able to test 4k YUV 420 yet, until we finish the modedb.

At the time I tested 420 in full-hd, I think. In order to test
vcb parsing. But yes, normal tv's wont have this kind of strange
EDIDs. And, modedb will increase even more in the next months: 8k
is almost out :)

Best regards,
Jose Miguel Abreu

>
> - Shashank
>>
>> Best regards,
>> Jose Miguel Abreu
>>
>>> And that we should re-prioritize the aspect ratio handling to
>>> target YUV 420 handling from CEA blocks.
>>> Shashank
>> [snip]
>>
>

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Sharma, Shashank]

Regards

Shashank


On 1/9/2017 6:53 PM, Jose Abreu wrote:
> Hi Shashank,
>
>
> On 09-01-2017 12:45, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/9/2017 4:41 PM, Jose Abreu wrote:
>>> Hi Shashank,
>>>
>>>
>>> Thanks for the review.
>>>
>>>
>>> On 09-01-2017 05:22, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 12/30/2016 10:23 PM, Jose Abreu wrote:
>>>>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>>>>> According to the spec the EDID may contain two blocks that
>>>>> signal this sampling mode:
>>>>>       - YCbCr 4:2:0 Video Data Block
>>>>>       - YCbCr 4:2:0 Video Capability Map Data Block
>>>>>
>>>>> The video data block contains the list of vic's were
>>>>> only YCbCr 4:2:0 sampling mode shall be used while the
>>>>> video capability map data block contains a mask were
>>>>> YCbCr 4:2:0 sampling mode may be used.
>>>>>
>>>>> This RFC patch adds support for parsing these two new blocks
>>>>> and introduces new flags to signal the drivers if the
>>>>> mode is 4:2:0'only or 4:2:0'able.
>>>>>
>>>>> The reason this is still a RFC is because there is no
>>>>> reference in kernel for this new sampling mode (specially in
>>>>> AVI infoframe part), so, I was hoping to hear some feedback
>>>>> first.
>>>>>
>>>>> Tested in a HDMI 2.0 compliance scenario.
>>>>>
>>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>>>> Cc: Sean Paul <seanpaul@chromium.org>
>>>>> Cc: David Airlie <airlied@linux.ie>
>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> ---
>>>>>     drivers/gpu/drm/drm_edid.c  | 139
>>>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>>>     drivers/gpu/drm/drm_modes.c |  10 +++-
>>>>>     include/uapi/drm/drm_mode.h |   6 ++
>>>>>     3 files changed, 151 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_edid.c
>>>>> b/drivers/gpu/drm/drm_edid.c
>>>>> index 67d6a73..6ce1a38 100644
>>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct
>>>>> drm_connector *connector,
>>>>>     #define VENDOR_BLOCK    0x03
>>>>>     #define SPEAKER_BLOCK    0x04
>>>>>     #define VIDEO_CAPABILITY_BLOCK    0x07
>>>>> +#define VIDEO_DATA_BLOCK_420    0x0E
>>>>> +#define VIDEO_CAP_BLOCK_420    0x0F
>>>>>     #define EDID_BASIC_AUDIO    (1 << 6)
>>>>>     #define EDID_CEA_YCRCB444    (1 << 5)
>>>>>     #define EDID_CEA_YCRCB422    (1 << 4)
>>>>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct
>>>>> drm_connector *connector, u16 structure,
>>>>>         return modes;
>>>>>     }
>>>>>     +static int add_420_mode(struct drm_connector *connector, u8
>>>>> vic)
>>>>> +{
>>>>> +    struct drm_device *dev = connector->dev;
>>>>> +    struct drm_display_mode *newmode;
>>>>> +
>>>>> +    if (!drm_valid_cea_vic(vic))
>>>>> +        return 0;
>>>>> +
>>>>> +    newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>>>> Sorry to start the review late, I missed this mail chain. It
>>>> would be great if you can please keep me in CC for this chain.
>>> Sure. Will do that next time.
>>>
>>>> Practically, YUV420 modes are being used for 4k and UHD video
>>>> modes. Now here, when we want to
>>>> add these modes from edid_cea_db, using the VIC index, we
>>>> should have full list of cea_modes from 1 - 107
>>>> Particularly 93-107 ( which is for new 38x21 and 40x21 modes,
>>>> added in CEA-861-F). right now, edid_cea_modes
>>>> cant index 4k modes, so practically this this patch series will
>>>> do nothing (even though its doing everything right)
>>> This is correct but not entirely true. I realize 4:2:0 is mostly
>>> used in 4k modes but it can also be used in any other video mode,
>>> as long as it is declared in the VCB.
>> I agree (that's why I called it practically).
>> As such I doubt we will find anything less than a 4k here, coz
>> HDMI 1.4b itself can driver 4k@30.
>> So the biggest benefit of YUV 420, which is half the clock, is
>> mostly useful in 4k@60 and above.
>> I guess we will see more cases of deep-color pixels at non-4k
>> modes soon.
>>>> To handle this scenario, I had added a patch series
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>>>>
>>>> (complete the cea modedb (VIC=65 onwards)) Now, this patch
>>>> series had dependency on new aspect ratios
>>>> being added in CEA-861-F which I tried to add in series
>>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>>>>
>>>> )
>>>> Which was added and later reverted by Ville
>>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119808_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=FAa6aHQ_HjlaVRzDm282p9bSY_tBiN1PngZBhsTqYdI&e=
>>>>
>>>> ).
>>> Yes, I remember that. If it was breaking userspace then there was
>>> nothing left to do, revert was needed.
>> As we discovered over the discussions, It dint break anything
>> as such :)
>> But it made the behavior change for some SW's (which was
>> expected), Anyways its gone now.
>>> I thing we should take
>>> your patch and rework/extend it so that userspace does not break
>>> as this is a most welcome feature. The new HDMI spec is almost
>>> ready, and yet, 2.0 features are still missing from the kernel.
>>> We should take advantage from our capability of accessing these
>>> specs, test equipment, compliance equipment ... and submit
>>> patches for these new features :)
>> I know. Unfortunately, last time when we spoke about it,  we
>> were required to write a full stack code across kernel, drm,
>> libdrm and X level, as keeping it
>> under a cap was not accepted. This seems to be a long term plan
>> to me.
> I really think we should make the exposure of this new 2.0
> features optional. Change the drivers and drm core first and then
> move to userland. We can't expect user to deploy the changes at
> the same time we apply them to kernel.
@Daniel, do you think we should re-visit our decision about keeping 
aspect ratio support under a cap, and add the kernel
mode support, so that it could unblock other things like this vcb and 
420_vdb parsing ?

- Shashank
>>>> In short, the method/sequence for effective development would
>>>> be:
>>>> - Add aspect ratio support in DRM
>>>> - Add HDMI 2.0 (CEA-861-F) aspect ratios
>>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>>>>
>>>> )
>>>> - Complete edid_cea_modes, adding new modes as per 4k VICs
>>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>>>>
>>>> )
>>>> - Parse these modes from 420_vdb and 420_vcb using
>>>> edid_cea_modes db[]  (This patch series)
>>>>
>>> I agree but this rfc does not depend (in terms of code) of any
>>> other patches. The vcb parsing part can be used right now, as for
>>> the vdb part we will have to wait until vic's list is completed.
>>> Thats one of the reasons i sent it in RFC: So that i could ear
>>> some comments before submitting a "real" patch.
>> Right, its so real code, that I forget almost every-time that
>> its a RFC :-)
>> But I thought its the right place to call, that, we wont be
>> able to test 4k YUV 420 yet, until we finish the modedb.
> At the time I tested 420 in full-hd, I think. In order to test
> vcb parsing. But yes, normal tv's wont have this kind of strange
> EDIDs. And, modedb will increase even more in the next months: 8k
> is almost out :)
>
> Best regards,
> Jose Miguel Abreu
>
>> - Shashank
>>> Best regards,
>>> Jose Miguel Abreu
>>>
>>>> And that we should re-prioritize the aspect ratio handling to
>>>> target YUV 420 handling from CEA blocks.
>>>> Shashank
>>> [snip]
>>>

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Ville Syrjälä]

On Thu, Jan 05, 2017 at 02:46:06PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 05-01-2017 11:46, Ville Syrjälä wrote:
> > On Thu, Jan 05, 2017 at 10:07:45AM +0000, Jose Abreu wrote:
> >> Hi Ville,
> >>
> >>
> >> On 04-01-2017 16:36, Ville Syrjälä wrote:
> >>> On Wed, Jan 04, 2017 at 04:15:01PM +0000, Jose Abreu wrote:
> >>  
> >> [snip]
> >>
> >>>>> Why does userspace need to know this? My thinking has been that the
> >>>>> driver would do the right thing automagically.
> >>>>>
> >>>>> We do probably want some kind of output colorspace property to allow the
> >>>>> user to select between RGB vs. YCbCr etc. But I think even with that we
> >>>>> should still allow the driver to automagically select YCbCr 4:2:0 output
> >>>>> since that's the only way the mode will work.
> >>>> I agree. When only 4:2:0 is supported there is no need to expose
> >>>> the flag to userspace. How shall then I signal drivers for this
> >>>> 4:2:0'only sampling mode?
> >>>>
> >>>> So, for the remaining modes, you propose a new field in the mode
> >>>> structure called 'colorspace' which contains the list of
> >>>> supported sampling modes for the given mode? I think it would be
> >>>> a nice addition. This way if a mode supports only RGB we only
> >>>> passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
> >>>> flag, ... And then user could select. We also have to inform user
> >>>> which one is being actually used.
> >>> IIRC there aren't any "RGB only" modes or anything like that. So
> >>> YCbCr 4:2:0 is the special case here. We could just add something to the
> >>> mode struct for it, or do we already have some other flags thing that's
> >>> not exposed to userspace? And I guess drivers should be able to opt into
> >>> supporting these 4:2:0 modes in similar way they opt into
> >>> interlaced/stereo/whatever.
> >> I mean, if a source EDID does not declare support for YCbCr modes
> >> (4:2:2 and 4:4:4 [i think they have to be both supported if sink
> >> supports != RGB]) then only RGB can be used. Or is any YCbCr that
> >> is pre-required? Still, I see your point. When EDID declares
> >> support for YCbCr then all modes can use it, and not only some of
> >> them.
> >>
> >> I think for stereo modes the flags can be opt in/out in userspace
> >> exposing. There is a function called
> >> drm_mode_expose_to_userspace() which only exposes stereo modes if
> >> user asks to. We could do something similar for 4:2:0 modes (or
> >> even for all pixel encoding). i.e. expose which encoding can be
> >> used in current video mode. What do you think?
> >>
> >> About drivers opting in for 4:2:0 modes, then you propose a new
> >> field in drm_connector (called for example ycbcr_420_allowed)
> >> which only does the parsing of the 4:2:0 modes and adds them to
> >> the list when set to true?
> > Thinking a bit more about this, we do have a slight problem with not
> > exposing this information to userspace. Namely we can't actually tell
> > whether any user provided mode would need to output as 4:2:0 or not.
> > With the new flag userspace could at least inherit that from the kernel
> > and pass it back in. But still, expanding the uapi for something like
> > this feels quite wrong to me. Can we simply look at a particular user
> > supplied mode and tell whether it needs to be output as 4:2:0 (based
> > on eg. dotclock)?
> >
> 
> The pixel clock rate is half of the TMDS character rate in 4:2:0
> (in 24 bit), but for example in deep color 48 bit it will be the
> same rate. There is also a reduction to half of htotal, hactive,
> hblank, hfront, hsync and hback but I don't think it's a good
> solution to guide us from there.

I was asking if we can look at a specific modeline and whether we
can tell from that if we would need to output it as 4:2:0.

> Why does it feel wrong to you
> expanding the uapi?

Because it requires changing every single userspace kms client. And
it's not something userspace should have to worry about.

> 
> I think its important to say that the chosen colorspace can
> improve performance in systems: for example, as I said, 4:2:0
> 24-bit uses half the rate that RGB 24-bit uses so we have less
> trafic in the bus. I am recently working with a FPGA connected
> trough pcie and I can definitely say that this is true. But, as
> expected, less traffic means less quality in final image so its
> not a matter of letting kernel decide, I think its a matter of
> user choosing between performance vs. quality.

Image quality control for userspace is a much bigger topic. And
something we have no real precedent for at the moment (apart from 
user choosing a different fb pixel format).

The performance arument is very hardware dependent, and not really
all that relevant IMO. If the user wants the big mode they either
get it or not depending on whether the system can deliver.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

Hi Ville,


On 10-01-2017 11:16, Ville Syrjälä wrote:
> On Thu, Jan 05, 2017 at 02:46:06PM +0000, Jose Abreu wrote:
>>

[snip]

>> The pixel clock rate is half of the TMDS character rate in 4:2:0
>> (in 24 bit), but for example in deep color 48 bit it will be the
>> same rate. There is also a reduction to half of htotal, hactive,
>> hblank, hfront, hsync and hback but I don't think it's a good
>> solution to guide us from there.
> I was asking if we can look at a specific modeline and whether we
> can tell from that if we would need to output it as 4:2:0.

Hmm, according to HDMI 2.0 spec there are no 4:2:0 only modes and
the only way to figure out if the mode is 4:2:0 only (or able) is
to parse the VCB and VBD blocks from EDID. The clock is half rate
but this is the source that has to figure it out. The mode is
still passed in a regular way (By VIC, by timing, ...).

>
>> Why does it feel wrong to you
>> expanding the uapi?
> Because it requires changing every single userspace kms client. And
> it's not something userspace should have to worry about.

I agree but, as Daniel said [1], we could make these new HDMI 2.0
features optional and only pass them to userspace if client asked
for them. What do you think?

[1]
https://lists.freedesktop.org/archives/dri-devel/2017-January/128683.html

>
>> I think its important to say that the chosen colorspace can
>> improve performance in systems: for example, as I said, 4:2:0
>> 24-bit uses half the rate that RGB 24-bit uses so we have less
>> trafic in the bus. I am recently working with a FPGA connected
>> trough pcie and I can definitely say that this is true. But, as
>> expected, less traffic means less quality in final image so its
>> not a matter of letting kernel decide, I think its a matter of
>> user choosing between performance vs. quality.
> Image quality control for userspace is a much bigger topic. And
> something we have no real precedent for at the moment (apart from 
> user choosing a different fb pixel format).
>
> The performance arument is very hardware dependent, and not really
> all that relevant IMO. If the user wants the big mode they either
> get it or not depending on whether the system can deliver.
>

Ok. But note that there is no nice way to figure this out. For
example, for a graphics card it all depends (apart from the
graphics HW) on the PCIe bus. If the bus is not free for enough
data rate then user can reach bottlenecks and not output at best
performance. If we gave user the ability to switch from, for
example, RGB to YCbCr 4:2:0 this bottleneck could be eliminated.
Unless of course we always prefer YCbCr 4:2:0, when possible. I
did this internally for bridge driver dw-hdmi. We always prefer
YCbCr over RGB when they are available. It is user transparent as
the controller does the necessary color space conversion, though,
not ideal in my opinion.

Best regards,
Jose Miguel Abreu

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Ville Syrjälä]

On Tue, Jan 10, 2017 at 12:26:53PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 10-01-2017 11:16, Ville Syrjälä wrote:
> > On Thu, Jan 05, 2017 at 02:46:06PM +0000, Jose Abreu wrote:
> >>
> 
> [snip]
> 
> >> The pixel clock rate is half of the TMDS character rate in 4:2:0
> >> (in 24 bit), but for example in deep color 48 bit it will be the
> >> same rate. There is also a reduction to half of htotal, hactive,
> >> hblank, hfront, hsync and hback but I don't think it's a good
> >> solution to guide us from there.
> > I was asking if we can look at a specific modeline and whether we
> > can tell from that if we would need to output it as 4:2:0.
> 
> Hmm, according to HDMI 2.0 spec there are no 4:2:0 only modes and
> the only way to figure out if the mode is 4:2:0 only (or able) is
> to parse the VCB and VBD blocks from EDID. The clock is half rate
> but this is the source that has to figure it out. The mode is
> still passed in a regular way (By VIC, by timing, ...).
> 
> >
> >> Why does it feel wrong to you
> >> expanding the uapi?
> > Because it requires changing every single userspace kms client. And
> > it's not something userspace should have to worry about.
> 
> I agree but, as Daniel said [1], we could make these new HDMI 2.0
> features optional and only pass them to userspace if client asked
> for them. What do you think?

Are you going to update all the userspace clients? Exposing HDMI 2.0
modes only for your favorite client doesn't sound like a good plan to
me.

If we simply compute from a specific modeline whether it needs to be
transmitted as 4:2:0, I suppose we could simply look for a matching
mode in the 4:2:0 mode. But that would mean that only the exact modes
listed by the EDID will work, and others might not.

> 
> [1]
> https://lists.freedesktop.org/archives/dri-devel/2017-January/128683.html
> 
> >
> >> I think its important to say that the chosen colorspace can
> >> improve performance in systems: for example, as I said, 4:2:0
> >> 24-bit uses half the rate that RGB 24-bit uses so we have less
> >> trafic in the bus. I am recently working with a FPGA connected
> >> trough pcie and I can definitely say that this is true. But, as
> >> expected, less traffic means less quality in final image so its
> >> not a matter of letting kernel decide, I think its a matter of
> >> user choosing between performance vs. quality.
> > Image quality control for userspace is a much bigger topic. And
> > something we have no real precedent for at the moment (apart from 
> > user choosing a different fb pixel format).
> >
> > The performance arument is very hardware dependent, and not really
> > all that relevant IMO. If the user wants the big mode they either
> > get it or not depending on whether the system can deliver.
> >
> 
> Ok. But note that there is no nice way to figure this out. For
> example, for a graphics card it all depends (apart from the
> graphics HW) on the PCIe bus. If the bus is not free for enough
> data rate then user can reach bottlenecks and not output at best
> performance. If we gave user the ability to switch from, for
> example, RGB to YCbCr 4:2:0 this bottleneck could be eliminated.

Userspace won't know anything about such bottlenecks. The kernel
can know it and hence should automagically drop into 4:2:0 mode
if necessary.

> Unless of course we always prefer YCbCr 4:2:0, when possible. I
> did this internally for bridge driver dw-hdmi. We always prefer
> YCbCr over RGB when they are available. It is user transparent as
> the controller does the necessary color space conversion, though,
> not ideal in my opinion.

My idea was that we'd have a property for the output colorspace and
would perhaps default to YCbCr for the CEA modes (as per CEA-861).
Though I'm sure some people would cry about that behaviour as well.
But for the cases where there is no choice but to use a specific
output colorspace, the kernel should just do it automagically IMO. No
point in manking life diffcult for userspace.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Ville Syrjälä]

On Tue, Jan 10, 2017 at 05:33:15PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 10, 2017 at 12:26:53PM +0000, Jose Abreu wrote:
> > Hi Ville,
> > 
> > 
> > On 10-01-2017 11:16, Ville Syrjälä wrote:
> > > On Thu, Jan 05, 2017 at 02:46:06PM +0000, Jose Abreu wrote:
> > >>
> > 
> > [snip]
> > 
> > >> The pixel clock rate is half of the TMDS character rate in 4:2:0
> > >> (in 24 bit), but for example in deep color 48 bit it will be the
> > >> same rate. There is also a reduction to half of htotal, hactive,
> > >> hblank, hfront, hsync and hback but I don't think it's a good
> > >> solution to guide us from there.
> > > I was asking if we can look at a specific modeline and whether we
> > > can tell from that if we would need to output it as 4:2:0.
> > 
> > Hmm, according to HDMI 2.0 spec there are no 4:2:0 only modes and
> > the only way to figure out if the mode is 4:2:0 only (or able) is
> > to parse the VCB and VBD blocks from EDID. The clock is half rate
> > but this is the source that has to figure it out. The mode is
> > still passed in a regular way (By VIC, by timing, ...).
> > 
> > >
> > >> Why does it feel wrong to you
> > >> expanding the uapi?
> > > Because it requires changing every single userspace kms client. And
> > > it's not something userspace should have to worry about.
> > 
> > I agree but, as Daniel said [1], we could make these new HDMI 2.0
> > features optional and only pass them to userspace if client asked
> > for them. What do you think?
> 
> Are you going to update all the userspace clients? Exposing HDMI 2.0
> modes only for your favorite client doesn't sound like a good plan to
> me.
> 
> If we simply compute from a specific modeline whether it needs to be
> transmitted as 4:2:0, I suppose we could simply look for a matching
> mode in the 4:2:0 mode. But that would mean that only the exact modes
> listed by the EDID will work, and others might not.

OK, so the 4:2:0 support is apparently listed only for specific VICs.
Hence we will need to just go through those lists to see if we can
(or in fact must) use 4:2:0 for a specific user specified mode.

I would say the only slight question mark at this point is whether we
should favor 4:4:4 at lower bpc or 4:2:0 at higher bpc if we can choose
between the two. My first instinct is to favor the 4:4:4 modes for now.
We could add some knobs later to let the user make that choice.

> 
> > 
> > [1]
> > https://lists.freedesktop.org/archives/dri-devel/2017-January/128683.html
> > 
> > >
> > >> I think its important to say that the chosen colorspace can
> > >> improve performance in systems: for example, as I said, 4:2:0
> > >> 24-bit uses half the rate that RGB 24-bit uses so we have less
> > >> trafic in the bus. I am recently working with a FPGA connected
> > >> trough pcie and I can definitely say that this is true. But, as
> > >> expected, less traffic means less quality in final image so its
> > >> not a matter of letting kernel decide, I think its a matter of
> > >> user choosing between performance vs. quality.
> > > Image quality control for userspace is a much bigger topic. And
> > > something we have no real precedent for at the moment (apart from 
> > > user choosing a different fb pixel format).
> > >
> > > The performance arument is very hardware dependent, and not really
> > > all that relevant IMO. If the user wants the big mode they either
> > > get it or not depending on whether the system can deliver.
> > >
> > 
> > Ok. But note that there is no nice way to figure this out. For
> > example, for a graphics card it all depends (apart from the
> > graphics HW) on the PCIe bus. If the bus is not free for enough
> > data rate then user can reach bottlenecks and not output at best
> > performance. If we gave user the ability to switch from, for
> > example, RGB to YCbCr 4:2:0 this bottleneck could be eliminated.
> 
> Userspace won't know anything about such bottlenecks. The kernel
> can know it and hence should automagically drop into 4:2:0 mode
> if necessary.
> 
> > Unless of course we always prefer YCbCr 4:2:0, when possible. I
> > did this internally for bridge driver dw-hdmi. We always prefer
> > YCbCr over RGB when they are available. It is user transparent as
> > the controller does the necessary color space conversion, though,
> > not ideal in my opinion.
> 
> My idea was that we'd have a property for the output colorspace and
> would perhaps default to YCbCr for the CEA modes (as per CEA-861).
> Though I'm sure some people would cry about that behaviour as well.
> But for the cases where there is no choice but to use a specific
> output colorspace, the kernel should just do it automagically IMO. No
> point in manking life diffcult for userspace.
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

Hi Ville,


[snip]

>> Are you going to update all the userspace clients? Exposing HDMI 2.0
>> modes only for your favorite client doesn't sound like a good plan to
>> me.
>>
>> If we simply compute from a specific modeline whether it needs to be
>> transmitted as 4:2:0, I suppose we could simply look for a matching
>> mode in the 4:2:0 mode. But that would mean that only the exact modes
>> listed by the EDID will work, and others might not.
> OK, so the 4:2:0 support is apparently listed only for specific VICs.

Hmm, the spec is not very clear. It lists video timings which may
be used with YCbCr 4:2:0 but does not explicitly say that only
these timings can be used. Anyway, I checked with a colleague who
has direct access to HDMI Forum and indeed only VIC 96, 97, 101,
102, 106 and 107 can be used with 4:2:0, so you are correct. He
said that the initial intention of this pixel encoding was to
give 60Hz refresh rate in 4k to users who had limited
performance, so it was only intended for higher resolutions.

> Hence we will need to just go through those lists to see if we can
> (or in fact must) use 4:2:0 for a specific user specified mode.

We don't have yet support for these VICs, so this will have to
wait :(

>
> I would say the only slight question mark at this point is whether we
> should favor 4:4:4 at lower bpc or 4:2:0 at higher bpc if we can choose
> between the two. My first instinct is to favor the 4:4:4 modes for now.
> We could add some knobs later to let the user make that choice.

I agree that 4:4:4 should be preferred.

[snip]

>>> Ok. But note that there is no nice way to figure this out. For
>>> example, for a graphics card it all depends (apart from the
>>> graphics HW) on the PCIe bus. If the bus is not free for enough
>>> data rate then user can reach bottlenecks and not output at best
>>> performance. If we gave user the ability to switch from, for
>>> example, RGB to YCbCr 4:2:0 this bottleneck could be eliminated.
>> Userspace won't know anything about such bottlenecks. The kernel
>> can know it and hence should automagically drop into 4:2:0 mode
>> if necessary.
>>
>>> Unless of course we always prefer YCbCr 4:2:0, when possible. I
>>> did this internally for bridge driver dw-hdmi. We always prefer
>>> YCbCr over RGB when they are available. It is user transparent as
>>> the controller does the necessary color space conversion, though,
>>> not ideal in my opinion.
>> My idea was that we'd have a property for the output colorspace and
>> would perhaps default to YCbCr for the CEA modes (as per CEA-861).
>> Though I'm sure some people would cry about that behaviour as well.
>> But for the cases where there is no choice but to use a specific
>> output colorspace, the kernel should just do it automagically IMO. No
>> point in manking life diffcult for userspace.

But we already have color_formats field in drm_display_info
struct, right? Shouldn't we instead create for example a helper
which returns the best output colorspace? According to what you
said it would be something like:

if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
    return YCBCR444;
else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
    return YCBCR422;
else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
&& vic_is_420)
    return YCBCR420;
else
    return RGB444; /* Mandatory by spec */

>>
>> -- 
>> Ville Syrjälä
>> Intel OTC

Best regards,
Jose Miguel Abreu

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Ville Syrjälä]

On Tue, Jan 10, 2017 at 05:01:08PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> [snip]
> 
> >> Are you going to update all the userspace clients? Exposing HDMI 2.0
> >> modes only for your favorite client doesn't sound like a good plan to
> >> me.
> >>
> >> If we simply compute from a specific modeline whether it needs to be
> >> transmitted as 4:2:0, I suppose we could simply look for a matching
> >> mode in the 4:2:0 mode. But that would mean that only the exact modes
> >> listed by the EDID will work, and others might not.
> > OK, so the 4:2:0 support is apparently listed only for specific VICs.
> 
> Hmm, the spec is not very clear. It lists video timings which may
> be used with YCbCr 4:2:0 but does not explicitly say that only
> these timings can be used. Anyway, I checked with a colleague who
> has direct access to HDMI Forum and indeed only VIC 96, 97, 101,
> 102, 106 and 107 can be used with 4:2:0, so you are correct. He
> said that the initial intention of this pixel encoding was to
> give 60Hz refresh rate in 4k to users who had limited
> performance, so it was only intended for higher resolutions.
> 
> > Hence we will need to just go through those lists to see if we can
> > (or in fact must) use 4:2:0 for a specific user specified mode.
> 
> We don't have yet support for these VICs, so this will have to
> wait :(
> 
> >
> > I would say the only slight question mark at this point is whether we
> > should favor 4:4:4 at lower bpc or 4:2:0 at higher bpc if we can choose
> > between the two. My first instinct is to favor the 4:4:4 modes for now.
> > We could add some knobs later to let the user make that choice.
> 
> I agree that 4:4:4 should be preferred.
> 
> [snip]
> 
> >>> Ok. But note that there is no nice way to figure this out. For
> >>> example, for a graphics card it all depends (apart from the
> >>> graphics HW) on the PCIe bus. If the bus is not free for enough
> >>> data rate then user can reach bottlenecks and not output at best
> >>> performance. If we gave user the ability to switch from, for
> >>> example, RGB to YCbCr 4:2:0 this bottleneck could be eliminated.
> >> Userspace won't know anything about such bottlenecks. The kernel
> >> can know it and hence should automagically drop into 4:2:0 mode
> >> if necessary.
> >>
> >>> Unless of course we always prefer YCbCr 4:2:0, when possible. I
> >>> did this internally for bridge driver dw-hdmi. We always prefer
> >>> YCbCr over RGB when they are available. It is user transparent as
> >>> the controller does the necessary color space conversion, though,
> >>> not ideal in my opinion.
> >> My idea was that we'd have a property for the output colorspace and
> >> would perhaps default to YCbCr for the CEA modes (as per CEA-861).
> >> Though I'm sure some people would cry about that behaviour as well.
> >> But for the cases where there is no choice but to use a specific
> >> output colorspace, the kernel should just do it automagically IMO. No
> >> point in manking life diffcult for userspace.
> 
> But we already have color_formats field in drm_display_info
> struct, right? Shouldn't we instead create for example a helper
> which returns the best output colorspace? According to what you
> said it would be something like:
> 
> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>     return YCBCR444;
> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>     return YCBCR422;
> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
> && vic_is_420)
>     return YCBCR420;
> else
>     return RGB444; /* Mandatory by spec */

Perhaps. But it would have to be more involved than that since there
might limitations on eg. the max TMDS clock imposed by the source or
cable/dongle which presumably might require that we pick 4:2:0 over
4:4:4.

It would also need to account which formats are actually supported by
the source.

I guess for bpc it would be enough to just consider 8bpc in such a
function, and then the driver can bump it up afterwards if possible.

As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
4:4:4 and thus doesn't provide any benefit as such. We could later add
a property to let the user choose between RGB vs. YCbCr more explicitly.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

Hi Ville,


On 10-01-2017 17:21, Ville Syrjälä wrote:

[snip]

>> But we already have color_formats field in drm_display_info
>> struct, right? Shouldn't we instead create for example a helper
>> which returns the best output colorspace? According to what you
>> said it would be something like:
>>
>> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>>     return YCBCR444;
>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>>     return YCBCR422;
>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
>> && vic_is_420)
>>     return YCBCR420;
>> else
>>     return RGB444; /* Mandatory by spec */
> Perhaps. But it would have to be more involved than that since there
> might limitations on eg. the max TMDS clock imposed by the source or
> cable/dongle which presumably might require that we pick 4:2:0 over
> 4:4:4.
>
> It would also need to account which formats are actually supported by
> the source.
>
> I guess for bpc it would be enough to just consider 8bpc in such a
> function, and then the driver can bump it up afterwards if possible.

But the max tmds clock will probably be involved in deep color
modes, as 24bpp has always a 1x factor in every YCbCr, except
4:2:0. So, the sink has a max tmds but this gets into account
when the vic list present in the EDID is built, but not
considered in deep color modes, unless the EDID is broken.

>
> As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
> for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
> leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
> 4:4:4 and thus doesn't provide any benefit as such. We could later add
> a property to let the user choose between RGB vs. YCbCr more explicitly.
>

Hmm, I am trying to implement this but I am facing a difficulty:
how will I fallback to YCbCr? RGB is always supported and the max
tmds only enters in deep color modes. For reference here is a
simple struct i created with the different tmds character rate
factors for the different encodings (I think they are correct,
but cross check please):

#define DRM_CS_DESC(cs, f, b) \
    .colorspace = (cs), .factor_to_khz = (f), .bpc = (b)

static const struct drm_mode_colorspace_desc {
    u32 colorspace;
    u32 factor_to_khz;
    u32 bpc;
} drm_mode_colorspace_factors = { /* Ordered by descending
preference */
    { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 2000, 48) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 2000, 48) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1500, 36) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1500, 36) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1250, 30) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1250, 30) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1000, 24) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1000, 24) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 24) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 30) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 36) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 1000, 48) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 750, 36) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 625, 30) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 500, 24) },

Notice how YCbCr 4:4:4 will never get picked: it has the same
factor of RGB 4:4:4 for every bpc. So, the sink must support RGB
4:4:4 and may support YCbCr. What I didn't check was that if it
is possible to have support for deep color YCbCr without having
support for deep color RGB 4:4:4. Do you know if this can happen?

Best regards,
Jose Miguel Abreu

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Ville Syrjälä]

On Wed, Jan 11, 2017 at 10:27:03AM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 10-01-2017 17:21, Ville Syrjälä wrote:
> 
> [snip]
> 
> >> But we already have color_formats field in drm_display_info
> >> struct, right? Shouldn't we instead create for example a helper
> >> which returns the best output colorspace? According to what you
> >> said it would be something like:
> >>
> >> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> >>     return YCBCR444;
> >> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
> >>     return YCBCR422;
> >> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
> >> && vic_is_420)
> >>     return YCBCR420;
> >> else
> >>     return RGB444; /* Mandatory by spec */
> > Perhaps. But it would have to be more involved than that since there
> > might limitations on eg. the max TMDS clock imposed by the source or
> > cable/dongle which presumably might require that we pick 4:2:0 over
> > 4:4:4.
> >
> > It would also need to account which formats are actually supported by
> > the source.
> >
> > I guess for bpc it would be enough to just consider 8bpc in such a
> > function, and then the driver can bump it up afterwards if possible.
> 
> But the max tmds clock will probably be involved in deep color
> modes, as 24bpp has always a 1x factor in every YCbCr, except
> 4:2:0. So, the sink has a max tmds but this gets into account
> when the vic list present in the EDID is built, but not
> considered in deep color modes, unless the EDID is broken.
> 
> >
> > As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
> > for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
> > leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
> > 4:4:4 and thus doesn't provide any benefit as such. We could later add
> > a property to let the user choose between RGB vs. YCbCr more explicitly.
> >
> 
> Hmm, I am trying to implement this but I am facing a difficulty:
> how will I fallback to YCbCr? RGB is always supported and the max
> tmds only enters in deep color modes. For reference here is a
> simple struct i created with the different tmds character rate
> factors for the different encodings (I think they are correct,
> but cross check please):
> 
> #define DRM_CS_DESC(cs, f, b) \
>     .colorspace = (cs), .factor_to_khz = (f), .bpc = (b)
> 
> static const struct drm_mode_colorspace_desc {
>     u32 colorspace;
>     u32 factor_to_khz;
>     u32 bpc;
> } drm_mode_colorspace_factors = { /* Ordered by descending
> preference */
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 2000, 48) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 2000, 48) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1500, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1500, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1250, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1250, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1000, 24) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1000, 24) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 24) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 1000, 48) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 750, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 625, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 500, 24) },
> 
> Notice how YCbCr 4:4:4 will never get picked: it has the same
> factor of RGB 4:4:4 for every bpc. So, the sink must support RGB
> 4:4:4 and may support YCbCr. What I didn't check was that if it
> is possible to have support for deep color YCbCr without having
> support for deep color RGB 4:4:4. Do you know if this can happen?

I don't think that's possible. So the 4:4:4 RGB vs. YCbCr choice is
probably something we have to leave up to the user. Although I have
a vague recollection that CEA-861 says that you should prefer YCbCr
for CE modes and RGB for IT modes. If we want to follow that I think we
want a property similar to the "Broadcast RGB" thing that allows you to
select between "Automatic", "RGB", and "YCbCr". Not sure if we should
also allow the user to explicitly select the subsampling mode for YCbCr.
I also think we should probably still fall back to YCbCr 4:2:0
automagically even if the user explicitly asked for RGB if we can't
light up the mode with RGB 4:4:4.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Jose Abreu]

Hi Ville,


Sorry for the late reply.


On 11-01-2017 11:36, Ville Syrjälä wrote:
> On Wed, Jan 11, 2017 at 10:27:03AM +0000, Jose Abreu wrote:
>> Hi Ville,
>>
>>
>> On 10-01-2017 17:21, Ville Syrjälä wrote:
>>
>> [snip]
>>
>>>> But we already have color_formats field in drm_display_info
>>>> struct, right? Shouldn't we instead create for example a helper
>>>> which returns the best output colorspace? According to what you
>>>> said it would be something like:
>>>>
>>>> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>>>>     return YCBCR444;
>>>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>>>>     return YCBCR422;
>>>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
>>>> && vic_is_420)
>>>>     return YCBCR420;
>>>> else
>>>>     return RGB444; /* Mandatory by spec */
>>> Perhaps. But it would have to be more involved than that since there
>>> might limitations on eg. the max TMDS clock imposed by the source or
>>> cable/dongle which presumably might require that we pick 4:2:0 over
>>> 4:4:4.
>>>
>>> It would also need to account which formats are actually supported by
>>> the source.
>>>
>>> I guess for bpc it would be enough to just consider 8bpc in such a
>>> function, and then the driver can bump it up afterwards if possible.
>> But the max tmds clock will probably be involved in deep color
>> modes, as 24bpp has always a 1x factor in every YCbCr, except
>> 4:2:0. So, the sink has a max tmds but this gets into account
>> when the vic list present in the EDID is built, but not
>> considered in deep color modes, unless the EDID is broken.
>>
>>> As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
>>> for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
>>> leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
>>> 4:4:4 and thus doesn't provide any benefit as such. We could later add
>>> a property to let the user choose between RGB vs. YCbCr more explicitly.
>>>
>> Hmm, I am trying to implement this but I am facing a difficulty:
>> how will I fallback to YCbCr? RGB is always supported and the max
>> tmds only enters in deep color modes. For reference here is a
>> simple struct i created with the different tmds character rate
>> factors for the different encodings (I think they are correct,
>> but cross check please):
>>
>> #define DRM_CS_DESC(cs, f, b) \
>>     .colorspace = (cs), .factor_to_khz = (f), .bpc = (b)
>>
>> static const struct drm_mode_colorspace_desc {
>>     u32 colorspace;
>>     u32 factor_to_khz;
>>     u32 bpc;
>> } drm_mode_colorspace_factors = { /* Ordered by descending
>> preference */
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 2000, 48) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 2000, 48) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1500, 36) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1500, 36) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1250, 30) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1250, 30) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1000, 24) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1000, 24) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 24) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 30) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 36) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 1000, 48) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 750, 36) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 625, 30) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 500, 24) },
>>
>> Notice how YCbCr 4:4:4 will never get picked: it has the same
>> factor of RGB 4:4:4 for every bpc. So, the sink must support RGB
>> 4:4:4 and may support YCbCr. What I didn't check was that if it
>> is possible to have support for deep color YCbCr without having
>> support for deep color RGB 4:4:4. Do you know if this can happen?
> I don't think that's possible. So the 4:4:4 RGB vs. YCbCr choice is
> probably something we have to leave up to the user. Although I have
> a vague recollection that CEA-861 says that you should prefer YCbCr
> for CE modes and RGB for IT modes. 

RGB Full Range is the default for IT modes. As for CE modes it
says it depends on vactive, which I am not quite understanding
why (pg. 34, CEA-861-F).

> If we want to follow that I think we
> want a property similar to the "Broadcast RGB" thing that allows you to
> select between "Automatic", "RGB", and "YCbCr". Not sure if we should
> also allow the user to explicitly select the subsampling mode for YCbCr.

I think we can start by only RGB vs. YCbCr vs. automatic.

> I also think we should probably still fall back to YCbCr 4:2:0
> automagically even if the user explicitly asked for RGB if we can't
> light up the mode with RGB 4:4:4.
>

Agreed. I will start by that but in order for this to work in a
real scenario (I got it working by changing EDID manually) the
list of VIC's must be expanded to the new HDMI 2.0 VIC's. A patch
was sent here a while ago (not by me) and I think you commented
on that. I don't know whats the status of the patch now but it
wasn't merged.

Anyway, regarding this I think we could:
    - Reuse this existing RFC where it concerns EDID parsing
    - Add new flags (which will not be exported to userspace) to
signal YCbCr 4:2:0'only and 'able modes
    - Add a helper function to compute best colorspace which will
always return RGB (for now) unless the mode is 4:2:0 only or
unless the mode can't use RGB because of max clock limitations.

What do you think?

Best regards,
Jose Miguel Abreu

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

[Ville Syrjälä]

On Mon, Jan 16, 2017 at 01:24:01PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> Sorry for the late reply.
> 
> 
> On 11-01-2017 11:36, Ville Syrjälä wrote:
> > On Wed, Jan 11, 2017 at 10:27:03AM +0000, Jose Abreu wrote:
> >> Hi Ville,
> >>
> >>
> >> On 10-01-2017 17:21, Ville Syrjälä wrote:
> >>
> >> [snip]
> >>
> >>>> But we already have color_formats field in drm_display_info
> >>>> struct, right? Shouldn't we instead create for example a helper
> >>>> which returns the best output colorspace? According to what you
> >>>> said it would be something like:
> >>>>
> >>>> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> >>>>     return YCBCR444;
> >>>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
> >>>>     return YCBCR422;
> >>>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
> >>>> && vic_is_420)
> >>>>     return YCBCR420;
> >>>> else
> >>>>     return RGB444; /* Mandatory by spec */
> >>> Perhaps. But it would have to be more involved than that since there
> >>> might limitations on eg. the max TMDS clock imposed by the source or
> >>> cable/dongle which presumably might require that we pick 4:2:0 over
> >>> 4:4:4.
> >>>
> >>> It would also need to account which formats are actually supported by
> >>> the source.
> >>>
> >>> I guess for bpc it would be enough to just consider 8bpc in such a
> >>> function, and then the driver can bump it up afterwards if possible.
> >> But the max tmds clock will probably be involved in deep color
> >> modes, as 24bpp has always a 1x factor in every YCbCr, except
> >> 4:2:0. So, the sink has a max tmds but this gets into account
> >> when the vic list present in the EDID is built, but not
> >> considered in deep color modes, unless the EDID is broken.
> >>
> >>> As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
> >>> for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
> >>> leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
> >>> 4:4:4 and thus doesn't provide any benefit as such. We could later add
> >>> a property to let the user choose between RGB vs. YCbCr more explicitly.
> >>>
> >> Hmm, I am trying to implement this but I am facing a difficulty:
> >> how will I fallback to YCbCr? RGB is always supported and the max
> >> tmds only enters in deep color modes. For reference here is a
> >> simple struct i created with the different tmds character rate
> >> factors for the different encodings (I think they are correct,
> >> but cross check please):
> >>
> >> #define DRM_CS_DESC(cs, f, b) \
> >>     .colorspace = (cs), .factor_to_khz = (f), .bpc = (b)
> >>
> >> static const struct drm_mode_colorspace_desc {
> >>     u32 colorspace;
> >>     u32 factor_to_khz;
> >>     u32 bpc;
> >> } drm_mode_colorspace_factors = { /* Ordered by descending
> >> preference */
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 2000, 48) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 2000, 48) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1500, 36) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1500, 36) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1250, 30) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1250, 30) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1000, 24) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1000, 24) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 24) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 30) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 36) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 1000, 48) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 750, 36) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 625, 30) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 500, 24) },
> >>
> >> Notice how YCbCr 4:4:4 will never get picked: it has the same
> >> factor of RGB 4:4:4 for every bpc. So, the sink must support RGB
> >> 4:4:4 and may support YCbCr. What I didn't check was that if it
> >> is possible to have support for deep color YCbCr without having
> >> support for deep color RGB 4:4:4. Do you know if this can happen?
> > I don't think that's possible. So the 4:4:4 RGB vs. YCbCr choice is
> > probably something we have to leave up to the user. Although I have
> > a vague recollection that CEA-861 says that you should prefer YCbCr
> > for CE modes and RGB for IT modes. 
> 
> RGB Full Range is the default for IT modes. As for CE modes it
> says it depends on vactive, which I am not quite understanding
> why (pg. 34, CEA-861-F).

I think that vactive note is just referring to the SD->BT.601 and
HD->BT.709 rule.

> 
> > If we want to follow that I think we
> > want a property similar to the "Broadcast RGB" thing that allows you to
> > select between "Automatic", "RGB", and "YCbCr". Not sure if we should
> > also allow the user to explicitly select the subsampling mode for YCbCr.
> 
> I think we can start by only RGB vs. YCbCr vs. automatic.
> 
> > I also think we should probably still fall back to YCbCr 4:2:0
> > automagically even if the user explicitly asked for RGB if we can't
> > light up the mode with RGB 4:4:4.
> >
> 
> Agreed. I will start by that but in order for this to work in a
> real scenario (I got it working by changing EDID manually) the
> list of VIC's must be expanded to the new HDMI 2.0 VIC's. A patch
> was sent here a while ago (not by me) and I think you commented
> on that. I don't know whats the status of the patch now but it
> wasn't merged.

The new VICs were reverted due to the "exposing aspect ratio as mode
flags broke userspace" thing. What we need to do is add the new VICs
without changing the userspace API. And I don't see any reason why
we can't do just that. But no such patch has materialized AFAIK.

> 
> Anyway, regarding this I think we could:
>     - Reuse this existing RFC where it concerns EDID parsing
>     - Add new flags (which will not be exported to userspace) to
> signal YCbCr 4:2:0'only and 'able modes
>     - Add a helper function to compute best colorspace which will
> always return RGB (for now) unless the mode is 4:2:0 only or
> unless the mode can't use RGB because of max clock limitations.
> 
> What do you think?

Sounds reasonable to me.

-- 
Ville Syrjälä
Intel OTC