Re: [Mesa-dev] [PATCH v2] egl_dri2: add support for using modifier attributes in eglCreateImageKHR

2016-11-18 Thread Daniel Stone 
Hi Emil,

On 18 November 2016 at 15:24, Emil Velikov  wrote:
> On 18 November 2016 at 15:17, Daniel Stone  wrote:
>> Actually, present-and-zero modifier has a very well-defined meaning:
>> it _forces_ linear interpretation of the buffer, whereas a non-present
>> modifier may cause a kernel query (e.g. i915_gem_get_tiling) to
>> discover a hidden tiling mode. So, if present, the modifier should be
>> passed.
>>
> You are suggesting that we should track "has_modifier" (as opposed to
> nonzero_modifier_found) and pass it to DmaBuf2 regardless of the
> contents, right ?

Yep, exactly that.

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

Re: [Mesa-dev] [PATCH v2] egl_dri2: add support for using modifier attributes in eglCreateImageKHR

2016-11-18 Thread Emil Velikov 
On 18 November 2016 at 15:17, Daniel Stone  wrote:
> Hi,
>
> On 18 November 2016 at 14:50, Emil Velikov  wrote:
>> On 16 November 2016 at 09:28, Varad Gautam  wrote:
>>> +   if (nonzero_modifier_found && dri2_dpy->image->createImageFromDmaBufs2) 
>>> {
>>> +  dri_image =
>>> + dri2_dpy->image->createImageFromDmaBufs2(dri2_dpy->dri_screen,
>>> +attrs.Width, attrs.Height, attrs.DMABufFourCC.Value,
>>> +fds, num_fds, pitches, offsets, modifiers,
>>> +attrs.DMABufYuvColorSpaceHint.Value,
>>> +attrs.DMABufSampleRangeHint.Value,
>>> +attrs.DMABufChromaHorizontalSiting.Value,
>>> +attrs.DMABufChromaVerticalSiting.Value,
>>> +,
>>> +NULL);
>>> +   } else {
>>> +  if (nonzero_modifier_found) {
>>> + _eglError(EGL_BAD_MATCH, "unsupported dma_buf format modifier");
>>> + return EGL_NO_IMAGE_KHR;
>>> +  }
>>> +
>> Using something like the following might be better?
>>
>> if (nonzero_modifier_found) {
>>if (!dri2_dpy->image->createImageFromDmaBufs2)
>>  # assert should never reach here, since the extension should be
>> advertised only if the API is available.
>>use new API
>> else
>>use old API
>
> Actually, present-and-zero modifier has a very well-defined meaning:
> it _forces_ linear interpretation of the buffer, whereas a non-present
> modifier may cause a kernel query (e.g. i915_gem_get_tiling) to
> discover a hidden tiling mode. So, if present, the modifier should be
> passed.
>
You are suggesting that we should track "has_modifier" (as opposed to
nonzero_modifier_found) and pass it to DmaBuf2 regardless of the
contents, right ?

Just double-checking.
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] egl_dri2: add support for using modifier attributes in eglCreateImageKHR

2016-11-18 Thread Daniel Stone 
Hi,

On 18 November 2016 at 14:50, Emil Velikov  wrote:
> On 16 November 2016 at 09:28, Varad Gautam  wrote:
>> +   if (nonzero_modifier_found && dri2_dpy->image->createImageFromDmaBufs2) {
>> +  dri_image =
>> + dri2_dpy->image->createImageFromDmaBufs2(dri2_dpy->dri_screen,
>> +attrs.Width, attrs.Height, attrs.DMABufFourCC.Value,
>> +fds, num_fds, pitches, offsets, modifiers,
>> +attrs.DMABufYuvColorSpaceHint.Value,
>> +attrs.DMABufSampleRangeHint.Value,
>> +attrs.DMABufChromaHorizontalSiting.Value,
>> +attrs.DMABufChromaVerticalSiting.Value,
>> +,
>> +NULL);
>> +   } else {
>> +  if (nonzero_modifier_found) {
>> + _eglError(EGL_BAD_MATCH, "unsupported dma_buf format modifier");
>> + return EGL_NO_IMAGE_KHR;
>> +  }
>> +
> Using something like the following might be better?
>
> if (nonzero_modifier_found) {
>if (!dri2_dpy->image->createImageFromDmaBufs2)
>  # assert should never reach here, since the extension should be
> advertised only if the API is available.
>use new API
> else
>use old API

Actually, present-and-zero modifier has a very well-defined meaning:
it _forces_ linear interpretation of the buffer, whereas a non-present
modifier may cause a kernel query (e.g. i915_gem_get_tiling) to
discover a hidden tiling mode. So, if present, the modifier should be
passed.

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

Re: [Mesa-dev] [PATCH v2] egl_dri2: add support for using modifier attributes in eglCreateImageKHR

2016-11-18 Thread Emil Velikov 
On 16 November 2016 at 09:28, Varad Gautam  wrote:
> From: Pekka Paalanen 
>
> allow creating EGLImages with dmabuf format modifiers when target is
> EGL_LINUX_DMA_BUF_EXT for EGL_EXT_image_dma_buf_import_modifiers.
>
> v2: clear modifier assembling and error label name (Eric Engestrom)
>
> Signed-off-by: Pekka Paalanen 
> Signed-off-by: Varad Gautam 
> Reviewed-by: Eric Engestrom 
> ---

> +   int nonzero_modifier_found = 0;
> unsigned error;
>
> /**
> @@ -2106,18 +2125,44 @@ dri2_create_image_dma_buf(_EGLDisplay *disp, 
> _EGLContext *ctx,
>fds[i] = attrs.DMABufPlaneFds[i].Value;
>pitches[i] = attrs.DMABufPlanePitches[i].Value;
>offsets[i] = attrs.DMABufPlaneOffsets[i].Value;
> +  if (attrs.DMABufPlaneModifiersLo[i].IsPresent) {
> + modifiers[i] = (attrs.DMABufPlaneModifiersHi[i].Value << 32) |
> +attrs.DMABufPlaneModifiersLo[i].Value;
> + if (modifiers[i] != 0)
> +nonzero_modifier_found = EGL_TRUE;
integer storage and EGL_TRUE -> bool and true/false ?


> +   if (nonzero_modifier_found && dri2_dpy->image->createImageFromDmaBufs2) {
> +  dri_image =
> + dri2_dpy->image->createImageFromDmaBufs2(dri2_dpy->dri_screen,
> +attrs.Width, attrs.Height, attrs.DMABufFourCC.Value,
> +fds, num_fds, pitches, offsets, modifiers,
> +attrs.DMABufYuvColorSpaceHint.Value,
> +attrs.DMABufSampleRangeHint.Value,
> +attrs.DMABufChromaHorizontalSiting.Value,
> +attrs.DMABufChromaVerticalSiting.Value,
> +,
> +NULL);
> +   } else {
> +  if (nonzero_modifier_found) {
> + _eglError(EGL_BAD_MATCH, "unsupported dma_buf format modifier");
> + return EGL_NO_IMAGE_KHR;
> +  }
> +
Using something like the following might be better?

if (nonzero_modifier_found) {
   if (!dri2_dpy->image->createImageFromDmaBufs2)
 # assert should never reach here, since the extension should be
advertised only if the API is available.
   use new API
else
   use old API


> +  case EGL_DMA_BUF_PLANE3_MODIFIER_LO_EXT:
> + if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> +goto bad_param;
> + attrs->DMABufPlaneModifiersLo[3].Value = val;
> + attrs->DMABufPlaneModifiersLo[3].IsPresent = EGL_TRUE;
> + break;
> +  case EGL_DMA_BUF_PLANE3_MODIFIER_HI_EXT:
> + if (!dpy->Extensions.EXT_image_dma_buf_import_modifiers)
> +goto bad_param;
> + attrs->DMABufPlaneModifiersHi[3].Value = val;
> + attrs->DMABufPlaneModifiersHi[3].IsPresent = EGL_TRUE;
> + break;
>case EGL_YUV_COLOR_SPACE_HINT_EXT:
>   if (val != EGL_ITU_REC601_EXT && val != EGL_ITU_REC709_EXT &&
>   val != EGL_ITU_REC2020_EXT) {
> @@ -181,6 +229,7 @@ _eglParseImageAttribList(_EGLImageAttribs *attrs, 
> _EGLDisplay *dpy,
>   }
>   break;
>
> +bad_param:
Using goto to jump to another case statement is "evil". Please don't use them.


Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v2] egl_dri2: add support for using modifier attributes in eglCreateImageKHR

2016-11-16 Thread Varad Gautam 
From: Pekka Paalanen 

allow creating EGLImages with dmabuf format modifiers when target is
EGL_LINUX_DMA_BUF_EXT for EGL_EXT_image_dma_buf_import_modifiers.

v2: clear modifier assembling and error label name (Eric Engestrom)

Signed-off-by: Pekka Paalanen 
Signed-off-by: Varad Gautam 
Reviewed-by: Eric Engestrom 
---
 src/egl/drivers/dri2/egl_dri2.c | 67 ++---
 src/egl/main/egldisplay.h   |  1 +
 src/egl/main/eglimage.c | 49 ++
 src/egl/main/eglimage.h |  2 ++
 4 files changed, 108 insertions(+), 11 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 58d16e1..f98416c 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1937,6 +1937,21 @@ dri2_check_dma_buf_attribs(const _EGLImageAttribs *attrs)
   }
}
 
+   /**
+* If  is EGL_LINUX_DMA_BUF_EXT, both or neither of the following
+* attribute values may be given.
+*
+* This is referring to EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT and
+* EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT, and the same for other planes.
+*/
+   for (i = 0; i < DMA_BUF_MAX_PLANES; ++i) {
+  if (attrs->DMABufPlaneModifiersLo[i].IsPresent !=
+  attrs->DMABufPlaneModifiersHi[i].IsPresent) {
+ _eglError(EGL_BAD_PARAMETER, "modifier attribute lo or hi missing");
+ return EGL_FALSE;
+  }
+   }
+
return EGL_TRUE;
 }
 
@@ -2043,7 +2058,9 @@ dri2_check_dma_buf_format(const _EGLImageAttribs *attrs)
for (i = plane_n; i < DMA_BUF_MAX_PLANES; ++i) {
   if (attrs->DMABufPlaneFds[i].IsPresent ||
   attrs->DMABufPlaneOffsets[i].IsPresent ||
-  attrs->DMABufPlanePitches[i].IsPresent) {
+  attrs->DMABufPlanePitches[i].IsPresent ||
+  attrs->DMABufPlaneModifiersLo[i].IsPresent ||
+  attrs->DMABufPlaneModifiersHi[i].IsPresent) {
  _eglError(EGL_BAD_ATTRIBUTE, "too many plane attributes");
  return 0;
   }
@@ -2076,6 +2093,8 @@ dri2_create_image_dma_buf(_EGLDisplay *disp, _EGLContext 
*ctx,
int fds[DMA_BUF_MAX_PLANES];
int pitches[DMA_BUF_MAX_PLANES];
int offsets[DMA_BUF_MAX_PLANES];
+   uint64_t modifiers[DMA_BUF_MAX_PLANES];
+   int nonzero_modifier_found = 0;
unsigned error;
 
/**
@@ -2106,18 +2125,44 @@ dri2_create_image_dma_buf(_EGLDisplay *disp, 
_EGLContext *ctx,
   fds[i] = attrs.DMABufPlaneFds[i].Value;
   pitches[i] = attrs.DMABufPlanePitches[i].Value;
   offsets[i] = attrs.DMABufPlaneOffsets[i].Value;
+  if (attrs.DMABufPlaneModifiersLo[i].IsPresent) {
+ modifiers[i] = (attrs.DMABufPlaneModifiersHi[i].Value << 32) |
+attrs.DMABufPlaneModifiersLo[i].Value;
+ if (modifiers[i] != 0)
+nonzero_modifier_found = EGL_TRUE;
+  } else {
+ modifiers[i] = 0;
+  }
}
 
-   dri_image =
-  dri2_dpy->image->createImageFromDmaBufs(dri2_dpy->dri_screen,
- attrs.Width, attrs.Height, attrs.DMABufFourCC.Value,
- fds, num_fds, pitches, offsets,
- attrs.DMABufYuvColorSpaceHint.Value,
- attrs.DMABufSampleRangeHint.Value,
- attrs.DMABufChromaHorizontalSiting.Value,
- attrs.DMABufChromaVerticalSiting.Value,
- ,
- NULL);
+   if (nonzero_modifier_found && dri2_dpy->image->createImageFromDmaBufs2) {
+  dri_image =
+ dri2_dpy->image->createImageFromDmaBufs2(dri2_dpy->dri_screen,
+attrs.Width, attrs.Height, attrs.DMABufFourCC.Value,
+fds, num_fds, pitches, offsets, modifiers,
+attrs.DMABufYuvColorSpaceHint.Value,
+attrs.DMABufSampleRangeHint.Value,
+attrs.DMABufChromaHorizontalSiting.Value,
+attrs.DMABufChromaVerticalSiting.Value,
+,
+NULL);
+   } else {
+  if (nonzero_modifier_found) {
+ _eglError(EGL_BAD_MATCH, "unsupported dma_buf format modifier");
+ return EGL_NO_IMAGE_KHR;
+  }
+
+  dri_image =
+ dri2_dpy->image->createImageFromDmaBufs(dri2_dpy->dri_screen,
+attrs.Width, attrs.Height, attrs.DMABufFourCC.Value,
+fds, num_fds, pitches, offsets,
+attrs.DMABufYuvColorSpaceHint.Value,
+attrs.DMABufSampleRangeHint.Value,
+attrs.DMABufChromaHorizontalSiting.Value,
+attrs.DMABufChromaVerticalSiting.Value,
+,
+NULL);
+   }
dri2_create_image_khr_texture_error(error);
 
if (!dri_image)
diff --git a/src/egl/main/egldisplay.h b/src/egl/main/egldisplay.h
index 62d9a11..7a59dc5 100644
--- a/src/egl/main/egldisplay.h
+++ b/src/egl/main/egldisplay.h
@@ -101,6 +101,7 @@ struct _egl_extensions
EGLBoolean EXT_buffer_age;
EGLBoolean EXT_create_context_robustness;
EGLBoolean EXT_image_dma_buf_import;
+   EGLBoolean