| Message ID | 20240213234513.2411604-1-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/dp: move intel_dp_vsc_sdp_pack() to generic helper | expand |
On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well. > Lets move this to drm_dp_helper to achieve this. > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> My preference would be to have packing functions in drivers/video/hdmi.c, as we already have hdmi_audio_infoframe_pack_for_dp() there. > --- > drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_dp.c | 73 +---------------------- > include/drm/display/drm_dp_helper.h | 3 + > 3 files changed, 84 insertions(+), 70 deletions(-) > > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c > index b1ca3a1100da..066cfbbf7a91 100644 > --- a/drivers/gpu/drm/display/drm_dp_helper.c > +++ b/drivers/gpu/drm/display/drm_dp_helper.c > @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev, > } > EXPORT_SYMBOL(drm_dp_vsc_sdp_log); > > +/** > + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp > + * @vsc: vsc sdp initialized according to its purpose as defined in > + * table 2-118 - table 2-120 in DP 1.4a specification > + * @sdp: valid handle to the generic dp_sdp which will be packed > + * @size: valid size of the passed sdp handle > + * > + * Returns length of sdp on success and error code on failure > + */ > +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, > + struct dp_sdp *sdp, size_t size) I know that you are just moving the function. Maybe there can be patch#2, which drops the size argument? The struct dp_sdp already has a defined size. The i915 driver just passes sizeof(sdp), which is more or less useless. > +{ > + size_t length = sizeof(struct dp_sdp); > + > + if (size < length) > + return -ENOSPC; > + > + memset(sdp, 0, size); > + > + /* > + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 > + * VSC SDP Header Bytes > + */ > + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */ > + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */ > + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */ > + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */ > + > + if (vsc->revision == 0x6) { > + sdp->db[0] = 1; > + sdp->db[3] = 1; > + } > + > + /* > + * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry > + * Format as per DP 1.4a spec and DP 2.0 respectively. > + */ > + if (!(vsc->revision == 0x5 || vsc->revision == 0x7)) > + goto out; > + > + /* VSC SDP Payload for DB16 through DB18 */ > + /* Pixel Encoding and Colorimetry Formats */ > + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */ > + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */ > + > + switch (vsc->bpc) { > + case 6: > + /* 6bpc: 0x0 */ > + break; > + case 8: > + sdp->db[17] = 0x1; /* DB17[3:0] */ > + break; > + case 10: > + sdp->db[17] = 0x2; > + break; > + case 12: > + sdp->db[17] = 0x3; > + break; > + case 16: > + sdp->db[17] = 0x4; > + break; > + default: > + WARN(1, "Missing case %d\n", vsc->bpc); > + return -EINVAL; > + } > + > + /* Dynamic Range and Component Bit Depth */ > + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA) > + sdp->db[17] |= 0x80; /* DB17[7] */ > + > + /* Content Type */ > + sdp->db[18] = vsc->content_type & 0x7; > + > +out: > + return length; > +} > +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack); > + > /** > * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON > * @dpcd: DisplayPort configuration data > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index f5ef95da5534..e94db51aeeb7 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state, > return false; > } > > -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, > - struct dp_sdp *sdp, size_t size) > -{ > - size_t length = sizeof(struct dp_sdp); > - > - if (size < length) > - return -ENOSPC; > - > - memset(sdp, 0, size); > - > - /* > - * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 > - * VSC SDP Header Bytes > - */ > - sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */ > - sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */ > - sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */ > - sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */ > - > - if (vsc->revision == 0x6) { > - sdp->db[0] = 1; > - sdp->db[3] = 1; > - } > - > - /* > - * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry > - * Format as per DP 1.4a spec and DP 2.0 respectively. > - */ > - if (!(vsc->revision == 0x5 || vsc->revision == 0x7)) > - goto out; > - > - /* VSC SDP Payload for DB16 through DB18 */ > - /* Pixel Encoding and Colorimetry Formats */ > - sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */ > - sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */ > - > - switch (vsc->bpc) { > - case 6: > - /* 6bpc: 0x0 */ > - break; > - case 8: > - sdp->db[17] = 0x1; /* DB17[3:0] */ > - break; > - case 10: > - sdp->db[17] = 0x2; > - break; > - case 12: > - sdp->db[17] = 0x3; > - break; > - case 16: > - sdp->db[17] = 0x4; > - break; > - default: > - MISSING_CASE(vsc->bpc); > - break; > - } > - /* Dynamic Range and Component Bit Depth */ > - if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA) > - sdp->db[17] |= 0x80; /* DB17[7] */ > - > - /* Content Type */ > - sdp->db[18] = vsc->content_type & 0x7; > - > -out: > - return length; > -} > - > static ssize_t > intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915, > const struct hdmi_drm_infoframe *drm_infoframe, > @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder, > > switch (type) { > case DP_SDP_VSC: > - len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp, > - sizeof(sdp)); > + len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp, > + sizeof(sdp)); > break; > case HDMI_PACKET_TYPE_GAMUT_METADATA: > len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv, > @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder, > struct dp_sdp sdp = {}; > ssize_t len; > > - len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp)); > + len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp)); > > if (drm_WARN_ON(&dev_priv->drm, len < 0)) > return; > diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h > index 863b2e7add29..f8db34a2f7a5 100644 > --- a/include/drm/display/drm_dp_helper.h > +++ b/include/drm/display/drm_dp_helper.h > @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive, > int bpp_x16, unsigned long flags); > int drm_dp_bw_channel_coding_efficiency(bool is_uhbr); > > +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, > + struct dp_sdp *sdp, size_t size); > + > #endif /* _DRM_DP_HELPER_H_ */ > -- > 2.34.1 >
On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote: > On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well. >> Lets move this to drm_dp_helper to achieve this. >> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > My preference would be to have packing functions in > drivers/video/hdmi.c, as we already have > hdmi_audio_infoframe_pack_for_dp() there. > My preference is drm_dp_helper because it already has some VSC SDP stuff and after discussion with Ville on IRC, I decided to post it this way. hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the hdmi audio infoframe fields were re-used and packed into a DP SDP thereby re-using the existing struct hdmi_audio_infoframe . This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct dp_sdp both of which had prior usages already in this file. So it all adds up and makes sense to me to be in this file. I will let the other DRM core maintainers comment on this. Ville, Jani? >> --- >> drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++ >> drivers/gpu/drm/i915/display/intel_dp.c | 73 +---------------------- >> include/drm/display/drm_dp_helper.h | 3 + >> 3 files changed, 84 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c >> index b1ca3a1100da..066cfbbf7a91 100644 >> --- a/drivers/gpu/drm/display/drm_dp_helper.c >> +++ b/drivers/gpu/drm/display/drm_dp_helper.c >> @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev, >> } >> EXPORT_SYMBOL(drm_dp_vsc_sdp_log); >> >> +/** >> + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp >> + * @vsc: vsc sdp initialized according to its purpose as defined in >> + * table 2-118 - table 2-120 in DP 1.4a specification >> + * @sdp: valid handle to the generic dp_sdp which will be packed >> + * @size: valid size of the passed sdp handle >> + * >> + * Returns length of sdp on success and error code on failure >> + */ >> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, >> + struct dp_sdp *sdp, size_t size) > > I know that you are just moving the function. Maybe there can be > patch#2, which drops the size argument? The struct dp_sdp already has > a defined size. The i915 driver just passes sizeof(sdp), which is more > or less useless. > Yes this is a valid point, I also noticed this. I can post it on top of this once we get an agreement and ack on this patch first. >> +{ >> + size_t length = sizeof(struct dp_sdp); >> + >> + if (size < length) >> + return -ENOSPC; >> + >> + memset(sdp, 0, size); >> + >> + /* >> + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 >> + * VSC SDP Header Bytes >> + */ >> + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */ >> + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */ >> + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */ >> + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */ >> + >> + if (vsc->revision == 0x6) { >> + sdp->db[0] = 1; >> + sdp->db[3] = 1; >> + } >> + >> + /* >> + * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry >> + * Format as per DP 1.4a spec and DP 2.0 respectively. >> + */ >> + if (!(vsc->revision == 0x5 || vsc->revision == 0x7)) >> + goto out; >> + >> + /* VSC SDP Payload for DB16 through DB18 */ >> + /* Pixel Encoding and Colorimetry Formats */ >> + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */ >> + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */ >> + >> + switch (vsc->bpc) { >> + case 6: >> + /* 6bpc: 0x0 */ >> + break; >> + case 8: >> + sdp->db[17] = 0x1; /* DB17[3:0] */ >> + break; >> + case 10: >> + sdp->db[17] = 0x2; >> + break; >> + case 12: >> + sdp->db[17] = 0x3; >> + break; >> + case 16: >> + sdp->db[17] = 0x4; >> + break; >> + default: >> + WARN(1, "Missing case %d\n", vsc->bpc); >> + return -EINVAL; >> + } >> + >> + /* Dynamic Range and Component Bit Depth */ >> + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA) >> + sdp->db[17] |= 0x80; /* DB17[7] */ >> + >> + /* Content Type */ >> + sdp->db[18] = vsc->content_type & 0x7; >> + >> +out: >> + return length; >> +} >> +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack); >> + >> /** >> * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON >> * @dpcd: DisplayPort configuration data >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >> index f5ef95da5534..e94db51aeeb7 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> @@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state, >> return false; >> } >> >> -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, >> - struct dp_sdp *sdp, size_t size) >> -{ >> - size_t length = sizeof(struct dp_sdp); >> - >> - if (size < length) >> - return -ENOSPC; >> - >> - memset(sdp, 0, size); >> - >> - /* >> - * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 >> - * VSC SDP Header Bytes >> - */ >> - sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */ >> - sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */ >> - sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */ >> - sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */ >> - >> - if (vsc->revision == 0x6) { >> - sdp->db[0] = 1; >> - sdp->db[3] = 1; >> - } >> - >> - /* >> - * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry >> - * Format as per DP 1.4a spec and DP 2.0 respectively. >> - */ >> - if (!(vsc->revision == 0x5 || vsc->revision == 0x7)) >> - goto out; >> - >> - /* VSC SDP Payload for DB16 through DB18 */ >> - /* Pixel Encoding and Colorimetry Formats */ >> - sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */ >> - sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */ >> - >> - switch (vsc->bpc) { >> - case 6: >> - /* 6bpc: 0x0 */ >> - break; >> - case 8: >> - sdp->db[17] = 0x1; /* DB17[3:0] */ >> - break; >> - case 10: >> - sdp->db[17] = 0x2; >> - break; >> - case 12: >> - sdp->db[17] = 0x3; >> - break; >> - case 16: >> - sdp->db[17] = 0x4; >> - break; >> - default: >> - MISSING_CASE(vsc->bpc); >> - break; >> - } >> - /* Dynamic Range and Component Bit Depth */ >> - if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA) >> - sdp->db[17] |= 0x80; /* DB17[7] */ >> - >> - /* Content Type */ >> - sdp->db[18] = vsc->content_type & 0x7; >> - >> -out: >> - return length; >> -} >> - >> static ssize_t >> intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915, >> const struct hdmi_drm_infoframe *drm_infoframe, >> @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder, >> >> switch (type) { >> case DP_SDP_VSC: >> - len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp, >> - sizeof(sdp)); >> + len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp, >> + sizeof(sdp)); >> break; >> case HDMI_PACKET_TYPE_GAMUT_METADATA: >> len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv, >> @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder, >> struct dp_sdp sdp = {}; >> ssize_t len; >> >> - len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp)); >> + len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp)); >> >> if (drm_WARN_ON(&dev_priv->drm, len < 0)) >> return; >> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h >> index 863b2e7add29..f8db34a2f7a5 100644 >> --- a/include/drm/display/drm_dp_helper.h >> +++ b/include/drm/display/drm_dp_helper.h >> @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive, >> int bpp_x16, unsigned long flags); >> int drm_dp_bw_channel_coding_efficiency(bool is_uhbr); >> >> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, >> + struct dp_sdp *sdp, size_t size); >> + >> #endif /* _DRM_DP_HELPER_H_ */ >> -- >> 2.34.1 >> > >
On Wed, Feb 14, 2024 at 09:17:34AM -0800, Abhinav Kumar wrote: > > > On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote: > > On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well. > >> Lets move this to drm_dp_helper to achieve this. > >> > >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > > > My preference would be to have packing functions in > > drivers/video/hdmi.c, as we already have > > hdmi_audio_infoframe_pack_for_dp() there. > > > > My preference is drm_dp_helper because it already has some VSC SDP stuff > and after discussion with Ville on IRC, I decided to post it this way. > > hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the > hdmi audio infoframe fields were re-used and packed into a DP SDP > thereby re-using the existing struct hdmi_audio_infoframe . > > This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct > dp_sdp both of which had prior usages already in this file. > > So it all adds up and makes sense to me to be in this file. > > I will let the other DRM core maintainers comment on this. > > Ville, Jani? Yeah, I'm not sure bloating the (poorly named) hdmi.c with all SDP stuff is a great idea. Since other related stuff already lives in the drm_dp_helper.c that seems reasonable to me at this time. And if we get a decent amount of this then probably all DP SDP stuff should be extracted into its own file. There are of course a few overlaps here andthere (the audio SDP I guess, and the CTA infoframe SDP). But I'm not sure that actually needs any SDP specific stuff in hdmi.c, or could we just let hdmi.c deal with the actual CTA-861 stuff and then have the DP SDP code wrap that up in its own thing externally? Dunno, haven't really looked at the details. > > >> --- > >> drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/display/intel_dp.c | 73 +---------------------- > >> include/drm/display/drm_dp_helper.h | 3 + > >> 3 files changed, 84 insertions(+), 70 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c > >> index b1ca3a1100da..066cfbbf7a91 100644 > >> --- a/drivers/gpu/drm/display/drm_dp_helper.c > >> +++ b/drivers/gpu/drm/display/drm_dp_helper.c > >> @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev, > >> } > >> EXPORT_SYMBOL(drm_dp_vsc_sdp_log); > >> > >> +/** > >> + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp > >> + * @vsc: vsc sdp initialized according to its purpose as defined in > >> + * table 2-118 - table 2-120 in DP 1.4a specification > >> + * @sdp: valid handle to the generic dp_sdp which will be packed > >> + * @size: valid size of the passed sdp handle > >> + * > >> + * Returns length of sdp on success and error code on failure > >> + */ > >> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, > >> + struct dp_sdp *sdp, size_t size) > > > > I know that you are just moving the function. Maybe there can be > > patch#2, which drops the size argument? The struct dp_sdp already has > > a defined size. The i915 driver just passes sizeof(sdp), which is more > > or less useless. > > > > Yes this is a valid point, I also noticed this. I can post it on top of > this once we get an agreement and ack on this patch first. > > >> +{ > >> + size_t length = sizeof(struct dp_sdp); > >> + > >> + if (size < length) > >> + return -ENOSPC; > >> + > >> + memset(sdp, 0, size); > >> + > >> + /* > >> + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 > >> + * VSC SDP Header Bytes > >> + */ > >> + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */ > >> + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */ > >> + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */ > >> + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */ > >> + > >> + if (vsc->revision == 0x6) { > >> + sdp->db[0] = 1; > >> + sdp->db[3] = 1; > >> + } > >> + > >> + /* > >> + * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry > >> + * Format as per DP 1.4a spec and DP 2.0 respectively. > >> + */ > >> + if (!(vsc->revision == 0x5 || vsc->revision == 0x7)) > >> + goto out; > >> + > >> + /* VSC SDP Payload for DB16 through DB18 */ > >> + /* Pixel Encoding and Colorimetry Formats */ > >> + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */ > >> + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */ > >> + > >> + switch (vsc->bpc) { > >> + case 6: > >> + /* 6bpc: 0x0 */ > >> + break; > >> + case 8: > >> + sdp->db[17] = 0x1; /* DB17[3:0] */ > >> + break; > >> + case 10: > >> + sdp->db[17] = 0x2; > >> + break; > >> + case 12: > >> + sdp->db[17] = 0x3; > >> + break; > >> + case 16: > >> + sdp->db[17] = 0x4; > >> + break; > >> + default: > >> + WARN(1, "Missing case %d\n", vsc->bpc); > >> + return -EINVAL; > >> + } > >> + > >> + /* Dynamic Range and Component Bit Depth */ > >> + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA) > >> + sdp->db[17] |= 0x80; /* DB17[7] */ > >> + > >> + /* Content Type */ > >> + sdp->db[18] = vsc->content_type & 0x7; > >> + > >> +out: > >> + return length; > >> +} > >> +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack); > >> + > >> /** > >> * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON > >> * @dpcd: DisplayPort configuration data > >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > >> index f5ef95da5534..e94db51aeeb7 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c > >> @@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state, > >> return false; > >> } > >> > >> -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, > >> - struct dp_sdp *sdp, size_t size) > >> -{ > >> - size_t length = sizeof(struct dp_sdp); > >> - > >> - if (size < length) > >> - return -ENOSPC; > >> - > >> - memset(sdp, 0, size); > >> - > >> - /* > >> - * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 > >> - * VSC SDP Header Bytes > >> - */ > >> - sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */ > >> - sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */ > >> - sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */ > >> - sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */ > >> - > >> - if (vsc->revision == 0x6) { > >> - sdp->db[0] = 1; > >> - sdp->db[3] = 1; > >> - } > >> - > >> - /* > >> - * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry > >> - * Format as per DP 1.4a spec and DP 2.0 respectively. > >> - */ > >> - if (!(vsc->revision == 0x5 || vsc->revision == 0x7)) > >> - goto out; > >> - > >> - /* VSC SDP Payload for DB16 through DB18 */ > >> - /* Pixel Encoding and Colorimetry Formats */ > >> - sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */ > >> - sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */ > >> - > >> - switch (vsc->bpc) { > >> - case 6: > >> - /* 6bpc: 0x0 */ > >> - break; > >> - case 8: > >> - sdp->db[17] = 0x1; /* DB17[3:0] */ > >> - break; > >> - case 10: > >> - sdp->db[17] = 0x2; > >> - break; > >> - case 12: > >> - sdp->db[17] = 0x3; > >> - break; > >> - case 16: > >> - sdp->db[17] = 0x4; > >> - break; > >> - default: > >> - MISSING_CASE(vsc->bpc); > >> - break; > >> - } > >> - /* Dynamic Range and Component Bit Depth */ > >> - if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA) > >> - sdp->db[17] |= 0x80; /* DB17[7] */ > >> - > >> - /* Content Type */ > >> - sdp->db[18] = vsc->content_type & 0x7; > >> - > >> -out: > >> - return length; > >> -} > >> - > >> static ssize_t > >> intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915, > >> const struct hdmi_drm_infoframe *drm_infoframe, > >> @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder, > >> > >> switch (type) { > >> case DP_SDP_VSC: > >> - len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp, > >> - sizeof(sdp)); > >> + len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp, > >> + sizeof(sdp)); > >> break; > >> case HDMI_PACKET_TYPE_GAMUT_METADATA: > >> len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv, > >> @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder, > >> struct dp_sdp sdp = {}; > >> ssize_t len; > >> > >> - len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp)); > >> + len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp)); > >> > >> if (drm_WARN_ON(&dev_priv->drm, len < 0)) > >> return; > >> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h > >> index 863b2e7add29..f8db34a2f7a5 100644 > >> --- a/include/drm/display/drm_dp_helper.h > >> +++ b/include/drm/display/drm_dp_helper.h > >> @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive, > >> int bpp_x16, unsigned long flags); > >> int drm_dp_bw_channel_coding_efficiency(bool is_uhbr); > >> > >> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, > >> + struct dp_sdp *sdp, size_t size); > >> + > >> #endif /* _DRM_DP_HELPER_H_ */ > >> -- > >> 2.34.1 > >> > > > >
On 2/14/2024 10:02 AM, Ville Syrjälä wrote: > On Wed, Feb 14, 2024 at 09:17:34AM -0800, Abhinav Kumar wrote: >> >> >> On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote: >>> On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well. >>>> Lets move this to drm_dp_helper to achieve this. >>>> >>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> >>> My preference would be to have packing functions in >>> drivers/video/hdmi.c, as we already have >>> hdmi_audio_infoframe_pack_for_dp() there. >>> >> >> My preference is drm_dp_helper because it already has some VSC SDP stuff >> and after discussion with Ville on IRC, I decided to post it this way. >> >> hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the >> hdmi audio infoframe fields were re-used and packed into a DP SDP >> thereby re-using the existing struct hdmi_audio_infoframe . >> >> This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct >> dp_sdp both of which had prior usages already in this file. >> >> So it all adds up and makes sense to me to be in this file. >> >> I will let the other DRM core maintainers comment on this. >> >> Ville, Jani? > > Yeah, I'm not sure bloating the (poorly named) hdmi.c with all > SDP stuff is a great idea. Since other related stuff already > lives in the drm_dp_helper.c that seems reasonable to me at this > time. And if we get a decent amount of this then probably all > DP SDP stuff should be extracted into its own file. > Yes, thanks. > There are of course a few overlaps here andthere (the audio SDP > I guess, and the CTA infoframe SDP). But I'm not sure that actually > needs any SDP specific stuff in hdmi.c, or could we just let hdmi.c > deal with the actual CTA-861 stuff and then have the DP SDP code > wrap that up in its own thing externally? Dunno, haven't really > looked at the details. > Thats a good way to look at it. this packing is from DP spec and not CTA so makes more sense to be in this file. In that case, R-b? >> >>>> --- >>>> drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/display/intel_dp.c | 73 +---------------------- >>>> include/drm/display/drm_dp_helper.h | 3 + >>>> 3 files changed, 84 insertions(+), 70 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c >>>> index b1ca3a1100da..066cfbbf7a91 100644 >>>> --- a/drivers/gpu/drm/display/drm_dp_helper.c >>>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c >>>> @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev, >>>> } >>>> EXPORT_SYMBOL(drm_dp_vsc_sdp_log); >>>> >>>> +/** >>>> + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp >>>> + * @vsc: vsc sdp initialized according to its purpose as defined in >>>> + * table 2-118 - table 2-120 in DP 1.4a specification >>>> + * @sdp: valid handle to the generic dp_sdp which will be packed >>>> + * @size: valid size of the passed sdp handle >>>> + * >>>> + * Returns length of sdp on success and error code on failure >>>> + */ >>>> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, >>>> + struct dp_sdp *sdp, size_t size) >>> >>> I know that you are just moving the function. Maybe there can be >>> patch#2, which drops the size argument? The struct dp_sdp already has >>> a defined size. The i915 driver just passes sizeof(sdp), which is more >>> or less useless. >>> >> >> Yes this is a valid point, I also noticed this. I can post it on top of >> this once we get an agreement and ack on this patch first. >> >>>> +{ >>>> + size_t length = sizeof(struct dp_sdp); >>>> + >>>> + if (size < length) >>>> + return -ENOSPC; >>>> + >>>> + memset(sdp, 0, size); >>>> + >>>> + /* >>>> + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 >>>> + * VSC SDP Header Bytes >>>> + */ >>>> + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */ >>>> + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */ >>>> + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */ >>>> + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */ >>>> + >>>> + if (vsc->revision == 0x6) { >>>> + sdp->db[0] = 1; >>>> + sdp->db[3] = 1; >>>> + } >>>> + >>>> + /* >>>> + * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry >>>> + * Format as per DP 1.4a spec and DP 2.0 respectively. >>>> + */ >>>> + if (!(vsc->revision == 0x5 || vsc->revision == 0x7)) >>>> + goto out; >>>> + >>>> + /* VSC SDP Payload for DB16 through DB18 */ >>>> + /* Pixel Encoding and Colorimetry Formats */ >>>> + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */ >>>> + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */ >>>> + >>>> + switch (vsc->bpc) { >>>> + case 6: >>>> + /* 6bpc: 0x0 */ >>>> + break; >>>> + case 8: >>>> + sdp->db[17] = 0x1; /* DB17[3:0] */ >>>> + break; >>>> + case 10: >>>> + sdp->db[17] = 0x2; >>>> + break; >>>> + case 12: >>>> + sdp->db[17] = 0x3; >>>> + break; >>>> + case 16: >>>> + sdp->db[17] = 0x4; >>>> + break; >>>> + default: >>>> + WARN(1, "Missing case %d\n", vsc->bpc); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + /* Dynamic Range and Component Bit Depth */ >>>> + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA) >>>> + sdp->db[17] |= 0x80; /* DB17[7] */ >>>> + >>>> + /* Content Type */ >>>> + sdp->db[18] = vsc->content_type & 0x7; >>>> + >>>> +out: >>>> + return length; >>>> +} >>>> +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack); >>>> + >>>> /** >>>> * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON >>>> * @dpcd: DisplayPort configuration data >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >>>> index f5ef95da5534..e94db51aeeb7 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >>>> @@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state, >>>> return false; >>>> } >>>> >>>> -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, >>>> - struct dp_sdp *sdp, size_t size) >>>> -{ >>>> - size_t length = sizeof(struct dp_sdp); >>>> - >>>> - if (size < length) >>>> - return -ENOSPC; >>>> - >>>> - memset(sdp, 0, size); >>>> - >>>> - /* >>>> - * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 >>>> - * VSC SDP Header Bytes >>>> - */ >>>> - sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */ >>>> - sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */ >>>> - sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */ >>>> - sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */ >>>> - >>>> - if (vsc->revision == 0x6) { >>>> - sdp->db[0] = 1; >>>> - sdp->db[3] = 1; >>>> - } >>>> - >>>> - /* >>>> - * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry >>>> - * Format as per DP 1.4a spec and DP 2.0 respectively. >>>> - */ >>>> - if (!(vsc->revision == 0x5 || vsc->revision == 0x7)) >>>> - goto out; >>>> - >>>> - /* VSC SDP Payload for DB16 through DB18 */ >>>> - /* Pixel Encoding and Colorimetry Formats */ >>>> - sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */ >>>> - sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */ >>>> - >>>> - switch (vsc->bpc) { >>>> - case 6: >>>> - /* 6bpc: 0x0 */ >>>> - break; >>>> - case 8: >>>> - sdp->db[17] = 0x1; /* DB17[3:0] */ >>>> - break; >>>> - case 10: >>>> - sdp->db[17] = 0x2; >>>> - break; >>>> - case 12: >>>> - sdp->db[17] = 0x3; >>>> - break; >>>> - case 16: >>>> - sdp->db[17] = 0x4; >>>> - break; >>>> - default: >>>> - MISSING_CASE(vsc->bpc); >>>> - break; >>>> - } >>>> - /* Dynamic Range and Component Bit Depth */ >>>> - if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA) >>>> - sdp->db[17] |= 0x80; /* DB17[7] */ >>>> - >>>> - /* Content Type */ >>>> - sdp->db[18] = vsc->content_type & 0x7; >>>> - >>>> -out: >>>> - return length; >>>> -} >>>> - >>>> static ssize_t >>>> intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915, >>>> const struct hdmi_drm_infoframe *drm_infoframe, >>>> @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder, >>>> >>>> switch (type) { >>>> case DP_SDP_VSC: >>>> - len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp, >>>> - sizeof(sdp)); >>>> + len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp, >>>> + sizeof(sdp)); >>>> break; >>>> case HDMI_PACKET_TYPE_GAMUT_METADATA: >>>> len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv, >>>> @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder, >>>> struct dp_sdp sdp = {}; >>>> ssize_t len; >>>> >>>> - len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp)); >>>> + len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp)); >>>> >>>> if (drm_WARN_ON(&dev_priv->drm, len < 0)) >>>> return; >>>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h >>>> index 863b2e7add29..f8db34a2f7a5 100644 >>>> --- a/include/drm/display/drm_dp_helper.h >>>> +++ b/include/drm/display/drm_dp_helper.h >>>> @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive, >>>> int bpp_x16, unsigned long flags); >>>> int drm_dp_bw_channel_coding_efficiency(bool is_uhbr); >>>> >>>> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, >>>> + struct dp_sdp *sdp, size_t size); >>>> + >>>> #endif /* _DRM_DP_HELPER_H_ */ >>>> -- >>>> 2.34.1 >>>> >>> >>> >
On Wed, 14 Feb 2024 at 20:08, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 2/14/2024 10:02 AM, Ville Syrjälä wrote: > > On Wed, Feb 14, 2024 at 09:17:34AM -0800, Abhinav Kumar wrote: > >> > >> > >> On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote: > >>> On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>> > >>>> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well. > >>>> Lets move this to drm_dp_helper to achieve this. > >>>> > >>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > >>> > >>> My preference would be to have packing functions in > >>> drivers/video/hdmi.c, as we already have > >>> hdmi_audio_infoframe_pack_for_dp() there. > >>> > >> > >> My preference is drm_dp_helper because it already has some VSC SDP stuff > >> and after discussion with Ville on IRC, I decided to post it this way. > >> > >> hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the > >> hdmi audio infoframe fields were re-used and packed into a DP SDP > >> thereby re-using the existing struct hdmi_audio_infoframe . > >> > >> This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct > >> dp_sdp both of which had prior usages already in this file. > >> > >> So it all adds up and makes sense to me to be in this file. > >> > >> I will let the other DRM core maintainers comment on this. > >> > >> Ville, Jani? > > > > Yeah, I'm not sure bloating the (poorly named) hdmi.c with all > > SDP stuff is a great idea. Since other related stuff already > > lives in the drm_dp_helper.c that seems reasonable to me at this > > time. And if we get a decent amount of this then probably all > > DP SDP stuff should be extracted into its own file. > > > > Yes, thanks. > > > There are of course a few overlaps here andthere (the audio SDP > > I guess, and the CTA infoframe SDP). But I'm not sure that actually > > needs any SDP specific stuff in hdmi.c, or could we just let hdmi.c > > deal with the actual CTA-861 stuff and then have the DP SDP code > > wrap that up in its own thing externally? Dunno, haven't really > > looked at the details. > > > > Thats a good way to look at it. this packing is from DP spec and not CTA > so makes more sense to be in this file. > > In that case, R-b? > > >> > >>>> --- > >>>> drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++ > >>>> drivers/gpu/drm/i915/display/intel_dp.c | 73 +---------------------- > >>>> include/drm/display/drm_dp_helper.h | 3 + > >>>> 3 files changed, 84 insertions(+), 70 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c > >>>> index b1ca3a1100da..066cfbbf7a91 100644 > >>>> --- a/drivers/gpu/drm/display/drm_dp_helper.c > >>>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c > >>>> @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev, > >>>> } > >>>> EXPORT_SYMBOL(drm_dp_vsc_sdp_log); > >>>> > >>>> +/** > >>>> + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp > >>>> + * @vsc: vsc sdp initialized according to its purpose as defined in > >>>> + * table 2-118 - table 2-120 in DP 1.4a specification > >>>> + * @sdp: valid handle to the generic dp_sdp which will be packed > >>>> + * @size: valid size of the passed sdp handle > >>>> + * > >>>> + * Returns length of sdp on success and error code on failure > >>>> + */ > >>>> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, > >>>> + struct dp_sdp *sdp, size_t size) > >>> > >>> I know that you are just moving the function. Maybe there can be > >>> patch#2, which drops the size argument? The struct dp_sdp already has > >>> a defined size. The i915 driver just passes sizeof(sdp), which is more > >>> or less useless. > >>> > >> > >> Yes this is a valid point, I also noticed this. I can post it on top of > >> this once we get an agreement and ack on this patch first. > >> From my side, with the promise of the size fixup. Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c index b1ca3a1100da..066cfbbf7a91 100644 --- a/drivers/gpu/drm/display/drm_dp_helper.c +++ b/drivers/gpu/drm/display/drm_dp_helper.c @@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev, } EXPORT_SYMBOL(drm_dp_vsc_sdp_log); +/** + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp + * @vsc: vsc sdp initialized according to its purpose as defined in + * table 2-118 - table 2-120 in DP 1.4a specification + * @sdp: valid handle to the generic dp_sdp which will be packed + * @size: valid size of the passed sdp handle + * + * Returns length of sdp on success and error code on failure + */ +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, + struct dp_sdp *sdp, size_t size) +{ + size_t length = sizeof(struct dp_sdp); + + if (size < length) + return -ENOSPC; + + memset(sdp, 0, size); + + /* + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 + * VSC SDP Header Bytes + */ + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */ + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */ + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */ + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */ + + if (vsc->revision == 0x6) { + sdp->db[0] = 1; + sdp->db[3] = 1; + } + + /* + * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry + * Format as per DP 1.4a spec and DP 2.0 respectively. + */ + if (!(vsc->revision == 0x5 || vsc->revision == 0x7)) + goto out; + + /* VSC SDP Payload for DB16 through DB18 */ + /* Pixel Encoding and Colorimetry Formats */ + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */ + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */ + + switch (vsc->bpc) { + case 6: + /* 6bpc: 0x0 */ + break; + case 8: + sdp->db[17] = 0x1; /* DB17[3:0] */ + break; + case 10: + sdp->db[17] = 0x2; + break; + case 12: + sdp->db[17] = 0x3; + break; + case 16: + sdp->db[17] = 0x4; + break; + default: + WARN(1, "Missing case %d\n", vsc->bpc); + return -EINVAL; + } + + /* Dynamic Range and Component Bit Depth */ + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA) + sdp->db[17] |= 0x80; /* DB17[7] */ + + /* Content Type */ + sdp->db[18] = vsc->content_type & 0x7; + +out: + return length; +} +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack); + /** * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON * @dpcd: DisplayPort configuration data diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index f5ef95da5534..e94db51aeeb7 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state, return false; } -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, - struct dp_sdp *sdp, size_t size) -{ - size_t length = sizeof(struct dp_sdp); - - if (size < length) - return -ENOSPC; - - memset(sdp, 0, size); - - /* - * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 - * VSC SDP Header Bytes - */ - sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */ - sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */ - sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */ - sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */ - - if (vsc->revision == 0x6) { - sdp->db[0] = 1; - sdp->db[3] = 1; - } - - /* - * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry - * Format as per DP 1.4a spec and DP 2.0 respectively. - */ - if (!(vsc->revision == 0x5 || vsc->revision == 0x7)) - goto out; - - /* VSC SDP Payload for DB16 through DB18 */ - /* Pixel Encoding and Colorimetry Formats */ - sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */ - sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */ - - switch (vsc->bpc) { - case 6: - /* 6bpc: 0x0 */ - break; - case 8: - sdp->db[17] = 0x1; /* DB17[3:0] */ - break; - case 10: - sdp->db[17] = 0x2; - break; - case 12: - sdp->db[17] = 0x3; - break; - case 16: - sdp->db[17] = 0x4; - break; - default: - MISSING_CASE(vsc->bpc); - break; - } - /* Dynamic Range and Component Bit Depth */ - if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA) - sdp->db[17] |= 0x80; /* DB17[7] */ - - /* Content Type */ - sdp->db[18] = vsc->content_type & 0x7; - -out: - return length; -} - static ssize_t intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915, const struct hdmi_drm_infoframe *drm_infoframe, @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder, switch (type) { case DP_SDP_VSC: - len = intel_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp, - sizeof(sdp)); + len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp, + sizeof(sdp)); break; case HDMI_PACKET_TYPE_GAMUT_METADATA: len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv, @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder *encoder, struct dp_sdp sdp = {}; ssize_t len; - len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp)); + len = drm_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp)); if (drm_WARN_ON(&dev_priv->drm, len < 0)) return; diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h index 863b2e7add29..f8db34a2f7a5 100644 --- a/include/drm/display/drm_dp_helper.h +++ b/include/drm/display/drm_dp_helper.h @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive, int bpp_x16, unsigned long flags); int drm_dp_bw_channel_coding_efficiency(bool is_uhbr); +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, + struct dp_sdp *sdp, size_t size); + #endif /* _DRM_DP_HELPER_H_ */
intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well. Lets move this to drm_dp_helper to achieve this. Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/display/drm_dp_helper.c | 78 +++++++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_dp.c | 73 +---------------------- include/drm/display/drm_dp_helper.h | 3 + 3 files changed, 84 insertions(+), 70 deletions(-)