Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

From: Tomi Valkeinen
Date: Fri May 02 2025 - 06:56:06 EST

Hi,

On 02/05/2025 13:47, Devarsh Thakkar wrote:
Hi Tomi,

Thanks for the quick revert.

On 02/05/25 13:37, Tomi Valkeinen wrote:
Hi,

On 02/05/2025 10:06, Devarsh Thakkar wrote:
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.

We have, as an example, these two functions:

void dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool
enable)

static void dispc_k3_vid_write_irqstatus(struct dispc_device *dispc, u32
hw_plane, dispc_irq_t vidstat)

When the caller calls these functions on AM62L, what does it provide in
'hw_plane' when it wants to enable the first plane of write the
irqstatus for the first plane?

It uses hw_id i.e. 1 for all vid irqstatus related registers since it is
accessing am65x common region register space which has vid on idx0 which
we want to skip for am62l.

For dispc_plane_enable(), the caller uses
0, for dispc_k3_vid_write_irqstatus(), the caller uses 1...

Yes above is correct, and I think that's how it is supposed to be.

No it's not. Both functions have "hw_plane" parameter, yet they require a different value to be used even when referring to the same plane.

With a quick look at the code, changing the callers to pass the "old
style" hw_plane as the parameter to those irq functions, and the
functions internally get the hw_id, would solve most of the problems.

I don't follow above, hw_plane has 0 so it should not be used for
programming irq related functions which expect idx 1 as explained above.

We have various functions in tidss_dispc.c that have hw_plane as a parameter. But the caller is supposed to know that for some functions hw_plane is a plane index from 0, and for some it's the hw_id from vid_info[].

There's still dispc_k3_set_irqenable() which manages 'main_disable' and
needs the hw_id, but maybe that's fine, even if a bit confusing.


I still feel there is no inherent bug here, but let me know if you want
me to put some debug prints or get register dump so that we can double
confirm.

I'm not saying there's a bug. I'm saying it's bad code and will cause confusion and bugs in the future.

Tomi