Re: [Mesa-dev] [PATCH] st/va: it is valid to release the VABuffer of an exported resource

2016-06-08 Thread Christian König 
I'm clearly not deep enough into VA-API to judge if that is correct or 
not, but it sounds sane to me.


So feel free to add an Acked-by: Christian König 
 on the patch.


Cheers,
Christian.

Am 08.06.2016 um 13:13 schrieb Julien Isorce:

Hi Christian,

Thx for the review.

pipe_resource_reference(, NULL)  will decrement reference counting 
(p_atomic_dec  res->count). But the va surface still has the initial 
reference since it created that resource.
So calling vaDestroyImage on a derived image will call VaDestroyBuffer 
but the decrementation wont't reach 0.


It is just wrong that vlVaDestroyBuffer relies on the export_refcount 
flag. I also compared with vaapi intel driver and they have same flag 
and it is not present in their vaDestroyBuffer.


Cheers
Julien


On 8 June 2016 at 09:22, Christian König > wrote:


Am 02.06.2016 um 16:03 schrieb Julien Isorce:

Signed-off-by: Julien Isorce >


Actually I'm not sure if that is correct.

If you release the VABuffer of an exported resource you won't be
able to properly close the handle with vlVaReleaseBufferHandle().

On the other hand the semantic VA requires for
vlVaAcquireBufferHandle() and vlVaReleaseBufferHandle() is
complete nonsense for DMA-buf handles.

Christian.

---
  src/gallium/state_trackers/va/buffer.c | 8 +---
  1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/gallium/state_trackers/va/buffer.c
b/src/gallium/state_trackers/va/buffer.c
index 2fd8661..7d3167b 100644
--- a/src/gallium/state_trackers/va/buffer.c
+++ b/src/gallium/state_trackers/va/buffer.c
@@ -192,14 +192,8 @@ vlVaDestroyBuffer(VADriverContextP ctx,
VABufferID buf_id)
return VA_STATUS_ERROR_INVALID_BUFFER;
 }
  -   if (buf->derived_surface.resource) {
-  if (buf->export_refcount > 0) {
- pipe_mutex_unlock(drv->mutex);
- return VA_STATUS_ERROR_INVALID_BUFFER;
-  }
-
+   if (buf->derived_surface.resource)
pipe_resource_reference(>derived_surface.resource, NULL);
-   }
   FREE(buf->data);
 FREE(buf);


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




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

Re: [Mesa-dev] [PATCH] st/va: it is valid to release the VABuffer of an exported resource

2016-06-08 Thread Julien Isorce 
Hi Christian,

Thx for the review.

pipe_resource_reference(, NULL)  will decrement reference counting
(p_atomic_dec  res->count). But the va surface still has the initial
reference since it created that resource.
So calling vaDestroyImage on a derived image will call VaDestroyBuffer but
the decrementation wont't reach 0.

It is just wrong that vlVaDestroyBuffer relies on the export_refcount flag.
I also compared with vaapi intel driver and they have same flag and it is
not present in their vaDestroyBuffer.

Cheers
Julien


On 8 June 2016 at 09:22, Christian König  wrote:

> Am 02.06.2016 um 16:03 schrieb Julien Isorce:
>
>> Signed-off-by: Julien Isorce 
>>
>
> Actually I'm not sure if that is correct.
>
> If you release the VABuffer of an exported resource you won't be able to
> properly close the handle with vlVaReleaseBufferHandle().
>
> On the other hand the semantic VA requires for vlVaAcquireBufferHandle()
> and vlVaReleaseBufferHandle() is complete nonsense for DMA-buf handles.
>
> Christian.
>
> ---
>>   src/gallium/state_trackers/va/buffer.c | 8 +---
>>   1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/va/buffer.c
>> b/src/gallium/state_trackers/va/buffer.c
>> index 2fd8661..7d3167b 100644
>> --- a/src/gallium/state_trackers/va/buffer.c
>> +++ b/src/gallium/state_trackers/va/buffer.c
>> @@ -192,14 +192,8 @@ vlVaDestroyBuffer(VADriverContextP ctx, VABufferID
>> buf_id)
>> return VA_STATUS_ERROR_INVALID_BUFFER;
>>  }
>>   -   if (buf->derived_surface.resource) {
>> -  if (buf->export_refcount > 0) {
>> - pipe_mutex_unlock(drv->mutex);
>> - return VA_STATUS_ERROR_INVALID_BUFFER;
>> -  }
>> -
>> +   if (buf->derived_surface.resource)
>> pipe_resource_reference(>derived_surface.resource, NULL);
>> -   }
>>FREE(buf->data);
>>  FREE(buf);
>>
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/va: it is valid to release the VABuffer of an exported resource

2016-06-08 Thread Christian König 

Am 02.06.2016 um 16:03 schrieb Julien Isorce:

Signed-off-by: Julien Isorce 


Actually I'm not sure if that is correct.

If you release the VABuffer of an exported resource you won't be able to 
properly close the handle with vlVaReleaseBufferHandle().


On the other hand the semantic VA requires for vlVaAcquireBufferHandle() 
and vlVaReleaseBufferHandle() is complete nonsense for DMA-buf handles.


Christian.


---
  src/gallium/state_trackers/va/buffer.c | 8 +---
  1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/gallium/state_trackers/va/buffer.c 
b/src/gallium/state_trackers/va/buffer.c
index 2fd8661..7d3167b 100644
--- a/src/gallium/state_trackers/va/buffer.c
+++ b/src/gallium/state_trackers/va/buffer.c
@@ -192,14 +192,8 @@ vlVaDestroyBuffer(VADriverContextP ctx, VABufferID buf_id)
return VA_STATUS_ERROR_INVALID_BUFFER;
 }
  
-   if (buf->derived_surface.resource) {

-  if (buf->export_refcount > 0) {
- pipe_mutex_unlock(drv->mutex);
- return VA_STATUS_ERROR_INVALID_BUFFER;
-  }
-
+   if (buf->derived_surface.resource)
pipe_resource_reference(>derived_surface.resource, NULL);
-   }
  
 FREE(buf->data);

 FREE(buf);


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

[Mesa-dev] [PATCH] st/va: it is valid to release the VABuffer of an exported resource

2016-06-02 Thread Julien Isorce 
Signed-off-by: Julien Isorce 
---
 src/gallium/state_trackers/va/buffer.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/gallium/state_trackers/va/buffer.c 
b/src/gallium/state_trackers/va/buffer.c
index 2fd8661..7d3167b 100644
--- a/src/gallium/state_trackers/va/buffer.c
+++ b/src/gallium/state_trackers/va/buffer.c
@@ -192,14 +192,8 @@ vlVaDestroyBuffer(VADriverContextP ctx, VABufferID buf_id)
   return VA_STATUS_ERROR_INVALID_BUFFER;
}
 
-   if (buf->derived_surface.resource) {
-  if (buf->export_refcount > 0) {
- pipe_mutex_unlock(drv->mutex);
- return VA_STATUS_ERROR_INVALID_BUFFER;
-  }
-
+   if (buf->derived_surface.resource)
   pipe_resource_reference(>derived_surface.resource, NULL);
-   }
 
FREE(buf->data);
FREE(buf);
-- 
1.9.1

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