Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On Wed, 14 Feb 2024 at 20:08, Abhinav Kumar 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 > >>> 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 > >>> > >>> 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 -- With best wishes Dmitry
Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
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 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
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;
+
Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
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
> > 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
> >
> > 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; /*
Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote:
On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar 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
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
Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar 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
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; /*
[PATCH] 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.
Signed-off-by: Abhinav Kumar
---
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] |=
