Message-ID: <3abc1087618c822e5676e67a3ec2e64e506dc5ec@intel.com>
Date: Thu, 16 Oct 2025 19:36:57 +0300
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Yaroslav Bolyukin <iam@...h.pw>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>
Cc: Harry Wentland <harry.wentland@....com>, Leo Li <sunpeng.li@....com>,
 Rodrigo Siqueira <siqueira@...lia.com>, Alex Deucher
 <alexander.deucher@....com>, Christian König
 <christian.koenig@....com>,
 Wayne Lin <Wayne.Lin@....com>, amd-gfx@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, Yaroslav
 Bolyukin <iam@...h.pw>
Subject: Re: [PATCH v4 1/2] drm/edid: parse DRM VESA dsc bpp target

On Thu, 16 Oct 2025, Yaroslav Bolyukin <iam@...h.pw> wrote:
> As per DisplayID v2.0 Errata E9 spec "DSC pass-through timing support"
> VESA vendor-specific data block may contain target DSC bits per pixel
> fields

Thanks for the patch.

I think there's just too much going on in a single patch. Should
probably be split to several patches:

- rename drm_parse_vesa_mso_data() to drm_parse_vesa_specific_block()

- handle DSC pass-through parts in the above, including the macros for
  parsing that (but nothing about timing here yet), and adding to
  display_info

- note that the above would be needed to backport mso support for 7 byte
  vendor blocks to stable!

- Add the detailed timing parsing in a separate patch

>
> Signed-off-by: Yaroslav Bolyukin <iam@...h.pw>
> ---
>  drivers/gpu/drm/drm_displayid_internal.h |  8 ++++
>  drivers/gpu/drm/drm_edid.c               | 61 ++++++++++++++++--------
>  include/drm/drm_connector.h              |  6 +++
>  include/drm/drm_modes.h                  | 10 ++++
>  4 files changed, 64 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_displayid_internal.h b/drivers/gpu/drm/drm_displayid_internal.h
> index 957dd0619f5c..d008a98994bb 100644
> --- a/drivers/gpu/drm/drm_displayid_internal.h
> +++ b/drivers/gpu/drm/drm_displayid_internal.h
> @@ -97,6 +97,10 @@ struct displayid_header {
>  	u8 ext_count;
>  } __packed;
>  
> +#define DISPLAYID_BLOCK_REV				GENMASK(2, 0)
> +#define DISPLAYID_BLOCK_PASSTHROUGH_TIMINGS_SUPPORT	BIT(3)
> +#define DISPLAYID_BLOCK_DESCRIPTOR_PAYLOAD_BYTES	GENMASK(6, 4)

These two are related to the rev of struct
displayid_detailed_timing_block only, and should probably be defined
next to it.

> +
>  struct displayid_block {
>  	u8 tag;
>  	u8 rev;
> @@ -144,12 +148,16 @@ struct displayid_formula_timing_block {
>  
>  #define DISPLAYID_VESA_MSO_OVERLAP	GENMASK(3, 0)
>  #define DISPLAYID_VESA_MSO_MODE		GENMASK(6, 5)
> +#define DISPLAYID_VESA_DSC_BPP_INT	GENMASK(5, 0)
> +#define DISPLAYID_VESA_DSC_BPP_FRACT	GENMASK(3, 0)
>  
>  struct displayid_vesa_vendor_specific_block {
>  	struct displayid_block base;
>  	u8 oui[3];
>  	u8 data_structure_type;
>  	u8 mso;
> +	u8 dsc_bpp_int;
> +	u8 dsc_bpp_fract;
>  } __packed;
>  
>  /*
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e2e85345aa9a..6e42e55b41f9 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6524,8 +6524,8 @@ static void drm_get_monitor_range(struct drm_connector *connector,
>  		    info->monitor_range.min_vfreq, info->monitor_range.max_vfreq);
>  }
>  
> -static void drm_parse_vesa_mso_data(struct drm_connector *connector,
> -				    const struct displayid_block *block)
> +static void drm_parse_vesa_specific_block(struct drm_connector *connector,
> +					  const struct displayid_block *block)
>  {
>  	struct displayid_vesa_vendor_specific_block *vesa =
>  		(struct displayid_vesa_vendor_specific_block *)block;
> @@ -6541,7 +6541,7 @@ static void drm_parse_vesa_mso_data(struct drm_connector *connector,
>  	if (oui(vesa->oui[0], vesa->oui[1], vesa->oui[2]) != VESA_IEEE_OUI)
>  		return;
>  
> -	if (sizeof(*vesa) != sizeof(*block) + block->num_bytes) {
> +	if (block->num_bytes < 5) {
>  		drm_dbg_kms(connector->dev,
>  			    "[CONNECTOR:%d:%s] Unexpected VESA vendor block size\n",
>  			    connector->base.id, connector->name);
> @@ -6564,28 +6564,40 @@ static void drm_parse_vesa_mso_data(struct drm_connector *connector,
>  		break;
>  	}
>  
> -	if (!info->mso_stream_count) {
> -		info->mso_pixel_overlap = 0;
> -		return;
> -	}
> +	info->mso_pixel_overlap = 0;

Nitpick, I kind of like having this in the else path below instead of
first setting it to 0 and then setting it again to something else.

>  
> -	info->mso_pixel_overlap = FIELD_GET(DISPLAYID_VESA_MSO_OVERLAP, vesa->mso);
> -	if (info->mso_pixel_overlap > 8) {
> -		drm_dbg_kms(connector->dev,
> -			    "[CONNECTOR:%d:%s] Reserved MSO pixel overlap value %u\n",
> -			    connector->base.id, connector->name,
> -			    info->mso_pixel_overlap);
> -		info->mso_pixel_overlap = 8;
> +	if (info->mso_stream_count) {
> +		info->mso_pixel_overlap = FIELD_GET(DISPLAYID_VESA_MSO_OVERLAP, vesa->mso);
> +		if (info->mso_pixel_overlap > 8) {
> +			drm_dbg_kms(connector->dev,
> +				    "[CONNECTOR:%d:%s] Reserved MSO pixel overlap value %u\n",
> +				    connector->base.id, connector->name,
> +				    info->mso_pixel_overlap);
> +			info->mso_pixel_overlap = 8;
> +		}
>  	}
>  
>  	drm_dbg_kms(connector->dev,
>  		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>  		    connector->base.id, connector->name,
>  		    info->mso_stream_count, info->mso_pixel_overlap);

Not sure we want to debug log this unless info->mso_stream_count !=
0. This is a rare feature.

Side note, we seem to be lacking the check for
data_structure_type. Probably my bad. I'm not asking you to fix it, but
hey, if you're up for it, another patch is welcome! ;)

> +
> +	if (block->num_bytes < 7) {
> +		/* DSC bpp is optional */
> +		return;
> +	}
> +
> +	info->dp_dsc_bpp = FIELD_GET(DISPLAYID_VESA_DSC_BPP_INT, vesa->dsc_bpp_int) << 4 |
> +			   FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
> +
> +	drm_dbg_kms(connector->dev,
> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
> +		    connector->base.id, connector->name,
> +		    info->dp_dsc_bpp);
>  }
>  
> -static void drm_update_mso(struct drm_connector *connector,
> -			   const struct drm_edid *drm_edid)
> +static void drm_update_vesa_specific_block(struct drm_connector *connector,
> +					   const struct drm_edid *drm_edid)
>  {
>  	const struct displayid_block *block;
>  	struct displayid_iter iter;
> @@ -6593,7 +6605,7 @@ static void drm_update_mso(struct drm_connector *connector,
>  	displayid_iter_edid_begin(drm_edid, &iter);
>  	displayid_iter_for_each(block, &iter) {
>  		if (block->tag == DATA_BLOCK_2_VENDOR_SPECIFIC)
> -			drm_parse_vesa_mso_data(connector, block);
> +			drm_parse_vesa_specific_block(connector, block);
>  	}
>  	displayid_iter_end(&iter);
>  }
> @@ -6630,6 +6642,7 @@ static void drm_reset_display_info(struct drm_connector *connector)
>  	info->mso_stream_count = 0;
>  	info->mso_pixel_overlap = 0;
>  	info->max_dsc_bpp = 0;
> +	info->dp_dsc_bpp = 0;
>  
>  	kfree(info->vics);
>  	info->vics = NULL;
> @@ -6753,7 +6766,7 @@ static void update_display_info(struct drm_connector *connector,
>  	if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB422)
>  		info->color_formats |= DRM_COLOR_FORMAT_YCBCR422;
>  
> -	drm_update_mso(connector, drm_edid);
> +	drm_update_vesa_specific_block(connector, drm_edid);
>  
>  out:
>  	if (drm_edid_has_internal_quirk(connector, EDID_QUIRK_NON_DESKTOP)) {
> @@ -6784,7 +6797,8 @@ static void update_display_info(struct drm_connector *connector,
>  
>  static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *dev,
>  							    const struct displayid_detailed_timings_1 *timings,
> -							    bool type_7)
> +							    bool type_7,
> +							    int rev)

If we added struct displayid_detailed_timing_block *block parameter
(between dev and timings), the function could figure it all out from
there instead of having to pass several parameters. Dunno which is
cleaner. It's also not neat to pass rev as int, when it's really data
that has to be parsed.

>  {
>  	struct drm_display_mode *mode;
>  	unsigned int pixel_clock = (timings->pixel_clock[0] |
> @@ -6805,6 +6819,10 @@ static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *d
>  	if (!mode)
>  		return NULL;
>  
> +	if (type_7 && FIELD_GET(DISPLAYID_BLOCK_REV, rev) >= 1)
> +		mode->dsc_passthrough_timings_support =
> +			!!(rev & DISPLAYID_BLOCK_PASSTHROUGH_TIMINGS_SUPPORT);

I wonder if it would make life easier all around if we just filled the
dp_dsc_bpp in the mode itself, instead of having a flag and having to
look it up separately?

> +
>  	/* resolution is kHz for type VII, and 10 kHz for type I */
>  	mode->clock = type_7 ? pixel_clock : pixel_clock * 10;
>  	mode->hdisplay = hactive;
> @@ -6846,7 +6864,7 @@ static int add_displayid_detailed_1_modes(struct drm_connector *connector,
>  	for (i = 0; i < num_timings; i++) {
>  		struct displayid_detailed_timings_1 *timings = &det->timings[i];
>  
> -		newmode = drm_mode_displayid_detailed(connector->dev, timings, type_7);
> +		newmode = drm_mode_displayid_detailed(connector->dev, timings, type_7, block->rev);
>  		if (!newmode)
>  			continue;
>  
> @@ -6893,7 +6911,8 @@ static int add_displayid_formula_modes(struct drm_connector *connector,
>  	struct drm_display_mode *newmode;
>  	int num_modes = 0;
>  	bool type_10 = block->tag == DATA_BLOCK_2_TYPE_10_FORMULA_TIMING;
> -	int timing_size = 6 + ((formula_block->base.rev & 0x70) >> 4);
> +	int timing_size = 6 +
> +		FIELD_GET(DISPLAYID_BLOCK_DESCRIPTOR_PAYLOAD_BYTES, formula_block->base.rev);

I think this is an unrelated change. Probably something we want, but
should not be in the same patch with the rest.

>  
>  	/* extended blocks are not supported yet */
>  	if (timing_size != 6)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 8f34f4b8183d..01640fcf7464 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -837,6 +837,12 @@ struct drm_display_info {
>  	 */
>  	u32 max_dsc_bpp;
>  
> +	/**
> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
> +	 * DSC bits per pixel in 6.4 fixed point format. 0 means undefined.
> +	 */
> +	u16 dp_dsc_bpp;

It's slightly annoying that we have max_dsc_bpp which is int, and
dp_dsc_bpp, which is 6.4 fixed point. The drm_dp_helper.c uses _x16
suffix for the 6.4 bpp, so maybe do the same here, dp_dsc_bpp_x16?

> +
>  	/**
>  	 * @vics: Array of vics_len VICs. Internal to EDID parsing.
>  	 */
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index b9bb92e4b029..312e5c03af9a 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -417,6 +417,16 @@ struct drm_display_mode {
>  	 */
>  	enum hdmi_picture_aspect picture_aspect_ratio;
>  
> +	/**
> +	 * @dsc_passthrough_timing_support:
> +	 *
> +	 * Indicates whether this mode timing descriptor is supported
> +	 * with specific target DSC bits per pixel only.
> +	 *
> +	 * VESA vendor-specific data block shall exist with the relevant
> +	 * DSC bits per pixel declaration when this flag is set to true.
> +	 */
> +	bool dsc_passthrough_timings_support;
>  };
>  
>  /**

-- 
Jani Nikula, Intel

Powered by blists - more mailing lists