Message-ID: <88993439-bfdc-418c-95c6-d6d8bdb5b87f@ideasonboard.com>
Date: Wed, 30 Apr 2025 20:42:47 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Devarsh Thakkar <devarsht@...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

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?

> 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.

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.

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).

>>
>>>                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

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.

  Tomi

Powered by blists - more mailing lists