* [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
@ 2024-02-13 23:45 Abhinav Kumar
2024-02-14 0:11 ` ✗ Fi.CI.BUILD: failure for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Abhinav Kumar @ 2024-02-13 23:45 UTC (permalink / raw
To: dri-devel, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Jani Nikula, Rodrigo Vivi,
Joonas Lahtinen, Tvrtko Ursulin
Cc: Abhinav Kumar, robdclark, freedreno, dmitry.baryshkov, intel-gfx,
ville.syrjala, quic_jesszhan, linux-kernel, intel-xe
intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
Lets move this to drm_dp_helper to achieve this.
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++
drivers/gpu/drm/i915/display/intel_dp.c | 73 +----------------------
include/drm/display/drm_dp_helper.h | 3 +
3 files changed, 84 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index b1ca3a1100da..066cfbbf7a91 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
+ * @vsc: vsc sdp initialized according to its purpose as defined in
+ * table 2-118 - table 2-120 in DP 1.4a specification
+ * @sdp: valid handle to the generic dp_sdp which will be packed
+ * @size: valid size of the passed sdp handle
+ *
+ * Returns length of sdp on success and error code on failure
+ */
+ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
+ struct dp_sdp *sdp, size_t size)
+{
+ size_t length = sizeof(struct dp_sdp);
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(sdp, 0, size);
+
+ /*
+ * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
+ * VSC SDP Header Bytes
+ */
+ sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
+ sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
+ sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
+ sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
+
+ if (vsc->revision == 0x6) {
+ sdp->db[0] = 1;
+ sdp->db[3] = 1;
+ }
+
+ /*
+ * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
+ * Format as per DP 1.4a spec and DP 2.0 respectively.
+ */
+ if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
+ goto out;
+
+ /* VSC SDP Payload for DB16 through DB18 */
+ /* Pixel Encoding and Colorimetry Formats */
+ sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
+ sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
+
+ switch (vsc->bpc) {
+ case 6:
+ /* 6bpc: 0x0 */
+ break;
+ case 8:
+ sdp->db[17] = 0x1; /* DB17[3:0] */
+ break;
+ case 10:
+ sdp->db[17] = 0x2;
+ break;
+ case 12:
+ sdp->db[17] = 0x3;
+ break;
+ case 16:
+ sdp->db[17] = 0x4;
+ break;
+ default:
+ WARN(1, "Missing case %d\n", vsc->bpc);
+ return -EINVAL;
+ }
+
+ /* Dynamic Range and Component Bit Depth */
+ if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
+ sdp->db[17] |= 0x80; /* DB17[7] */
+
+ /* Content Type */
+ sdp->db[18] = vsc->content_type & 0x7;
+
+out:
+ return length;
+}
+EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
+
/**
* drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
* @dpcd: DisplayPort configuration data
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index f5ef95da5534..e94db51aeeb7 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state,
return false;
}
-static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
- struct dp_sdp *sdp, size_t size)
-{
- size_t length = sizeof(struct dp_sdp);
-
- if (size < length)
- return -ENOSPC;
-
- memset(sdp, 0, size);
-
- /*
- * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
- * VSC SDP Header Bytes
- */
- sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
- sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
- sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
- sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
-
- if (vsc->revision == 0x6) {
- sdp->db[0] = 1;
- sdp->db[3] = 1;
- }
-
- /*
- * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
- * Format as per DP 1.4a spec and DP 2.0 respectively.
- */
- if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
- goto out;
-
- /* VSC SDP Payload for DB16 through DB18 */
- /* Pixel Encoding and Colorimetry Formats */
- sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
- sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
-
- switch (vsc->bpc) {
- case 6:
- /* 6bpc: 0x0 */
- break;
- case 8:
- sdp->db[17] = 0x1; /* DB17[3:0] */
- break;
- case 10:
- sdp->db[17] = 0x2;
- break;
- case 12:
- sdp->db[17] = 0x3;
- break;
- case 16:
- sdp->db[17] = 0x4;
- break;
- default:
- MISSING_CASE(vsc->bpc);
- break;
- }
- /* Dynamic Range and Component Bit Depth */
- if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
- sdp->db[17] |= 0x80; /* DB17[7] */
-
- /* Content Type */
- sdp->db[18] = vsc->content_type & 0x7;
-
-out:
- return length;
-}
-
static ssize_t
intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915,
const struct hdmi_drm_infoframe *drm_infoframe,
@@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder,
switch (type) {
case DP_SDP_VSC:
- len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
- sizeof(sdp));
+ len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
+ sizeof(sdp));
break;
case HDMI_PACKET_TYPE_GAMUT_METADATA:
len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv,
@@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder,
struct dp_sdp sdp = {};
ssize_t len;
- len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
+ len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
if (drm_WARN_ON(&dev_priv->drm, len < 0))
return;
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index 863b2e7add29..f8db34a2f7a5 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
int bpp_x16, unsigned long flags);
int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
+ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
+ struct dp_sdp *sdp, size_t size);
+
#endif /* _DRM_DP_HELPER_H_ */
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* ✗ Fi.CI.BUILD: failure for drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
2024-02-13 23:45 [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper Abhinav Kumar
@ 2024-02-14 0:11 ` Patchwork
2024-02-14 0:18 ` ✗ CI.Patch_applied: " Patchwork
2024-02-14 8:15 ` [PATCH] " Dmitry Baryshkov
2 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2024-02-14 0:11 UTC (permalink / raw
To: Abhinav Kumar; +Cc: intel-gfx
== Series Details ==
Series: drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
URL : https://patchwork.freedesktop.org/series/129866/
State : failure
== Summary ==
Error: patch https://patchwork.freedesktop.org/api/1.0/series/129866/revisions/1/mbox/ not applied
Applying: drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
Using index info to reconstruct a base tree...
M drivers/gpu/drm/display/drm_dp_helper.c
M drivers/gpu/drm/i915/display/intel_dp.c
M include/drm/display/drm_dp_helper.h
Falling back to patching base and 3-way merge...
Auto-merging include/drm/display/drm_dp_helper.h
Auto-merging drivers/gpu/drm/i915/display/intel_dp.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/display/intel_dp.c
Auto-merging drivers/gpu/drm/display/drm_dp_helper.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced
^ permalink raw reply [flat|nested] 8+ messages in thread
* ✗ CI.Patch_applied: failure for drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
2024-02-13 23:45 [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper Abhinav Kumar
2024-02-14 0:11 ` ✗ Fi.CI.BUILD: failure for " Patchwork
@ 2024-02-14 0:18 ` Patchwork
2024-02-14 8:15 ` [PATCH] " Dmitry Baryshkov
2 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2024-02-14 0:18 UTC (permalink / raw
To: Abhinav Kumar; +Cc: intel-xe
== Series Details ==
Series: drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
URL : https://patchwork.freedesktop.org/series/129865/
State : failure
== Summary ==
=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: e3e6421a4 drm-tip: 2024y-02m-13d-21h-33m-36s UTC integration manifest
=== git am output follows ===
error: patch failed: drivers/gpu/drm/i915/display/intel_dp.c:4297
error: drivers/gpu/drm/i915/display/intel_dp.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Applying: drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
Patch failed at 0001 drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
2024-02-13 23:45 [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper Abhinav Kumar
2024-02-14 0:11 ` ✗ Fi.CI.BUILD: failure for " Patchwork
2024-02-14 0:18 ` ✗ CI.Patch_applied: " Patchwork
@ 2024-02-14 8:15 ` Dmitry Baryshkov
2024-02-14 17:17 ` Abhinav Kumar
2 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2024-02-14 8:15 UTC (permalink / raw
To: Abhinav Kumar
Cc: dri-devel, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Jani Nikula, Rodrigo Vivi,
Joonas Lahtinen, Tvrtko Ursulin, robdclark, freedreno, intel-gfx,
ville.syrjala, quic_jesszhan, linux-kernel, intel-xe
On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
> Lets move this to drm_dp_helper to achieve this.
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
My preference would be to have packing functions in
drivers/video/hdmi.c, as we already have
hdmi_audio_infoframe_pack_for_dp() there.
> ---
> drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_dp.c | 73 +----------------------
> include/drm/display/drm_dp_helper.h | 3 +
> 3 files changed, 84 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index b1ca3a1100da..066cfbbf7a91 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
> }
> EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
>
> +/**
> + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
> + * @vsc: vsc sdp initialized according to its purpose as defined in
> + * table 2-118 - table 2-120 in DP 1.4a specification
> + * @sdp: valid handle to the generic dp_sdp which will be packed
> + * @size: valid size of the passed sdp handle
> + *
> + * Returns length of sdp on success and error code on failure
> + */
> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> + struct dp_sdp *sdp, size_t size)
I know that you are just moving the function. Maybe there can be
patch#2, which drops the size argument? The struct dp_sdp already has
a defined size. The i915 driver just passes sizeof(sdp), which is more
or less useless.
> +{
> + size_t length = sizeof(struct dp_sdp);
> +
> + if (size < length)
> + return -ENOSPC;
> +
> + memset(sdp, 0, size);
> +
> + /*
> + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> + * VSC SDP Header Bytes
> + */
> + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
> + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
> + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
> + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> +
> + if (vsc->revision == 0x6) {
> + sdp->db[0] = 1;
> + sdp->db[3] = 1;
> + }
> +
> + /*
> + * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
> + * Format as per DP 1.4a spec and DP 2.0 respectively.
> + */
> + if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
> + goto out;
> +
> + /* VSC SDP Payload for DB16 through DB18 */
> + /* Pixel Encoding and Colorimetry Formats */
> + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
> + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
> +
> + switch (vsc->bpc) {
> + case 6:
> + /* 6bpc: 0x0 */
> + break;
> + case 8:
> + sdp->db[17] = 0x1; /* DB17[3:0] */
> + break;
> + case 10:
> + sdp->db[17] = 0x2;
> + break;
> + case 12:
> + sdp->db[17] = 0x3;
> + break;
> + case 16:
> + sdp->db[17] = 0x4;
> + break;
> + default:
> + WARN(1, "Missing case %d\n", vsc->bpc);
> + return -EINVAL;
> + }
> +
> + /* Dynamic Range and Component Bit Depth */
> + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
> + sdp->db[17] |= 0x80; /* DB17[7] */
> +
> + /* Content Type */
> + sdp->db[18] = vsc->content_type & 0x7;
> +
> +out:
> + return length;
> +}
> +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
> +
> /**
> * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
> * @dpcd: DisplayPort configuration data
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index f5ef95da5534..e94db51aeeb7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state,
> return false;
> }
>
> -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> - struct dp_sdp *sdp, size_t size)
> -{
> - size_t length = sizeof(struct dp_sdp);
> -
> - if (size < length)
> - return -ENOSPC;
> -
> - memset(sdp, 0, size);
> -
> - /*
> - * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> - * VSC SDP Header Bytes
> - */
> - sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
> - sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
> - sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
> - sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> -
> - if (vsc->revision == 0x6) {
> - sdp->db[0] = 1;
> - sdp->db[3] = 1;
> - }
> -
> - /*
> - * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
> - * Format as per DP 1.4a spec and DP 2.0 respectively.
> - */
> - if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
> - goto out;
> -
> - /* VSC SDP Payload for DB16 through DB18 */
> - /* Pixel Encoding and Colorimetry Formats */
> - sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
> - sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
> -
> - switch (vsc->bpc) {
> - case 6:
> - /* 6bpc: 0x0 */
> - break;
> - case 8:
> - sdp->db[17] = 0x1; /* DB17[3:0] */
> - break;
> - case 10:
> - sdp->db[17] = 0x2;
> - break;
> - case 12:
> - sdp->db[17] = 0x3;
> - break;
> - case 16:
> - sdp->db[17] = 0x4;
> - break;
> - default:
> - MISSING_CASE(vsc->bpc);
> - break;
> - }
> - /* Dynamic Range and Component Bit Depth */
> - if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
> - sdp->db[17] |= 0x80; /* DB17[7] */
> -
> - /* Content Type */
> - sdp->db[18] = vsc->content_type & 0x7;
> -
> -out:
> - return length;
> -}
> -
> static ssize_t
> intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915,
> const struct hdmi_drm_infoframe *drm_infoframe,
> @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder,
>
> switch (type) {
> case DP_SDP_VSC:
> - len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
> - sizeof(sdp));
> + len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
> + sizeof(sdp));
> break;
> case HDMI_PACKET_TYPE_GAMUT_METADATA:
> len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv,
> @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder,
> struct dp_sdp sdp = {};
> ssize_t len;
>
> - len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
> + len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
>
> if (drm_WARN_ON(&dev_priv->drm, len < 0))
> return;
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 863b2e7add29..f8db34a2f7a5 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
> int bpp_x16, unsigned long flags);
> int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
>
> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> + struct dp_sdp *sdp, size_t size);
> +
> #endif /* _DRM_DP_HELPER_H_ */
> --
> 2.34.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
2024-02-14 8:15 ` [PATCH] " Dmitry Baryshkov
@ 2024-02-14 17:17 ` Abhinav Kumar
2024-02-14 18:02 ` Ville Syrjälä
0 siblings, 1 reply; 8+ messages in thread
From: Abhinav Kumar @ 2024-02-14 17:17 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: dri-devel, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Jani Nikula, Rodrigo Vivi,
Joonas Lahtinen, Tvrtko Ursulin, robdclark, freedreno, intel-gfx,
ville.syrjala, quic_jesszhan, linux-kernel, intel-xe
On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote:
> On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
>> Lets move this to drm_dp_helper to achieve this.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> My preference would be to have packing functions in
> drivers/video/hdmi.c, as we already have
> hdmi_audio_infoframe_pack_for_dp() there.
>
My preference is drm_dp_helper because it already has some VSC SDP stuff
and after discussion with Ville on IRC, I decided to post it this way.
hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the
hdmi audio infoframe fields were re-used and packed into a DP SDP
thereby re-using the existing struct hdmi_audio_infoframe .
This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct
dp_sdp both of which had prior usages already in this file.
So it all adds up and makes sense to me to be in this file.
I will let the other DRM core maintainers comment on this.
Ville, Jani?
>> ---
>> drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++
>> drivers/gpu/drm/i915/display/intel_dp.c | 73 +----------------------
>> include/drm/display/drm_dp_helper.h | 3 +
>> 3 files changed, 84 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
>> index b1ca3a1100da..066cfbbf7a91 100644
>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
>> @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
>> }
>> EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
>>
>> +/**
>> + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
>> + * @vsc: vsc sdp initialized according to its purpose as defined in
>> + * table 2-118 - table 2-120 in DP 1.4a specification
>> + * @sdp: valid handle to the generic dp_sdp which will be packed
>> + * @size: valid size of the passed sdp handle
>> + *
>> + * Returns length of sdp on success and error code on failure
>> + */
>> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
>> + struct dp_sdp *sdp, size_t size)
>
> I know that you are just moving the function. Maybe there can be
> patch#2, which drops the size argument? The struct dp_sdp already has
> a defined size. The i915 driver just passes sizeof(sdp), which is more
> or less useless.
>
Yes this is a valid point, I also noticed this. I can post it on top of
this once we get an agreement and ack on this patch first.
>> +{
>> + size_t length = sizeof(struct dp_sdp);
>> +
>> + if (size < length)
>> + return -ENOSPC;
>> +
>> + memset(sdp, 0, size);
>> +
>> + /*
>> + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
>> + * VSC SDP Header Bytes
>> + */
>> + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
>> + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
>> + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
>> + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
>> +
>> + if (vsc->revision == 0x6) {
>> + sdp->db[0] = 1;
>> + sdp->db[3] = 1;
>> + }
>> +
>> + /*
>> + * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
>> + * Format as per DP 1.4a spec and DP 2.0 respectively.
>> + */
>> + if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
>> + goto out;
>> +
>> + /* VSC SDP Payload for DB16 through DB18 */
>> + /* Pixel Encoding and Colorimetry Formats */
>> + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
>> + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
>> +
>> + switch (vsc->bpc) {
>> + case 6:
>> + /* 6bpc: 0x0 */
>> + break;
>> + case 8:
>> + sdp->db[17] = 0x1; /* DB17[3:0] */
>> + break;
>> + case 10:
>> + sdp->db[17] = 0x2;
>> + break;
>> + case 12:
>> + sdp->db[17] = 0x3;
>> + break;
>> + case 16:
>> + sdp->db[17] = 0x4;
>> + break;
>> + default:
>> + WARN(1, "Missing case %d\n", vsc->bpc);
>> + return -EINVAL;
>> + }
>> +
>> + /* Dynamic Range and Component Bit Depth */
>> + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
>> + sdp->db[17] |= 0x80; /* DB17[7] */
>> +
>> + /* Content Type */
>> + sdp->db[18] = vsc->content_type & 0x7;
>> +
>> +out:
>> + return length;
>> +}
>> +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
>> +
>> /**
>> * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
>> * @dpcd: DisplayPort configuration data
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index f5ef95da5534..e94db51aeeb7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state,
>> return false;
>> }
>>
>> -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
>> - struct dp_sdp *sdp, size_t size)
>> -{
>> - size_t length = sizeof(struct dp_sdp);
>> -
>> - if (size < length)
>> - return -ENOSPC;
>> -
>> - memset(sdp, 0, size);
>> -
>> - /*
>> - * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
>> - * VSC SDP Header Bytes
>> - */
>> - sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
>> - sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
>> - sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
>> - sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
>> -
>> - if (vsc->revision == 0x6) {
>> - sdp->db[0] = 1;
>> - sdp->db[3] = 1;
>> - }
>> -
>> - /*
>> - * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
>> - * Format as per DP 1.4a spec and DP 2.0 respectively.
>> - */
>> - if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
>> - goto out;
>> -
>> - /* VSC SDP Payload for DB16 through DB18 */
>> - /* Pixel Encoding and Colorimetry Formats */
>> - sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
>> - sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
>> -
>> - switch (vsc->bpc) {
>> - case 6:
>> - /* 6bpc: 0x0 */
>> - break;
>> - case 8:
>> - sdp->db[17] = 0x1; /* DB17[3:0] */
>> - break;
>> - case 10:
>> - sdp->db[17] = 0x2;
>> - break;
>> - case 12:
>> - sdp->db[17] = 0x3;
>> - break;
>> - case 16:
>> - sdp->db[17] = 0x4;
>> - break;
>> - default:
>> - MISSING_CASE(vsc->bpc);
>> - break;
>> - }
>> - /* Dynamic Range and Component Bit Depth */
>> - if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
>> - sdp->db[17] |= 0x80; /* DB17[7] */
>> -
>> - /* Content Type */
>> - sdp->db[18] = vsc->content_type & 0x7;
>> -
>> -out:
>> - return length;
>> -}
>> -
>> static ssize_t
>> intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915,
>> const struct hdmi_drm_infoframe *drm_infoframe,
>> @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder,
>>
>> switch (type) {
>> case DP_SDP_VSC:
>> - len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
>> - sizeof(sdp));
>> + len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
>> + sizeof(sdp));
>> break;
>> case HDMI_PACKET_TYPE_GAMUT_METADATA:
>> len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv,
>> @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder,
>> struct dp_sdp sdp = {};
>> ssize_t len;
>>
>> - len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
>> + len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
>>
>> if (drm_WARN_ON(&dev_priv->drm, len < 0))
>> return;
>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
>> index 863b2e7add29..f8db34a2f7a5 100644
>> --- a/include/drm/display/drm_dp_helper.h
>> +++ b/include/drm/display/drm_dp_helper.h
>> @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
>> int bpp_x16, unsigned long flags);
>> int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
>>
>> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
>> + struct dp_sdp *sdp, size_t size);
>> +
>> #endif /* _DRM_DP_HELPER_H_ */
>> --
>> 2.34.1
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
2024-02-14 17:17 ` Abhinav Kumar
@ 2024-02-14 18:02 ` Ville Syrjälä
2024-02-14 18:08 ` Abhinav Kumar
0 siblings, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2024-02-14 18:02 UTC (permalink / raw
To: Abhinav Kumar
Cc: Dmitry Baryshkov, dri-devel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Jani Nikula,
Rodrigo Vivi, Joonas Lahtinen, Tvrtko Ursulin, robdclark,
freedreno, intel-gfx, quic_jesszhan, linux-kernel, intel-xe
On Wed, Feb 14, 2024 at 09:17:34AM -0800, Abhinav Kumar wrote:
>
>
> On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote:
> > On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
> >> Lets move this to drm_dp_helper to achieve this.
> >>
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >
> > My preference would be to have packing functions in
> > drivers/video/hdmi.c, as we already have
> > hdmi_audio_infoframe_pack_for_dp() there.
> >
>
> My preference is drm_dp_helper because it already has some VSC SDP stuff
> and after discussion with Ville on IRC, I decided to post it this way.
>
> hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the
> hdmi audio infoframe fields were re-used and packed into a DP SDP
> thereby re-using the existing struct hdmi_audio_infoframe .
>
> This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct
> dp_sdp both of which had prior usages already in this file.
>
> So it all adds up and makes sense to me to be in this file.
>
> I will let the other DRM core maintainers comment on this.
>
> Ville, Jani?
Yeah, I'm not sure bloating the (poorly named) hdmi.c with all
SDP stuff is a great idea. Since other related stuff already
lives in the drm_dp_helper.c that seems reasonable to me at this
time. And if we get a decent amount of this then probably all
DP SDP stuff should be extracted into its own file.
There are of course a few overlaps here andthere (the audio SDP
I guess, and the CTA infoframe SDP). But I'm not sure that actually
needs any SDP specific stuff in hdmi.c, or could we just let hdmi.c
deal with the actual CTA-861 stuff and then have the DP SDP code
wrap that up in its own thing externally? Dunno, haven't really
looked at the details.
>
> >> ---
> >> drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++
> >> drivers/gpu/drm/i915/display/intel_dp.c | 73 +----------------------
> >> include/drm/display/drm_dp_helper.h | 3 +
> >> 3 files changed, 84 insertions(+), 70 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> >> index b1ca3a1100da..066cfbbf7a91 100644
> >> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> >> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> >> @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
> >> }
> >> EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
> >>
> >> +/**
> >> + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
> >> + * @vsc: vsc sdp initialized according to its purpose as defined in
> >> + * table 2-118 - table 2-120 in DP 1.4a specification
> >> + * @sdp: valid handle to the generic dp_sdp which will be packed
> >> + * @size: valid size of the passed sdp handle
> >> + *
> >> + * Returns length of sdp on success and error code on failure
> >> + */
> >> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> >> + struct dp_sdp *sdp, size_t size)
> >
> > I know that you are just moving the function. Maybe there can be
> > patch#2, which drops the size argument? The struct dp_sdp already has
> > a defined size. The i915 driver just passes sizeof(sdp), which is more
> > or less useless.
> >
>
> Yes this is a valid point, I also noticed this. I can post it on top of
> this once we get an agreement and ack on this patch first.
>
> >> +{
> >> + size_t length = sizeof(struct dp_sdp);
> >> +
> >> + if (size < length)
> >> + return -ENOSPC;
> >> +
> >> + memset(sdp, 0, size);
> >> +
> >> + /*
> >> + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> >> + * VSC SDP Header Bytes
> >> + */
> >> + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
> >> + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
> >> + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
> >> + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> >> +
> >> + if (vsc->revision == 0x6) {
> >> + sdp->db[0] = 1;
> >> + sdp->db[3] = 1;
> >> + }
> >> +
> >> + /*
> >> + * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
> >> + * Format as per DP 1.4a spec and DP 2.0 respectively.
> >> + */
> >> + if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
> >> + goto out;
> >> +
> >> + /* VSC SDP Payload for DB16 through DB18 */
> >> + /* Pixel Encoding and Colorimetry Formats */
> >> + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
> >> + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
> >> +
> >> + switch (vsc->bpc) {
> >> + case 6:
> >> + /* 6bpc: 0x0 */
> >> + break;
> >> + case 8:
> >> + sdp->db[17] = 0x1; /* DB17[3:0] */
> >> + break;
> >> + case 10:
> >> + sdp->db[17] = 0x2;
> >> + break;
> >> + case 12:
> >> + sdp->db[17] = 0x3;
> >> + break;
> >> + case 16:
> >> + sdp->db[17] = 0x4;
> >> + break;
> >> + default:
> >> + WARN(1, "Missing case %d\n", vsc->bpc);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* Dynamic Range and Component Bit Depth */
> >> + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
> >> + sdp->db[17] |= 0x80; /* DB17[7] */
> >> +
> >> + /* Content Type */
> >> + sdp->db[18] = vsc->content_type & 0x7;
> >> +
> >> +out:
> >> + return length;
> >> +}
> >> +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
> >> +
> >> /**
> >> * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
> >> * @dpcd: DisplayPort configuration data
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >> index f5ef95da5534..e94db51aeeb7 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> @@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state,
> >> return false;
> >> }
> >>
> >> -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> >> - struct dp_sdp *sdp, size_t size)
> >> -{
> >> - size_t length = sizeof(struct dp_sdp);
> >> -
> >> - if (size < length)
> >> - return -ENOSPC;
> >> -
> >> - memset(sdp, 0, size);
> >> -
> >> - /*
> >> - * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> >> - * VSC SDP Header Bytes
> >> - */
> >> - sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
> >> - sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
> >> - sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
> >> - sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> >> -
> >> - if (vsc->revision == 0x6) {
> >> - sdp->db[0] = 1;
> >> - sdp->db[3] = 1;
> >> - }
> >> -
> >> - /*
> >> - * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
> >> - * Format as per DP 1.4a spec and DP 2.0 respectively.
> >> - */
> >> - if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
> >> - goto out;
> >> -
> >> - /* VSC SDP Payload for DB16 through DB18 */
> >> - /* Pixel Encoding and Colorimetry Formats */
> >> - sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
> >> - sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
> >> -
> >> - switch (vsc->bpc) {
> >> - case 6:
> >> - /* 6bpc: 0x0 */
> >> - break;
> >> - case 8:
> >> - sdp->db[17] = 0x1; /* DB17[3:0] */
> >> - break;
> >> - case 10:
> >> - sdp->db[17] = 0x2;
> >> - break;
> >> - case 12:
> >> - sdp->db[17] = 0x3;
> >> - break;
> >> - case 16:
> >> - sdp->db[17] = 0x4;
> >> - break;
> >> - default:
> >> - MISSING_CASE(vsc->bpc);
> >> - break;
> >> - }
> >> - /* Dynamic Range and Component Bit Depth */
> >> - if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
> >> - sdp->db[17] |= 0x80; /* DB17[7] */
> >> -
> >> - /* Content Type */
> >> - sdp->db[18] = vsc->content_type & 0x7;
> >> -
> >> -out:
> >> - return length;
> >> -}
> >> -
> >> static ssize_t
> >> intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915,
> >> const struct hdmi_drm_infoframe *drm_infoframe,
> >> @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder,
> >>
> >> switch (type) {
> >> case DP_SDP_VSC:
> >> - len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
> >> - sizeof(sdp));
> >> + len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
> >> + sizeof(sdp));
> >> break;
> >> case HDMI_PACKET_TYPE_GAMUT_METADATA:
> >> len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv,
> >> @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder,
> >> struct dp_sdp sdp = {};
> >> ssize_t len;
> >>
> >> - len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
> >> + len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
> >>
> >> if (drm_WARN_ON(&dev_priv->drm, len < 0))
> >> return;
> >> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> >> index 863b2e7add29..f8db34a2f7a5 100644
> >> --- a/include/drm/display/drm_dp_helper.h
> >> +++ b/include/drm/display/drm_dp_helper.h
> >> @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
> >> int bpp_x16, unsigned long flags);
> >> int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
> >>
> >> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> >> + struct dp_sdp *sdp, size_t size);
> >> +
> >> #endif /* _DRM_DP_HELPER_H_ */
> >> --
> >> 2.34.1
> >>
> >
> >
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
2024-02-14 18:02 ` Ville Syrjälä
@ 2024-02-14 18:08 ` Abhinav Kumar
2024-02-14 19:10 ` Dmitry Baryshkov
0 siblings, 1 reply; 8+ messages in thread
From: Abhinav Kumar @ 2024-02-14 18:08 UTC (permalink / raw
To: Ville Syrjälä
Cc: Dmitry Baryshkov, dri-devel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Jani Nikula,
Rodrigo Vivi, Joonas Lahtinen, Tvrtko Ursulin, robdclark,
freedreno, intel-gfx, quic_jesszhan, linux-kernel, intel-xe
On 2/14/2024 10:02 AM, Ville Syrjälä wrote:
> On Wed, Feb 14, 2024 at 09:17:34AM -0800, Abhinav Kumar wrote:
>>
>>
>> On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote:
>>> On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
>>>> Lets move this to drm_dp_helper to achieve this.
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>
>>> My preference would be to have packing functions in
>>> drivers/video/hdmi.c, as we already have
>>> hdmi_audio_infoframe_pack_for_dp() there.
>>>
>>
>> My preference is drm_dp_helper because it already has some VSC SDP stuff
>> and after discussion with Ville on IRC, I decided to post it this way.
>>
>> hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the
>> hdmi audio infoframe fields were re-used and packed into a DP SDP
>> thereby re-using the existing struct hdmi_audio_infoframe .
>>
>> This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct
>> dp_sdp both of which had prior usages already in this file.
>>
>> So it all adds up and makes sense to me to be in this file.
>>
>> I will let the other DRM core maintainers comment on this.
>>
>> Ville, Jani?
>
> Yeah, I'm not sure bloating the (poorly named) hdmi.c with all
> SDP stuff is a great idea. Since other related stuff already
> lives in the drm_dp_helper.c that seems reasonable to me at this
> time. And if we get a decent amount of this then probably all
> DP SDP stuff should be extracted into its own file.
>
Yes, thanks.
> There are of course a few overlaps here andthere (the audio SDP
> I guess, and the CTA infoframe SDP). But I'm not sure that actually
> needs any SDP specific stuff in hdmi.c, or could we just let hdmi.c
> deal with the actual CTA-861 stuff and then have the DP SDP code
> wrap that up in its own thing externally? Dunno, haven't really
> looked at the details.
>
Thats a good way to look at it. this packing is from DP spec and not CTA
so makes more sense to be in this file.
In that case, R-b?
>>
>>>> ---
>>>> drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++
>>>> drivers/gpu/drm/i915/display/intel_dp.c | 73 +----------------------
>>>> include/drm/display/drm_dp_helper.h | 3 +
>>>> 3 files changed, 84 insertions(+), 70 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
>>>> index b1ca3a1100da..066cfbbf7a91 100644
>>>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
>>>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
>>>> @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
>>>> }
>>>> EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
>>>>
>>>> +/**
>>>> + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
>>>> + * @vsc: vsc sdp initialized according to its purpose as defined in
>>>> + * table 2-118 - table 2-120 in DP 1.4a specification
>>>> + * @sdp: valid handle to the generic dp_sdp which will be packed
>>>> + * @size: valid size of the passed sdp handle
>>>> + *
>>>> + * Returns length of sdp on success and error code on failure
>>>> + */
>>>> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
>>>> + struct dp_sdp *sdp, size_t size)
>>>
>>> I know that you are just moving the function. Maybe there can be
>>> patch#2, which drops the size argument? The struct dp_sdp already has
>>> a defined size. The i915 driver just passes sizeof(sdp), which is more
>>> or less useless.
>>>
>>
>> Yes this is a valid point, I also noticed this. I can post it on top of
>> this once we get an agreement and ack on this patch first.
>>
>>>> +{
>>>> + size_t length = sizeof(struct dp_sdp);
>>>> +
>>>> + if (size < length)
>>>> + return -ENOSPC;
>>>> +
>>>> + memset(sdp, 0, size);
>>>> +
>>>> + /*
>>>> + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
>>>> + * VSC SDP Header Bytes
>>>> + */
>>>> + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
>>>> + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
>>>> + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
>>>> + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
>>>> +
>>>> + if (vsc->revision == 0x6) {
>>>> + sdp->db[0] = 1;
>>>> + sdp->db[3] = 1;
>>>> + }
>>>> +
>>>> + /*
>>>> + * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
>>>> + * Format as per DP 1.4a spec and DP 2.0 respectively.
>>>> + */
>>>> + if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
>>>> + goto out;
>>>> +
>>>> + /* VSC SDP Payload for DB16 through DB18 */
>>>> + /* Pixel Encoding and Colorimetry Formats */
>>>> + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
>>>> + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
>>>> +
>>>> + switch (vsc->bpc) {
>>>> + case 6:
>>>> + /* 6bpc: 0x0 */
>>>> + break;
>>>> + case 8:
>>>> + sdp->db[17] = 0x1; /* DB17[3:0] */
>>>> + break;
>>>> + case 10:
>>>> + sdp->db[17] = 0x2;
>>>> + break;
>>>> + case 12:
>>>> + sdp->db[17] = 0x3;
>>>> + break;
>>>> + case 16:
>>>> + sdp->db[17] = 0x4;
>>>> + break;
>>>> + default:
>>>> + WARN(1, "Missing case %d\n", vsc->bpc);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + /* Dynamic Range and Component Bit Depth */
>>>> + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
>>>> + sdp->db[17] |= 0x80; /* DB17[7] */
>>>> +
>>>> + /* Content Type */
>>>> + sdp->db[18] = vsc->content_type & 0x7;
>>>> +
>>>> +out:
>>>> + return length;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
>>>> +
>>>> /**
>>>> * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
>>>> * @dpcd: DisplayPort configuration data
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> index f5ef95da5534..e94db51aeeb7 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> @@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state,
>>>> return false;
>>>> }
>>>>
>>>> -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
>>>> - struct dp_sdp *sdp, size_t size)
>>>> -{
>>>> - size_t length = sizeof(struct dp_sdp);
>>>> -
>>>> - if (size < length)
>>>> - return -ENOSPC;
>>>> -
>>>> - memset(sdp, 0, size);
>>>> -
>>>> - /*
>>>> - * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
>>>> - * VSC SDP Header Bytes
>>>> - */
>>>> - sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
>>>> - sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
>>>> - sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
>>>> - sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
>>>> -
>>>> - if (vsc->revision == 0x6) {
>>>> - sdp->db[0] = 1;
>>>> - sdp->db[3] = 1;
>>>> - }
>>>> -
>>>> - /*
>>>> - * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
>>>> - * Format as per DP 1.4a spec and DP 2.0 respectively.
>>>> - */
>>>> - if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
>>>> - goto out;
>>>> -
>>>> - /* VSC SDP Payload for DB16 through DB18 */
>>>> - /* Pixel Encoding and Colorimetry Formats */
>>>> - sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
>>>> - sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
>>>> -
>>>> - switch (vsc->bpc) {
>>>> - case 6:
>>>> - /* 6bpc: 0x0 */
>>>> - break;
>>>> - case 8:
>>>> - sdp->db[17] = 0x1; /* DB17[3:0] */
>>>> - break;
>>>> - case 10:
>>>> - sdp->db[17] = 0x2;
>>>> - break;
>>>> - case 12:
>>>> - sdp->db[17] = 0x3;
>>>> - break;
>>>> - case 16:
>>>> - sdp->db[17] = 0x4;
>>>> - break;
>>>> - default:
>>>> - MISSING_CASE(vsc->bpc);
>>>> - break;
>>>> - }
>>>> - /* Dynamic Range and Component Bit Depth */
>>>> - if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
>>>> - sdp->db[17] |= 0x80; /* DB17[7] */
>>>> -
>>>> - /* Content Type */
>>>> - sdp->db[18] = vsc->content_type & 0x7;
>>>> -
>>>> -out:
>>>> - return length;
>>>> -}
>>>> -
>>>> static ssize_t
>>>> intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915,
>>>> const struct hdmi_drm_infoframe *drm_infoframe,
>>>> @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder,
>>>>
>>>> switch (type) {
>>>> case DP_SDP_VSC:
>>>> - len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
>>>> - sizeof(sdp));
>>>> + len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
>>>> + sizeof(sdp));
>>>> break;
>>>> case HDMI_PACKET_TYPE_GAMUT_METADATA:
>>>> len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv,
>>>> @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder,
>>>> struct dp_sdp sdp = {};
>>>> ssize_t len;
>>>>
>>>> - len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
>>>> + len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
>>>>
>>>> if (drm_WARN_ON(&dev_priv->drm, len < 0))
>>>> return;
>>>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
>>>> index 863b2e7add29..f8db34a2f7a5 100644
>>>> --- a/include/drm/display/drm_dp_helper.h
>>>> +++ b/include/drm/display/drm_dp_helper.h
>>>> @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
>>>> int bpp_x16, unsigned long flags);
>>>> int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
>>>>
>>>> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
>>>> + struct dp_sdp *sdp, size_t size);
>>>> +
>>>> #endif /* _DRM_DP_HELPER_H_ */
>>>> --
>>>> 2.34.1
>>>>
>>>
>>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
2024-02-14 18:08 ` Abhinav Kumar
@ 2024-02-14 19:10 ` Dmitry Baryshkov
0 siblings, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2024-02-14 19:10 UTC (permalink / raw
To: Abhinav Kumar
Cc: Ville Syrjälä, dri-devel, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter,
Jani Nikula, Rodrigo Vivi, Joonas Lahtinen, Tvrtko Ursulin,
robdclark, freedreno, intel-gfx, quic_jesszhan, linux-kernel,
intel-xe
On Wed, 14 Feb 2024 at 20:08, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 2/14/2024 10:02 AM, Ville Syrjälä wrote:
> > On Wed, Feb 14, 2024 at 09:17:34AM -0800, Abhinav Kumar wrote:
> >>
> >>
> >> On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote:
> >>> On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
> >>>> Lets move this to drm_dp_helper to achieve this.
> >>>>
> >>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>>
> >>> My preference would be to have packing functions in
> >>> drivers/video/hdmi.c, as we already have
> >>> hdmi_audio_infoframe_pack_for_dp() there.
> >>>
> >>
> >> My preference is drm_dp_helper because it already has some VSC SDP stuff
> >> and after discussion with Ville on IRC, I decided to post it this way.
> >>
> >> hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the
> >> hdmi audio infoframe fields were re-used and packed into a DP SDP
> >> thereby re-using the existing struct hdmi_audio_infoframe .
> >>
> >> This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct
> >> dp_sdp both of which had prior usages already in this file.
> >>
> >> So it all adds up and makes sense to me to be in this file.
> >>
> >> I will let the other DRM core maintainers comment on this.
> >>
> >> Ville, Jani?
> >
> > Yeah, I'm not sure bloating the (poorly named) hdmi.c with all
> > SDP stuff is a great idea. Since other related stuff already
> > lives in the drm_dp_helper.c that seems reasonable to me at this
> > time. And if we get a decent amount of this then probably all
> > DP SDP stuff should be extracted into its own file.
> >
>
> Yes, thanks.
>
> > There are of course a few overlaps here andthere (the audio SDP
> > I guess, and the CTA infoframe SDP). But I'm not sure that actually
> > needs any SDP specific stuff in hdmi.c, or could we just let hdmi.c
> > deal with the actual CTA-861 stuff and then have the DP SDP code
> > wrap that up in its own thing externally? Dunno, haven't really
> > looked at the details.
> >
>
> Thats a good way to look at it. this packing is from DP spec and not CTA
> so makes more sense to be in this file.
>
> In that case, R-b?
>
> >>
> >>>> ---
> >>>> drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++
> >>>> drivers/gpu/drm/i915/display/intel_dp.c | 73 +----------------------
> >>>> include/drm/display/drm_dp_helper.h | 3 +
> >>>> 3 files changed, 84 insertions(+), 70 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> >>>> index b1ca3a1100da..066cfbbf7a91 100644
> >>>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> >>>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> >>>> @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
> >>>> }
> >>>> EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
> >>>>
> >>>> +/**
> >>>> + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
> >>>> + * @vsc: vsc sdp initialized according to its purpose as defined in
> >>>> + * table 2-118 - table 2-120 in DP 1.4a specification
> >>>> + * @sdp: valid handle to the generic dp_sdp which will be packed
> >>>> + * @size: valid size of the passed sdp handle
> >>>> + *
> >>>> + * Returns length of sdp on success and error code on failure
> >>>> + */
> >>>> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> >>>> + struct dp_sdp *sdp, size_t size)
> >>>
> >>> I know that you are just moving the function. Maybe there can be
> >>> patch#2, which drops the size argument? The struct dp_sdp already has
> >>> a defined size. The i915 driver just passes sizeof(sdp), which is more
> >>> or less useless.
> >>>
> >>
> >> Yes this is a valid point, I also noticed this. I can post it on top of
> >> this once we get an agreement and ack on this patch first.
> >>
From my side, with the promise of the size fixup.
Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-02-14 19:10 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-13 23:45 [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper Abhinav Kumar
2024-02-14 0:11 ` ✗ Fi.CI.BUILD: failure for " Patchwork
2024-02-14 0:18 ` ✗ CI.Patch_applied: " Patchwork
2024-02-14 8:15 ` [PATCH] " Dmitry Baryshkov
2024-02-14 17:17 ` Abhinav Kumar
2024-02-14 18:02 ` Ville Syrjälä
2024-02-14 18:08 ` Abhinav Kumar
2024-02-14 19:10 ` Dmitry Baryshkov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.