[Mesa-dev] [PATCH v3 4/6] nouveau: Add framebuffer modifier support

[Thierry Reding]

On Fri, Mar 02, 2018 at 12:28:02PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 2 March 2018 at 12:06, Thierry Reding <thierry.reding at gmail.com> wrote:
> > On Thu, Mar 01, 2018 at 09:37:28AM -0500, Ilia Mirkin wrote:
> >> > +static void
> >> > +nvc0_query_dmabuf_modifiers(struct pipe_screen *screen,
> >> > +                            enum pipe_format format, int max,
> >>
> >> Maybe change this to "unsigned max"? That would avoid unnecessary
> >> complications below to check if it's negative.
> >
> > I would like that very much, but I'm afraid it's probably too late to
> > change this now because the signedness is handed down directly from the
> > EGL_EXT_image_dma_buf_import_modifiers extension:
> >
> >         https://www.khronos.org/registry/EGL/extensions/EXT/EGL_EXT_image_dma_buf_import_modifiers.txt
> >
> > Adding Pekka and Daniel for visibility, maybe there were reasons for why
> > these are explicitly signed. I'm not familiar with the process of
> > getting an extension changed, but, even though a very minor change, this
> > would be changing the API of a completed extension, which doesn't seem
> > like something that you're supposed to do.
> 
> The main reason was the precedent of, e.g., eglChooseConfig using
> EGLint. It's not ideal, but then again every time I type 'int i' these
> days I get a momentary shiver not knowing if it should be an unsigned
> int or not.

It also seems like EGL doesn't even define EGLuint, which would be a
good reason why everything is signed in the EGL API.

> > That said, the revision history of the extension mentions that revision
> > 4 introduced a new type for the modifiers, so perhaps there is some
> > leeway in what we can do.
> 
> r4 was made before the extension was actually finalised/used anywhere,
> so it's not really precedent.
> 
> > I guess we could always force a cast in dri2_query_dma_buf_modifiers()
> > if changing the extension is not possible. That way we could at least
> > internally not worry about the signedness.
> 
> If you think that'd make for a better Gallium interface, by all means
> please go for it, especially if it makes sense in the context of the
> rest of the Gallium API.

Yeah, I think it'd make sense. Gallium is already fairly consistently
using unsigned for values that can never be negative.

I was already halfway through the conversion when I noticed that this
comes originally from the EGL extension, so I'll wrap all of it into a
patch and send it out for review.

Thanks,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180302/825b909c/attachment-0001.sig>