[PATCH v4 0/2] DisplayID DSC passthrough timing support

[PATCH v4 0/2] DisplayID DSC passthrough timing support

[Yaroslav Bolyukin]

VESA DisplayID spec allows the device to force its DSC bits per pixel
value.

For example, the HTC Vive Pro 2 VR headset uses this value in
high-resolution modes (3680x1836@90-120, 4896x2448@90-120), and when the
kernel doesn't respect this parameter, garbage is displayed on the HMD
instead.

Me and other users have successfully tested the old (v3) version of this
patch (which was applying DSC BPP value unconditionally, thus incorrect:
https://lkml.org/lkml/2023/2/26/116) on Vive Pro 2 and
Bigscreen Beyond VR headsets, and have been using it daily, it is known
to work and doesn't seem to break anything else since 2022.

Previously, I didn't have enough dedication to get it merged, I hope
this time I will manage to get it to v6.19 :D

Regarding driver support - I have looked at amdgpu and Nvidia's
open-gpu-kernel-modules, and both seem to have some indication for this
value; however, in Linux, it is unused in both.

First patch implements parsing of DSC BPP values and display mode VII
timings flag which mandates that the DSC BPP value should actually be
used for this display mode.

The second patch implements handling of this value for AMDGPU driver.

The only thing that I don't like in the current implementation, is how
the value of `dsc_passthrough_timings_support` flag is propagated from
the connector display modes to the mode created in `DRM_IOCTL_MODE_SETCRTC`
handler (which is used for VR display initialization in Monado and
StreamVR), it feels like this flag should be initialized by the kernel
itself, but as far as I can see there is no correct way to do this, as
the timing constraints calculation belongs to the individual drivers.

Another problem with how this flag is set, is that there is no hard
connection between modes creaded in `SETCRTC` and the modes actually
defined by connector, so I implement an assumption that this flag should
be the same between choosen mode and the preferred display mode. Given
that previously due to the missing support for this flag displays
were only showing garbage, I believe this assumption won't break
anything.

Both of those downsides are due to the fact my understanding of DRM
subsystem is not that high. If another implementation would be proposed
by AMDGPU maintainers - I will gladly implement it here.

v3->v4:
 * This patch now parses timings support flag on type VII block, instead
   of applying it unconditionally. Previously I didn't understand the
   spec properly.
 * Now it also is not being applied for non-supported and/or non-VII
   blocks in amdgpu driver.

Regards,

Lach

Yaroslav Bolyukin (2):
  drm/edid: parse DRM VESA dsc bpp target
  drm/amd: use fixed dsc bits-per-pixel from edid

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 ++++-
 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 +++
 5 files changed, 77 insertions(+), 22 deletions(-)

-- 
2.51.0

[PATCH v4 1/2] drm/edid: parse DRM VESA dsc bpp target

[Yaroslav Bolyukin]

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

Signed-off-by: Yaroslav Bolyukin <iam@lach.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)
+
 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;
 
-	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);
+
+	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)
 {
 	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);
+
 	/* 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);
 
 	/* 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;
+
 	/**
 	 * @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;
 };
 
 /**
-- 
2.51.0

[PATCH v4 2/2] drm/amd: use fixed dsc bits-per-pixel from edid

[Yaroslav Bolyukin]

VESA vendor header from DisplayID spec may contain fixed bit per pixel
rate, it should be respected by drm driver

Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 0d03e324d5b9..ebe5bb4eecf8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6521,6 +6521,11 @@ static void fill_stream_properties_from_drm_display_mode(
 
 	stream->output_color_space = get_output_color_space(timing_out, connector_state);
 	stream->content_type = get_output_content_type(connector_state);
+
+	/* DisplayID Type VII pass-through timings. */
+	if (mode_in->dsc_passthrough_timings_support && info->dp_dsc_bpp != 0) {
+		stream->timing.dsc_fixed_bits_per_pixel_x16 = info->dp_dsc_bpp;
+	}
 }
 
 static void fill_audio_info(struct audio_info *audio_info,
@@ -7067,6 +7072,13 @@ create_stream_for_sink(struct drm_connector *connector,
 					&mode, preferred_mode, scale);
 
 			preferred_refresh = drm_mode_vrefresh(preferred_mode);
+
+			/*
+			 * HACK: In case of multiple supported modes, we should look at the matching mode to decide this flag.
+			 * But what is matching mode, how should it be decided?
+			 * Assuming that only preferred mode would have this flag.
+			 */
+			mode.dsc_passthrough_timings_support = preferred_mode->dsc_passthrough_timings_support;
 		}
 	}
 
@@ -7756,7 +7768,7 @@ create_validate_stream_for_sink(struct drm_connector *connector,
 			drm_dbg_kms(connector->dev, "%s:%d Validation failed with %d, retrying w/ YUV420\n",
 				    __func__, __LINE__, dc_result);
 			aconnector->force_yuv420_output = true;
-		}
+}
 		stream = create_validate_stream_for_sink(connector, drm_mode,
 							 dm_state, old_stream);
 		aconnector->force_yuv422_output = false;
-- 
2.51.0

Re: [PATCH v4 1/2] drm/edid: parse DRM VESA dsc bpp target

[Jani Nikula]

On Thu, 16 Oct 2025, Yaroslav Bolyukin <iam@lach.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@lach.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

Re: [PATCH v4 2/2] drm/amd: use fixed dsc bits-per-pixel from edid

[Jani Nikula]

On Thu, 16 Oct 2025, Yaroslav Bolyukin <iam@lach.pw> wrote:
> VESA vendor header from DisplayID spec may contain fixed bit per pixel
> rate, it should be respected by drm driver
>
> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 0d03e324d5b9..ebe5bb4eecf8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6521,6 +6521,11 @@ static void fill_stream_properties_from_drm_display_mode(
>  
>  	stream->output_color_space = get_output_color_space(timing_out, connector_state);
>  	stream->content_type = get_output_content_type(connector_state);
> +
> +	/* DisplayID Type VII pass-through timings. */
> +	if (mode_in->dsc_passthrough_timings_support && info->dp_dsc_bpp != 0) {
> +		stream->timing.dsc_fixed_bits_per_pixel_x16 = info->dp_dsc_bpp;
> +	}

If we had mode->dp_dsc_bpp_x16 directly, or something better named, this
would be simpler. This will eventually be replicated in a bunch of
drivers.

BR,
Jani.

>  }
>  
>  static void fill_audio_info(struct audio_info *audio_info,
> @@ -7067,6 +7072,13 @@ create_stream_for_sink(struct drm_connector *connector,
>  					&mode, preferred_mode, scale);
>  
>  			preferred_refresh = drm_mode_vrefresh(preferred_mode);
> +
> +			/*
> +			 * HACK: In case of multiple supported modes, we should look at the matching mode to decide this flag.
> +			 * But what is matching mode, how should it be decided?
> +			 * Assuming that only preferred mode would have this flag.
> +			 */
> +			mode.dsc_passthrough_timings_support = preferred_mode->dsc_passthrough_timings_support;
>  		}
>  	}
>  
> @@ -7756,7 +7768,7 @@ create_validate_stream_for_sink(struct drm_connector *connector,
>  			drm_dbg_kms(connector->dev, "%s:%d Validation failed with %d, retrying w/ YUV420\n",
>  				    __func__, __LINE__, dc_result);
>  			aconnector->force_yuv420_output = true;
> -		}
> +}
>  		stream = create_validate_stream_for_sink(connector, drm_mode,
>  							 dm_state, old_stream);
>  		aconnector->force_yuv422_output = false;

-- 
Jani Nikula, Intel

Re: [PATCH v4 1/2] drm/edid: parse DRM VESA dsc bpp target

[Yaroslav]

On 2025-10-16 18:36, Jani Nikula wrote:
 > On Thu, 16 Oct 2025, Yaroslav Bolyukin <iam@lach.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.

Thanks for the quick review! :D

 > 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!

Sorry, can you elaborate? Right now stable kernel just ignores 
everything going after 5th byte, so it "supports 7 byte blocks" by 
ignoring them.

 > - Add the detailed timing parsing in a separate patch
 >
I'll split the patch as requested
 >>
 >> Signed-off-by: Yaroslav Bolyukin <iam@lach.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.

BLOCK_REV is handled identically for all the displayid block types 
afaik, and DISPLAYID_BLOCK_DESCRIPTOR_PAYLOAD_BYTES is unrelated to the 
timings block, I didn't want to spread the masks around the file, but 
will do if you think that's better.

 >> +
 >>   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! ;)
I see, MSO overlap/stream count shouldn't be parsed for eDP, I'll do it.
Is that what you meant by "note that the above would be needed to 
backport mso support for 7 byte vendor blocks to stable!"?
 >> +
 >> +	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.

I agree, just didn't like passing both the block and struct from the 
block (timings param), but it should be fine, I'll redo it.

 >>   {
 >>   	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?

They are stored in the separate blocks, and vesa vendor specific block 
can be located after the timings blocks, meaning to do that we need to 
iterate over all the mode blocks again and parse their timings support 
flag from rev again to fill this data. I don't like this either, but 
seems like this is the most logical implementation.

We also have max_dsc_bpp declared in display_mode, and it should be 
related to this.

It also won't help with the fact that it is hard to handle mode flag for 
the modes created at runtime (see AMDGPU patch). I believe there should 
be a fancier way to do this, but this anin't it.

I still have troubles understanding why does this flag need to exist, as 
far as I can see, every device with passthrough timings doesn't have 
both modes using them and not using them, and the implementation doesn't 
look good due to this fact.

On VivePro2 there is a HID command to switch between display modes: 
modes without dsc_bpp are grouped, and two of the of the high resolution 
modes have different dsc_bpp_x16 values on them. I believe it is just 
this flag is redundant, as there are no devices in the wild having set 
dsc_bpp, and the flag unset, but I try to follow the spec, and here we are.

 >> +
 >>   	/* 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.

I'll split the patches, would it be ok to have it in the same patchset?
Same question for mso data_structure_type.

 >>
 >>   	/* 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?

Yep, didn't notice we already have bpp value in display_info.

 >> +
 >>   	/**
 >>   	 * @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;
 >>   };
 >>
 >>   /**

Regards,

Lach

Re: [PATCH v4 2/2] drm/amd: use fixed dsc bits-per-pixel from edid

[Yaroslav]

On 2025-10-16 18:39, Jani Nikula wrote:
> On Thu, 16 Oct 2025, Yaroslav Bolyukin <iam@lach.pw> wrote:
>> VESA vendor header from DisplayID spec may contain fixed bit per pixel
>> rate, it should be respected by drm driver
>>
>> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 0d03e324d5b9..ebe5bb4eecf8 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6521,6 +6521,11 @@ static void fill_stream_properties_from_drm_display_mode(
>>   
>>   	stream->output_color_space = get_output_color_space(timing_out, connector_state);
>>   	stream->content_type = get_output_content_type(connector_state);
>> +
>> +	/* DisplayID Type VII pass-through timings. */
>> +	if (mode_in->dsc_passthrough_timings_support && info->dp_dsc_bpp != 0) {
>> +		stream->timing.dsc_fixed_bits_per_pixel_x16 = info->dp_dsc_bpp;
>> +	}
> 
> If we had mode->dp_dsc_bpp_x16 directly, or something better named, this
> would be simpler. This will eventually be replicated in a bunch of
> drivers.
> 
> BR,
> Jani.

Unfortunately, that won't solve the problem that there is no good way to 
pass this value from display_modes created from e.g 
DRM_IOCTL_MODE_SETCRTC. I believe it would be better if there was a 
syscall to inherit from a native mode instead of creating a new one, but 
this is too far fetched.

I'll check how this was implemented by nvidia open driver, as they have 
added support for this value in their driver somehow.
  >>   }
>>   
>>   static void fill_audio_info(struct audio_info *audio_info,
>> @@ -7067,6 +7072,13 @@ create_stream_for_sink(struct drm_connector *connector,
>>   					&mode, preferred_mode, scale);
>>   
>>   			preferred_refresh = drm_mode_vrefresh(preferred_mode);
>> +
>> +			/*
>> +			 * HACK: In case of multiple supported modes, we should look at the matching mode to decide this flag.
>> +			 * But what is matching mode, how should it be decided?
>> +			 * Assuming that only preferred mode would have this flag.
>> +			 */
>> +			mode.dsc_passthrough_timings_support = preferred_mode->dsc_passthrough_timings_support;
>>   		}
>>   	}
>>   
>> @@ -7756,7 +7768,7 @@ create_validate_stream_for_sink(struct drm_connector *connector,
>>   			drm_dbg_kms(connector->dev, "%s:%d Validation failed with %d, retrying w/ YUV420\n",
>>   				    __func__, __LINE__, dc_result);
>>   			aconnector->force_yuv420_output = true;
>> -		}
>> +}>>   		stream = create_validate_stream_for_sink(connector, drm_mode,
>>   							 dm_state, old_stream);
>>   		aconnector->force_yuv422_output = false;
> 

Regards,

Lach

Re: [PATCH v4 1/2] drm/edid: parse DRM VESA dsc bpp target

[Ville Syrjälä]

On Thu, Oct 16, 2025 at 07:11:48PM +0200, Yaroslav wrote:
> On 2025-10-16 18:36, Jani Nikula wrote:
>  > On Thu, 16 Oct 2025, Yaroslav Bolyukin <iam@lach.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.
> 
> Thanks for the quick review! :D
> 
>  > 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!
> 
> Sorry, can you elaborate? Right now stable kernel just ignores 
> everything going after 5th byte, so it "supports 7 byte blocks" by 
> ignoring them.
> 
>  > - Add the detailed timing parsing in a separate patch
>  >
> I'll split the patch as requested
>  >>
>  >> Signed-off-by: Yaroslav Bolyukin <iam@lach.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.
> 
> BLOCK_REV is handled identically for all the displayid block types 
> afaik, and DISPLAYID_BLOCK_DESCRIPTOR_PAYLOAD_BYTES is unrelated to the 
> timings block, I didn't want to spread the masks around the file, but 
> will do if you think that's better.
> 
>  >> +
>  >>   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! ;)
> I see, MSO overlap/stream count shouldn't be parsed for eDP, I'll do it.
> Is that what you meant by "note that the above would be needed to 
> backport mso support for 7 byte vendor blocks to stable!"?
>  >> +
>  >> +	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.
> 
> I agree, just didn't like passing both the block and struct from the 
> block (timings param), but it should be fine, I'll redo it.
> 
>  >>   {
>  >>   	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?
> 
> They are stored in the separate blocks, and vesa vendor specific block 
> can be located after the timings blocks, meaning to do that we need to 
> iterate over all the mode blocks again and parse their timings support 
> flag from rev again to fill this data. I don't like this either, but 
> seems like this is the most logical implementation.
> 
> We also have max_dsc_bpp declared in display_mode, and it should be 
> related to this.
> 
> It also won't help with the fact that it is hard to handle mode flag for 
> the modes created at runtime (see AMDGPU patch). I believe there should 
> be a fancier way to do this, but this anin't it.
> 
> I still have troubles understanding why does this flag need to exist, as 
> far as I can see, every device with passthrough timings doesn't have 
> both modes using them and not using them, and the implementation doesn't 
> look good due to this fact.

This looks like it would need to be handled in the same as the
"420 only" stuff. But since this doesn't use the VIC it's going to
be even more annoying. Basically you'd have to store the pass-through
timings in eg. display info and then check against that list whenever
you have to figure out if the mode you're looking at is one of these
pass through modes.

> 
> On VivePro2 there is a HID command to switch between display modes: 
> modes without dsc_bpp are grouped, and two of the of the high resolution 
> modes have different dsc_bpp_x16 values on them. I believe it is just 
> this flag is redundant, as there are no devices in the wild having set 
> dsc_bpp, and the flag unset, but I try to follow the spec, and here we are.
> 
>  >> +
>  >>   	/* 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.
> 
> I'll split the patches, would it be ok to have it in the same patchset?
> Same question for mso data_structure_type.
> 
>  >>
>  >>   	/* 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?
> 
> Yep, didn't notice we already have bpp value in display_info.
> 
>  >> +
>  >>   	/**
>  >>   	 * @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;
>  >>   };
>  >>
>  >>   /**
> 
> Regards,
> 
> Lach

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 1/2] drm/edid: parse DRM VESA dsc bpp target

[Yaroslav]

On 2025-10-16 22:45, Ville Syrjälä wrote:
> On Thu, Oct 16, 2025 at 07:11:48PM +0200, Yaroslav wrote:
>> On 2025-10-16 18:36, Jani Nikula wrote:
>>   > On Thu, 16 Oct 2025, Yaroslav Bolyukin <iam@lach.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.
>>
>> Thanks for the quick review! :D
>>
>>   > 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!
>>
>> Sorry, can you elaborate? Right now stable kernel just ignores
>> everything going after 5th byte, so it "supports 7 byte blocks" by
>> ignoring them.
>>
>>   > - Add the detailed timing parsing in a separate patch
>>   >
>> I'll split the patch as requested
>>   >>
>>   >> Signed-off-by: Yaroslav Bolyukin <iam@lach.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.
>>
>> BLOCK_REV is handled identically for all the displayid block types
>> afaik, and DISPLAYID_BLOCK_DESCRIPTOR_PAYLOAD_BYTES is unrelated to the
>> timings block, I didn't want to spread the masks around the file, but
>> will do if you think that's better.
>>
>>   >> +
>>   >>   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! ;)
>> I see, MSO overlap/stream count shouldn't be parsed for eDP, I'll do it.
>> Is that what you meant by "note that the above would be needed to
>> backport mso support for 7 byte vendor blocks to stable!"?
>>   >> +
>>   >> +	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.
>>
>> I agree, just didn't like passing both the block and struct from the
>> block (timings param), but it should be fine, I'll redo it.
>>
>>   >>   {
>>   >>   	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?
>>
>> They are stored in the separate blocks, and vesa vendor specific block
>> can be located after the timings blocks, meaning to do that we need to
>> iterate over all the mode blocks again and parse their timings support
>> flag from rev again to fill this data. I don't like this either, but
>> seems like this is the most logical implementation.
>>
>> We also have max_dsc_bpp declared in display_mode, and it should be
>> related to this.
>>
>> It also won't help with the fact that it is hard to handle mode flag for
>> the modes created at runtime (see AMDGPU patch). I believe there should
>> be a fancier way to do this, but this anin't it.
>>
>> I still have troubles understanding why does this flag need to exist, as
>> far as I can see, every device with passthrough timings doesn't have
>> both modes using them and not using them, and the implementation doesn't
>> look good due to this fact.
> 
> This looks like it would need to be handled in the same as the
> "420 only" stuff. But since this doesn't use the VIC it's going to
> be even more annoying. Basically you'd have to store the pass-through
> timings in eg. display info and then check against that list whenever
> you have to figure out if the mode you're looking at is one of these
> pass through modes.

Except right now DRM_IOCTL_MODE_SETCRTC allows userspace to create 
arbitrary drm_display_mode struct value (I have no idea if that even 
works, but as far as I can see nothing prevents that in amdgpu driver 
implementation), and for that case there needs to be an exception in 
case if all the modes are passthru, because then passed mode should use 
dsc_bpp value regardless (i.e device only supports passthru)?

This behavior is not declared by spec, but based on my testing I can 
only assume that this flag is only a hint, and no hardware supports both 
modes with fixed dsc_bpp value and without it.

And with Jani Nikula's suggestion there is a matter of which dsc_bpp 
value to use, as with proposed move of this value to the mode itself, in 
theory there might be different values for the modes, even if during 
edid parsing only one value (from VESA vendor specific block) might appear.

It just feels too fragile and incomplete.
>> On VivePro2 there is a HID command to switch between display modes:
>> modes without dsc_bpp are grouped, and two of the of the high resolution
>> modes have different dsc_bpp_x16 values on them. I believe it is just
>> this flag is redundant, as there are no devices in the wild having set
>> dsc_bpp, and the flag unset, but I try to follow the spec, and here we are.
>>
>>   >> +
>>   >>   	/* 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.
>>
>> I'll split the patches, would it be ok to have it in the same patchset?
>> Same question for mso data_structure_type.
>>
>>   >>
>>   >>   	/* 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?
>>
>> Yep, didn't notice we already have bpp value in display_info.
>>
>>   >> +
>>   >>   	/**
>>   >>   	 * @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;
>>   >>   };
>>   >>
>>   >>   /**
>>
>> Regards,
>>
>> Lach
> 

Re: [PATCH v4 1/2] drm/edid: parse DRM VESA dsc bpp target

[Ville Syrjälä]

On Fri, Oct 17, 2025 at 12:24:53AM +0200, Yaroslav wrote:
> 
> 
> On 2025-10-16 22:45, Ville Syrjälä wrote:
> > On Thu, Oct 16, 2025 at 07:11:48PM +0200, Yaroslav wrote:
> >> On 2025-10-16 18:36, Jani Nikula wrote:
> >>   > On Thu, 16 Oct 2025, Yaroslav Bolyukin <iam@lach.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.
> >>
> >> Thanks for the quick review! :D
> >>
> >>   > 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!
> >>
> >> Sorry, can you elaborate? Right now stable kernel just ignores
> >> everything going after 5th byte, so it "supports 7 byte blocks" by
> >> ignoring them.
> >>
> >>   > - Add the detailed timing parsing in a separate patch
> >>   >
> >> I'll split the patch as requested
> >>   >>
> >>   >> Signed-off-by: Yaroslav Bolyukin <iam@lach.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.
> >>
> >> BLOCK_REV is handled identically for all the displayid block types
> >> afaik, and DISPLAYID_BLOCK_DESCRIPTOR_PAYLOAD_BYTES is unrelated to the
> >> timings block, I didn't want to spread the masks around the file, but
> >> will do if you think that's better.
> >>
> >>   >> +
> >>   >>   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! ;)
> >> I see, MSO overlap/stream count shouldn't be parsed for eDP, I'll do it.
> >> Is that what you meant by "note that the above would be needed to
> >> backport mso support for 7 byte vendor blocks to stable!"?
> >>   >> +
> >>   >> +	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.
> >>
> >> I agree, just didn't like passing both the block and struct from the
> >> block (timings param), but it should be fine, I'll redo it.
> >>
> >>   >>   {
> >>   >>   	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?
> >>
> >> They are stored in the separate blocks, and vesa vendor specific block
> >> can be located after the timings blocks, meaning to do that we need to
> >> iterate over all the mode blocks again and parse their timings support
> >> flag from rev again to fill this data. I don't like this either, but
> >> seems like this is the most logical implementation.
> >>
> >> We also have max_dsc_bpp declared in display_mode, and it should be
> >> related to this.
> >>
> >> It also won't help with the fact that it is hard to handle mode flag for
> >> the modes created at runtime (see AMDGPU patch). I believe there should
> >> be a fancier way to do this, but this anin't it.
> >>
> >> I still have troubles understanding why does this flag need to exist, as
> >> far as I can see, every device with passthrough timings doesn't have
> >> both modes using them and not using them, and the implementation doesn't
> >> look good due to this fact.
> > 
> > This looks like it would need to be handled in the same as the
> > "420 only" stuff. But since this doesn't use the VIC it's going to
> > be even more annoying. Basically you'd have to store the pass-through
> > timings in eg. display info and then check against that list whenever
> > you have to figure out if the mode you're looking at is one of these
> > pass through modes.
> 
> Except right now DRM_IOCTL_MODE_SETCRTC allows userspace to create 
> arbitrary drm_display_mode struct value (I have no idea if that even 
> works,

That is how it always works. The requested mode always comes straight
from userspace, not from the kernel. Thats' why you need to do the
lookup. And that is exactly what we do for the "420 only" stuff, and
to figure out what VIC to put into the AVI infoframe.

But those all previous cases were a bit easier since it's all based
on the VIC/CEA modes list, so we can do the lookup against a fixed
list. With this stuff you have to generate the list from the
DisplayID somewhere.

> but as far as I can see nothing prevents that in amdgpu driver 
> implementation), and for that case there needs to be an exception in 
> case if all the modes are passthru, because then passed mode should use 
> dsc_bpp value regardless (i.e device only supports passthru)?
> 
> This behavior is not declared by spec, but based on my testing I can 
> only assume that this flag is only a hint, and no hardware supports both 
> modes with fixed dsc_bpp value and without it.
> 
> And with Jani Nikula's suggestion there is a matter of which dsc_bpp 
> value to use, as with proposed move of this value to the mode itself, in 
> theory there might be different values for the modes, even if during 
> edid parsing only one value (from VESA vendor specific block) might appear.

Adding anything to the kernel modes is pretty much pointless. The
parsed modes are just there to be passed to userspace, and then any
additional info you stuff in there is immediately lost. The only way
to preserve it would be extend the uapi with new data and have it be
routed back in by userspace. But then you get into the whole "old
userspace might not like new stuff" issue which is why the kernel eg.
hides the stereo 3D stuff from userspace unless userspace explicitly
says it knows what to do with it.

> 
> It just feels too fragile and incomplete.
> >> On VivePro2 there is a HID command to switch between display modes:
> >> modes without dsc_bpp are grouped, and two of the of the high resolution
> >> modes have different dsc_bpp_x16 values on them. I believe it is just
> >> this flag is redundant, as there are no devices in the wild having set
> >> dsc_bpp, and the flag unset, but I try to follow the spec, and here we are.
> >>
> >>   >> +
> >>   >>   	/* 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.
> >>
> >> I'll split the patches, would it be ok to have it in the same patchset?
> >> Same question for mso data_structure_type.
> >>
> >>   >>
> >>   >>   	/* 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?
> >>
> >> Yep, didn't notice we already have bpp value in display_info.
> >>
> >>   >> +
> >>   >>   	/**
> >>   >>   	 * @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;
> >>   >>   };
> >>   >>
> >>   >>   /**
> >>
> >> Regards,
> >>
> >> Lach
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH v4 1/2] drm/edid: parse DRM VESA dsc bpp target

[Yaroslav]

On 2025-10-17 00:55, Ville Syrjälä wrote:
> On Fri, Oct 17, 2025 at 12:24:53AM +0200, Yaroslav wrote:
>>
>>
>> On 2025-10-16 22:45, Ville Syrjälä wrote:
>>> On Thu, Oct 16, 2025 at 07:11:48PM +0200, Yaroslav wrote:
>>>> On 2025-10-16 18:36, Jani Nikula wrote:
>>>>    > On Thu, 16 Oct 2025, Yaroslav Bolyukin <iam@lach.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.
>>>>
>>>> Thanks for the quick review! :D
>>>>
>>>>    > 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!
>>>>
>>>> Sorry, can you elaborate? Right now stable kernel just ignores
>>>> everything going after 5th byte, so it "supports 7 byte blocks" by
>>>> ignoring them.
>>>>
>>>>    > - Add the detailed timing parsing in a separate patch
>>>>    >
>>>> I'll split the patch as requested
>>>>    >>
>>>>    >> Signed-off-by: Yaroslav Bolyukin <iam@lach.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.
>>>>
>>>> BLOCK_REV is handled identically for all the displayid block types
>>>> afaik, and DISPLAYID_BLOCK_DESCRIPTOR_PAYLOAD_BYTES is unrelated to the
>>>> timings block, I didn't want to spread the masks around the file, but
>>>> will do if you think that's better.
>>>>
>>>>    >> +
>>>>    >>   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! ;)
>>>> I see, MSO overlap/stream count shouldn't be parsed for eDP, I'll do it.
>>>> Is that what you meant by "note that the above would be needed to
>>>> backport mso support for 7 byte vendor blocks to stable!"?
>>>>    >> +
>>>>    >> +	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.
>>>>
>>>> I agree, just didn't like passing both the block and struct from the
>>>> block (timings param), but it should be fine, I'll redo it.
>>>>
>>>>    >>   {
>>>>    >>   	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?
>>>>
>>>> They are stored in the separate blocks, and vesa vendor specific block
>>>> can be located after the timings blocks, meaning to do that we need to
>>>> iterate over all the mode blocks again and parse their timings support
>>>> flag from rev again to fill this data. I don't like this either, but
>>>> seems like this is the most logical implementation.
>>>>
>>>> We also have max_dsc_bpp declared in display_mode, and it should be
>>>> related to this.
>>>>
>>>> It also won't help with the fact that it is hard to handle mode flag for
>>>> the modes created at runtime (see AMDGPU patch). I believe there should
>>>> be a fancier way to do this, but this anin't it.
>>>>
>>>> I still have troubles understanding why does this flag need to exist, as
>>>> far as I can see, every device with passthrough timings doesn't have
>>>> both modes using them and not using them, and the implementation doesn't
>>>> look good due to this fact.
>>>
>>> This looks like it would need to be handled in the same as the
>>> "420 only" stuff. But since this doesn't use the VIC it's going to
>>> be even more annoying. Basically you'd have to store the pass-through
>>> timings in eg. display info and then check against that list whenever
>>> you have to figure out if the mode you're looking at is one of these
>>> pass through modes.
>>
>> Except right now DRM_IOCTL_MODE_SETCRTC allows userspace to create
>> arbitrary drm_display_mode struct value (I have no idea if that even
>> works,
> 
> That is how it always works. The requested mode always comes straight
> from userspace, not from the kernel. Thats' why you need to do the
> lookup. And that is exactly what we do for the "420 only" stuff, and
> to figure out what VIC to put into the AVI infoframe.
> 
> But those all previous cases were a bit easier since it's all based
> on the VIC/CEA modes list, so we can do the lookup against a fixed
> list. With this stuff you have to generate the list from the
> DisplayID somewhere.

It is unclear to me how should matching work for this case, as far as I 
can see, SteamVR sometimes doesn't use proper frame_rate (E.g creating 
mode with 120.0 FPS, while we only have 120.023836 FPS in EDID), and it 
should still use fixed bpp in that case. Given mode from userspace, 
should we search for the smallest edid-declared mode that fits mode 
passed by user? But how should edid-declared mode list be sorted in this 
case? Is this even correct behavior, given there is no defenition for 
that in the spec, and no known hardware to test how this implementation 
would behave?

>> but as far as I can see nothing prevents that in amdgpu driver
>> implementation), and for that case there needs to be an exception in
>> case if all the modes are passthru, because then passed mode should use
>> dsc_bpp value regardless (i.e device only supports passthru)?
>>
>> This behavior is not declared by spec, but based on my testing I can
>> only assume that this flag is only a hint, and no hardware supports both
>> modes with fixed dsc_bpp value and without it.
>>
>> And with Jani Nikula's suggestion there is a matter of which dsc_bpp
>> value to use, as with proposed move of this value to the mode itself, in
>> theory there might be different values for the modes, even if during
>> edid parsing only one value (from VESA vendor specific block) might appear.
> 
> Adding anything to the kernel modes is pretty much pointless. The
> parsed modes are just there to be passed to userspace, and then any
> additional info you stuff in there is immediately lost. The only way
> to preserve it would be extend the uapi with new data and have it be
> routed back in by userspace. But then you get into the whole "old
> userspace might not like new stuff" issue which is why the kernel eg.
> hides the stereo 3D stuff from userspace unless userspace explicitly
> says it knows what to do with it.

Makes sense. Then what about this, slightly incorrect, yet 
implementation which would work for every known current hardware using 
dsc passthru flag - do it as before (patch v3), by applying fixed dsc 
bpp value to every mode used, but ONLY if device specified this flag on 
every mode declared in edid?

Right now it is unclear to me what to do for the corner-cases such as 
not every mode having this flag and userspace requesting mode that was 
not present in edid, nothing about that is said in the DisplayID spec 
(maybe there is some other spec to follow?), and no other hardware 
implementing this part of spec to understand how it would behave.

Postponing proper handling of this flag until there is any device where 
this implementation won't work.

>> It just feels too fragile and incomplete.
>>>> On VivePro2 there is a HID command to switch between display modes:
>>>> modes without dsc_bpp are grouped, and two of the of the high resolution
>>>> modes have different dsc_bpp_x16 values on them. I believe it is just
>>>> this flag is redundant, as there are no devices in the wild having set
>>>> dsc_bpp, and the flag unset, but I try to follow the spec, and here we are.
>>>>
>>>>    >> +
>>>>    >>   	/* 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.
>>>>
>>>> I'll split the patches, would it be ok to have it in the same patchset?
>>>> Same question for mso data_structure_type.
>>>>
>>>>    >>
>>>>    >>   	/* 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?
>>>>
>>>> Yep, didn't notice we already have bpp value in display_info.
>>>>
>>>>    >> +
>>>>    >>   	/**
>>>>    >>   	 * @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;
>>>>    >>   };
>>>>    >>
>>>>    >>   /**
>>>>
>>>> Regards,
>>>>
>>>> Lach
>>>
>