| 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: <1f8c43cd-8c26-4e42-b144-b91f5ffc2e2e@ti.com>
Date: Wed, 30 Apr 2025 22:07:18 +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 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
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].
>
>> 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.
>
>> 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 ??
[1]: https://gist.github.com/devarsht/82505ca69f0bd5d9788bfc240d2e83d4
Regards
Devarsh
Powered by blists - more mailing lists