Messages in this thread | | | Date | Wed, 14 Feb 2024 10:08:38 -0800 | Subject | Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper | From | Abhinav Kumar <> |
| |
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 >>>> >>> >>> >
| |