On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:
Adding Alex, Christian and DRM lists to the thread.
> Hi KSPP community,
>
> I've been working on replacing 1-element arrays with flex array members
> on the drm/amdgpu files. I came across one insteresting case which I
> may need to pick your brains to find a solution for it.
>
> The structure below has two fake flexible arrays but I would get an
> error if I try make them both FAM. How should/could I deal with the
> asRegIndexBuf in this case? In theory, DECLARE_FLEX_ARRAY would "work"
> but that doesn't seem to be its intended usage as far I've searched.
> (unless I got it wrong, if that's the case, feel free to set me straight)
>
> Any ideas?
>
> struct _ATOM_INIT_REG_BLOCK {
> USHORT usRegIndexTblSize; /* 0 2 */
> USHORT usRegDataBlkSize; /* 2 2 */
> ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1]; /* 4 3 */
> ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1]; /* 7 8 */
I didn't find evidence that asRegDataBuf is used anywhere in the
codebase[1].
>
> /* size: 15, cachelines: 1, members: 4 */
> /* last cacheline: 15 bytes */
> } __attribute__((__packed__));
Alex, Christian,
It looks like this structure is only being used as a template to populate
instances of struct atom_mc_reg_table[2] and that these[3] are the only
places where asRegIndexBuf[] is being used. Apparently, this array is only
being used to retrieve it's address so that a pointer can jump[4] in chucks
of size sizeof(ATOM_INIT_REG_INDEX_FORMAT):
drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1461:
1461: format = (ATOM_INIT_REG_INDEX_FORMAT *)
1462: ((u8 *)format + sizeof(ATOM_INIT_REG_INDEX_FORMAT));
up to (VBIOS_MC_REGISTER_ARRAY_SIZE * sizeof(ATOM_INIT_REG_INDEX_FORMAT))[5],
As I pointed out above, I don't see asRegDataBuf[] being used in the
codebase (of course it may describe firmware memory layout). Instead,
there is this jump to a block of data past asRegIndexBuf[]:
drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1444:
1444: ATOM_MEMORY_SETTING_DATA_BLOCK *reg_data =
1445: (ATOM_MEMORY_SETTING_DATA_BLOCK *)
1446: ((u8 *)reg_block + (2 * sizeof(u16)) +
1447: le16_to_cpu(reg_block->usRegIndexTblSize));
So, it seems the one relevant array, from the kernel side, is
asRegIndexBuf[]. I wonder if we really need asRegDataBuf[] in that
structure... or if we could try modifying that struct and only have
asRegIndexBuf[] as last member? and then we can transform it into a
flex-array member.
If for any strong reasong we cannot remove asRegDataBuf[] then I think we
could give it a try and use DECLARE_FLEX_ARRAY() to declare both arrays
in the structure.
But first, of course, Alex, Christian, it'd be really great if we can
have your input and feedback. :)
Thanks!
--
Gustavo
[1] https://elixir.bootlin.com/linux/latest/C/ident/asRegDataBuf
[2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c#L1441
[3] https://elixir.bootlin.com/linux/latest/C/ident/asRegIndexBuf
