[RESEND-CI v4 04/15] drm/edid: parse YCBCR 420 videomodes from EDID
Sharma, Shashank
shashank.sharma at intel.com
Fri Jun 30 12:08:09 UTC 2017
Regards
Shashank
On 6/30/2017 5:28 PM, Ville Syrjälä wrote:
> On Fri, Jun 30, 2017 at 10:47:48AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 6/27/2017 5:22 PM, Ville Syrjälä wrote:
>>> On Wed, Jun 21, 2017 at 04:04:02PM +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 at linux.intel.com>
>>>> Cc: Jose Abreu <joabreu at synopsys.com>
>>>> Cc: Emil Velikov <emil.l.velikov at gmail.com>
>>>>
>>>> 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()
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>>> ---
>>>> drivers/gpu/drm/drm_edid.c | 157 +++++++++++++++++++++++++++++++++++++++++++-
>>>> include/drm/drm_connector.h | 20 ++++++
>>>> 2 files changed, 174 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index e2d1b30..8c7e73b 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)
>>>> @@ -3153,15 +3155,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;
>>>> +
>>>> + 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;
>>>> +
>>>> + 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 (test_bit(i, &hdmi->y420_cmdb_map))
>>>> + drm_add_cmdb_modes(connector, db[i]);
>>>> +
>>>> drm_mode_probed_add(connector, mode);
>>>> modes++;
>>>> }
>>>> @@ -3437,6 +3503,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];
>>>> +}
>>>> +
>>>> +static int
>>>> cea_db_payload_len(const u8 *db)
>>>> {
>>>> return db[0] & 0x1f;
>>>> @@ -3497,9 +3569,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) != VIDEO_CAPABILITY_BLOCK)
>>> Still would like to see that patch to clean up the current extended tag
>>> usage...
>> Yep, its on the way, running a bit loaded.
>>>> + return false;
>>>> +
>>>> + if (!cea_db_payload_len(db))
>>>> + return false;
>>>> +
>>>> + if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)
>>>> + return false;
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> +static bool cea_db_is_y420vdb(const u8 *db)
>>>> +{
>>>> + if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
>>>> + return false;
>>>> +
>>>> + if (!cea_db_payload_len(db))
>>>> + return false;
>>>> +
>>>> + 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 (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)
>>>> {
>>>> @@ -3522,10 +3663,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_y420vdb(db) &&
>>>> + connector->ycbcr_420_allowed) {
>>>> + const u8 *vdb420 = &db[2];
>>>> +
>>>> + /* Add 4:2:0(only) modes present in EDID */
>>>> + modes += do_y420vdb_modes(connector,
>>>> + vdb420,
>>>> + dbl - 1);
>>>> }
>>>> }
>>>> }
>>>> @@ -4206,6 +4354,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);
>>>> }
>>>> }
>>>>
>>>> @@ -4460,6 +4610,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.h
>>>> index 7493fd3..ce2212d 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -137,6 +137,25 @@ struct drm_scdc {
>>>> struct drm_hdmi_info {
>>>> /** @scdc: sink's scdc support and capabilities */
>>>> struct drm_scdc scdc;
>>>> +
>>>> + /**
>>>> + * @y420_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 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 */
>>>> + unsigned long y420_cmdb_map;
>>> u64, otherwise it's busted on 32bit machines.
>> Got it.
>>> Mostl this looks pretty good. I think the main problem is the lack of
>>> mode validation that would reject the 4:2:0 only modes if not supported
>>> by the driver/device. I guess that's in a later patch?
>> Yep, its the next patch in the series.
> The only thing the next patch does is set up the y420_dc_modes, but it
> doesn't use that for anything, and neither does it filter anything. I
> can't find any mode filtering patch actually.
Perhaps I misunderstood your question. If you are asking for the patch,
in which you asked to remove the
420_only modes, at connector_probe level (instead of blocking the read
of 420_only modes based on connector->y420_allowed),
its being prepared now. It should be available with V5.
- Shashank
>>> If so we need to
>>> reorder the series a bit to avoid regeressions.
>> That patch uses parameters added in this patch, to detect the 4:2:0 only
>> modes. So cant make it before this patch.
>> Let me see what best can I do on the reordering.
>>
>> - Shashank
>>>> };
>>>>
>>>> /**
>>>> @@ -200,6 +219,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
More information about the dri-devel
mailing list