[PATCH v3] drm/edid DSC pass-through timing support

[PATCH v3] drm/edid DSC pass-through 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, the garbage is displayed on HMD
instead.

I am unaware of any other hardware using this value; however, multiple
users have tested this patch on the Vive Pro 2, and it is known to work and
not break anything else.

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. 

Amdgpu integration was trivial, so I implemented it in the second patch;
other kernel drivers still need the support of this value, and I don't have
the necessary hardware to implement and test the handling of this value on
them.

BR,
Yaroslav

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

 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +
 .../gpu/drm/amd/display/dc/core/dc_stream.c   |  2 +
 drivers/gpu/drm/amd/display/dc/dc_types.h     |  3 ++
 drivers/gpu/drm/drm_edid.c                    | 42 ++++++++++++-------
 include/drm/drm_connector.h                   |  6 +++
 include/drm/drm_displayid.h                   |  4 ++
 6 files changed, 45 insertions(+), 14 deletions(-)


base-commit: a48bba98380cb0b43dcd01d276c7efc282e3c33f
--
2.39.1

[PATCH v3] drm/edid DSC pass-through 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, the garbage is displayed on HMD
instead.

I am unaware of any other hardware using this value; however, multiple
users have tested this patch on the Vive Pro 2, and it is known to work and
not break anything else.

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. 

Amdgpu integration was trivial, so I implemented it in the second patch;
other kernel drivers still need the support of this value, and I don't have
the necessary hardware to implement and test the handling of this value on
them.

BR,
Yaroslav

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

 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +
 .../gpu/drm/amd/display/dc/core/dc_stream.c   |  2 +
 drivers/gpu/drm/amd/display/dc/dc_types.h     |  3 ++
 drivers/gpu/drm/drm_edid.c                    | 42 ++++++++++++-------
 include/drm/drm_connector.h                   |  6 +++
 include/drm/drm_displayid.h                   |  4 ++
 6 files changed, 45 insertions(+), 14 deletions(-)


base-commit: a48bba98380cb0b43dcd01d276c7efc282e3c33f
--
2.39.1

[PATCH v3] drm/edid DSC pass-through 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, the garbage is displayed on HMD
instead.

I am unaware of any other hardware using this value; however, multiple
users have tested this patch on the Vive Pro 2, and it is known to work and
not break anything else.

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. 

Amdgpu integration was trivial, so I implemented it in the second patch;
other kernel drivers still need the support of this value, and I don't have
the necessary hardware to implement and test the handling of this value on
them.

BR,
Yaroslav

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

 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +
 .../gpu/drm/amd/display/dc/core/dc_stream.c   |  2 +
 drivers/gpu/drm/amd/display/dc/dc_types.h     |  3 ++
 drivers/gpu/drm/drm_edid.c                    | 42 ++++++++++++-------
 include/drm/drm_connector.h                   |  6 +++
 include/drm/drm_displayid.h                   |  4 ++
 6 files changed, 45 insertions(+), 14 deletions(-)


base-commit: a48bba98380cb0b43dcd01d276c7efc282e3c33f
--
2.39.1

[PATCH v3 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_edid.c  | 38 +++++++++++++++++++++++++------------
 include/drm/drm_connector.h |  6 ++++++
 include/drm/drm_displayid.h |  4 ++++
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3d0a4da661bc..aa88ac82cbe0 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -6338,7 +6338,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);
@@ -6361,24 +6361,37 @@ 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;
+
+	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;
+		}
 
-	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
 			    connector->base.id, connector->name,
-			    info->mso_pixel_overlap);
-		info->mso_pixel_overlap = 8;
+			    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) * 16
+		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
+
 	drm_dbg_kms(connector->dev,
-		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
+		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
 		    connector->base.id, connector->name,
-		    info->mso_stream_count, info->mso_pixel_overlap);
+		    info->dp_dsc_bpp);
 }
 
 static void drm_update_mso(struct drm_connector *connector,
@@ -6425,6 +6438,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;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 7b5048516185..1d01e0146a7f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -719,6 +719,12 @@ struct drm_display_info {
 	 */
 	u32 max_dsc_bpp;
 
+	/**
+	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
+	 * DST 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_displayid.h b/include/drm/drm_displayid.h
index 49649eb8447e..0fc3afbd1675 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -131,12 +131,16 @@ struct displayid_detailed_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;
 
 /* DisplayID iteration */
-- 
2.39.1

[PATCH v3 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_edid.c  | 38 +++++++++++++++++++++++++------------
 include/drm/drm_connector.h |  6 ++++++
 include/drm/drm_displayid.h |  4 ++++
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3d0a4da661bc..aa88ac82cbe0 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -6338,7 +6338,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);
@@ -6361,24 +6361,37 @@ 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;
+
+	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;
+		}
 
-	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
 			    connector->base.id, connector->name,
-			    info->mso_pixel_overlap);
-		info->mso_pixel_overlap = 8;
+			    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) * 16
+		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
+
 	drm_dbg_kms(connector->dev,
-		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
+		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
 		    connector->base.id, connector->name,
-		    info->mso_stream_count, info->mso_pixel_overlap);
+		    info->dp_dsc_bpp);
 }
 
 static void drm_update_mso(struct drm_connector *connector,
@@ -6425,6 +6438,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;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 7b5048516185..1d01e0146a7f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -719,6 +719,12 @@ struct drm_display_info {
 	 */
 	u32 max_dsc_bpp;
 
+	/**
+	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
+	 * DST 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_displayid.h b/include/drm/drm_displayid.h
index 49649eb8447e..0fc3afbd1675 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -131,12 +131,16 @@ struct displayid_detailed_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;
 
 /* DisplayID iteration */
-- 
2.39.1

[PATCH v3 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_edid.c  | 38 +++++++++++++++++++++++++------------
 include/drm/drm_connector.h |  6 ++++++
 include/drm/drm_displayid.h |  4 ++++
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3d0a4da661bc..aa88ac82cbe0 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -6338,7 +6338,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);
@@ -6361,24 +6361,37 @@ 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;
+
+	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;
+		}
 
-	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
 			    connector->base.id, connector->name,
-			    info->mso_pixel_overlap);
-		info->mso_pixel_overlap = 8;
+			    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) * 16
+		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
+
 	drm_dbg_kms(connector->dev,
-		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
+		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
 		    connector->base.id, connector->name,
-		    info->mso_stream_count, info->mso_pixel_overlap);
+		    info->dp_dsc_bpp);
 }
 
 static void drm_update_mso(struct drm_connector *connector,
@@ -6425,6 +6438,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;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 7b5048516185..1d01e0146a7f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -719,6 +719,12 @@ struct drm_display_info {
 	 */
 	u32 max_dsc_bpp;
 
+	/**
+	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
+	 * DST 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_displayid.h b/include/drm/drm_displayid.h
index 49649eb8447e..0fc3afbd1675 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -131,12 +131,16 @@ struct displayid_detailed_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;
 
 /* DisplayID iteration */
-- 
2.39.1

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

[Jani Nikula]

On Sun, 26 Feb 2023, 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
>
> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
> ---
>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>  include/drm/drm_connector.h |  6 ++++++
>  include/drm/drm_displayid.h |  4 ++++
>  3 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3d0a4da661bc..aa88ac82cbe0 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6338,7 +6338,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);
> @@ -6361,24 +6361,37 @@ 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;
> +
> +	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;
> +		}
>  
> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>  			    connector->base.id, connector->name,
> -			    info->mso_pixel_overlap);
> -		info->mso_pixel_overlap = 8;
> +			    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) * 16
> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
> +

Matter of taste, but I'd probably use << 4 and |. *shrug*

>  	drm_dbg_kms(connector->dev,
> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>  		    connector->base.id, connector->name,
> -		    info->mso_stream_count, info->mso_pixel_overlap);
> +		    info->dp_dsc_bpp);
>  }
>  
>  static void drm_update_mso(struct drm_connector *connector,
> @@ -6425,6 +6438,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;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7b5048516185..1d01e0146a7f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -719,6 +719,12 @@ struct drm_display_info {
>  	 */
>  	u32 max_dsc_bpp;
>  
> +	/**
> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
> +	 * DST bits per pixel in 6.4 fixed point format. 0 means undefined

DST?

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +	 */
> +	u16 dp_dsc_bpp;
> +
>  	/**
>  	 * @vics: Array of vics_len VICs. Internal to EDID parsing.
>  	 */
> diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
> index 49649eb8447e..0fc3afbd1675 100644
> --- a/include/drm/drm_displayid.h
> +++ b/include/drm/drm_displayid.h
> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>  
>  /* DisplayID iteration */

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Jani Nikula]

On Sun, 26 Feb 2023, 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
>
> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
> ---
>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>  include/drm/drm_connector.h |  6 ++++++
>  include/drm/drm_displayid.h |  4 ++++
>  3 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3d0a4da661bc..aa88ac82cbe0 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6338,7 +6338,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);
> @@ -6361,24 +6361,37 @@ 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;
> +
> +	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;
> +		}
>  
> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>  			    connector->base.id, connector->name,
> -			    info->mso_pixel_overlap);
> -		info->mso_pixel_overlap = 8;
> +			    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) * 16
> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
> +

Matter of taste, but I'd probably use << 4 and |. *shrug*

>  	drm_dbg_kms(connector->dev,
> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>  		    connector->base.id, connector->name,
> -		    info->mso_stream_count, info->mso_pixel_overlap);
> +		    info->dp_dsc_bpp);
>  }
>  
>  static void drm_update_mso(struct drm_connector *connector,
> @@ -6425,6 +6438,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;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7b5048516185..1d01e0146a7f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -719,6 +719,12 @@ struct drm_display_info {
>  	 */
>  	u32 max_dsc_bpp;
>  
> +	/**
> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
> +	 * DST bits per pixel in 6.4 fixed point format. 0 means undefined

DST?

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +	 */
> +	u16 dp_dsc_bpp;
> +
>  	/**
>  	 * @vics: Array of vics_len VICs. Internal to EDID parsing.
>  	 */
> diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
> index 49649eb8447e..0fc3afbd1675 100644
> --- a/include/drm/drm_displayid.h
> +++ b/include/drm/drm_displayid.h
> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>  
>  /* DisplayID iteration */

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Jani Nikula]

On Sun, 26 Feb 2023, 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
>
> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
> ---
>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>  include/drm/drm_connector.h |  6 ++++++
>  include/drm/drm_displayid.h |  4 ++++
>  3 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3d0a4da661bc..aa88ac82cbe0 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6338,7 +6338,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);
> @@ -6361,24 +6361,37 @@ 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;
> +
> +	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;
> +		}
>  
> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>  			    connector->base.id, connector->name,
> -			    info->mso_pixel_overlap);
> -		info->mso_pixel_overlap = 8;
> +			    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) * 16
> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
> +

Matter of taste, but I'd probably use << 4 and |. *shrug*

>  	drm_dbg_kms(connector->dev,
> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>  		    connector->base.id, connector->name,
> -		    info->mso_stream_count, info->mso_pixel_overlap);
> +		    info->dp_dsc_bpp);
>  }
>  
>  static void drm_update_mso(struct drm_connector *connector,
> @@ -6425,6 +6438,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;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7b5048516185..1d01e0146a7f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -719,6 +719,12 @@ struct drm_display_info {
>  	 */
>  	u32 max_dsc_bpp;
>  
> +	/**
> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
> +	 * DST bits per pixel in 6.4 fixed point format. 0 means undefined

DST?

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +	 */
> +	u16 dp_dsc_bpp;
> +
>  	/**
>  	 * @vics: Array of vics_len VICs. Internal to EDID parsing.
>  	 */
> diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
> index 49649eb8447e..0fc3afbd1675 100644
> --- a/include/drm/drm_displayid.h
> +++ b/include/drm/drm_displayid.h
> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>  
>  /* DisplayID iteration */

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Harry Wentland]

On 2/26/23 09:10, Yaroslav Bolyukin 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
> 

According to the errata this should only apply to VII timings. The way
it is currently implemented will make it apply to everything which is
not what we want.

Can we add this field to drm_mode_info instead of drm_display_info and
set it inside drm_mode_displayid_detailed when parsing a type_7 timing?

Harry


> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
> ---
>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>  include/drm/drm_connector.h |  6 ++++++
>  include/drm/drm_displayid.h |  4 ++++
>  3 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3d0a4da661bc..aa88ac82cbe0 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6338,7 +6338,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);
> @@ -6361,24 +6361,37 @@ 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;
> +
> +	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;
> +		}
>  
> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>  			    connector->base.id, connector->name,
> -			    info->mso_pixel_overlap);
> -		info->mso_pixel_overlap = 8;
> +			    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) * 16
> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
> +
>  	drm_dbg_kms(connector->dev,
> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>  		    connector->base.id, connector->name,
> -		    info->mso_stream_count, info->mso_pixel_overlap);
> +		    info->dp_dsc_bpp);
>  }
>  
>  static void drm_update_mso(struct drm_connector *connector,
> @@ -6425,6 +6438,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;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7b5048516185..1d01e0146a7f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -719,6 +719,12 @@ struct drm_display_info {
>  	 */
>  	u32 max_dsc_bpp;
>  
> +	/**
> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
> index 49649eb8447e..0fc3afbd1675 100644
> --- a/include/drm/drm_displayid.h
> +++ b/include/drm/drm_displayid.h
> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>  
>  /* DisplayID iteration */

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

[Harry Wentland]

On 2/26/23 09:10, Yaroslav Bolyukin 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
> 

According to the errata this should only apply to VII timings. The way
it is currently implemented will make it apply to everything which is
not what we want.

Can we add this field to drm_mode_info instead of drm_display_info and
set it inside drm_mode_displayid_detailed when parsing a type_7 timing?

Harry


> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
> ---
>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>  include/drm/drm_connector.h |  6 ++++++
>  include/drm/drm_displayid.h |  4 ++++
>  3 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3d0a4da661bc..aa88ac82cbe0 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6338,7 +6338,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);
> @@ -6361,24 +6361,37 @@ 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;
> +
> +	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;
> +		}
>  
> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>  			    connector->base.id, connector->name,
> -			    info->mso_pixel_overlap);
> -		info->mso_pixel_overlap = 8;
> +			    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) * 16
> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
> +
>  	drm_dbg_kms(connector->dev,
> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>  		    connector->base.id, connector->name,
> -		    info->mso_stream_count, info->mso_pixel_overlap);
> +		    info->dp_dsc_bpp);
>  }
>  
>  static void drm_update_mso(struct drm_connector *connector,
> @@ -6425,6 +6438,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;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7b5048516185..1d01e0146a7f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -719,6 +719,12 @@ struct drm_display_info {
>  	 */
>  	u32 max_dsc_bpp;
>  
> +	/**
> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
> index 49649eb8447e..0fc3afbd1675 100644
> --- a/include/drm/drm_displayid.h
> +++ b/include/drm/drm_displayid.h
> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>  
>  /* DisplayID iteration */

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

[Harry Wentland]

On 2/26/23 09:10, Yaroslav Bolyukin 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
> 

According to the errata this should only apply to VII timings. The way
it is currently implemented will make it apply to everything which is
not what we want.

Can we add this field to drm_mode_info instead of drm_display_info and
set it inside drm_mode_displayid_detailed when parsing a type_7 timing?

Harry


> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
> ---
>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>  include/drm/drm_connector.h |  6 ++++++
>  include/drm/drm_displayid.h |  4 ++++
>  3 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3d0a4da661bc..aa88ac82cbe0 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6338,7 +6338,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);
> @@ -6361,24 +6361,37 @@ 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;
> +
> +	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;
> +		}
>  
> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>  			    connector->base.id, connector->name,
> -			    info->mso_pixel_overlap);
> -		info->mso_pixel_overlap = 8;
> +			    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) * 16
> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
> +
>  	drm_dbg_kms(connector->dev,
> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>  		    connector->base.id, connector->name,
> -		    info->mso_stream_count, info->mso_pixel_overlap);
> +		    info->dp_dsc_bpp);
>  }
>  
>  static void drm_update_mso(struct drm_connector *connector,
> @@ -6425,6 +6438,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;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7b5048516185..1d01e0146a7f 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -719,6 +719,12 @@ struct drm_display_info {
>  	 */
>  	u32 max_dsc_bpp;
>  
> +	/**
> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
> index 49649eb8447e..0fc3afbd1675 100644
> --- a/include/drm/drm_displayid.h
> +++ b/include/drm/drm_displayid.h
> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>  
>  /* DisplayID iteration */

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

[Jani Nikula]

On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
> On 2/26/23 09:10, Yaroslav Bolyukin 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
>> 
>
> According to the errata this should only apply to VII timings. The way
> it is currently implemented will make it apply to everything which is
> not what we want.
>
> Can we add this field to drm_mode_info instead of drm_display_info and
> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?

That's actually difficult to do nicely. I think the patch at hand is
fine, and it's fine to add the information to drm_display_info. It's a
dependency to parsing the modes.

How the info will actually be used is a different matter, and obviously
needs to follow the spec. As it is, *this* patch doesn't say anything
about that. But whether it's handled in VII timings parsing or
elsewhere, I still think this part is fine.


BR,
Jani.

>
> Harry
>
>
>> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
>> ---
>>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>>  include/drm/drm_connector.h |  6 ++++++
>>  include/drm/drm_displayid.h |  4 ++++
>>  3 files changed, 36 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 3d0a4da661bc..aa88ac82cbe0 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -6338,7 +6338,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);
>> @@ -6361,24 +6361,37 @@ 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;
>> +
>> +	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;
>> +		}
>>  
>> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>  			    connector->base.id, connector->name,
>> -			    info->mso_pixel_overlap);
>> -		info->mso_pixel_overlap = 8;
>> +			    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) * 16
>> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
>> +
>>  	drm_dbg_kms(connector->dev,
>> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>>  		    connector->base.id, connector->name,
>> -		    info->mso_stream_count, info->mso_pixel_overlap);
>> +		    info->dp_dsc_bpp);
>>  }
>>  
>>  static void drm_update_mso(struct drm_connector *connector,
>> @@ -6425,6 +6438,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;
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 7b5048516185..1d01e0146a7f 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -719,6 +719,12 @@ struct drm_display_info {
>>  	 */
>>  	u32 max_dsc_bpp;
>>  
>> +	/**
>> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
>> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
>> index 49649eb8447e..0fc3afbd1675 100644
>> --- a/include/drm/drm_displayid.h
>> +++ b/include/drm/drm_displayid.h
>> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>>  
>>  /* DisplayID iteration */
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Jani Nikula]

On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
> On 2/26/23 09:10, Yaroslav Bolyukin 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
>> 
>
> According to the errata this should only apply to VII timings. The way
> it is currently implemented will make it apply to everything which is
> not what we want.
>
> Can we add this field to drm_mode_info instead of drm_display_info and
> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?

That's actually difficult to do nicely. I think the patch at hand is
fine, and it's fine to add the information to drm_display_info. It's a
dependency to parsing the modes.

How the info will actually be used is a different matter, and obviously
needs to follow the spec. As it is, *this* patch doesn't say anything
about that. But whether it's handled in VII timings parsing or
elsewhere, I still think this part is fine.


BR,
Jani.

>
> Harry
>
>
>> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
>> ---
>>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>>  include/drm/drm_connector.h |  6 ++++++
>>  include/drm/drm_displayid.h |  4 ++++
>>  3 files changed, 36 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 3d0a4da661bc..aa88ac82cbe0 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -6338,7 +6338,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);
>> @@ -6361,24 +6361,37 @@ 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;
>> +
>> +	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;
>> +		}
>>  
>> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>  			    connector->base.id, connector->name,
>> -			    info->mso_pixel_overlap);
>> -		info->mso_pixel_overlap = 8;
>> +			    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) * 16
>> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
>> +
>>  	drm_dbg_kms(connector->dev,
>> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>>  		    connector->base.id, connector->name,
>> -		    info->mso_stream_count, info->mso_pixel_overlap);
>> +		    info->dp_dsc_bpp);
>>  }
>>  
>>  static void drm_update_mso(struct drm_connector *connector,
>> @@ -6425,6 +6438,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;
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 7b5048516185..1d01e0146a7f 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -719,6 +719,12 @@ struct drm_display_info {
>>  	 */
>>  	u32 max_dsc_bpp;
>>  
>> +	/**
>> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
>> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
>> index 49649eb8447e..0fc3afbd1675 100644
>> --- a/include/drm/drm_displayid.h
>> +++ b/include/drm/drm_displayid.h
>> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>>  
>>  /* DisplayID iteration */
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Jani Nikula]

On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
> On 2/26/23 09:10, Yaroslav Bolyukin 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
>> 
>
> According to the errata this should only apply to VII timings. The way
> it is currently implemented will make it apply to everything which is
> not what we want.
>
> Can we add this field to drm_mode_info instead of drm_display_info and
> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?

That's actually difficult to do nicely. I think the patch at hand is
fine, and it's fine to add the information to drm_display_info. It's a
dependency to parsing the modes.

How the info will actually be used is a different matter, and obviously
needs to follow the spec. As it is, *this* patch doesn't say anything
about that. But whether it's handled in VII timings parsing or
elsewhere, I still think this part is fine.


BR,
Jani.

>
> Harry
>
>
>> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
>> ---
>>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>>  include/drm/drm_connector.h |  6 ++++++
>>  include/drm/drm_displayid.h |  4 ++++
>>  3 files changed, 36 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 3d0a4da661bc..aa88ac82cbe0 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -6338,7 +6338,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);
>> @@ -6361,24 +6361,37 @@ 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;
>> +
>> +	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;
>> +		}
>>  
>> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>  			    connector->base.id, connector->name,
>> -			    info->mso_pixel_overlap);
>> -		info->mso_pixel_overlap = 8;
>> +			    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) * 16
>> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
>> +
>>  	drm_dbg_kms(connector->dev,
>> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>>  		    connector->base.id, connector->name,
>> -		    info->mso_stream_count, info->mso_pixel_overlap);
>> +		    info->dp_dsc_bpp);
>>  }
>>  
>>  static void drm_update_mso(struct drm_connector *connector,
>> @@ -6425,6 +6438,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;
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 7b5048516185..1d01e0146a7f 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -719,6 +719,12 @@ struct drm_display_info {
>>  	 */
>>  	u32 max_dsc_bpp;
>>  
>> +	/**
>> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
>> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
>> index 49649eb8447e..0fc3afbd1675 100644
>> --- a/include/drm/drm_displayid.h
>> +++ b/include/drm/drm_displayid.h
>> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>>  
>>  /* DisplayID iteration */
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Harry Wentland]

On 2/27/23 12:12, Jani Nikula wrote:
> On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
>> On 2/26/23 09:10, Yaroslav Bolyukin 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
>>>
>>
>> According to the errata this should only apply to VII timings. The way
>> it is currently implemented will make it apply to everything which is
>> not what we want.
>>
>> Can we add this field to drm_mode_info instead of drm_display_info and
>> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?
> 
> That's actually difficult to do nicely. I think the patch at hand is
> fine, and it's fine to add the information to drm_display_info. It's a
> dependency to parsing the modes.
> 
> How the info will actually be used is a different matter, and obviously
> needs to follow the spec. As it is, *this* patch doesn't say anything
> about that. But whether it's handled in VII timings parsing or
> elsewhere, I still think this part is fine.
> 

This patch itself is okay but without further work can't be used by
drivers since they don't currently have an indication whether a mode
is based on a VII timing.

Harry

> 
> BR,
> Jani.
> 
>>
>> Harry
>>
>>
>>> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
>>> ---
>>>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>>>  include/drm/drm_connector.h |  6 ++++++
>>>  include/drm/drm_displayid.h |  4 ++++
>>>  3 files changed, 36 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 3d0a4da661bc..aa88ac82cbe0 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -6338,7 +6338,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);
>>> @@ -6361,24 +6361,37 @@ 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;
>>> +
>>> +	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;
>>> +		}
>>>  
>>> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>>  			    connector->base.id, connector->name,
>>> -			    info->mso_pixel_overlap);
>>> -		info->mso_pixel_overlap = 8;
>>> +			    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) * 16
>>> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
>>> +
>>>  	drm_dbg_kms(connector->dev,
>>> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>>>  		    connector->base.id, connector->name,
>>> -		    info->mso_stream_count, info->mso_pixel_overlap);
>>> +		    info->dp_dsc_bpp);
>>>  }
>>>  
>>>  static void drm_update_mso(struct drm_connector *connector,
>>> @@ -6425,6 +6438,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;
>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>> index 7b5048516185..1d01e0146a7f 100644
>>> --- a/include/drm/drm_connector.h
>>> +++ b/include/drm/drm_connector.h
>>> @@ -719,6 +719,12 @@ struct drm_display_info {
>>>  	 */
>>>  	u32 max_dsc_bpp;
>>>  
>>> +	/**
>>> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
>>> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
>>> index 49649eb8447e..0fc3afbd1675 100644
>>> --- a/include/drm/drm_displayid.h
>>> +++ b/include/drm/drm_displayid.h
>>> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>>>  
>>>  /* DisplayID iteration */
>>
> 

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

[Harry Wentland]

On 2/27/23 12:12, Jani Nikula wrote:
> On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
>> On 2/26/23 09:10, Yaroslav Bolyukin 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
>>>
>>
>> According to the errata this should only apply to VII timings. The way
>> it is currently implemented will make it apply to everything which is
>> not what we want.
>>
>> Can we add this field to drm_mode_info instead of drm_display_info and
>> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?
> 
> That's actually difficult to do nicely. I think the patch at hand is
> fine, and it's fine to add the information to drm_display_info. It's a
> dependency to parsing the modes.
> 
> How the info will actually be used is a different matter, and obviously
> needs to follow the spec. As it is, *this* patch doesn't say anything
> about that. But whether it's handled in VII timings parsing or
> elsewhere, I still think this part is fine.
> 

This patch itself is okay but without further work can't be used by
drivers since they don't currently have an indication whether a mode
is based on a VII timing.

Harry

> 
> BR,
> Jani.
> 
>>
>> Harry
>>
>>
>>> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
>>> ---
>>>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>>>  include/drm/drm_connector.h |  6 ++++++
>>>  include/drm/drm_displayid.h |  4 ++++
>>>  3 files changed, 36 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 3d0a4da661bc..aa88ac82cbe0 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -6338,7 +6338,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);
>>> @@ -6361,24 +6361,37 @@ 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;
>>> +
>>> +	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;
>>> +		}
>>>  
>>> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>>  			    connector->base.id, connector->name,
>>> -			    info->mso_pixel_overlap);
>>> -		info->mso_pixel_overlap = 8;
>>> +			    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) * 16
>>> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
>>> +
>>>  	drm_dbg_kms(connector->dev,
>>> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>>>  		    connector->base.id, connector->name,
>>> -		    info->mso_stream_count, info->mso_pixel_overlap);
>>> +		    info->dp_dsc_bpp);
>>>  }
>>>  
>>>  static void drm_update_mso(struct drm_connector *connector,
>>> @@ -6425,6 +6438,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;
>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>> index 7b5048516185..1d01e0146a7f 100644
>>> --- a/include/drm/drm_connector.h
>>> +++ b/include/drm/drm_connector.h
>>> @@ -719,6 +719,12 @@ struct drm_display_info {
>>>  	 */
>>>  	u32 max_dsc_bpp;
>>>  
>>> +	/**
>>> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
>>> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
>>> index 49649eb8447e..0fc3afbd1675 100644
>>> --- a/include/drm/drm_displayid.h
>>> +++ b/include/drm/drm_displayid.h
>>> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>>>  
>>>  /* DisplayID iteration */
>>
> 

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

[Harry Wentland]

On 2/27/23 12:12, Jani Nikula wrote:
> On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
>> On 2/26/23 09:10, Yaroslav Bolyukin 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
>>>
>>
>> According to the errata this should only apply to VII timings. The way
>> it is currently implemented will make it apply to everything which is
>> not what we want.
>>
>> Can we add this field to drm_mode_info instead of drm_display_info and
>> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?
> 
> That's actually difficult to do nicely. I think the patch at hand is
> fine, and it's fine to add the information to drm_display_info. It's a
> dependency to parsing the modes.
> 
> How the info will actually be used is a different matter, and obviously
> needs to follow the spec. As it is, *this* patch doesn't say anything
> about that. But whether it's handled in VII timings parsing or
> elsewhere, I still think this part is fine.
> 

This patch itself is okay but without further work can't be used by
drivers since they don't currently have an indication whether a mode
is based on a VII timing.

Harry

> 
> BR,
> Jani.
> 
>>
>> Harry
>>
>>
>>> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
>>> ---
>>>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>>>  include/drm/drm_connector.h |  6 ++++++
>>>  include/drm/drm_displayid.h |  4 ++++
>>>  3 files changed, 36 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 3d0a4da661bc..aa88ac82cbe0 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -6338,7 +6338,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);
>>> @@ -6361,24 +6361,37 @@ 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;
>>> +
>>> +	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;
>>> +		}
>>>  
>>> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>>  			    connector->base.id, connector->name,
>>> -			    info->mso_pixel_overlap);
>>> -		info->mso_pixel_overlap = 8;
>>> +			    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) * 16
>>> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
>>> +
>>>  	drm_dbg_kms(connector->dev,
>>> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>>>  		    connector->base.id, connector->name,
>>> -		    info->mso_stream_count, info->mso_pixel_overlap);
>>> +		    info->dp_dsc_bpp);
>>>  }
>>>  
>>>  static void drm_update_mso(struct drm_connector *connector,
>>> @@ -6425,6 +6438,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;
>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>> index 7b5048516185..1d01e0146a7f 100644
>>> --- a/include/drm/drm_connector.h
>>> +++ b/include/drm/drm_connector.h
>>> @@ -719,6 +719,12 @@ struct drm_display_info {
>>>  	 */
>>>  	u32 max_dsc_bpp;
>>>  
>>> +	/**
>>> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
>>> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
>>> index 49649eb8447e..0fc3afbd1675 100644
>>> --- a/include/drm/drm_displayid.h
>>> +++ b/include/drm/drm_displayid.h
>>> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>>>  
>>>  /* DisplayID iteration */
>>
> 

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

[Jani Nikula]

On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
> On 2/27/23 12:12, Jani Nikula wrote:
>> On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
>>> On 2/26/23 09:10, Yaroslav Bolyukin 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
>>>>
>>>
>>> According to the errata this should only apply to VII timings. The way
>>> it is currently implemented will make it apply to everything which is
>>> not what we want.
>>>
>>> Can we add this field to drm_mode_info instead of drm_display_info and
>>> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?
>> 
>> That's actually difficult to do nicely. I think the patch at hand is
>> fine, and it's fine to add the information to drm_display_info. It's a
>> dependency to parsing the modes.
>> 
>> How the info will actually be used is a different matter, and obviously
>> needs to follow the spec. As it is, *this* patch doesn't say anything
>> about that. But whether it's handled in VII timings parsing or
>> elsewhere, I still think this part is fine.
>> 
>
> This patch itself is okay but without further work can't be used by
> drivers since they don't currently have an indication whether a mode
> is based on a VII timing.

Right, agreed.

All I'm saying is info->dp_dsc_bpp is the way to pass that info from
VESA vendor block to mode parsing.

Come to think of it, this patch should probably also rename
drm_update_mso() and drm_parse_vesa_mso_data() to reflect the more
generic VESA vendor block parsing instead of just MSO.

BR,
Jani.

>
> Harry
>
>> 
>> BR,
>> Jani.
>> 
>>>
>>> Harry
>>>
>>>
>>>> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
>>>> ---
>>>>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>>>>  include/drm/drm_connector.h |  6 ++++++
>>>>  include/drm/drm_displayid.h |  4 ++++
>>>>  3 files changed, 36 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index 3d0a4da661bc..aa88ac82cbe0 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -6338,7 +6338,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);
>>>> @@ -6361,24 +6361,37 @@ 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;
>>>> +
>>>> +	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;
>>>> +		}
>>>>  
>>>> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>>>  			    connector->base.id, connector->name,
>>>> -			    info->mso_pixel_overlap);
>>>> -		info->mso_pixel_overlap = 8;
>>>> +			    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) * 16
>>>> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
>>>> +
>>>>  	drm_dbg_kms(connector->dev,
>>>> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>>> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>>>>  		    connector->base.id, connector->name,
>>>> -		    info->mso_stream_count, info->mso_pixel_overlap);
>>>> +		    info->dp_dsc_bpp);
>>>>  }
>>>>  
>>>>  static void drm_update_mso(struct drm_connector *connector,
>>>> @@ -6425,6 +6438,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;
>>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>>> index 7b5048516185..1d01e0146a7f 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -719,6 +719,12 @@ struct drm_display_info {
>>>>  	 */
>>>>  	u32 max_dsc_bpp;
>>>>  
>>>> +	/**
>>>> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
>>>> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
>>>> index 49649eb8447e..0fc3afbd1675 100644
>>>> --- a/include/drm/drm_displayid.h
>>>> +++ b/include/drm/drm_displayid.h
>>>> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>>>>  
>>>>  /* DisplayID iteration */
>>>
>> 
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Jani Nikula]

On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
> On 2/27/23 12:12, Jani Nikula wrote:
>> On Mon, 27 Feb 2023, Harry Wentland <harry.wentland@amd.com> wrote:
>>> On 2/26/23 09:10, Yaroslav Bolyukin 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
>>>>
>>>
>>> According to the errata this should only apply to VII timings. The way
>>> it is currently implemented will make it apply to everything which is
>>> not what we want.
>>>
>>> Can we add this field to drm_mode_info instead of drm_display_info and
>>> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?
>> 
>> That's actually difficult to do nicely. I think the patch at hand is
>> fine, and it's fine to add the information to drm_display_info. It's a
>> dependency to parsing the modes.
>> 
>> How the info will actually be used is a different matter, and obviously
>> needs to follow the spec. As it is, *this* patch doesn't say anything
>> about that. But whether it's handled in VII timings parsing or
>> elsewhere, I still think this part is fine.
>> 
>
> This patch itself is okay but without further work can't be used by
> drivers since they don't currently have an indication whether a mode
> is based on a VII timing.

Right, agreed.

All I'm saying is info->dp_dsc_bpp is the way to pass that info from
VESA vendor block to mode parsing.

Come to think of it, this patch should probably also rename
drm_update_mso() and drm_parse_vesa_mso_data() to reflect the more
generic VESA vendor block parsing instead of just MSO.

BR,
Jani.

>
> Harry
>
>> 
>> BR,
>> Jani.
>> 
>>>
>>> Harry
>>>
>>>
>>>> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
>>>> ---
>>>>  drivers/gpu/drm/drm_edid.c  | 38 +++++++++++++++++++++++++------------
>>>>  include/drm/drm_connector.h |  6 ++++++
>>>>  include/drm/drm_displayid.h |  4 ++++
>>>>  3 files changed, 36 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index 3d0a4da661bc..aa88ac82cbe0 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -6338,7 +6338,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);
>>>> @@ -6361,24 +6361,37 @@ 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;
>>>> +
>>>> +	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;
>>>> +		}
>>>>  
>>>> -	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:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>>>  			    connector->base.id, connector->name,
>>>> -			    info->mso_pixel_overlap);
>>>> -		info->mso_pixel_overlap = 8;
>>>> +			    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) * 16
>>>> +		+ FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
>>>> +
>>>>  	drm_dbg_kms(connector->dev,
>>>> -		    "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>>> +		    "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>>>>  		    connector->base.id, connector->name,
>>>> -		    info->mso_stream_count, info->mso_pixel_overlap);
>>>> +		    info->dp_dsc_bpp);
>>>>  }
>>>>  
>>>>  static void drm_update_mso(struct drm_connector *connector,
>>>> @@ -6425,6 +6438,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;
>>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>>> index 7b5048516185..1d01e0146a7f 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -719,6 +719,12 @@ struct drm_display_info {
>>>>  	 */
>>>>  	u32 max_dsc_bpp;
>>>>  
>>>> +	/**
>>>> +	 * @dp_dsc_bpp: DP Display-Stream-Compression (DSC) timing's target
>>>> +	 * DST 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_displayid.h b/include/drm/drm_displayid.h
>>>> index 49649eb8447e..0fc3afbd1675 100644
>>>> --- a/include/drm/drm_displayid.h
>>>> +++ b/include/drm/drm_displayid.h
>>>> @@ -131,12 +131,16 @@ struct displayid_detailed_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;
>>>>  
>>>>  /* DisplayID iteration */
>>>
>> 
>

-- 
Jani Nikula, Intel Open Source Graphics Center

[PATCH v3 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>
Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 ++
 drivers/gpu/drm/amd/display/dc/core/dc_stream.c           | 2 ++
 drivers/gpu/drm/amd/display/dc/dc_types.h                 | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 6fdc2027c2b4..dba720d5df4c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -89,6 +89,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 
 	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
 
+	edid_caps->dsc_fixed_bits_per_pixel_x16 = connector->display_info.dp_dsc_bpp;
+
 	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
 	if (sad_count <= 0)
 		return result;
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
index 72b261ad9587..a82362417379 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
@@ -103,6 +103,8 @@ static bool dc_stream_construct(struct dc_stream_state *stream,
 
 	/* EDID CAP translation for HDMI 2.0 */
 	stream->timing.flags.LTE_340MCSC_SCRAMBLE = dc_sink_data->edid_caps.lte_340mcsc_scramble;
+	stream->timing.dsc_fixed_bits_per_pixel_x16 =
+		dc_sink_data->edid_caps.dsc_fixed_bits_per_pixel_x16;
 
 	memset(&stream->timing.dsc_cfg, 0, sizeof(stream->timing.dsc_cfg));
 	stream->timing.dsc_cfg.num_slices_h = 0;
diff --git a/drivers/gpu/drm/amd/display/dc/dc_types.h b/drivers/gpu/drm/amd/display/dc/dc_types.h
index 27d0242d6cbd..22fedf4c7547 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_types.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_types.h
@@ -228,6 +228,9 @@ struct dc_edid_caps {
 	bool edid_hdmi;
 	bool hdr_supported;
 
+	/* DisplayPort caps */
+	uint32_t dsc_fixed_bits_per_pixel_x16;
+
 	struct dc_panel_patch panel_patch;
 };
 
-- 
2.39.1

[PATCH v3 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>
Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 ++
 drivers/gpu/drm/amd/display/dc/core/dc_stream.c           | 2 ++
 drivers/gpu/drm/amd/display/dc/dc_types.h                 | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 6fdc2027c2b4..dba720d5df4c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -89,6 +89,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 
 	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
 
+	edid_caps->dsc_fixed_bits_per_pixel_x16 = connector->display_info.dp_dsc_bpp;
+
 	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
 	if (sad_count <= 0)
 		return result;
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
index 72b261ad9587..a82362417379 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
@@ -103,6 +103,8 @@ static bool dc_stream_construct(struct dc_stream_state *stream,
 
 	/* EDID CAP translation for HDMI 2.0 */
 	stream->timing.flags.LTE_340MCSC_SCRAMBLE = dc_sink_data->edid_caps.lte_340mcsc_scramble;
+	stream->timing.dsc_fixed_bits_per_pixel_x16 =
+		dc_sink_data->edid_caps.dsc_fixed_bits_per_pixel_x16;
 
 	memset(&stream->timing.dsc_cfg, 0, sizeof(stream->timing.dsc_cfg));
 	stream->timing.dsc_cfg.num_slices_h = 0;
diff --git a/drivers/gpu/drm/amd/display/dc/dc_types.h b/drivers/gpu/drm/amd/display/dc/dc_types.h
index 27d0242d6cbd..22fedf4c7547 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_types.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_types.h
@@ -228,6 +228,9 @@ struct dc_edid_caps {
 	bool edid_hdmi;
 	bool hdr_supported;
 
+	/* DisplayPort caps */
+	uint32_t dsc_fixed_bits_per_pixel_x16;
+
 	struct dc_panel_patch panel_patch;
 };
 
-- 
2.39.1

[PATCH v3 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>
Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 ++
 drivers/gpu/drm/amd/display/dc/core/dc_stream.c           | 2 ++
 drivers/gpu/drm/amd/display/dc/dc_types.h                 | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 6fdc2027c2b4..dba720d5df4c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -89,6 +89,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 
 	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
 
+	edid_caps->dsc_fixed_bits_per_pixel_x16 = connector->display_info.dp_dsc_bpp;
+
 	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
 	if (sad_count <= 0)
 		return result;
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
index 72b261ad9587..a82362417379 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
@@ -103,6 +103,8 @@ static bool dc_stream_construct(struct dc_stream_state *stream,
 
 	/* EDID CAP translation for HDMI 2.0 */
 	stream->timing.flags.LTE_340MCSC_SCRAMBLE = dc_sink_data->edid_caps.lte_340mcsc_scramble;
+	stream->timing.dsc_fixed_bits_per_pixel_x16 =
+		dc_sink_data->edid_caps.dsc_fixed_bits_per_pixel_x16;
 
 	memset(&stream->timing.dsc_cfg, 0, sizeof(stream->timing.dsc_cfg));
 	stream->timing.dsc_cfg.num_slices_h = 0;
diff --git a/drivers/gpu/drm/amd/display/dc/dc_types.h b/drivers/gpu/drm/amd/display/dc/dc_types.h
index 27d0242d6cbd..22fedf4c7547 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_types.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_types.h
@@ -228,6 +228,9 @@ struct dc_edid_caps {
 	bool edid_hdmi;
 	bool hdr_supported;
 
+	/* DisplayPort caps */
+	uint32_t dsc_fixed_bits_per_pixel_x16;
+
 	struct dc_panel_patch panel_patch;
 };
 
-- 
2.39.1

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

[Harry Wentland]

On 2/26/23 09:10, Yaroslav Bolyukin wrote:
> VESA vendor header from DisplayID spec may contain fixed bit per pixel
> rate, it should be respected by drm driver
> 

This will apply the fixed bpp for all modes. I don't think that's right.
It should apply only to VII timings.

Harry

> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
> Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 ++
>  drivers/gpu/drm/amd/display/dc/core/dc_stream.c           | 2 ++
>  drivers/gpu/drm/amd/display/dc/dc_types.h                 | 3 +++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 6fdc2027c2b4..dba720d5df4c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -89,6 +89,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>  
>  	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
>  
> +	edid_caps->dsc_fixed_bits_per_pixel_x16 = connector->display_info.dp_dsc_bpp;
> +
>  	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
>  	if (sad_count <= 0)
>  		return result;
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> index 72b261ad9587..a82362417379 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> @@ -103,6 +103,8 @@ static bool dc_stream_construct(struct dc_stream_state *stream,
>  
>  	/* EDID CAP translation for HDMI 2.0 */
>  	stream->timing.flags.LTE_340MCSC_SCRAMBLE = dc_sink_data->edid_caps.lte_340mcsc_scramble;
> +	stream->timing.dsc_fixed_bits_per_pixel_x16 =
> +		dc_sink_data->edid_caps.dsc_fixed_bits_per_pixel_x16;
>  
>  	memset(&stream->timing.dsc_cfg, 0, sizeof(stream->timing.dsc_cfg));
>  	stream->timing.dsc_cfg.num_slices_h = 0;
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_types.h b/drivers/gpu/drm/amd/display/dc/dc_types.h
> index 27d0242d6cbd..22fedf4c7547 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_types.h
> @@ -228,6 +228,9 @@ struct dc_edid_caps {
>  	bool edid_hdmi;
>  	bool hdr_supported;
>  
> +	/* DisplayPort caps */
> +	uint32_t dsc_fixed_bits_per_pixel_x16;
> +
>  	struct dc_panel_patch panel_patch;
>  };
>  

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

[Harry Wentland]

On 2/26/23 09:10, Yaroslav Bolyukin wrote:
> VESA vendor header from DisplayID spec may contain fixed bit per pixel
> rate, it should be respected by drm driver
> 

This will apply the fixed bpp for all modes. I don't think that's right.
It should apply only to VII timings.

Harry

> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
> Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 ++
>  drivers/gpu/drm/amd/display/dc/core/dc_stream.c           | 2 ++
>  drivers/gpu/drm/amd/display/dc/dc_types.h                 | 3 +++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 6fdc2027c2b4..dba720d5df4c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -89,6 +89,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>  
>  	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
>  
> +	edid_caps->dsc_fixed_bits_per_pixel_x16 = connector->display_info.dp_dsc_bpp;
> +
>  	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
>  	if (sad_count <= 0)
>  		return result;
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> index 72b261ad9587..a82362417379 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> @@ -103,6 +103,8 @@ static bool dc_stream_construct(struct dc_stream_state *stream,
>  
>  	/* EDID CAP translation for HDMI 2.0 */
>  	stream->timing.flags.LTE_340MCSC_SCRAMBLE = dc_sink_data->edid_caps.lte_340mcsc_scramble;
> +	stream->timing.dsc_fixed_bits_per_pixel_x16 =
> +		dc_sink_data->edid_caps.dsc_fixed_bits_per_pixel_x16;
>  
>  	memset(&stream->timing.dsc_cfg, 0, sizeof(stream->timing.dsc_cfg));
>  	stream->timing.dsc_cfg.num_slices_h = 0;
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_types.h b/drivers/gpu/drm/amd/display/dc/dc_types.h
> index 27d0242d6cbd..22fedf4c7547 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_types.h
> @@ -228,6 +228,9 @@ struct dc_edid_caps {
>  	bool edid_hdmi;
>  	bool hdr_supported;
>  
> +	/* DisplayPort caps */
> +	uint32_t dsc_fixed_bits_per_pixel_x16;
> +
>  	struct dc_panel_patch panel_patch;
>  };
>  

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

[Harry Wentland]

On 2/26/23 09:10, Yaroslav Bolyukin wrote:
> VESA vendor header from DisplayID spec may contain fixed bit per pixel
> rate, it should be respected by drm driver
> 

This will apply the fixed bpp for all modes. I don't think that's right.
It should apply only to VII timings.

Harry

> Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
> Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 ++
>  drivers/gpu/drm/amd/display/dc/core/dc_stream.c           | 2 ++
>  drivers/gpu/drm/amd/display/dc/dc_types.h                 | 3 +++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 6fdc2027c2b4..dba720d5df4c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -89,6 +89,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>  
>  	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
>  
> +	edid_caps->dsc_fixed_bits_per_pixel_x16 = connector->display_info.dp_dsc_bpp;
> +
>  	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
>  	if (sad_count <= 0)
>  		return result;
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> index 72b261ad9587..a82362417379 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> @@ -103,6 +103,8 @@ static bool dc_stream_construct(struct dc_stream_state *stream,
>  
>  	/* EDID CAP translation for HDMI 2.0 */
>  	stream->timing.flags.LTE_340MCSC_SCRAMBLE = dc_sink_data->edid_caps.lte_340mcsc_scramble;
> +	stream->timing.dsc_fixed_bits_per_pixel_x16 =
> +		dc_sink_data->edid_caps.dsc_fixed_bits_per_pixel_x16;
>  
>  	memset(&stream->timing.dsc_cfg, 0, sizeof(stream->timing.dsc_cfg));
>  	stream->timing.dsc_cfg.num_slices_h = 0;
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_types.h b/drivers/gpu/drm/amd/display/dc/dc_types.h
> index 27d0242d6cbd..22fedf4c7547 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_types.h
> @@ -228,6 +228,9 @@ struct dc_edid_caps {
>  	bool edid_hdmi;
>  	bool hdr_supported;
>  
> +	/* DisplayPort caps */
> +	uint32_t dsc_fixed_bits_per_pixel_x16;
> +
>  	struct dc_panel_patch panel_patch;
>  };
>