Re: [RFC xserver 05/16] DRI3/Glamor: Implement PixmapFromBuffers request

[Daniel Stone]

Hi,

On 28 July 2017 at 00:03, Eric Anholt  wrote:
> Daniel Stone  writes:
>> +{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .bpp = 32 },
>> +{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .bpp = 32 },
>> +{ .format = DRM_FORMAT_RGBA1010102, .depth = 30, .bpp = 32 },
>> +{ .format = DRM_FORMAT_BGRA1010102, .depth = 30, .bpp = 32 },
>
> Regardless of the resolution of the core rendering/depth/etc discussion
> elsewhere, these 4 would be depth 32.

Yeah, obviously correct. Thanks for the detailed look. :)

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC xserver 05/16] DRI3/Glamor: Implement PixmapFromBuffers request

[Eric Anholt]

Daniel Stone  writes:

> From: Louis-Francis Ratté-Boulianne 
>
> Signed-off-by: Louis-Francis Ratté-Boulianne 
> Signed-off-by: Daniel Stone 
> ---

> +{ .format = DRM_FORMAT_ARGB2101010, .depth = 30, .bpp = 32 },
> +{ .format = DRM_FORMAT_ABGR2101010, .depth = 30, .bpp = 32 },
> +{ .format = DRM_FORMAT_RGBA1010102, .depth = 30, .bpp = 32 },
> +{ .format = DRM_FORMAT_BGRA1010102, .depth = 30, .bpp = 32 },

Regardless of the resolution of the core rendering/depth/etc discussion
elsewhere, these 4 would be depth 32.


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC xserver 05/16] DRI3/Glamor: Implement PixmapFromBuffers request

[Emil Velikov]

Hi Dan, Louis,

Seems like an earlier patch (03/16) introduces a broken behaviour only
to be fixed here...

On 8 June 2017 at 19:43, Daniel Stone  wrote:

> @@ -159,11 +196,13 @@ proc_dri3_pixmap_from_buffer(ClientPtr client)
>  if (fd < 0)
>  return BadValue;
>
> -rc = dri3_pixmap_from_fd(,
> - drawable->pScreen, fd,
> - stuff->width, stuff->height,
> - stuff->stride, stuff->depth,
> - stuff->bpp);
> +offset = 0;
> +stride = stuff->stride;
> +format = drm_format_for_depth(stuff->bpp, stuff->depth);
> +rc = dri3_pixmap_from_fds(,
> +  drawable->pScreen, 1, ,
> +  stuff->width, stuff->height,
> +  , , format, 0);

... this type of hunks is what I have in mind.

As the diff suggests the patch is trying to do way too much at the
same time, IMHO.
- A: Introduce the interface (dri3/dri3.h)
- B: add the implementation
- C: fixes the user (introduced just a patch ago) to use the correct interface
- D: adds the backend implementations

I think it's great to split it roughly as
 - patch X: (A+B)
 - patch X+1: C (squashed with 03/16)
 - patch X+n: D - add backend implementations, all or one at a time

It will be easier to review while no intermittent break will be introduced ;-)

Additionally, the ifndef(EGL_extension...) should be dropped with
fall-back definitions provided as applicable. From a quick look I
cannot find where you're checking the extensions prior to using them.
Search for GLAMOR_CHECK_EGL_EXTENSION and "The EGL layer needs to have
the following" details.

HTH
Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC xserver 05/16] DRI3/Glamor: Implement PixmapFromBuffers request

[Daniel Stone]

Hi Michel,

On 9 June 2017 at 03:36, Michel Dänzer  wrote:
> On 09/06/17 03:43 AM, Daniel Stone wrote:
>> @@ -66,7 +68,7 @@ typedef struct dri3_screen_info {
>>  uint32_tversion;
>>
>>  dri3_open_proc  open;
>> -dri3_pixmap_from_fd_procpixmap_from_fd;
>> +dri3_pixmap_from_fds_proc   pixmap_from_fds;
>>  dri3_fd_from_pixmap_procfd_from_pixmap;
>>  dri3_get_formats_proc   get_formats;
>>  dri3_get_modifiers_proc get_modifiers;
>
> Completely dropping the pixmap_from_fd hook might be a bit harsh. It
> would mean drivers without pixmap_from_fds support (e.g.
> xf86-video-amdgpu/ati with EXA) can't support DRI3 at all, right?

Yeah, I think we'd missed that. It sounds like it'd be much better to
just add a new hook; this also makes the implementation patches split
out much more nicely, as the request handler and definitions can be
added completely separately to the Glamor support. Thanks for the
suggestion!

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC xserver 05/16] DRI3/Glamor: Implement PixmapFromBuffers request

[Michel Dänzer]

On 09/06/17 03:43 AM, Daniel Stone wrote:
> From: Louis-Francis Ratté-Boulianne 
> 
> Signed-off-by: Louis-Francis Ratté-Boulianne 
> Signed-off-by: Daniel Stone 

[...]

> diff --git a/dri3/dri3.h b/dri3/dri3.h
> index 40b8474c0..4d6699f5e 100644
> --- a/dri3/dri3.h
> +++ b/dri3/dri3.h
> @@ -39,13 +39,15 @@ typedef int (*dri3_open_client_proc)(ClientPtr client,
>   RRProviderPtr provider,
>   int *fd);
>  
> -typedef PixmapPtr (*dri3_pixmap_from_fd_proc) (ScreenPtr screen,
> -   int fd,
> -   CARD16 width,
> -   CARD16 height,
> -   CARD16 stride,
> -   CARD8 depth,
> -   CARD8 bpp);
> +typedef PixmapPtr (*dri3_pixmap_from_fds_proc) (ScreenPtr screen,
> +CARD8 num_fds,
> +int *fds,
> +CARD16 width,
> +CARD16 height,
> +CARD32 *strides,
> +CARD32 *offsets,
> +CARD32 format,
> +uint64_t modifiers);
>  
>  typedef int (*dri3_fd_from_pixmap_proc) (ScreenPtr screen,
>   PixmapPtr pixmap,
> @@ -66,7 +68,7 @@ typedef struct dri3_screen_info {
>  uint32_tversion;
>  
>  dri3_open_proc  open;
> -dri3_pixmap_from_fd_procpixmap_from_fd;
> +dri3_pixmap_from_fds_proc   pixmap_from_fds;
>  dri3_fd_from_pixmap_procfd_from_pixmap;
>  dri3_get_formats_proc   get_formats;
>  dri3_get_modifiers_proc get_modifiers;

Completely dropping the pixmap_from_fd hook might be a bit harsh. It
would mean drivers without pixmap_from_fds support (e.g.
xf86-video-amdgpu/ati with EXA) can't support DRI3 at all, right?


> +static PixmapPtr
> +glamor_pixmap_from_fds(ScreenPtr screen,
> +   CARD8 num_fds, int *fds,
> +   CARD16 width, CARD16 height,
> +   CARD32 *strides, CARD32 *offsets,
> +   CARD32 format, uint64_t modifier)
>  {

[...]

> @@ -612,7 +748,7 @@ glamor_dri3_open_client(ClientPtr client,
>  static dri3_screen_info_rec glamor_dri3_info = {
>  .version = 1,
>  .open_client = glamor_dri3_open_client,
> -.pixmap_from_fd = glamor_pixmap_from_fd,
> +.pixmap_from_fds = glamor_pixmap_from_fds,
>  .fd_from_pixmap = glamor_fd_from_pixmap,
>  .get_formats = glamor_get_formats,
>  .get_modifiers = glamor_get_modifiers,

glamor_pixmap_from_fds also needs to be exported to drivers.


Same corresponding comments on patch 6.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel