On 22/05/2025 23:05, Lucas De Marchi wrote:
On Fri, May 23, 2025 at 06:32:43AM +1000, Dave Airlie wrote:
On Fri, 23 May 2025 at 01:10, Lucas De Marchi
<lucas.demar...@intel.com> wrote:
On Thu, May 22, 2025 at 04:52:18PM +1000, Dave Airlie wrote:
>From: Dave Airlie <airl...@redhat.com>
>
>This reduces this struct from 16 to 8 bytes, and it gets embedded
>into a lot of things.
>
>Signed-off-by: Dave Airlie <airl...@redhat.com>
Replied too early on cover. Chatting with Michal Wajdeczko today, this
may break things as we then can't byte-address anymore. It seems
particularly dangerous when using the iosys_map_wr/iosys_map_rd as
there's nothing preventing an unaligned address and we increment the map
with the sizeof() of a struct that could be __packed. Example: in
xe_guc_ads.c we use it to write packed structs like guc_gt_system_info.
In this particular case it doesn't give unaligned address, but we should
probably then protect iosys_map from doing the wrong thing.
So, if we are keeping this patch, we should probably protect
initially-unaligned addresses and the iosys_map_incr() call?
oh interesting, my thoughts was of course nobody would want to use
this for < 32-byte aligned ptrs :-)
but I forgot about using the incr for stuff, I do wonder if the incr
could be modelled on a base + offset, as I do think for a lot of stuff
we'd want to retain the original vaddr for unmapping or other things,
for the parts of the code that want to update "inner blocks", the
approach is to copy the struct and operate on that. Example from
drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c:guc_prep_golden_context:
info_map = IOSYS_MAP_INIT_OFFSET(&guc->ads_map,
offsetof(struct __guc_ads_blob, system_info));
then that function can manipulate its "local map" without affecting the
one used for really mapping the memory.
Also drm_panic uses that to draw into the frame buffer:
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/gpu/drm/drm_panic.c#L181
So in case of 16bits or 24bits pixel width, it might be unaligned.
As it is used only to save one argument to the "blit" function, I think
saving 8 bytes in the iosys_map struct, and using an additional offset
argument is doable.
It is also used the same way in drm_format_helper:
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/gpu/drm/drm_format_helper.c#L290
Best regards,
--
Jocelyn
I'll play around a bit more next week with at least protecting against
bad uses.
thanks
Lucas De Marchi
Dave.
