| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fa63e9c1-3cec-41df-c643-33950346b76c@quicinc.com>
Date: Wed, 14 Feb 2024 10:08:38 -0800
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Ville Syrjälä <ville.syrjala@...ux.intel.com>
CC: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
<dri-devel@...ts.freedesktop.org>,
Maarten Lankhorst
<maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Joonas Lahtinen
<joonas.lahtinen@...ux.intel.com>,
Tvrtko Ursulin
<tvrtko.ursulin@...ux.intel.com>,
<robdclark@...il.com>, <freedreno@...ts.freedesktop.org>,
<intel-gfx@...ts.freedesktop.org>, <quic_jesszhan@...cinc.com>,
<linux-kernel@...r.kernel.org>, <intel-xe@...ts.freedesktop.org>
Subject: 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 <quic_abhinavk@...cinc.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@...cinc.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
>>>>
>>>
>>>
>
Powered by blists - more mailing lists