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

[Daniel Stone]

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.

> 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.

Cheers,
Daniel