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."
> + */
> +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.
> +{
> + int modes = 0, i;
> + struct drm_device *dev = connector->dev;
> + struct drm_display_mode *newmode;
This variable can be moved into the loop scope.
> + 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.
> +
> + 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.
> + bitmap_set(hdmi->ycbcr420_vdb_modes, vic, 1);
__set_bit()
> + 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.
> +{
> + 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. 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.
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.
> +
> +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.
> +
> + 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.
> + 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.
> +
> + 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.
> + 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.
> + 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.
> 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
> };
>
> /**
> @@ -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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
