On Wed, 14 Feb 2024 at 20:08, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> > >>> > >>> 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@xxxxxxxxxx> -- With best wishes Dmitry
