Regards Shashank On 7/13/2017 9:51 PM, Ville Syrjälä wrote:
On Thu, Jul 13, 2017 at 09:03:12PM +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. V4: Addressed review comments from Ville: - s/ycbcr_420_vdb/y420vdb - s/ycbcr_420_vcb/y420cmdb - Be less verbose on description of do_y420vdb_modes - Move newmode variable in the loop scope. - Use svd_to_vic() to get a VIC, instead of 0x7f - Remove bitmap description for CMDB modes & VDB modes - Dont add connector->ycbcr_420_allowed check for cmdb modes - Remove 'len' variable, in is_y420cmdb function, which is used only once - Add length check in is_y420vdb function - Remove unnecessary if (!db) check in function parse_y420cmdb_bitmap - Do not add print about YCBCR 420 modes - Fix indentation in few places - Move ycbcr420_dc_modes in next patch, where its used - Add a separate patch for movement of drm_add_display_info() V5: Addressed review comments from Ville: - Add the patch which cleans up the current EXTENDED_TAG usage - Make y420_cmdb_map u64 - Do not block ycbcr420 modes while parsing the EDID, rather add a separate helper function to prune ycbcr420-only modes from connector's probed modes. V6: Rebase V7: Move this patch after the 420_only validation patch (Ville) Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> --- drivers/gpu/drm/drm_edid.c | 148 +++++++++++++++++++++++++++++++++++++++++++- include/drm/drm_connector.h | 12 ++++ 2 files changed, 158 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 96eee5a..b86afb9 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2783,6 +2783,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, #define SPEAKER_BLOCK 0x04 #define USE_EXTENDED_TAG 0x07 #define EXT_VIDEO_CAPABILITY_BLOCK 0x00 +#define EXT_VIDEO_DATA_BLOCK_420 0x0E +#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F #define EDID_BASIC_AUDIO (1 << 6) #define EDID_CEA_YCRCB444 (1 << 5) #define EDID_CEA_YCRCB422 (1 << 4) @@ -3155,15 +3157,79 @@ drm_display_mode_from_vic_index(struct drm_connector *connector, return newmode; }+/*+ * do_y420vdb_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 + * + * Parse the CEA-861-F YCBCR 420 Video Data Block (Y420VDB) + * which contains modes which can be supported in YCBCR 420 + * output format only. + */ +static int do_y420vdb_modes(struct drm_connector *connector, + const u8 *svds, u8 svds_len) +{ + int modes = 0, i; + struct drm_device *dev = connector->dev; + struct drm_display_info *info = &connector->display_info; + struct drm_hdmi_info *hdmi = &info->hdmi; + + for (i = 0; i < svds_len; i++) { + u8 vic = svd_to_vic(svds[i]); + struct drm_display_mode *newmode; +Hmm. Looks like here we will need the drm_valid_cea_vic() check because the vic is coming from an untrusted source.
Agree, there should be one.
I guess its a good idea to be careful while adding modes, I can add a check here too.+ newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]); + if (!newmode) + break; + bitmap_set(hdmi->y420_vdb_modes, vic, 1); + drm_mode_probed_add(connector, newmode); + modes++; + } + + if (modes > 0) + info->color_formats |= DRM_COLOR_FORMAT_YCRCB420; + return modes; +} + +/* + * drm_add_cmdb_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_cmdb_modes(struct drm_connector *connector, u8 svd) +{ + u8 vic = svd_to_vic(svd); + struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;And maybe here. It's not actually needed since we know it's valid because drm_display_mode_from_vic_index() returned a mode, but since that's not immediately obvious from here we might want to have the check. But if you disagree feel free to leave it out.
At some point I think we might want to reorganize drm_display_mode_from_vic_index() such that we actually pass it the vic instead. Then we could also pass the vic here directly.+ + bitmap_set(hdmi->y420_cmdb_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 (hdmi->y420_cmdb_map & (1 << i))I think this needs to be something like if (i < 64 && hdmi->y420_cmdb_map & (1ULL << i))
Yeah, that's also one preventive measure, which we can add. - Shashank
With those changes I think this part should be good to go.+ drm_add_cmdb_modes(connector, db[i]); + drm_mode_probed_add(connector, mode); modes++; } @@ -3505,9 +3571,78 @@ static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) return oui == HDMI_FORUM_IEEE_OUI; }+static bool cea_db_is_y420cmdb(const u8 *db)+{ + + if (cea_db_tag(db) != USE_EXTENDED_TAG) + return false; + + if (!cea_db_payload_len(db)) + return false; + + if (cea_db_extended_tag(db) != EXT_VIDEO_CAP_BLOCK_Y420CMDB) + return false; + + return true; +} + +static bool cea_db_is_y420vdb(const u8 *db) +{ + if (cea_db_tag(db) != USE_EXTENDED_TAG) + return false; + + if (!cea_db_payload_len(db)) + return false; + + if (cea_db_extended_tag(db) != EXT_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 (map_len == 0) { + /* All CEA modes support ycbcr420 sampling also.*/ + hdmi->y420_cmdb_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) + info->color_formats |= DRM_COLOR_FORMAT_YCRCB420; + + hdmi->y420_cmdb_map = map; +} + static int add_cea_modes(struct drm_connector *connector, struct edid *edid) { @@ -3530,10 +3665,16 @@ 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_y420vdb(db)) { + const u8 *vdb420 = &db[2]; + + /* Add 4:2:0(only) modes present in EDID */ + modes += do_y420vdb_modes(connector, + vdb420, + dbl - 1); } } } @@ -4216,6 +4357,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_y420cmdb(db)) + drm_parse_y420cmdb_bitmap(connector, db); } }@@ -4477,6 +4620,7 @@ 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);diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.hindex 26dd3eb..225e092 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -143,6 +143,17 @@ struct drm_hdmi_info { * upto 128 VICs; */ unsigned long y420_vdb_modes[BITS_TO_LONGS(128)]; + + /** + * @y420_cmdb_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 y420_cmdb_modes[BITS_TO_LONGS(128)]; + + /** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */ + u64 y420_cmdb_map; };/**@@ -206,6 +217,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
