Re: [PATCH v2] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper

From: Abhinav Kumar
Date: Tue Feb 20 2024 - 14:27:44 EST




On 2/20/2024 11:20 AM, Dmitry Baryshkov wrote:
On Tue, 20 Feb 2024 at 21:05, Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:

On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:



On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote:
On Thu, 15 Feb 2024 at 21:08, 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.

changes in v2:
- rebased on top of drm-tip

Acked-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>

v1 had an explicit comment before the ack:


Yes, I remember the comment. I did not make any changes to v2 other than
just rebasing it on drm-tip to get the ack from i915 folks.

From my side, with the promise of the size fixup.

However I observe neither a second patch removing the size argument
nor it being dropped as a part of this patch.


Yes, now that in v2 we got the ack for this patch, I can spin a v3 with
the addition of the next patch to remove the size OR as another series
so as to not block the main series which needs this patch.

I would prefer the latter.

It doesn't work this way. The comment should have been fixed for v2.

This probably deserves some explanation. Currently there is only one
user of this function. So it is easy to fix it. Once there are several
users, you have to fix all of them at the same time, patching
different drm subtrees. That complicates the life of maintainers.


Yes, understood. Its easier to fix it now if its really needed.

Actually, I think the reason the size was passed was to make sure
a valid struct dp_sdp *sdp was being passed.

If we drop the size, we need to have a if (!sdp) check as there is a memset followed by dereference.

So maybe, if we want to keep this API protected, its not too bad to have?