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 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx