[Mesa-dev] [PATCH 1/8] etnaviv: create optional 2d pipe in screen

[Philipp Zabel]

On Fri, 2019-04-12 at 19:38 +0200, Lucas Stach wrote:
> The 2D pipe is useful to implement fast planar and interleaved YUV buffer
> imports. Not all systems have a 2D capable core, so this is strictly
> optional.
> 
> Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
> ---
>  src/gallium/drivers/etnaviv/etnaviv_context.c |  6 ++
>  src/gallium/drivers/etnaviv/etnaviv_context.h |  1 +
>  src/gallium/drivers/etnaviv/etnaviv_screen.c  | 68 +++++++++++++++++++
>  src/gallium/drivers/etnaviv/etnaviv_screen.h  |  6 ++
>  4 files changed, 81 insertions(+)
> 
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_context.c b/src/gallium/drivers/etnaviv/etnaviv_context.c
> index a59338490b62..631f551d0ad4 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_context.c
> +++ b/src/gallium/drivers/etnaviv/etnaviv_context.c
> @@ -78,6 +78,9 @@ etna_context_destroy(struct pipe_context *pctx)
>     if (ctx->stream)
>        etna_cmd_stream_del(ctx->stream);
>  
> +   if (ctx->stream2d)
> +      etna_cmd_stream_del(ctx->stream2d);
> +
>     slab_destroy_child(&ctx->transfer_pool);
>  
>     if (ctx->in_fence_fd != -1)
> @@ -434,6 +437,9 @@ etna_context_create(struct pipe_screen *pscreen, void *priv, unsigned flags)
>     if (ctx->stream == NULL)
>        goto fail;
>  
> +   if (screen->pipe2d)
> +      ctx->stream2d = etna_cmd_stream_new(screen->pipe2d, 0x1000, NULL, NULL);
> +
>     /* context ctxate setup */
>     ctx->specs = screen->specs;
>     ctx->screen = screen;
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_context.h b/src/gallium/drivers/etnaviv/etnaviv_context.h
> index a79d739100d9..2c6e5d6c3db1 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_context.h
> +++ b/src/gallium/drivers/etnaviv/etnaviv_context.h
> @@ -110,6 +110,7 @@ struct etna_context {
>     struct etna_specs specs;
>     struct etna_screen *screen;
>     struct etna_cmd_stream *stream;
> +   struct etna_cmd_stream *stream2d;
>  
>     /* which state objects need to be re-emit'd: */
>     enum {
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_screen.c b/src/gallium/drivers/etnaviv/etnaviv_screen.c
> index 62b6f1c80fae..0dea6056c75a 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_screen.c
> +++ b/src/gallium/drivers/etnaviv/etnaviv_screen.c
> @@ -95,6 +95,12 @@ etna_screen_destroy(struct pipe_screen *pscreen)
>     if (screen->gpu)
>        etna_gpu_del(screen->gpu);
>  
> +   if (screen->pipe2d)
> +      etna_pipe_del(screen->pipe2d);
> +
> +   if (screen->gpu2d)
> +      etna_gpu_del(screen->gpu2d);
> +
>     if (screen->ro)
>        FREE(screen->ro);
>  
> @@ -891,6 +897,66 @@ etna_screen_bo_from_handle(struct pipe_screen *pscreen,
>     return bo;
>  }
>  
> +static void etna_screen_init_2d(struct etna_screen *screen)
> +{
> +   struct etna_gpu *gpu2d = NULL;
> +   uint64_t val;
> +   int ret, i;
> +
> +   /* If the current GPU is a combined 2d/3D core, use it as 2D engine */
                                           ^ 2D

> +   if (screen->features[0] & chipFeatures_PIPE_2D)
> +      gpu2d = screen->gpu;

If the GPU is a combined 2D/3D core, is screen->gpu2d set anywhere?
I assume it isn't on purpose, so it can be used to check whether
etna_gpu_del(screen->gpu2d) must be called in etna_screen_destroy().

> +
> +   /* otherwise search for a 2D capable core */
> +   if (!gpu2d) {

This could just be

   else {
      /* otherwise search for a 2D capable core */

> +      for (i = 0;; i++) {
> +         gpu2d = etna_gpu_new(screen->dev, i);
> +         if (!gpu2d)
> +            return;
> +
> +         ret = etna_gpu_get_param(gpu2d, ETNA_GPU_FEATURES_0, &val);
> +         if (!ret && (val & chipFeatures_PIPE_2D)) {
> +            screen->gpu2d = gpu2d;
> +            break;
> +         }
> +
> +         etna_gpu_del(gpu2d);
> +      }
> +   }
> +
> +   if (etna_gpu_get_param(screen->gpu2d, ETNA_GPU_FEATURES_0, &val))

If the GPU is a combined 2D/3D core, and screen->gpu2d == NULL at this
point, won't this fail trying to dereference the gpu NULL pointer in
etna_gpu_get_param()? I suppose these should be using the local gpu2d
variable instead of screen->gpu2d.

Further, if the GPU is a combined 2D/3D core, we have already queried
GPU features just before etna_screen_init_2d() was called,
in etna_screen_create(). We could just copy features to features2d.

Nitpick: if the gpu2d is 2D-only, the only way we can arrive here is
from the break in the loop above, just after val was already set to the
ETNA_GPU_FEATURES_0 value. In that case the first call to
etna_gpu_get_parm() is superfluous.

> +      return;
> +   screen->features2d[0] = val;
> +
> +   if (etna_gpu_get_param(screen->gpu2d, ETNA_GPU_FEATURES_1, &val))
> +      return;
> +   screen->features2d[1] = val;
> +
> +   if (etna_gpu_get_param(screen->gpu2d, ETNA_GPU_FEATURES_2, &val))
> +      return;
> +   screen->features2d[2] = val;
> +
> +   if (etna_gpu_get_param(screen->gpu2d, ETNA_GPU_FEATURES_3, &val))
> +      return;
> +   screen->features2d[3] = val;
> +
> +   if (etna_gpu_get_param(screen->gpu2d, ETNA_GPU_FEATURES_4, &val))
> +      return;
> +   screen->features2d[4] = val;
> +
> +   if (etna_gpu_get_param(screen->gpu2d, ETNA_GPU_FEATURES_5, &val))
> +      return;
> +   screen->features2d[5] = val;
> +
> +   if (etna_gpu_get_param(screen->gpu2d, ETNA_GPU_FEATURES_6, &val))
> +      return;
> +   screen->features2d[6] = val;
> +
> +   screen->pipe2d = etna_pipe_new(gpu2d, ETNA_PIPE_2D);
> +   if (!screen->pipe2d)
> +      DBG("could not create 2d pipe");
> +}
> +
>  struct pipe_screen *
>  etna_screen_create(struct etna_device *dev, struct etna_gpu *gpu,
>                     struct renderonly *ro)
> @@ -984,6 +1050,8 @@ etna_screen_create(struct etna_device *dev, struct etna_gpu *gpu,
>     }
>     screen->features[6] = val;
>  
> +   etna_screen_init_2d(screen);
> +
>     if (!etna_get_specs(screen))
>        goto fail;
>  
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_screen.h b/src/gallium/drivers/etnaviv/etnaviv_screen.h
> index 9757985526ec..82733a379430 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_screen.h
> +++ b/src/gallium/drivers/etnaviv/etnaviv_screen.h
> @@ -60,6 +60,9 @@ enum viv_features_word {
>  #define VIV_FEATURE(screen, word, feature) \
>     ((screen->features[viv_ ## word] & (word ## _ ## feature)) != 0)
>  
> +#define VIV_2D_FEATURE(screen, word, feature) \
> +   ((screen->features2d[viv_ ## word] & (word ## _ ## feature)) != 0)
> +
>  struct etna_screen {
>     struct pipe_screen base;
>  
> @@ -68,7 +71,9 @@ struct etna_screen {
>  
>     struct etna_device *dev;
>     struct etna_gpu *gpu;
> +   struct etna_gpu *gpu2d;
>     struct etna_pipe *pipe;
> +   struct etna_pipe *pipe2d;
>     struct etna_perfmon *perfmon;
>     struct renderonly *ro;
>  
> @@ -78,6 +83,7 @@ struct etna_screen {
>     uint32_t model;
>     uint32_t revision;
>     uint32_t features[VIV_FEATURES_WORD_COUNT];
> +   uint32_t features2d[VIV_FEATURES_WORD_COUNT];
>  
>     struct etna_specs specs;
>  

regards
Philipp