[PATCH xserver 3/3] modesetting/drmmode: Use drmModeGetFB2

[Daniel Stone]

Hi Emil,

On 26 March 2018 at 16:22, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 23 March 2018 at 13:50, Daniel Stone <daniels at collabora.com> wrote:
>> Much like AddFB -> AddFB2, GetFB2 lets us get multiple buffers back as
>> well as modifier information. This lets us use -background none with
>> multiplanar formats, or modifiers which can't be inferred via a magic
>> side channel.
>>
> AFAICT there's nothing special (or wrong) with the new API.
>
> The flags field is a bit of an open question - should Xserver or
> libdrm manage the value passed to the kernel?
> Analogously - should we pass the flags back to the user (drmModeFB2::flags)?
>
> Current design seems perfectly fine IMHO, although others might disagree.

Thanks for looking at this! I think the flags question is a good one,
and that we should probably make the field RW: userspace declaring
what it supports (analagous to AddFB2), and the kernel overwriting
that with the intersection of what userspace and the kernel support
(not analogous, but since it's a getter rather than an add ...).

>> @@ -1090,31 +1094,63 @@ create_pixmap_for_fbcon(drmmode_ptr drmmode, ScrnInfoPtr pScrn, int fbcon_id)
>>      if (pixmap)
>>          return pixmap;
>>
>> -    fbcon = drmModeGetFB(drmmode->fd, fbcon_id);
>> -    if (fbcon == NULL)
>> -        return NULL;
>> +    fbcon2 = drmModeGetFB2(drmmode->fd, fbcon_id);
>> +    if (fbcon2) {
> Declare and initialize fbcon2 in one go - C99 feature that everyone
> has (even the people who use MSVC to build Xserver & friends).
> Then wrap the whole fbcon2 hunk in a #ifdef HAVE_GETFB2 + add a
> configure/meson check.

Sure; the only reason I didn't add an ifdef check is because there's
no version released with it yet.

> Alternatively add a weak drmModeGetFB2 function which returns NULL and
> forget all the above ;-)

If you have a good suggestion for implementing weak symbols then I'm
all ears, but last I saw from the Mesa experience no-one could figure
out how to do it properly.

>> +        width = fbcon2->width;
>> +        height = fbcon2->height;
>> +        memcpy(handles, fbcon2->handles, sizeof(handles));
>> +        memcpy(strides, fbcon2->pitches, sizeof(strides));
>> +        memcpy(offsets, fbcon2->offsets, sizeof(offsets));
>> +        modifier = fbcon2->modifier;
>> +        switch (fbcon2->pixel_format) {
>> +        case DRM_FORMAT_RGB565:
>> +            depth = 16;
> Missing bpp?

Correct.

> Idea: Instead of introducing another format <> {depth, bpp} mapping,
> might as well add some helpers?

I can only see that mapping inside drmmode_create_bo, which is
different as it creates everything with an alpha channel, i.e.
s/XRGB/ARGB/. Are there some others I'm missing?

>> +            break;
>> +        case DRM_FORMAT_XRGB8888:
>> +            depth = 24;
>> +            bpp = 32;
>> +            break;
>> +        case DRM_FORMAT_XRGB2101010:
>> +            depth = 30;
>> +            bpp = 32;
>> +        default:
>> +            break;
> Error instead of silently continuing?

Sure.

Cheers,
Daniel