Re: [Mesa-dev] [PATCH 2/2] st/dri: replace format conversion functions with single mapping table

[Marek Olšák]

On Thu, Jun 7, 2018, 4:59 AM Lucas Stach  wrote:

> Hi Marek,
>
> thanks for the review.
>
> Am Samstag, den 26.05.2018, 00:36 -0400 schrieb Marek Olšák:
> [...]
> >
> > > @@ -983,7 +760,7 @@ dri2_update_tex_buffer(struct dri_drawable
> *drawable,
> > >
> > >  static __DRIimage *
> > >  dri2_create_image_from_winsys(__DRIscreen *_screen,
> > > -  int width, int height, int format,
> > > +  int width, int height, enum pipe_format
> pf,
> > >int num_handles, struct winsys_handle
> *whandle,
> > >void *loaderPrivate)
> > >  {
> > > @@ -992,13 +769,8 @@ dri2_create_image_from_winsys(__DRIscreen
> *_screen,
> > > __DRIimage *img;
> > > struct pipe_resource templ;
> > > unsigned tex_usage = 0;
> > > -   enum pipe_format pf;
> > > int i;
> > >
> > > -   pf = dri2_format_to_pipe_format (format);
> > > -   if (pf == PIPE_FORMAT_NONE)
> > > -  return NULL;
> > > -
> > > if (pscreen->is_format_supported(pscreen, pf, screen->target, 0,
> > >  PIPE_BIND_RENDER_TARGET))
> > >tex_usage |= PIPE_BIND_RENDER_TARGET;
> > > @@ -1006,6 +778,20 @@ dri2_create_image_from_winsys(__DRIscreen
> *_screen,
> > >  PIPE_BIND_SAMPLER_VIEW))
> > >tex_usage |= PIPE_BIND_SAMPLER_VIEW;
> > >
> > > +   if (!tex_usage && util_format_is_yuv(pf)) {
> > > +  /* YUV format sampling can be emulated by the Mesa state
> tracker by
> > > +   * using multiple R8/RG88 samplers. So try to rewrite the pipe
> format.
> > > +   */
> > > +  pf = PIPE_FORMAT_R8_UNORM;
> > > +
> > > +  if (pscreen->is_format_supported(pscreen, pf, screen->target, 0,
> > > +   PIPE_BIND_SAMPLER_VIEW))
> > > + tex_usage |= PIPE_BIND_SAMPLER_VIEW;
> > > +   }
> >
> > This doesn't seem to belong in this commit.
>
> It does. The removed conversion functions would end up with
> PIPE_FORMAT_R8_UNORM for I420 or NV12 input FOURCCs, with the implicit
> assumption that all drivers need the emulation by the Mesa state-
> tracker.
>
> The new format table maps those to the proper PIPE_FORMAT_IYUV and
> PIPE_FORMAT_NV12, so in order to not break drivers which can't handle
> those formats natively and need the Mesa side emulation we need this
> hunk of code.
>
> > Other than that, the series is:
> >
> > > Reviewed-by: Marek Olšák 
>
> With the above explanation, can I keep your review for the whole patch?
>

Yes.

Marek


> Regards,
> Lucas
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] st/dri: replace format conversion functions with single mapping table

[Lucas Stach]

Hi Marek,

thanks for the review.

Am Samstag, den 26.05.2018, 00:36 -0400 schrieb Marek Olšák:
[...]
> 
> > @@ -983,7 +760,7 @@ dri2_update_tex_buffer(struct dri_drawable *drawable,
> > 
> >  static __DRIimage *
> >  dri2_create_image_from_winsys(__DRIscreen *_screen,
> > -                              int width, int height, int format,
> > +                              int width, int height, enum pipe_format pf,
> >                                int num_handles, struct winsys_handle 
> > *whandle,
> >                                void *loaderPrivate)
> >  {
> > @@ -992,13 +769,8 @@ dri2_create_image_from_winsys(__DRIscreen *_screen,
> >     __DRIimage *img;
> >     struct pipe_resource templ;
> >     unsigned tex_usage = 0;
> > -   enum pipe_format pf;
> >     int i;
> > 
> > -   pf = dri2_format_to_pipe_format (format);
> > -   if (pf == PIPE_FORMAT_NONE)
> > -      return NULL;
> > -
> >     if (pscreen->is_format_supported(pscreen, pf, screen->target, 0,
> >                                      PIPE_BIND_RENDER_TARGET))
> >        tex_usage |= PIPE_BIND_RENDER_TARGET;
> > @@ -1006,6 +778,20 @@ dri2_create_image_from_winsys(__DRIscreen *_screen,
> >                                      PIPE_BIND_SAMPLER_VIEW))
> >        tex_usage |= PIPE_BIND_SAMPLER_VIEW;
> > 
> > +   if (!tex_usage && util_format_is_yuv(pf)) {
> > +      /* YUV format sampling can be emulated by the Mesa state tracker by
> > +       * using multiple R8/RG88 samplers. So try to rewrite the pipe 
> > format.
> > +       */
> > +      pf = PIPE_FORMAT_R8_UNORM;
> > +
> > +      if (pscreen->is_format_supported(pscreen, pf, screen->target, 0,
> > +                                       PIPE_BIND_SAMPLER_VIEW))
> > +         tex_usage |= PIPE_BIND_SAMPLER_VIEW;
> > +   }
> 
> This doesn't seem to belong in this commit.

It does. The removed conversion functions would end up with
PIPE_FORMAT_R8_UNORM for I420 or NV12 input FOURCCs, with the implicit
assumption that all drivers need the emulation by the Mesa state-
tracker.

The new format table maps those to the proper PIPE_FORMAT_IYUV and
PIPE_FORMAT_NV12, so in order to not break drivers which can't handle
those formats natively and need the Mesa side emulation we need this
hunk of code.

> Other than that, the series is:
> 
> > Reviewed-by: Marek Olšák 

With the above explanation, can I keep your review for the whole patch?

Regards,
Lucas
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] st/dri: replace format conversion functions with single mapping table

[Marek Olšák]

On Thu, May 17, 2018 at 6:50 AM, Lucas Stach  wrote:

> Each time I have to touch the buffer import/export functions in the dri
> state tracker I get lost in the maze of functions converting between
> DRI_IMAGE_FOURCC, DRI_IMAGE_FORMAT, DRI_IMAGE_COMPONENTS and pipe format.
>
> Rip it out and replace by a single table, which defines the correspondence
> between the different representations.
>
> Also this now stores all the known representations in the __DRIimageRec,
> to avoid the loss of information we currently have when importing a buffer
> with a fourcc, which doesn't have a corresponding dri format.
>
> Signed-off-by: Lucas Stach 
> ---
>  src/gallium/state_trackers/dri/dri2.c   | 476 ++--
>  src/gallium/state_trackers/dri/dri_screen.h |   1 +
>  2 files changed, 138 insertions(+), 339 deletions(-)
>
> diff --git a/src/gallium/state_trackers/dri/dri2.c
> b/src/gallium/state_trackers/dri/dri2.c
> index 859161fb87ac..9c74ca54fc89 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -54,295 +54,72 @@
>  #define DRM_FORMAT_MOD_INVALID ((1ULL<<56) - 1)
>  #endif
>
> -static const int fourcc_formats[] = {
> -   __DRI_IMAGE_FOURCC_ARGB2101010,
> -   __DRI_IMAGE_FOURCC_XRGB2101010,
> -   __DRI_IMAGE_FOURCC_ABGR2101010,
> -   __DRI_IMAGE_FOURCC_XBGR2101010,
> -   __DRI_IMAGE_FOURCC_ARGB,
> -   __DRI_IMAGE_FOURCC_ABGR,
> -   __DRI_IMAGE_FOURCC_SARGB,
> -   __DRI_IMAGE_FOURCC_XRGB,
> -   __DRI_IMAGE_FOURCC_XBGR,
> -   __DRI_IMAGE_FOURCC_ARGB1555,
> -   __DRI_IMAGE_FOURCC_RGB565,
> -   __DRI_IMAGE_FOURCC_R8,
> -   __DRI_IMAGE_FOURCC_R16,
> -   __DRI_IMAGE_FOURCC_GR88,
> -   __DRI_IMAGE_FOURCC_GR1616,
> -   __DRI_IMAGE_FOURCC_YUV410,
> -   __DRI_IMAGE_FOURCC_YUV411,
> -   __DRI_IMAGE_FOURCC_YUV420,
> -   __DRI_IMAGE_FOURCC_YUV422,
> -   __DRI_IMAGE_FOURCC_YUV444,
> -   __DRI_IMAGE_FOURCC_YVU410,
> -   __DRI_IMAGE_FOURCC_YVU411,
> -   __DRI_IMAGE_FOURCC_YVU420,
> -   __DRI_IMAGE_FOURCC_YVU422,
> -   __DRI_IMAGE_FOURCC_YVU444,
> -   __DRI_IMAGE_FOURCC_NV12,
> -   __DRI_IMAGE_FOURCC_NV16,
> -   __DRI_IMAGE_FOURCC_YUYV
> -};
> -
> -static int convert_fourcc(int format, int *dri_components_p)
> -{
> +struct dri2_format_mapping {
> +   int dri_fourcc;
> +   int dri_format;
> int dri_components;
> -   switch(format) {
> -   case __DRI_IMAGE_FOURCC_RGB565:
> -  format = __DRI_IMAGE_FORMAT_RGB565;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RGB;
> -  break;
> -   case __DRI_IMAGE_FOURCC_ARGB:
> -  format = __DRI_IMAGE_FORMAT_ARGB;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RGBA;
> -  break;
> -   case __DRI_IMAGE_FOURCC_XRGB:
> -  format = __DRI_IMAGE_FORMAT_XRGB;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RGB;
> -  break;
> -   case __DRI_IMAGE_FOURCC_ABGR:
> -  format = __DRI_IMAGE_FORMAT_ABGR;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RGBA;
> -  break;
> -   case __DRI_IMAGE_FOURCC_XBGR:
> -  format = __DRI_IMAGE_FORMAT_XBGR;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RGB;
> -  break;
> -   case __DRI_IMAGE_FOURCC_ARGB2101010:
> -  format = __DRI_IMAGE_FORMAT_ARGB2101010;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RGBA;
> -  break;
> -   case __DRI_IMAGE_FOURCC_XRGB2101010:
> -  format = __DRI_IMAGE_FORMAT_XRGB2101010;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RGB;
> -  break;
> -   case __DRI_IMAGE_FOURCC_ABGR2101010:
> -  format = __DRI_IMAGE_FORMAT_ABGR2101010;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RGBA;
> -  break;
> -   case __DRI_IMAGE_FOURCC_XBGR2101010:
> -  format = __DRI_IMAGE_FORMAT_XBGR2101010;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RGB;
> -  break;
> -   case __DRI_IMAGE_FOURCC_R8:
> -  format = __DRI_IMAGE_FORMAT_R8;
> -  dri_components = __DRI_IMAGE_COMPONENTS_R;
> -  break;
> -   case __DRI_IMAGE_FOURCC_GR88:
> -  format = __DRI_IMAGE_FORMAT_GR88;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RG;
> -  break;
> -   case __DRI_IMAGE_FOURCC_R16:
> -  format = __DRI_IMAGE_FORMAT_R16;
> -  dri_components = __DRI_IMAGE_COMPONENTS_R;
> -  break;
> -   case __DRI_IMAGE_FOURCC_GR1616:
> -  format = __DRI_IMAGE_FORMAT_GR1616;
> -  dri_components = __DRI_IMAGE_COMPONENTS_RG;
> -  break;
> -   case __DRI_IMAGE_FOURCC_YUYV:
> -  format = __DRI_IMAGE_FORMAT_YUYV;
> -  dri_components = __DRI_IMAGE_COMPONENTS_Y_XUXV;
> -  break;
> -   /*
> -* For multi-planar YUV formats, we return the format of the first
> -* plane only.  Since there is only one caller which supports multi-
> -* planar YUV it gets to figure out the remaining planes on it's
> -* own.
> -*/
> -   case __DRI_IMAGE_FOURCC_YUV420:
> -   case __DRI_IMAGE_FOURCC_YVU420:
> -  format = __DRI_IMAGE_FORMAT_R8;
> -  dri_components = __DRI_IMAGE_COMPONENTS_Y_U_V;
> -  

[Mesa-dev] [PATCH 2/2] st/dri: replace format conversion functions with single mapping table

[Lucas Stach]

Each time I have to touch the buffer import/export functions in the dri
state tracker I get lost in the maze of functions converting between
DRI_IMAGE_FOURCC, DRI_IMAGE_FORMAT, DRI_IMAGE_COMPONENTS and pipe format.

Rip it out and replace by a single table, which defines the correspondence
between the different representations.

Also this now stores all the known representations in the __DRIimageRec,
to avoid the loss of information we currently have when importing a buffer
with a fourcc, which doesn't have a corresponding dri format.

Signed-off-by: Lucas Stach 
---
 src/gallium/state_trackers/dri/dri2.c   | 476 ++--
 src/gallium/state_trackers/dri/dri_screen.h |   1 +
 2 files changed, 138 insertions(+), 339 deletions(-)

diff --git a/src/gallium/state_trackers/dri/dri2.c 
b/src/gallium/state_trackers/dri/dri2.c
index 859161fb87ac..9c74ca54fc89 100644
--- a/src/gallium/state_trackers/dri/dri2.c
+++ b/src/gallium/state_trackers/dri/dri2.c
@@ -54,295 +54,72 @@
 #define DRM_FORMAT_MOD_INVALID ((1ULL<<56) - 1)
 #endif
 
-static const int fourcc_formats[] = {
-   __DRI_IMAGE_FOURCC_ARGB2101010,
-   __DRI_IMAGE_FOURCC_XRGB2101010,
-   __DRI_IMAGE_FOURCC_ABGR2101010,
-   __DRI_IMAGE_FOURCC_XBGR2101010,
-   __DRI_IMAGE_FOURCC_ARGB,
-   __DRI_IMAGE_FOURCC_ABGR,
-   __DRI_IMAGE_FOURCC_SARGB,
-   __DRI_IMAGE_FOURCC_XRGB,
-   __DRI_IMAGE_FOURCC_XBGR,
-   __DRI_IMAGE_FOURCC_ARGB1555,
-   __DRI_IMAGE_FOURCC_RGB565,
-   __DRI_IMAGE_FOURCC_R8,
-   __DRI_IMAGE_FOURCC_R16,
-   __DRI_IMAGE_FOURCC_GR88,
-   __DRI_IMAGE_FOURCC_GR1616,
-   __DRI_IMAGE_FOURCC_YUV410,
-   __DRI_IMAGE_FOURCC_YUV411,
-   __DRI_IMAGE_FOURCC_YUV420,
-   __DRI_IMAGE_FOURCC_YUV422,
-   __DRI_IMAGE_FOURCC_YUV444,
-   __DRI_IMAGE_FOURCC_YVU410,
-   __DRI_IMAGE_FOURCC_YVU411,
-   __DRI_IMAGE_FOURCC_YVU420,
-   __DRI_IMAGE_FOURCC_YVU422,
-   __DRI_IMAGE_FOURCC_YVU444,
-   __DRI_IMAGE_FOURCC_NV12,
-   __DRI_IMAGE_FOURCC_NV16,
-   __DRI_IMAGE_FOURCC_YUYV
-};
-
-static int convert_fourcc(int format, int *dri_components_p)
-{
+struct dri2_format_mapping {
+   int dri_fourcc;
+   int dri_format;
int dri_components;
-   switch(format) {
-   case __DRI_IMAGE_FOURCC_RGB565:
-  format = __DRI_IMAGE_FORMAT_RGB565;
-  dri_components = __DRI_IMAGE_COMPONENTS_RGB;
-  break;
-   case __DRI_IMAGE_FOURCC_ARGB:
-  format = __DRI_IMAGE_FORMAT_ARGB;
-  dri_components = __DRI_IMAGE_COMPONENTS_RGBA;
-  break;
-   case __DRI_IMAGE_FOURCC_XRGB:
-  format = __DRI_IMAGE_FORMAT_XRGB;
-  dri_components = __DRI_IMAGE_COMPONENTS_RGB;
-  break;
-   case __DRI_IMAGE_FOURCC_ABGR:
-  format = __DRI_IMAGE_FORMAT_ABGR;
-  dri_components = __DRI_IMAGE_COMPONENTS_RGBA;
-  break;
-   case __DRI_IMAGE_FOURCC_XBGR:
-  format = __DRI_IMAGE_FORMAT_XBGR;
-  dri_components = __DRI_IMAGE_COMPONENTS_RGB;
-  break;
-   case __DRI_IMAGE_FOURCC_ARGB2101010:
-  format = __DRI_IMAGE_FORMAT_ARGB2101010;
-  dri_components = __DRI_IMAGE_COMPONENTS_RGBA;
-  break;
-   case __DRI_IMAGE_FOURCC_XRGB2101010:
-  format = __DRI_IMAGE_FORMAT_XRGB2101010;
-  dri_components = __DRI_IMAGE_COMPONENTS_RGB;
-  break;
-   case __DRI_IMAGE_FOURCC_ABGR2101010:
-  format = __DRI_IMAGE_FORMAT_ABGR2101010;
-  dri_components = __DRI_IMAGE_COMPONENTS_RGBA;
-  break;
-   case __DRI_IMAGE_FOURCC_XBGR2101010:
-  format = __DRI_IMAGE_FORMAT_XBGR2101010;
-  dri_components = __DRI_IMAGE_COMPONENTS_RGB;
-  break;
-   case __DRI_IMAGE_FOURCC_R8:
-  format = __DRI_IMAGE_FORMAT_R8;
-  dri_components = __DRI_IMAGE_COMPONENTS_R;
-  break;
-   case __DRI_IMAGE_FOURCC_GR88:
-  format = __DRI_IMAGE_FORMAT_GR88;
-  dri_components = __DRI_IMAGE_COMPONENTS_RG;
-  break;
-   case __DRI_IMAGE_FOURCC_R16:
-  format = __DRI_IMAGE_FORMAT_R16;
-  dri_components = __DRI_IMAGE_COMPONENTS_R;
-  break;
-   case __DRI_IMAGE_FOURCC_GR1616:
-  format = __DRI_IMAGE_FORMAT_GR1616;
-  dri_components = __DRI_IMAGE_COMPONENTS_RG;
-  break;
-   case __DRI_IMAGE_FOURCC_YUYV:
-  format = __DRI_IMAGE_FORMAT_YUYV;
-  dri_components = __DRI_IMAGE_COMPONENTS_Y_XUXV;
-  break;
-   /*
-* For multi-planar YUV formats, we return the format of the first
-* plane only.  Since there is only one caller which supports multi-
-* planar YUV it gets to figure out the remaining planes on it's
-* own.
-*/
-   case __DRI_IMAGE_FOURCC_YUV420:
-   case __DRI_IMAGE_FOURCC_YVU420:
-  format = __DRI_IMAGE_FORMAT_R8;
-  dri_components = __DRI_IMAGE_COMPONENTS_Y_U_V;
-  break;
-   case __DRI_IMAGE_FOURCC_NV12:
-  format = __DRI_IMAGE_FORMAT_R8;
-  dri_components = __DRI_IMAGE_COMPONENTS_Y_UV;
-  break;
-   default:
-  return -1;
-   }
-   *dri_components_p = dri_components;
-   return format;
-}
-
-/* NOTE this probably isn't going to do the right thing for YUV images
- * (but I