[Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

[Emil Velikov]

Hi Ben,

Mostly pointing out a few things that look strange, pardon if some
seem too pedantic.

On 10 March 2017 at 01:48, Ben Widawsky <ben at bwidawsk.net> wrote:

> ---
>  include/GL/internal/dri_interface.h      | 27 ++++++++++++++++++++++++++-
Split the Infra from the i965 implementation ?

>  src/gallium/state_trackers/dri/dri2.c    |  1 +
Not needed.

>  src/mesa/drivers/dri/i965/intel_screen.c | 32 +++++++++++++++++++++++++++++++-
Patch should come as/after __DRI_IMAGE_ATTRIB_MODIFIER_* are honoured.
Or modifiers and modifiers count in general.

> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -1413,6 +1413,7 @@ static __DRIimageExtension dri2ImageExtension = {
>      .getCapabilities              = dri2_get_capabilities,
>      .mapImage                     = dri2_map_image,
>      .unmapImage                   = dri2_unmap_image,
> +    .createImageWithModifiers     = NULL,
>  };
>
Not needed - drop ?


> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> index 21786eb54a..3452572874 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -510,9 +510,11 @@ intel_destroy_image(__DRIimage *image)
>  }
>
>  static __DRIimage *
> -intel_create_image(__DRIscreen *dri_screen,
> +__intel_create_image(__DRIscreen *dri_screen,
Don't think there's (m)any functions that start with __ in 965.
Perhaps append "_common" ?


>     int cpp;
>     unsigned long pitch;
>
> +   /* Callers of this may specify a modifier, or a dri usage, but not both. The
> +    * newer modifier interface deprecates the older usage flags newer modifier
> +    * interface deprecates the older usage flags.
> +    */
> +   assert(!(use && count));
> +
What would happen if we don't have either old (use) or new (modifiers) ?
Shouldn't this be XOR ?


> @@ -840,6 +869,7 @@ static const __DRIimageExtension intelImageExtension = {
Afaict, nothing in this series bumps the version to 14.
So you either missed a patch or we have a bug somewhere ?

-Emil