Re: [PATCH v3 04/14] drm/edid: parse YCBCR 420 videomodes from EDID

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Yes, we should check for features, not for some standard version that
implements some/all of those features. But not convinced we need any
featur flags for the other things you listed. Aren't we already doing
all those things anyway?

Yep, I realized you will come back with this point, while typing these examples :-). Ok then, I will add a flag which sounds more like ycbcr_420_supported or so.

Regards
Shashank
On 6/15/2017 10:29 PM, Ville Syrjälä wrote:
On Thu, Jun 15, 2017 at 10:18:40PM +0530, Sharma, Shashank wrote:
Regards

Shashank


On 6/15/2017 9:42 PM, Ville Syrjälä wrote:
On Thu, Jun 15, 2017 at 09:05:10PM +0530, Sharma, Shashank wrote:
Regards

Shashank


On 6/15/2017 8:13 PM, Ville Syrjälä wrote:
On Wed, Jun 14, 2017 at 11:17:35PM +0530, Shashank Sharma wrote:
HDMI 2.0 spec adds support for YCBCR420 sub-sampled output.
CEA-861-F adds two new blocks in EDID's CEA extension blocks,
to provide information about sink's YCBCR420 output capabilities.

These blocks are:

- YCBCR420vdb(YCBCR 420 video data block):
This block contains VICs of video modes, which can be sopported only
in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal
SVD block, valid for YCBCR420 modes only.

- YCBCR420cmdb(YCBCR 420 capability map data block):
This block gives information about video modes which can support
YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This
block contains a bitmap index of normal svd videomodes, which can
support YCBCR420 output too.
So if bit 0 from first vcb byte is set, first video mode in the svd
list can support YCBCR420 output too. Bit 1 means second video mode
from svd list can support YCBCR420 output too, and so on.

This patch adds two bitmaps in display's hdmi_info structure, one each
for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch
adds:
- VDB modes (YCBCR 420 only modes) in connector's mode list, also makes
     an entry in the vdb_bitmap per vic.
- VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap.

Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
Cc: Jose Abreu <joabreu@xxxxxxxxxxxx>
Cc: Emil Velikov <emil.l.velikov@xxxxxxxxx>

V2: Addressed
       Review comments from Emil:
       - Use 1ULL<<i instead of 1<<i to make sure the output is 64bit.
       - Use the suggested method for updating dbmap.
       - Add documentation for YCBCR420_vcb_map to fix kbuild warning.

       Review comments from Ville:
       - Do not expose the YCBCR420 flags in uabi layer, keep it internal.
       - Save a map of YCBCR420 modes for future reference.
       - Check db length before trying to parse extended tag.
       - Add a warning if there are > 64 modes in capability map block.
       - Use y420cmdb in function names and macros while dealing with vcb
         to be aligned with spec.
       - Move the display information parsing block ahead of mode parsing
         blocks.

V3: Addressed design/review comments from Ville
       - Do not add flags in video modes, else we have to expose them to user
       - There should not be a UABI change, and kernel should detect the
         choice of the output based on type of mode, and the bitmaps.
       - Use standard bitops from kernel bitmap header, instead of calculating
         bit positions manually.

Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
---
    drivers/gpu/drm/drm_edid.c  | 193 ++++++++++++++++++++++++++++++++++++++++++--
    include/drm/drm_connector.h |  23 ++++++
    2 files changed, 211 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 6fd8a98..4953f87 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2781,7 +2781,9 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
    #define VIDEO_BLOCK     0x02
    #define VENDOR_BLOCK    0x03
    #define SPEAKER_BLOCK	0x04
-#define VIDEO_CAPABILITY_BLOCK	0x07
+#define VIDEO_CAPABILITY_BLOCK 0x07
+#define VIDEO_DATA_BLOCK_420	0x0E
+#define VIDEO_CAP_BLOCK_Y420CMDB 0x0F
    #define EDID_BASIC_AUDIO	(1 << 6)
    #define EDID_CEA_YCRCB444	(1 << 5)
    #define EDID_CEA_YCRCB422	(1 << 4)
@@ -3143,15 +3145,103 @@ drm_display_mode_from_vic_index(struct drm_connector *connector,
    	return newmode;
    }
+/*
+ * do_ycbcr_420_vdb_modes - Parse YCBCR 420 only modes
+ * @connector: connector corresponding to the HDMI sink
+ * @svds: start of the data block of CEA YCBCR 420 VDB
+ * @len: length of the CEA YCBCR 420 VDB
+ *
+ * CEA-861-F has added a new video block called YCBCR 420 Video
+ * Data Block (ycbcr 420 vdb). This block contains modes which
+ * support YCBCR 420 HDMI output (only YCBCR 420). This function
+ * parses the block and adds these modes into connector's mode list.
Seems a bit verbose. Maybe something like:
"Parse the CEA-861-F YCbCr 4:2:0 Video Data Block (Y420VDB)
which contains modes which only support YCbCr 4:2:0 output."
Got it.
+ */
+static int do_ycbcr_420_vdb_modes(struct drm_connector *connector,
+				   const u8 *svds, u8 svds_len)
Probably we could s/ycbcr_420_vdb/y420vdb/ all over to keep things
shorter and match the terminology in the spec. Same for y420cmdb.
Got it.
+{
+	int modes = 0, i;
+	struct drm_device *dev = connector->dev;
+	struct drm_display_mode *newmode;
This variable can be moved into the loop scope.
Ok.
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_hdmi_info *hdmi = &info->hdmi;
+
+	for (i = 0; i < svds_len; i++) {
+		u8 vic = svds[i] & 0x7f;
What's the 0x7f here? Native bit stuff? I'm thinkign we probably want
some kind of svd_to_vic() function to make sure everyone deals with
this stuff correctly.
Current VICs are 1 < VIC < 108, so seven bits required to show a VIC.
This is inspired from
drm_mode_from_cea_vic_index().
+
+		newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
+		if (!newmode)
+			break;
+		/*
+		 * ycbcr420_vdb_modes is a fix position 128 bit bitmap.
+		 * Every bit here represents a VIC, from CEA-861-F list.
+		 * So if bit[n] is set, it indicates vic[n+1] supports
+		 * YCBCR420 output. Bit 0 is dummy, as VICs start at 1.
+		 * ycbcr420_vcb_modes[0] = |VIC=64 |VIC=63 |.....|VIC=2 |VIC=1 |
+		 * ycbcr420_vcb_modes[1] = |VIC=128|VIC=127|.....|VIC=66|VIC=65|
+		 */
I think people can figure out how the standard bitmaps work without
having to explain it with such detail. If you want to elaborate on the
contents of the bitmap, then I think the comment should be next to
where the bitmap is defined.
There is already similar explanation :), I will probably remove this.
+		bitmap_set(hdmi->ycbcr420_vdb_modes, vic, 1);
__set_bit()
Any harm on bitmap_set ? I guess this is specially for bitmaps ...
+		drm_mode_probed_add(connector, newmode);
+		modes++;
+	}
+
+	if (modes > 0)
+		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
+	return modes;
+}
+
+/*
+ * drm_add_vcb_modes - Add a YCBCR 420 mode into bitmap
+ * @connector: connector corresponding to the HDMI sink
+ * @vic: CEA vic for the video mode to be added in the map
+ *
+ * Makes an entry for a videomode in the YCBCR 420 bitmap
+ */
+static void
+drm_add_vcb_modes(struct drm_connector *connector, u8 vic)
The "vcb" term doesn't seem to be in the spec. Should be "cmdb" maybe?

Or maybe we want ycbcr420_vdb_modes and ycbcr420_y420vdb_modes just to
make it clear to which VDB they relate with.
cmdb seems appropriate, which is aligned to the spec too, will do the
change.
+{
+	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
+
+	/* VICs are 1 to 127(107 defined till CEA-861-F) */
+	vic &= 127;
+
+	/*
+	 * ycbcr420_vcb_modes is a fix position 128 bit bitmap.
+	 * Every bit here represents a VIC, from CEA-861-F list.
+	 * So if bit[n] is set, it indicates vic[n+1] supports
+	 * YCBCR420 output. Bit 0 is dummy, as VICs start at 1.
+	 * ycbcr420_vcb_modes[0] = |VIC=64 |VIC=63 |.....|VIC=2 |VIC=1 |
+	 * ycbcr420_vcb_modes[1] = |VIC=128|VIC=127|.....|VIC=66|VIC=65|
+	 * Difference between a vcb_mode and vdb_mode is that a vcb_mode
+	 * can support other HDMI outputs like RGB/YCBCR444/422 also
+	 * along with YCBCR 240, whereas a vdb_mode can only support YCBCR
+	 * 420 HDMI output.
+	 */
+	bitmap_set(hdmi->ycbcr420_vcb_modes, vic, 1);
+}
+
    static int
    do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
    {
    	int i, modes = 0;
+	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
for (i = 0; i < len; i++) {
    		struct drm_display_mode *mode;
    		mode = drm_display_mode_from_vic_index(connector, db, len, i);
    		if (mode) {
+			/*
+			 * YCBCR420 capability block contains a bitmap which
+			 * gives the index of CEA modes from CEA VDB, which
+			 * can support YCBCR 420 sampling output also (apart
+			 * from RGB/YCBCR444 etc).
+			 * For example, if the bit 0 in bitmap is set,
+			 * first mode in VDB can support YCBCR420 output too.
+			 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
+			 */
+			if (connector->is_hdmi2_src &&
Hmm. I wonder if need this here at all. I guess we do need something for
the "420 only" modes, but not sure if it should be here or if we should
reject them only during mode validation. The latter would be nice
because it would then leave a message into the log that the mode was
present but was rejected.
Seems like a good idea, the only benefit of doing this is, this is in
DRM layer, and will protect other drivers
from accidentally adding 420_vcb modes. Mode valid would be internal to
driver, and applicable in I915 layer.
No, we should put into the probe helpers standard mode validation stuff.
No driver specifics needed apart from setting the support_ycbcr420 or
whatever flag.
Ok, got it.
So your wish is my command :)
I'm thinking we should probably call this
flag ycbcr420_allowed or something like that to match the other
foo_allowed flags we have in the connector for mode validation.
I don't like that honestly. I am hoping that this flag would be used
further, to identify a HDMI 2.0 src over HDMI 1.4
And what happens when when you have an otherwise HDMI 2.0 capable
source that can't output YCbCr 4:2:0?
This makes equation difficult, but in this way, we might need flags for
all sub-features like
4k@60, scrambling, scdc, scdc_rr, ycbcr420. May be, I will add a comment
Yes, we should check for features, not for some standard version that
implements some/all of those features. But not convinced we need any
featur flags for the other things you listed. Aren't we already doing
all those things anyway?

that if you
enable this flag, this is going to happen or something ?
source, beyond YCBCR420 feature, and at that time, it might be more
suitable to have hdmi_2_src. Does it make sense ?
So I think this could perhaps just be an uncoditional
if (test_bit(i, ...))
	__set_bit(vic, ...);

+				test_bit(i, &hdmi->ycbcr420_vcb_map))
+				drm_add_vcb_modes(connector, db[i]);
+
    			drm_mode_probed_add(connector, mode);
    			modes++;
    		}
@@ -3427,6 +3517,12 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
    }
static int
+cea_db_extended_tag(const u8 *db)
+{
+	return db[1];
+}
Could you clean up the current extended tag usage as a separate
prep patch? Would be nice to avoid the confusion of calling the
"Use extended tag" tag VIDEO_CAPABILITY_BLOCK.
Sure, adding in my to-do list.
+
+static int
    cea_db_payload_len(const u8 *db)
    {
    	return db[0] & 0x1f;
@@ -3487,9 +3583,81 @@ static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
    	return oui == HDMI_FORUM_IEEE_OUI;
    }
+static bool cea_db_is_ycbcr_420_cmdb(const u8 *db)
+{
+	u8 len = cea_db_payload_len(db);
'len' seems like a fairly pointless variable since it's used exactly
once.
Agree,
+
+	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
+		return false;
+
+	if (!len)
+		return false;
+
+	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)
+		return false;
+
+	return true;
+}
+
+static bool cea_db_is_ycbcr_420_vdb(const u8 *db)
+{
+	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
+		return false;
+
Length check missing.
Oops, got it.
+	if (cea_db_extended_tag(db) != VIDEO_DATA_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)
+static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
+				     const u8 *db)
+{
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_hdmi_info *hdmi = &info->hdmi;
+	u8 map_len = cea_db_payload_len(db) - 1;
+	u8 count;
+	u64 map = 0;
+
+	if (!db)
+		return;
Can't happen.
I assumed you were convince on my previous (lame) excuse of corrupt db :)
Will remove this check.
+
+	if (map_len == 0) {
+		/* All CEA modes support ycbcr420 sampling also.*/
+		hdmi->ycbcr420_vcb_map = U64_MAX;
+		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
+		return;
+	}
+
+	/*
+	 * This map indicates which of the existing CEA block modes
+	 * from VDB can support YCBCR420 output too. So if bit=0 is
+	 * set, first mode from VDB can support YCBCR420 output too.
+	 * We will parse and keep this map, before parsing VDB itself
+	 * to avoid going through the same block again and again.
+	 *
+	 * Spec is not clear about max possible size of this block.
+	 * Clamping max bitmap block size at 8 bytes. Every byte can
+	 * address 8 CEA modes, in this way this map can address
+	 * 8*8 = first 64 SVDs.
+	 */
+	if (WARN_ON_ONCE(map_len > 8))
+		map_len = 8;
+
+	for (count = 0; count < map_len; count++)
+		map |= (u64)db[2 + count] << (8 * count);
+
+	if (map) {
+		DRM_DEBUG_KMS("Sink supports ycbcr 420\n");
If we want to print something about the sinks color capabilities I think
it should be in central place instead of being sprinkled all over the
place.
Yes, seems like a good idea. I will remove this, and may be add a
separate patch to dump
sink capabilities.
+		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
+	}
+
+	hdmi->ycbcr420_vcb_map = map;
+}
+
    static int
    add_cea_modes(struct drm_connector *connector, struct edid *edid)
    {
@@ -3512,10 +3680,17 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid)
    				video = db + 1;
    				video_len = dbl;
    				modes += do_cea_modes(connector, video, dbl);
-			}
-			else if (cea_db_is_hdmi_vsdb(db)) {
+			} else if (cea_db_is_hdmi_vsdb(db)) {
    				hdmi = db;
    				hdmi_len = dbl;
+			} else if (cea_db_is_ycbcr_420_vdb(db) &&
+					connector->is_hdmi2_src) {
Broken indentation in a bunch of places.
Damn, I thought I am improving :(
+				const u8 *vdb420 = &db[2];
+
+				/* Add 4:2:0(only) modes present in EDID */
+				modes += do_ycbcr_420_vdb_modes(connector,
+								vdb420,
+								dbl - 1);
    			}
    		}
    	}
@@ -4196,6 +4371,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
    			drm_parse_hdmi_vsdb_video(connector, db);
    		if (cea_db_is_hdmi_forum_vsdb(db))
    			drm_parse_hdmi_forum_vsdb(connector, db);
+		if (cea_db_is_ycbcr_420_cmdb(db))
+			drm_parse_y420cmdb_bitmap(connector, db);
    	}
    }
@@ -4430,6 +4607,13 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
    	quirks = edid_get_quirks(edid);
/*
+	 * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks.
+	 * To avoid multiple parsing of same block, lets get the sink info
+	 * before parsing CEA modes.
+	 */
+	drm_add_display_info(connector, edid);
+
+	/*
    	 * EDID spec says modes should be preferred in this order:
    	 * - preferred detailed mode
    	 * - other detailed modes from base block
@@ -4450,14 +4634,13 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
    	num_modes += add_cea_modes(connector, edid);
    	num_modes += add_alternate_cea_modes(connector, edid);
    	num_modes += add_displayid_detailed_modes(connector, edid);
+
    	if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
    		num_modes += add_inferred_modes(connector, edid);
if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75))
    		edid_fixup_preferred(connector, quirks);
- drm_add_display_info(connector, edid);
-
I think this could be a separate patch, just in case it causes a
regression.
Got it.
    	if (quirks & EDID_QUIRK_FORCE_6BPC)
    		connector->display_info.bpc = 6;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 390319c..5a47c33 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -137,6 +137,28 @@ struct drm_scdc {
    struct drm_hdmi_info {
    	/** @scdc: sink's scdc support and capabilities */
    	struct drm_scdc scdc;
+
+	/**
+	 * @ycbcr420_vdb_modes: bitmap of modes which can support ycbcr420
+	 * output only (not normal RGB/YCBCR444/422 outputs). There are total
+	 * 107 VICs defined by CEA-861-F spec, so the size is 128 bits to map
+	 * upto 128 VICs;
+	 */
+	unsigned long ycbcr420_vdb_modes[BITS_TO_LONGS(128)];
+
+	/**
+	 * @ycbcr420_vcb_modes: bitmap of modes which can support ycbcr420
+	 * output also, along with normal HDMI outputs. There are total 107
+	 * VICs defined by CEA-861-F spec, so the size is 128 bits to map upto
+	 * 128 VICs;
+	 */
+	unsigned long ycbcr420_vcb_modes[BITS_TO_LONGS(128)];
+
+	/** @ycbcr420_vcb_map: bitmap of SVD index, to extraxt vcb modes */
+	unsigned long ycbcr420_vcb_map;
+
+	/** @ycbcr420_dc_modes: bitmap of deep color support index */
+	u8 ycbcr420_dc_modes;
unused
Its used. We are parsing 420 deep color info and checking the it in
hdmi_12bpc_possible()
Not in this patch.
Yes, actually that's in next patch (parse ycbcr_420_deep_color). I will
move it in next patch.

- Shashank
Shashank
    };
/**
@@ -200,6 +222,7 @@ struct drm_display_info {
    #define DRM_COLOR_FORMAT_RGB444		(1<<0)
    #define DRM_COLOR_FORMAT_YCRCB444	(1<<1)
    #define DRM_COLOR_FORMAT_YCRCB422	(1<<2)
+#define DRM_COLOR_FORMAT_YCRCB420	(1<<3)
/**
    	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
--
2.7.4

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux