[Mesa-dev] [v6 2/9] intel: do not create renderbuffers out of planar images

[Chad Versace]

On 05/30/2013 01:20 AM, Daniel Vetter wrote:
> On Thu, May 30, 2013 at 9:49 AM, Pohjolainen, Topi
> <topi.pohjolainen at intel.com> wrote:
>> On Tue, May 28, 2013 at 01:33:18PM -0700, Eric Anholt wrote:
>>> Topi Pohjolainen <topi.pohjolainen at intel.com> writes:
>>>
>>>> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
>>>> ---
>>>>   src/mesa/drivers/dri/intel/intel_fbo.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/intel/intel_fbo.c b/src/mesa/drivers/dri/intel/intel_fbo.c
>>>> index 69f8629..7ccbaa8 100644
>>>> --- a/src/mesa/drivers/dri/intel/intel_fbo.c
>>>> +++ b/src/mesa/drivers/dri/intel/intel_fbo.c
>>>> @@ -280,6 +280,10 @@ intel_image_target_renderbuffer_storage(struct gl_context *ctx,
>>>>      if (image == NULL)
>>>>         return;
>>>>
>>>> +   /* Planar buffers are not supported as render targets. */
>>>> +   if (image->planar_format && image->planar_format->nplanes > 1)
>>>> +      return;
>>>> +
>>>>      /* __DRIimage is opaque to the core so it has to be checked here */
>>>>      switch (image->format) {
>>>>      case MESA_FORMAT_RGBA8888_REV:
>>>
>>> OK, this answers one question I had about what our support was going to
>>> be.  But what about glEGLImageTargetTexture2DOES()?
>>>
>>> Are we only going to support planar textures with image_external?  My
>>> thought is "yes".  How about on HSW with the YUV sampler support?  Are
>>> we going to relayout the data in the incoming fds to a copy that can
>>> support sampling from them, or are we going to reject incoming fds that
>>> don't fit the required layout?  And if we decide to reject anything, are
>>> we going to reject it at the or the dmabuf_import stage or the
>>> image_external stage?
>>
>> I was also thinking that planar would be supported only by image_external, and I
>> hadn't even thought about HSW yet.

I think the same formats supported in image_external should also be supported
by dma_buf_import. This would allow us to write Piglit tests for any supported
image_external format.

If the set of supported dma_buf_import formats is a subset of the image_external
formats, then how do you propose we write tests (Piglit or non-Piglit) for the
extra image_external formats?

To avoid any implementation weirdness, though, I think we should prevent
creation of 2D texture targets (GL_TEXTURE_2D) from dma_buf images, and
permit only creation of external textures (GL_TEXTURE_EXTERNAL).

>> Clearly I haven't been thinking as far as you, I thought that images can be
>> created relatively freely - it would be their uses that would fail if not
>> supported. But this is wrong I guess. Once an image is created it should be
>> guaranteed to be usable for quite a number of things.
>>
>> Having said that I realize that I don't most likely know enough about all the
>> ways images can be used so that I could introduce proper constraints to the
>> image creation logic.
>
> I guess we have two questions here: The first is deciding where we
> fail the import process if the client api can't directly access the
> data in the imported dma_buf. I think it's better to fail late since
> doing all possible checks at egl image creation time might be ugly and
> too restrictive. Otoh failing late will make it even harder for
> applications to figure out what's possible.

I think we should fail at image creation time if the driver is unable
to texture from the given dma_buf configuration. If the image cannot
be textured from, it's a useless brick, so why allow its creation?

Validation of the image's texturability must be done at some time, most likely
at either either time of image creation or of texture creation. I don't see how
anything is gained by postponing the validation until time of
texture creation, so let's just do it early so as to prevent the creation of
bricked images.

> The other questions is who will be responsible for copying. Doing any
> kinds of transparent relayouting (e.g. when resampling a planar yuv
> with oes image external to something a hw sampler could directly cope
> with in case of a mismatch) doesn't sound too good: It'll break users
> if they try to re-use dma_bufs/egl_images and the client api objects
> created from them. So we might want to disallow any kind of egl image
> orphaning and any other non-zero-copy conversion for images backed by
> dma_bufs. Otoh this could get really ugly if we add a egl_image ->
> dma_buf export extension.

If we allow creation of a GL_TEXTURE_2D from dma_buf images, then the GL_OES_EGL_image
spec requires orphaning under some situations. Here is issue #1 of that spec:

     1.  What happens if an application tries to specify a new mipmap
         level (or respecify an existing mipmap level) for a texture
         object that was originally specified using
         EGLImageTargetTexture2D (e.g., by subsequent calls to
         {Copy}TexImage, GenerateMipmapOES, and/or setting the
         texture's GENERATE_MIPMAP_SGIS parameter to TRUE) ?

         RESOLVED:  If the application respecifies any properties of
         an EGLImage target texture, the GL should allocate additional
         memory for the respecified object, copying any data from
         previously-specified levels (including those in the EGLImage
         source).  The respecified texture object will not be an
         EGLImage target, potentially orphaning the EGLImage.

I'll say it again: I think we should prevent creation of 2D texture targets
from dma_buf images, and permit only creation of external textures.

> If we go with the "no orphaning for dma_buf backed egl_image" we could
> make the copying somewhat more explicit with something like the egl
> streams stuff. I haven't read the spec for that closely yet, but
> having a solid zero-copy guarantee with making copying explicit sounds
> good to me. Also, to better cope with interop issue we could guarantee
> that every egl image which can be imported can at least be
> hw-resampled into something useful. That way failing the
> dma_buf->eg_image step still gives a useful piece of information to
> users.
>
> Since this sounds like a spec question cc'ing all the people from the
> original dma_buf_import spec discussions.
S