[v2] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper

  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.

changes in v2:
	- rebased on top of drm-tip

Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
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 | 71 +---------------------
 include/drm/display/drm_dp_helper.h     |  3 +
 3 files changed, 83 insertions(+), 69 deletions(-)
  

On Thu, 15 Feb 2024, 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.
>
> changes in v2:
> 	- rebased on top of drm-tip
>
> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

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

> ---
>  drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c | 71 +---------------------
>  include/drm/display/drm_dp_helper.h     |  3 +
>  3 files changed, 83 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 8d6ce46471ae..6c91f400ecb1 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2913,6 +2913,84 @@ void drm_dp_vsc_sdp_log(struct drm_printer *p, const struct drm_dp_vsc_sdp *vsc)
>  }
>  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 217196196e50..a9458df475e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4089,73 +4089,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,
> @@ -4248,8 +4181,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,
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index d02014a87f12..8474504d4c88 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -812,4 +812,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_ */
  

On Thu, 15 Feb 2024 at 21:08, 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.
>
> changes in v2:
>         - rebased on top of drm-tip
>
> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

v1 had an explicit comment before the ack:

>    From my side, with the promise of the size fixup.

However I observe neither a second patch removing the size argument
nor it being dropped as a part of this patch.

> 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 | 71 +---------------------
>  include/drm/display/drm_dp_helper.h     |  3 +
>  3 files changed, 83 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 8d6ce46471ae..6c91f400ecb1 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2913,6 +2913,84 @@ void drm_dp_vsc_sdp_log(struct drm_printer *p, const struct drm_dp_vsc_sdp *vsc)
>  }
>  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 217196196e50..a9458df475e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4089,73 +4089,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,
> @@ -4248,8 +4181,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,
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index d02014a87f12..8474504d4c88 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -812,4 +812,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
>
  

On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote:
> On Thu, 15 Feb 2024 at 21:08, 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.
>>
>> changes in v2:
>>          - rebased on top of drm-tip
>>
>> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> v1 had an explicit comment before the ack:
> 

Yes, I remember the comment. I did not make any changes to v2 other than 
just rebasing it on drm-tip to get the ack from i915 folks.

>>     From my side, with the promise of the size fixup.
> 
> However I observe neither a second patch removing the size argument
> nor it being dropped as a part of this patch.
> 

Yes, now that in v2 we got the ack for this patch, I can spin a v3 with 
the addition of the next patch to remove the size OR as another series 
so as to not block the main series which needs this patch.

I would prefer the latter.

>> 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 | 71 +---------------------
>>   include/drm/display/drm_dp_helper.h     |  3 +
>>   3 files changed, 83 insertions(+), 69 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
>> index 8d6ce46471ae..6c91f400ecb1 100644
>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
>> @@ -2913,6 +2913,84 @@ void drm_dp_vsc_sdp_log(struct drm_printer *p, const struct drm_dp_vsc_sdp *vsc)
>>   }
>>   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 217196196e50..a9458df475e2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -4089,73 +4089,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,
>> @@ -4248,8 +4181,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,
>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
>> index d02014a87f12..8474504d4c88 100644
>> --- a/include/drm/display/drm_dp_helper.h
>> +++ b/include/drm/display/drm_dp_helper.h
>> @@ -812,4 +812,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
>>
> 
>
  

On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote:
> > On Thu, 15 Feb 2024 at 21:08, 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.
> >>
> >> changes in v2:
> >>          - rebased on top of drm-tip
> >>
> >> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >
> > v1 had an explicit comment before the ack:
> >
>
> Yes, I remember the comment. I did not make any changes to v2 other than
> just rebasing it on drm-tip to get the ack from i915 folks.
>
> >>     From my side, with the promise of the size fixup.
> >
> > However I observe neither a second patch removing the size argument
> > nor it being dropped as a part of this patch.
> >
>
> Yes, now that in v2 we got the ack for this patch, I can spin a v3 with
> the addition of the next patch to remove the size OR as another series
> so as to not block the main series which needs this patch.
>
> I would prefer the latter.

It doesn't work this way. The comment should have been fixed for v2.
  

On 2/20/2024 11:05 AM, Dmitry Baryshkov wrote:
> On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote:
>>> On Thu, 15 Feb 2024 at 21:08, 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.
>>>>
>>>> changes in v2:
>>>>           - rebased on top of drm-tip
>>>>
>>>> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> v1 had an explicit comment before the ack:
>>>
>>
>> Yes, I remember the comment. I did not make any changes to v2 other than
>> just rebasing it on drm-tip to get the ack from i915 folks.
>>
>>>>      From my side, with the promise of the size fixup.
>>>
>>> However I observe neither a second patch removing the size argument
>>> nor it being dropped as a part of this patch.
>>>
>>
>> Yes, now that in v2 we got the ack for this patch, I can spin a v3 with
>> the addition of the next patch to remove the size OR as another series
>> so as to not block the main series which needs this patch.
>>
>> I would prefer the latter.
> 
> It doesn't work this way. The comment should have been fixed for v2.
> 

Ack, I can post a v3 now by adding the removal in patch 2 of this series.

> 
>
  

On Tue, 20 Feb 2024 at 21:09, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 2/20/2024 11:05 AM, Dmitry Baryshkov wrote:
> > On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote:
> >>> On Thu, 15 Feb 2024 at 21:08, 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.
> >>>>
> >>>> changes in v2:
> >>>>           - rebased on top of drm-tip
> >>>>
> >>>> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>
> >>> v1 had an explicit comment before the ack:
> >>>
> >>
> >> Yes, I remember the comment. I did not make any changes to v2 other than
> >> just rebasing it on drm-tip to get the ack from i915 folks.
> >>
> >>>>      From my side, with the promise of the size fixup.
> >>>
> >>> However I observe neither a second patch removing the size argument
> >>> nor it being dropped as a part of this patch.
> >>>
> >>
> >> Yes, now that in v2 we got the ack for this patch, I can spin a v3 with
> >> the addition of the next patch to remove the size OR as another series
> >> so as to not block the main series which needs this patch.
> >>
> >> I would prefer the latter.
> >
> > It doesn't work this way. The comment should have been fixed for v2.
> >
>
> Ack, I can post a v3 now by adding the removal in patch 2 of this series.

Yes, please.
  

On Tue, 20 Feb 2024 at 21:05, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >
> >
> >
> > On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote:
> > > On Thu, 15 Feb 2024 at 21:08, 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.
> > >>
> > >> changes in v2:
> > >>          - rebased on top of drm-tip
> > >>
> > >> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >
> > > v1 had an explicit comment before the ack:
> > >
> >
> > Yes, I remember the comment. I did not make any changes to v2 other than
> > just rebasing it on drm-tip to get the ack from i915 folks.
> >
> > >>     From my side, with the promise of the size fixup.
> > >
> > > However I observe neither a second patch removing the size argument
> > > nor it being dropped as a part of this patch.
> > >
> >
> > Yes, now that in v2 we got the ack for this patch, I can spin a v3 with
> > the addition of the next patch to remove the size OR as another series
> > so as to not block the main series which needs this patch.
> >
> > I would prefer the latter.
>
> It doesn't work this way. The comment should have been fixed for v2.

This probably deserves some explanation. Currently there is only one
user of this function. So it is easy to fix it. Once there are several
users, you have to fix all of them at the same time, patching
different drm subtrees. That complicates the life of maintainers.
  

On 2/20/2024 11:20 AM, Dmitry Baryshkov wrote:
> On Tue, 20 Feb 2024 at 21:05, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote:
>>>> On Thu, 15 Feb 2024 at 21:08, 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.
>>>>>
>>>>> changes in v2:
>>>>>           - rebased on top of drm-tip
>>>>>
>>>>> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>
>>>> v1 had an explicit comment before the ack:
>>>>
>>>
>>> Yes, I remember the comment. I did not make any changes to v2 other than
>>> just rebasing it on drm-tip to get the ack from i915 folks.
>>>
>>>>>      From my side, with the promise of the size fixup.
>>>>
>>>> However I observe neither a second patch removing the size argument
>>>> nor it being dropped as a part of this patch.
>>>>
>>>
>>> Yes, now that in v2 we got the ack for this patch, I can spin a v3 with
>>> the addition of the next patch to remove the size OR as another series
>>> so as to not block the main series which needs this patch.
>>>
>>> I would prefer the latter.
>>
>> It doesn't work this way. The comment should have been fixed for v2.
> 
> This probably deserves some explanation. Currently there is only one
> user of this function. So it is easy to fix it. Once there are several
> users, you have to fix all of them at the same time, patching
> different drm subtrees. That complicates the life of maintainers.
> 

Yes, understood. Its easier to fix it now if its really needed.

Actually, I think the reason the size was passed was to make sure
a valid struct dp_sdp *sdp was being passed.

If we drop the size, we need to have a if (!sdp) check as there is a 
memset followed by dereference.

So maybe, if we want to keep this API protected, its not too bad to have?
  

On Tue, Feb 20, 2024 at 11:27:18AM -0800, Abhinav Kumar wrote:
> 
> 
> On 2/20/2024 11:20 AM, Dmitry Baryshkov wrote:
> > On Tue, 20 Feb 2024 at 21:05, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> >>
> >> On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>
> >>>
> >>>
> >>> On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote:
> >>>> On Thu, 15 Feb 2024 at 21:08, 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.
> >>>>>
> >>>>> changes in v2:
> >>>>>           - rebased on top of drm-tip
> >>>>>
> >>>>> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>
> >>>> v1 had an explicit comment before the ack:
> >>>>
> >>>
> >>> Yes, I remember the comment. I did not make any changes to v2 other than
> >>> just rebasing it on drm-tip to get the ack from i915 folks.
> >>>
> >>>>>      From my side, with the promise of the size fixup.
> >>>>
> >>>> However I observe neither a second patch removing the size argument
> >>>> nor it being dropped as a part of this patch.
> >>>>
> >>>
> >>> Yes, now that in v2 we got the ack for this patch, I can spin a v3 with
> >>> the addition of the next patch to remove the size OR as another series
> >>> so as to not block the main series which needs this patch.
> >>>
> >>> I would prefer the latter.
> >>
> >> It doesn't work this way. The comment should have been fixed for v2.
> > 
> > This probably deserves some explanation. Currently there is only one
> > user of this function. So it is easy to fix it. Once there are several
> > users, you have to fix all of them at the same time, patching
> > different drm subtrees. That complicates the life of maintainers.
> > 
> 
> Yes, understood. Its easier to fix it now if its really needed.
> 
> Actually, I think the reason the size was passed was to make sure
> a valid struct dp_sdp *sdp was being passed.

The size is supposed to be the size of *hardware* buffer where this
gets written into. But looks like this wasn't done correctly when
the code was copy-pasted from the HDMI inforframe code.
  

On 2/20/2024 11:41 AM, Ville Syrjälä wrote:
> On Tue, Feb 20, 2024 at 11:27:18AM -0800, Abhinav Kumar wrote:
>>
>>
>> On 2/20/2024 11:20 AM, Dmitry Baryshkov wrote:
>>> On Tue, 20 Feb 2024 at 21:05, Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>
>>>> On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote:
>>>>>> On Thu, 15 Feb 2024 at 21:08, 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.
>>>>>>>
>>>>>>> changes in v2:
>>>>>>>            - rebased on top of drm-tip
>>>>>>>
>>>>>>> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>
>>>>>> v1 had an explicit comment before the ack:
>>>>>>
>>>>>
>>>>> Yes, I remember the comment. I did not make any changes to v2 other than
>>>>> just rebasing it on drm-tip to get the ack from i915 folks.
>>>>>
>>>>>>>       From my side, with the promise of the size fixup.
>>>>>>
>>>>>> However I observe neither a second patch removing the size argument
>>>>>> nor it being dropped as a part of this patch.
>>>>>>
>>>>>
>>>>> Yes, now that in v2 we got the ack for this patch, I can spin a v3 with
>>>>> the addition of the next patch to remove the size OR as another series
>>>>> so as to not block the main series which needs this patch.
>>>>>
>>>>> I would prefer the latter.
>>>>
>>>> It doesn't work this way. The comment should have been fixed for v2.
>>>
>>> This probably deserves some explanation. Currently there is only one
>>> user of this function. So it is easy to fix it. Once there are several
>>> users, you have to fix all of them at the same time, patching
>>> different drm subtrees. That complicates the life of maintainers.
>>>
>>
>> Yes, understood. Its easier to fix it now if its really needed.
>>
>> Actually, I think the reason the size was passed was to make sure
>> a valid struct dp_sdp *sdp was being passed.
> 
> The size is supposed to be the size of *hardware* buffer where this
> gets written into. But looks like this wasn't done correctly when
> the code was copy-pasted from the HDMI inforframe code.
> 

Alright, in that case, let me post a patch to drop this and let me know 
if that works for you.