Re: [Intel-gfx] [PATCH v7 04/22] drm/edid: Use the pre-parsed VICs

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 04, 2023 at 12:05:19PM +0200, Jani Nikula wrote:
> Now that we have all the VICs in info->vics, use them to simplify access
> based on VIC index, i.e. on the order of VICs in the EDID, and avoid
> passing CTA VDB pointers around.
> 
> This also fixes the highly unlikely scenarios of a) multiple HDMI VSDBs,
> and b) HDMI VSDB 3D modes using VIC indexes that span across multiple
> CTA VDBs, and the combination of the two.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Cool. Takes care of my previously stated concern of
multiple CTA/HDMI data blocks.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_edid.c | 92 +++++++++++++-------------------------
>  1 file changed, 32 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 90846dc69061..7f0386175230 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4468,28 +4468,20 @@ static u8 svd_to_vic(u8 svd)
>  	return svd;
>  }
>  
> +/*
> + * Return a display mode for the 0-based vic_index'th VIC across all CTA VDBs in
> + * the EDID, or NULL on errors.
> + */
>  static struct drm_display_mode *
> -drm_display_mode_from_vic_index(struct drm_connector *connector,
> -				const u8 *video_db, u8 video_len,
> -				u8 video_index)
> +drm_display_mode_from_vic_index(struct drm_connector *connector, int vic_index)
>  {
> +	const struct drm_display_info *info = &connector->display_info;
>  	struct drm_device *dev = connector->dev;
> -	struct drm_display_mode *newmode;
> -	u8 vic;
>  
> -	if (video_db == NULL || video_index >= video_len)
> +	if (!info->vics || vic_index >= info->vics_len || !info->vics[vic_index])
>  		return NULL;
>  
> -	/* CEA modes are numbered 1..127 */
> -	vic = svd_to_vic(video_db[video_index]);
> -	if (!drm_valid_cea_vic(vic))
> -		return NULL;
> -
> -	newmode = drm_mode_duplicate(dev, cea_mode_for_vic(vic));
> -	if (!newmode)
> -		return NULL;
> -
> -	return newmode;
> +	return drm_display_mode_from_cea_vic(dev, info->vics[vic_index]);
>  }
>  
>  /*
> @@ -4538,9 +4530,8 @@ static int do_y420vdb_modes(struct drm_connector *connector,
>   * Makes an entry for a videomode in the YCBCR 420 bitmap
>   */
>  static void
> -drm_add_cmdb_modes(struct drm_connector *connector, u8 svd)
> +drm_add_cmdb_modes(struct drm_connector *connector, u8 vic)
>  {
> -	u8 vic = svd_to_vic(svd);
>  	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>  
>  	if (!drm_valid_cea_vic(vic))
> @@ -4577,16 +4568,20 @@ drm_display_mode_from_cea_vic(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_display_mode_from_cea_vic);
>  
> -static int
> -do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
> +/* Add modes based on VICs parsed in parse_cta_vdb() */
> +static int add_cta_vdb_modes(struct drm_connector *connector)
>  {
> +	const struct drm_display_info *info = &connector->display_info;
> +	const struct drm_hdmi_info *hdmi = &info->hdmi;
>  	int i, modes = 0;
> -	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>  
> -	for (i = 0; i < len; i++) {
> +	if (!info->vics)
> +		return 0;
> +
> +	for (i = 0; i < info->vics_len; i++) {
>  		struct drm_display_mode *mode;
>  
> -		mode = drm_display_mode_from_vic_index(connector, db, len, i);
> +		mode = drm_display_mode_from_vic_index(connector, i);
>  		if (mode) {
>  			/*
>  			 * YCBCR420 capability block contains a bitmap which
> @@ -4598,7 +4593,7 @@ do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
>  			 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
>  			 */
>  			if (i < 64 && hdmi->y420_cmdb_map & (1ULL << i))
> -				drm_add_cmdb_modes(connector, db[i]);
> +				drm_add_cmdb_modes(connector, info->vics[i]);
>  
>  			drm_mode_probed_add(connector, mode);
>  			modes++;
> @@ -4693,15 +4688,13 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
>  }
>  
>  static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
> -			       const u8 *video_db, u8 video_len, u8 video_index)
> +			       int vic_index)
>  {
>  	struct drm_display_mode *newmode;
>  	int modes = 0;
>  
>  	if (structure & (1 << 0)) {
> -		newmode = drm_display_mode_from_vic_index(connector, video_db,
> -							  video_len,
> -							  video_index);
> +		newmode = drm_display_mode_from_vic_index(connector, vic_index);
>  		if (newmode) {
>  			newmode->flags |= DRM_MODE_FLAG_3D_FRAME_PACKING;
>  			drm_mode_probed_add(connector, newmode);
> @@ -4709,9 +4702,7 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>  		}
>  	}
>  	if (structure & (1 << 6)) {
> -		newmode = drm_display_mode_from_vic_index(connector, video_db,
> -							  video_len,
> -							  video_index);
> +		newmode = drm_display_mode_from_vic_index(connector, vic_index);
>  		if (newmode) {
>  			newmode->flags |= DRM_MODE_FLAG_3D_TOP_AND_BOTTOM;
>  			drm_mode_probed_add(connector, newmode);
> @@ -4719,9 +4710,7 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>  		}
>  	}
>  	if (structure & (1 << 8)) {
> -		newmode = drm_display_mode_from_vic_index(connector, video_db,
> -							  video_len,
> -							  video_index);
> +		newmode = drm_display_mode_from_vic_index(connector, vic_index);
>  		if (newmode) {
>  			newmode->flags |= DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF;
>  			drm_mode_probed_add(connector, newmode);
> @@ -4742,8 +4731,7 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>   * also adds the stereo 3d modes when applicable.
>   */
>  static int
> -do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
> -		   const u8 *video_db, u8 video_len)
> +do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len)
>  {
>  	struct drm_display_info *info = &connector->display_info;
>  	int modes = 0, offset = 0, i, multi_present = 0, multi_len;
> @@ -4818,9 +4806,7 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>  		for (i = 0; i < 16; i++) {
>  			if (mask & (1 << i))
>  				modes += add_3d_struct_modes(connector,
> -						structure_all,
> -						video_db,
> -						video_len, i);
> +							     structure_all, i);
>  		}
>  	}
>  
> @@ -4857,8 +4843,6 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>  
>  		if (newflag != 0) {
>  			newmode = drm_display_mode_from_vic_index(connector,
> -								  video_db,
> -								  video_len,
>  								  vic_index);
>  
>  			if (newmode) {
> @@ -5249,20 +5233,16 @@ static int add_cea_modes(struct drm_connector *connector,
>  {
>  	const struct cea_db *db;
>  	struct cea_db_iter iter;
> -	const u8 *hdmi = NULL, *video = NULL;
> -	u8 hdmi_len = 0, video_len = 0;
> -	int modes = 0;
> +	int modes;
> +
> +	/* CTA VDB block VICs parsed earlier */
> +	modes = add_cta_vdb_modes(connector);
>  
>  	cea_db_iter_edid_begin(drm_edid, &iter);
>  	cea_db_iter_for_each(db, &iter) {
> -		if (cea_db_tag(db) == CTA_DB_VIDEO) {
> -			video = cea_db_data(db);
> -			video_len = cea_db_payload_len(db);
> -			modes += do_cea_modes(connector, video, video_len);
> -		} else if (cea_db_is_hdmi_vsdb(db)) {
> -			/* FIXME: Switch to use cea_db_data() */
> -			hdmi = (const u8 *)db;
> -			hdmi_len = cea_db_payload_len(db);
> +		if (cea_db_is_hdmi_vsdb(db)) {
> +			modes += do_hdmi_vsdb_modes(connector, (const u8 *)db,
> +						    cea_db_payload_len(db));
>  		} else if (cea_db_is_y420vdb(db)) {
>  			const u8 *vdb420 = cea_db_data(db) + 1;
>  
> @@ -5273,14 +5253,6 @@ static int add_cea_modes(struct drm_connector *connector,
>  	}
>  	cea_db_iter_end(&iter);
>  
> -	/*
> -	 * We parse the HDMI VSDB after having added the cea modes as we will be
> -	 * patching their flags when the sink supports stereo 3D.
> -	 */
> -	if (hdmi)
> -		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len,
> -					    video, video_len);
> -
>  	return modes;
>  }
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel