Re: [RESEND-CI v4 04/15] drm/edid: parse YCBCR 420 videomodes from EDID

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@linux.intel.com Cc: Jose Abreu joabreu@synopsys.com Cc: Emil Velikov emil.l.velikov@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@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.

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