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.4And 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 commentYes, 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.hindex 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;unusedIts 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. - ShashankShashank};/**@@ -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
