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] |=