Re: [PATCH] media: uvcvideo: Explicit alignment of uvc_frame and uvc_format

[Ricardo Ribalda]

Hi Laurent

On Fri, 22 Mar 2024 at 12:56, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Mon, May 01, 2023 at 04:49:31PM +0200, Ricardo Ribalda wrote:
> > Struct uvc_frame and uvc_format are packaged together on
> > streaming->formats on a sigle allocation.
>
> s/sigle/single/
>
> >
> > This is working fine because both structures have a field with a
> > pointer, but it will stop working when the sizeof() of any of those
> > structs is not a muliple of the sizeof(void*).
> >
> > Make that aligment contract explicit.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > ---
> > This is better than 3 allocations, and do not have any performance
> > penalty.
> > ---
> >  drivers/media/usb/uvc/uvcvideo.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 9a596c8d894a..03e8a543c8e6 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -252,7 +252,7 @@ struct uvc_frame {
> >       u8  bFrameIntervalType;
> >       u32 dwDefaultFrameInterval;
> >       u32 *dwFrameInterval;
> > -};
> > +} __aligned(sizeof(void *)); /* uvc_frame is packed on streaming->formats. */
>
> Don't we need u32 alignment here, not void * alignment, given that
> uvc_frame is followed by an array of u32 ?

Let me make sure that I explain myself :)

I made a small program in compiler explorer:
https://godbolt.org/z/7s9z8WTsx that shows the error that I want to
avoid

When we have a structure like this:

struct n_foo_bar {
   int n;
   struct foo *foo;
   struct bar *bar;
};

We expect that *foo and *bar point to memory addresses with the right
cpu alignment for each struct. Otherwise accessing foo and bar could
be slow or simply not work.

In the driver we are doing something like this to allocate the structure:

int size
struct n_foo_bar *out;

size = n*sizeof(struct foo)+n*sizeof(struct bar) +sizeof(struct n_foo_bar);
out = malloc(size);
if (!out)
  return out;

out->foo=(void *)(out)+sizeof(struct n_foo_bar);
out->bar=(void *)(out->foo)+n*sizeof(struct foo);

But that only works if sizeof(struct foo) is a multiple of the
alignment required by struct bar. We are "lucky" now because we have a
pointer in each struct and that gives us a void* padding. ... but if
we ever remove that pointer from the structure we will be in a bad
position.

With the  __aligned(sizeof(void *)); I want to explicitly say:

"Ey, this struct is embedded in another struct and they are allocated
contiguously"

Does it make more sense now?

>
> >
> >  struct uvc_format {
> >       u8 type;
> > @@ -266,7 +266,7 @@ struct uvc_format {
> >
> >       unsigned int nframes;
> >       struct uvc_frame *frame;
> > -};
> > +} __aligned(sizeof(void *)); /* uvc_format is packed on streaming->formats. */
>
> Same here, technically we need to ensure that the following uvc_frame
> will be aligned. void * alignment will give us that now, but that's not
> the actual constraint.
>
> Wouldn't it be better to handle the alignment constraints explicitly
> when allocating the memory ? It's not that uvc_frame and uvc_format have
> intrinsic alignment constraints, the constraints are only needed because
> of the way memory is allocated.
>
> >
> >  struct uvc_streaming_header {
> >       u8 bNumFormats;
> >
> > ---
> > base-commit: 58390c8ce1bddb6c623f62e7ed36383e7fa5c02f
> > change-id: 20230501-uvc-align-6ff202b68dab
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda