Message-ID: <466254e9-145f-4839-9451-a5f282ff02e9@ti.com>
Date: Fri, 2 May 2025 12:36:54 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
CC: <praneeth@...com>, <vigneshr@...com>, <aradhya.bhatia@...ux.dev>,
        <s-jain1@...com>, <r-donadkar@...com>, <j-choudhary@...com>,
        <h-shenoy@...com>, <jyri.sarha@....fi>, <airlied@...il.com>,
        <maarten.lankhorst@...ux.intel.com>, <mripard@...nel.org>,
        <tzimmermann@...e.de>, <dri-devel@...ts.freedesktop.org>,
        <simona@...ll.ch>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
        <conor+dt@...nel.org>
Subject: Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS
 cut-down versions

Hi Tomi

Thanks for quick comments.

On 30/04/25 23:12, Tomi Valkeinen wrote:
> On 30/04/2025 19:37, Devarsh Thakkar wrote:
>> Hi Tomi
>>
>> Thanks for the review.
>>
>> <snip>
>>>>    @@ -2025,7 +2101,7 @@ int dispc_plane_check(struct dispc_device
>>>> *dispc, u32 hw_plane,
>>>>                  const struct drm_plane_state *state,
>>>>                  u32 hw_videoport)
>>>>    {
>>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>>
>>> I don't think this is correct. You can't access the vid_info[] with the
>>> hw-id.
>>
>> I don't think hw_id is getting passed to hw_plane here. The
>> dispc_plane_check is called from tidss_plane_atomic_check which passes
>> hw_plane as tplane->hw_plane_id and this index starts from actually
>> instantiated planes i.e. from 0 and are contiguous as these are
> 
> Well, if tplane->hw_plane_id is not the HW plane id (i.e. it's misnamed
> now), and tidss_plane.c calls dispc_plane_enable() with tplane-
>>hw_plane_id as the hw_plane parameter, which is used as a HW plane
> ID... Then... One of these is wrong, no?
> 

As mentioned here [1], dispc_plane_enable acts on VID_* registers which
are only mapped per the instantiated/actual pipes present in the SoC, so
the indexing always starts from 0 and we need not worry about skipping
un-instantiated planes.

So hw_plane_id -> Index of only instantiated planes starting from 0
hw_id -> Hardware Index taking into account instantiated +
un-instantiated/skipped planes main used for common0/1 region registers
dealing with VID planes.


For e.g. for AM62L which includes VIDL pipe
hw_plane_id -> 0
hw_id -> 1


>> populated from vid_order array (hw_plane_id =
>> feat->vid_order[tidss->num_planes];) and not the hw_id index.
>>
>> So for e.g. for AM62L even though hw_id is 1 for VIDL hw_plane is
>> getting passed as 0 and that's how it is able to access the first and
>> only member of vid_info struct and read the properties correctly and
>> function properly as seen in test logs [1].
> 
> If for AM62L the tplane->hw_plane_id is 0, the the dispc_plane_enable()
> call would enable the wrong plane, wouldn't it?
> 
> But even if it all works, I think this highlights how confusing it is...
> 
>>
>>>
>>>>        u32 fourcc = state->fb->format->format;
>>>>        bool need_scaling = state->src_w >> 16 != state->crtc_w ||
>>>>            state->src_h >> 16 != state->crtc_h;
>>>> @@ -2096,7 +2172,7 @@ void dispc_plane_setup(struct dispc_device
>>>> *dispc, u32 hw_plane,
>>>>                   const struct drm_plane_state *state,
>>>>                   u32 hw_videoport)
>>>>    {
>>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>>
>>> Here too.
>>
>> Here also hw_plane is getting passed as 0 and not the hw_id which is 1
>> for AM62L.
>>
>>>
>>>>        u32 fourcc = state->fb->format->format;
>>>>        u16 cpp = state->fb->format->cpp[0];
>>>>        u32 fb_width = state->fb->pitches[0] / cpp;
>>>> @@ -2210,7 +2286,7 @@ static void dispc_k2g_plane_init(struct
>>>> dispc_device *dispc)
>>>>        /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>>>        REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>>>    -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>>>> hw_plane++) {
>>>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>>>>            u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>>>            u32 thr_low, thr_high;
>>>>            u32 mflag_low, mflag_high;
>>>> @@ -2226,7 +2302,7 @@ static void dispc_k2g_plane_init(struct
>>>> dispc_device *dispc)
>>>>              dev_dbg(dispc->dev,
>>>>                "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>>>> %u/%u preload %u\n",
>>>> -            dispc->feat->vid_name[hw_plane],
>>>> +            dispc->feat->vid_info[hw_plane].name,
>>>
>>> Here hw_plane is not actually the hw-id (anymore), but elsewhere in this
>>> function it is used as a hw-id, which is no longer correct.
>>
>> For accessing vid_info hw_plane needs to be used which is the index of
>> actually instantiated planes and I see it as correctly being passed for
>> AM62L too. hw_id is only for dispc_k3_vid* functions where we need to
>> skip the not-instantiated vid regions by adding the offset per the hw_id
>> index.
> 
> Hmm, sorry, I don't follow. If we use the same variable, hw_plane, to
> access the vid_info[], and as a parameter to functions that take
> hw_plane, e.g., dispc_vid_set_buf_threshold(), isn't one of those uses
> wrong?
> 
> Oh, wait... I think I see it now. For some functions using the hw_id as
> the hw_plane parameter is fine, as they access the VID's registers by
> just using, e.g. dispc_vid_write(), which gets the address correctly
> from dispc->base_vid[hw_plane], as that one is indexed from 0 to num_vids.
> 

Yes exactly.

> But some functions use registers that have bits based on the hw_id (like
> dispc_k3_vid_write_irqstatus), and then we use the hw_id for the
> hw_plane parameter. If that function were to also write a vid register,
> using the passed hw_plane, it wouldn't work, but I guess we don't do that.
> 

Yes, hw_id is only for dispc_k3_vid* functions dealing with common
region registers that play with VID pipes.

> It feels broken... We can't have 'hw_plane' that's sometimes the HW id
> (i.e. 1 for AM62L), and sometimes the driver's index (i.e. 0 for AM62L).
> 

Sorry I don't follow, what exactly is broken here. hw_plane is for
instantiated planes present in SoC used in context of VID* register
space while doing reg writess and hw_id is the plane hardware index
w.r.t larger K3 family i.e used in context for common register space.

>>>
>>>>                size,
>>>>                thr_high, thr_low,
>>>>                mflag_high, mflag_low,
>>>> @@ -2265,7 +2341,7 @@ static void dispc_k3_plane_init(struct
>>>> dispc_device *dispc)
>>>>        /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>>>        REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>>>    -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>>>> hw_plane++) {
>>>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>>>>            u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>>>            u32 thr_low, thr_high;
>>>>            u32 mflag_low, mflag_high;
>>>> @@ -2281,7 +2357,7 @@ static void dispc_k3_plane_init(struct
>>>> dispc_device *dispc)
>>>>              dev_dbg(dispc->dev,
>>>>                "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>>>> %u/%u preload %u\n",
>>>> -            dispc->feat->vid_name[hw_plane],
>>>> +            dispc->feat->vid_info[hw_plane].name,
>>>
>>> And here.
>>>
>>> All these issues make me wonder whether we have the right model. It's
>>> just too easy to get the usage wrong.
>>>
>>> I'm not sure which way to go here.
>>>
>>> Fix the current issues? It's a bit cumbersome to go from hw-id to the
>>> index (needs a search), just to get some hw properties.
>>>
>>> Or go back to the earlier one, with a vid array containing unused slots?
>>> That makes the for loops a bit harder.
>>>
>>> I need to think about it...
>>>
>>
>> Hmm, I don't think so, it seems to look fine to me and work fine too. I
>> have tested thoroughly for AM62L (which has uninstantiated vid region)
>> along with AM62x and AM62A with all planes displayed simultaneously. If
>> you want I can put on some test logs, create some dummy holes for VID
>> regions in AM62 and AM62A to put this on to some further negative tests.
>>
>> Also if naming convention is confusing (hw_id vs hw_plane) then maybe we
>> can use something else like vid_idx ??
> 
> It is confusing. But I think it's also broken, in the sense that e.g.
> dispc_k3_vid_write_irqstatus() has hw_plane parameter. But it's actually
> hw_id.
> 
> I'm not sure if naming them differently helps here. It's super
> confusing. What indices do we have?
> 
> - The lowest level HW IDs, e.g. for DISPC_VID_IRQSTATUS()
> - The index for the dispc->vid_info[]
> - The index to tidss->planes[]
> - drm_plane->index
> 

All above are for the instantiated ones so they start from 0.
Since we decide to use "common region" of AM65x family as you suggested
in V1 review comments [2] we need to handle the caveats associated with
VID and VIDL register bits for this common region only w.r.t SoCs which
don't include both of them.

So for e.g. am65x common region include both VID (idx0) and VIDL (idx1)
but am62L does not have VID, but the offset and bit indexing for VIDL in
both  AM65X and AM62L is still the same i.e. idx VIDL.


> Originally I kept the drm_plane and the HW index separate, so that the
> dispc.c doesn't really deal with the drm_plane at all. But I wonder if
> we need to change that, as drm_plane pointer can't really be
> "understood" wrong, whereas an two indices are easy to mix.
> 

Sorry I don't follow exactly on concern above and I am not able to catch
any bugs either in the testing done so far in multiple SoCs. If you see
any potential issue or a negative scenario I can help put debug logs or
provide a register dump.


For the naming if this is confusing, is cmn_vid_idx a better name to
highlight that it only deals with vid/vidl specific indexing and
register/bit writes for common region ?

[1] https://lore.kernel.org/all/096ff788-7a25-47a3-ad13-caff971cf0bc@ti.com/
[2]
https://lore.kernel.org/all/f6b20a29-1205-4f5e-87b6-fec58bd43545@ideasonboard.com/

Regards
Devarsh

>  Tomi
> 

Powered by blists - more mailing lists