On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxxx>
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
