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

[Dmitry Baryshkov]

On Mon, Dec 16, 2024 at 12:40:54AM -0500, Marek Olšák wrote:
> git send-email (or rather the way it sends email) has been banned by gmail
> due to being considered unsecure. I don't plan to find a way to make it
> work and I don't plan to use a different email provider. It doesn't matter
> because I'll be the committer of this patch in our amd-staging-drm-next
> branch.

I'm sorry, but I'd second Joshua. As a community we use certain methods
and certain approaches which makes it easier for other people to review
your patches. One of those includes sending patches in plain text
without any attachments, etc (documented under Documentation/process/).
All my patches are being sent using git-send-email or b4 send via GMail.
Other developers use web-relay provided by the B4 tool.

Next, the MAINTAINERS file lists Alex, Christian and Xinhui as
maintainers of the drm/amd tree. If the file is incorrect or incomplete,
please correct it.

Last, but not least, this patch will likely go into drm-misc-next or
drm-next instead of amd-saging-drm-next. It is not AMD-specific.

> Let's talk about the concept and brokenness of DRM_FORMAT_MOD_LINEAR, not
> send-email.

The biggest problem with your approach is tht it is not clear which
modifier to use. For example, if one of the formats requires 128-byte
alignment, should the driver list just 128B or both 128B and 256B? If at
some point we add 32B alignment, will we have to update the drivers?
Which format should be used for exported buffers? Please provide
corresponding driver patches that utilize new uAPI.

Also, please don't forget the backwards compatibility issue. If we
follow this approach, the drivers have to list both LINEAR and new
PITCH_ALIGN modifiers. So the userspace still will attempt to use
LINEAR.

It is true that such requirements are platform-specific and are usually
encoded in the compostitor. I think it might make more sense to export
the pitch requirements using the extra hint-like property, which then
can be used by a smart userspace.

> Marek
> 
> On Sun, Dec 15, 2024 at 9:08 PM Joshua Ashton <joshua at froggi.es> wrote:
> 
> > Not really for my benefit, more that it's the proper thing to do if you
> > want anyone to apply your patch.
> >
> > You should really just be using git send-email.
> >
> > On 12/15/24 11:57 PM, Marek Olšák wrote:
> > > Only for you: Attached.
> > >
> > > Marek
> > >
> > > On Sun, Dec 15, 2024 at 6:37 PM Joshua Ashton <joshua at froggi.es
> > > <mailto:joshua at froggi.es>> wrote:
> > >
> > >     You should just re-send the whole patch, probably.
> > >
> > >     On 12/15/24 8:54 PM, Marek Olšák wrote:
> > >      > Missed 2 lines from the diff:
> > >      >
> > >      > +#define DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_128B
> > >     fourcc_mod_code(NONE, 2)
> > >      > +#define DRM_FORMAT_MOD_LINEAR_PITCH_ALIGN_256B
> > >     fourcc_mod_code(NONE, 3)
> > >      >
> > >      > Marek
> > >      >
> > >      > On Sun, Dec 15, 2024 at 3:53 PM Marek Olšák <maraeo at gmail.com
> > >     <mailto:maraeo at gmail.com>
> > >      > <mailto:maraeo at gmail.com <mailto:maraeo at gmail.com>>> wrote:
> > >      >
> > >      >     The comment explains the problem with DRM_FORMAT_MOD_LINEAR.
> > >      >
> > >      >     Signed-off-by: Marek Olšák <marek.olsak at amd.com
> > >     <mailto:marek.olsak at amd.com>
> > >      >     <mailto:marek.olsak at amd.com <mailto:marek.olsak at amd.com>>>
> > >      >
> > >      >     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)
> > >      >
> > >
> > >     - Joshie 🐸✨
> > >
> >
> > - Joshie 🐸✨
> >
> >

-- 
With best wishes
Dmitry