Re: [PATCH] drm/fourcc: add LINEAR modifiers with an exact pitch alignment

On Mon, Dec 16, 2024 at 5:46 AM Lucas Stach <[email protected]> wrote:

> Am Montag, dem 16.12.2024 um 10:27 +0100 schrieb Michel Dänzer:
> > On 2024-12-15 21:53, Marek Olšák wrote:
> > > The comment explains the problem with DRM_FORMAT_MOD_LINEAR.
> > >
> > > Signed-off-by: Marek Olšák <[email protected] <mailto:
> [email protected]>>
> > >
> > > diff --git a/include/uapi/drm/drm_fourcc.h
> b/include/uapi/drm/drm_fourcc.h
> > > index 78abd819fd62e..8ec4163429014 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -484,9 +484,27 @@ extern "C" {
> > >   * modifier (e.g. not setting DRM_MODE_FB_MODIFIERS in the DRM_ADDFB2
> ioctl),
> > >   * which tells the driver to also take driver-internal information
> into account
> > >   * and so might actually result in a tiled framebuffer.
> > > + *
> > > + * WARNING:
> > > + * There are drivers out there that expose DRM_FORMAT_MOD_LINEAR, but
> only
> > > + * support a certain pitch alignment and can't import images with
> this modifier
> > > + * if the pitch alignment isn't exactly the one supported. They can
> however
> > > + * allocate images with this modifier and other drivers can import
> them only
> > > + * if they support the same pitch alignment. Thus,
> DRM_FORMAT_MOD_LINEAR is
> > > + * fundamentically incompatible across devices and is the only
> modifier that
> > > + * has a chance of not working. The PITCH_ALIGN modifiers should be
> used
> > > + * instead.
> > >   */
> > >  #define DRM_FORMAT_MOD_LINEAR  fourcc_mod_code(NONE, 0)
> > >
> > > +/* Linear layout modifiers with an explicit pitch alignment in bytes.
> > > + * Exposing this modifier requires that the pitch alignment is exactly
> > > + * the number in the definition.
> > > + */
> > > +#define DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_64B fourcc_mod_code(NONE, 1)
> >
> > It's not clear what you mean by "requires that the pitch alignment is
> exactly
> > the number in the definition", since a pitch which is aligned to 256
> bytes is
> > also aligned to 128 & 64 bytes. Do you mean the pitch must be exactly
> the width
> > rounded up to the next / smallest possible multiple of the specified
> number of bytes?
>
> I guess that's the intention here, as some AMD GPUs apparently have
> this limitation that they need an exact aligned pitch.
>
> If we open the can of worms to overhaul the linear modifier, I think it
> would make sense to also add modifiers for the more common restriction,
> where the pitch needs to be aligned to a specific granule, but the
> engine doesn't care if things get overaligned to a multiple of the
> granule. Having both sets would probably make it easier for the reader
> to see the difference to the exact pitch modifiers proposed in this
> patch.
>

That's a good point.

It could be:
- LINEAR_PITCH_ALIGN_EXACT_#B
- LINEAR_PITCH_ALIGN_MULTIPLE_#B

Drivers that expose the MULTIPLE ones should also expose the EXACT ones
that are equivalent. Other drivers will only expose the EXACT ones but not
the MULTIPLE ones.

Marek