Re: [Mesa-dev] [PATCH 1/2] st/va: release held locks in error paths

2018-01-19 Thread Emil Velikov 
On 19 January 2018 at 14:46, Eric Engestrom  wrote:
> On Tuesday, 2018-01-16 18:35:40 +, Emil Velikov wrote:
>> On 15 January 2018 at 22:03, Grazvydas Ignotas  wrote:
>> > Found with the help of following Coccinelle semantic patch:
>> > // 
>> > @@
>> > expression E;
>> > @@
>> >
>> >   \(pthread_mutex_lock\|mtx_lock\|simple_mtx_lock\)(E)
>> >   ...
>> > (
>> >   \(pthread_mutex_unlock\|mtx_unlock\|simple_mtx_unlock\)(E);
>> >   ...
>> >   return ...;
>> > |
>> > + maybe need_unlock(E);
>> >   return ...;
>> > )
>> > // 
>> >
>> > Signed-off-by: Grazvydas Ignotas 
>> Grazvydas please add the stable tag, if you haven't pushed the patch.
>>
>> Thinking out loud: Should have these the Coccinelle bits in-tree. This
>> way we can check and address such issues quicker ;-)
>
> Agreed; sadly I don't think it can be integrated into the build systems
> easily, but we could have a $mesa/bin/cocci/ folder with all our scripts,
> and build it with something like:
>  $ cd $(mktemp -d)
>  $ CC=$mesa/bin/cocci-check meson $mesa
>  $ ninja
>
> (bonus: would work with any other build system that respects $CC)
>
> Now, someone *just* needs to write that `cocci-check` script to set up
> everything and run all the cocci scripts in the folder :P
>
> I'll give it a stab if/when I have some time, but I'm happy for anyone
> to take this up :)

That's roughly what I was thinking although I was about to keep it one
step at a time.
 1. Get people to add cocci patches
 2. Write a 'master' script to run them all (+ generate patches and send out?)
 3. Integrate 2) into CI/build process/other

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

Re: [Mesa-dev] [PATCH 1/2] st/va: release held locks in error paths

2018-01-19 Thread Eric Engestrom 
On Tuesday, 2018-01-16 18:35:40 +, Emil Velikov wrote:
> On 15 January 2018 at 22:03, Grazvydas Ignotas  wrote:
> > Found with the help of following Coccinelle semantic patch:
> > // 
> > @@
> > expression E;
> > @@
> >
> >   \(pthread_mutex_lock\|mtx_lock\|simple_mtx_lock\)(E)
> >   ...
> > (
> >   \(pthread_mutex_unlock\|mtx_unlock\|simple_mtx_unlock\)(E);
> >   ...
> >   return ...;
> > |
> > + maybe need_unlock(E);
> >   return ...;
> > )
> > // 
> >
> > Signed-off-by: Grazvydas Ignotas 
> Grazvydas please add the stable tag, if you haven't pushed the patch.
> 
> Thinking out loud: Should have these the Coccinelle bits in-tree. This
> way we can check and address such issues quicker ;-)

Agreed; sadly I don't think it can be integrated into the build systems
easily, but we could have a $mesa/bin/cocci/ folder with all our scripts,
and build it with something like:
 $ cd $(mktemp -d)
 $ CC=$mesa/bin/cocci-check meson $mesa
 $ ninja

(bonus: would work with any other build system that respects $CC)

Now, someone *just* needs to write that `cocci-check` script to set up
everything and run all the cocci scripts in the folder :P

I'll give it a stab if/when I have some time, but I'm happy for anyone
to take this up :)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] st/va: release held locks in error paths

2018-01-16 Thread Emil Velikov 
On 15 January 2018 at 22:03, Grazvydas Ignotas  wrote:
> Found with the help of following Coccinelle semantic patch:
> // 
> @@
> expression E;
> @@
>
>   \(pthread_mutex_lock\|mtx_lock\|simple_mtx_lock\)(E)
>   ...
> (
>   \(pthread_mutex_unlock\|mtx_unlock\|simple_mtx_unlock\)(E);
>   ...
>   return ...;
> |
> + maybe need_unlock(E);
>   return ...;
> )
> // 
>
> Signed-off-by: Grazvydas Ignotas 
Grazvydas please add the stable tag, if you haven't pushed the patch.

Thinking out loud: Should have these the Coccinelle bits in-tree. This
way we can check and address such issues quicker ;-)

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

Re: [Mesa-dev] [PATCH 1/2] st/va: release held locks in error paths

2018-01-16 Thread Christian König 

Am 16.01.2018 um 11:20 schrieb Grazvydas Ignotas:

On Tue, Jan 16, 2018 at 10:15 AM, Christian König
 wrote:

Reviewed-by: Christian König 

Do you have commit right by now or should Leo or I commit that for you?

Yes I do.


Of course you have :)

My fault I mixed up the mails/names,
Christian.




Thanks for the help,
Christian.


Am 15.01.2018 um 23:03 schrieb Grazvydas Ignotas:

Found with the help of following Coccinelle semantic patch:
// 
@@
expression E;
@@

\(pthread_mutex_lock\|mtx_lock\|simple_mtx_lock\)(E)
...
(
\(pthread_mutex_unlock\|mtx_unlock\|simple_mtx_unlock\)(E);
...
return ...;
|
+ maybe need_unlock(E);
return ...;
)
// 

Signed-off-by: Grazvydas Ignotas 
---
   src/gallium/state_trackers/va/config.c  | 4 +++-
   src/gallium/state_trackers/va/image.c   | 4 +++-
   src/gallium/state_trackers/va/picture.c | 4 +++-
   3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/gallium/state_trackers/va/config.c
b/src/gallium/state_trackers/va/config.c
index 25043d6..7bc031a 100644
--- a/src/gallium/state_trackers/va/config.c
+++ b/src/gallium/state_trackers/va/config.c
@@ -306,12 +306,14 @@ vlVaDestroyConfig(VADriverContextP ctx, VAConfigID
config_id)
 return VA_STATUS_ERROR_INVALID_CONTEXT;
mtx_lock(>mutex);
  config = handle_table_get(drv->htab, config_id);
   -   if (!config)
+   if (!config) {
+  mtx_unlock(>mutex);
 return VA_STATUS_ERROR_INVALID_CONFIG;
+   }
FREE(config);
  handle_table_remove(drv->htab, config_id);
  mtx_unlock(>mutex);
   diff --git a/src/gallium/state_trackers/va/image.c
b/src/gallium/state_trackers/va/image.c
index 86ae868..3f892c9 100644
--- a/src/gallium/state_trackers/va/image.c
+++ b/src/gallium/state_trackers/va/image.c
@@ -546,12 +546,14 @@ vlVaPutImage(VADriverContextP ctx, VASurfaceID
surface, VAImageID image,
 tex,
 0,
 PIPE_TRANSFER_WRITE |
 PIPE_TRANSFER_DISCARD_RANGE,
 _box, );
-if (map == NULL)
+if (map == NULL) {
+   mtx_unlock(>mutex);
  return VA_STATUS_ERROR_OPERATION_FAILED;
+}
 u_copy_nv12_from_yv12((const void * const*) data, pitches,
i, j,
 transfer->stride, tex->array_size,
 map, dst_box.width, dst_box.height);
   pipe_transfer_unmap(drv->pipe, transfer);
diff --git a/src/gallium/state_trackers/va/picture.c
b/src/gallium/state_trackers/va/picture.c
index 8951573..cfcf986 100644
--- a/src/gallium/state_trackers/va/picture.c
+++ b/src/gallium/state_trackers/va/picture.c
@@ -675,13 +675,15 @@ vlVaEndPicture(VADriverContextP ctx, VAContextID
context_id)
   dst_rect.x1 = src_rect.x1 = surf->templat.width;
   dst_rect.y1 = src_rect.y1 = surf->templat.height;
   vl_compositor_yuv_deint_full(>cstate, >compositor,
old_buf, surf->buffer,
_rect, _rect,
VL_COMPOSITOR_WEAVE);
- } else
+ } else {
   /* Can't convert from progressive to interlaced yet */
+mtx_unlock(>mutex);
   return VA_STATUS_ERROR_INVALID_SURFACE;
+ }
 }
   old_buf->destroy(old_buf);
 context->target = surf->buffer;
  }



___
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 1/2] st/va: release held locks in error paths

2018-01-16 Thread Grazvydas Ignotas 
On Tue, Jan 16, 2018 at 10:15 AM, Christian König
 wrote:
> Reviewed-by: Christian König 
>
> Do you have commit right by now or should Leo or I commit that for you?

Yes I do.

>
> Thanks for the help,
> Christian.
>
>
> Am 15.01.2018 um 23:03 schrieb Grazvydas Ignotas:
>>
>> Found with the help of following Coccinelle semantic patch:
>> // 
>> @@
>> expression E;
>> @@
>>
>>\(pthread_mutex_lock\|mtx_lock\|simple_mtx_lock\)(E)
>>...
>> (
>>\(pthread_mutex_unlock\|mtx_unlock\|simple_mtx_unlock\)(E);
>>...
>>return ...;
>> |
>> + maybe need_unlock(E);
>>return ...;
>> )
>> // 
>>
>> Signed-off-by: Grazvydas Ignotas 
>> ---
>>   src/gallium/state_trackers/va/config.c  | 4 +++-
>>   src/gallium/state_trackers/va/image.c   | 4 +++-
>>   src/gallium/state_trackers/va/picture.c | 4 +++-
>>   3 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/va/config.c
>> b/src/gallium/state_trackers/va/config.c
>> index 25043d6..7bc031a 100644
>> --- a/src/gallium/state_trackers/va/config.c
>> +++ b/src/gallium/state_trackers/va/config.c
>> @@ -306,12 +306,14 @@ vlVaDestroyConfig(VADriverContextP ctx, VAConfigID
>> config_id)
>> return VA_STATUS_ERROR_INVALID_CONTEXT;
>>mtx_lock(>mutex);
>>  config = handle_table_get(drv->htab, config_id);
>>   -   if (!config)
>> +   if (!config) {
>> +  mtx_unlock(>mutex);
>> return VA_STATUS_ERROR_INVALID_CONFIG;
>> +   }
>>FREE(config);
>>  handle_table_remove(drv->htab, config_id);
>>  mtx_unlock(>mutex);
>>   diff --git a/src/gallium/state_trackers/va/image.c
>> b/src/gallium/state_trackers/va/image.c
>> index 86ae868..3f892c9 100644
>> --- a/src/gallium/state_trackers/va/image.c
>> +++ b/src/gallium/state_trackers/va/image.c
>> @@ -546,12 +546,14 @@ vlVaPutImage(VADriverContextP ctx, VASurfaceID
>> surface, VAImageID image,
>> tex,
>> 0,
>> PIPE_TRANSFER_WRITE |
>> PIPE_TRANSFER_DISCARD_RANGE,
>> _box, );
>> -if (map == NULL)
>> +if (map == NULL) {
>> +   mtx_unlock(>mutex);
>>  return VA_STATUS_ERROR_OPERATION_FAILED;
>> +}
>> u_copy_nv12_from_yv12((const void * const*) data, pitches,
>> i, j,
>> transfer->stride, tex->array_size,
>> map, dst_box.width, dst_box.height);
>>   pipe_transfer_unmap(drv->pipe, transfer);
>> diff --git a/src/gallium/state_trackers/va/picture.c
>> b/src/gallium/state_trackers/va/picture.c
>> index 8951573..cfcf986 100644
>> --- a/src/gallium/state_trackers/va/picture.c
>> +++ b/src/gallium/state_trackers/va/picture.c
>> @@ -675,13 +675,15 @@ vlVaEndPicture(VADriverContextP ctx, VAContextID
>> context_id)
>>   dst_rect.x1 = src_rect.x1 = surf->templat.width;
>>   dst_rect.y1 = src_rect.y1 = surf->templat.height;
>>   vl_compositor_yuv_deint_full(>cstate, >compositor,
>>old_buf, surf->buffer,
>>_rect, _rect,
>> VL_COMPOSITOR_WEAVE);
>> - } else
>> + } else {
>>   /* Can't convert from progressive to interlaced yet */
>> +mtx_unlock(>mutex);
>>   return VA_STATUS_ERROR_INVALID_SURFACE;
>> + }
>> }
>>   old_buf->destroy(old_buf);
>> context->target = surf->buffer;
>>  }
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] st/va: release held locks in error paths

2018-01-16 Thread Christian König 

Reviewed-by: Christian König 

Do you have commit right by now or should Leo or I commit that for you?

Thanks for the help,
Christian.

Am 15.01.2018 um 23:03 schrieb Grazvydas Ignotas:

Found with the help of following Coccinelle semantic patch:
// 
@@
expression E;
@@

   \(pthread_mutex_lock\|mtx_lock\|simple_mtx_lock\)(E)
   ...
(
   \(pthread_mutex_unlock\|mtx_unlock\|simple_mtx_unlock\)(E);
   ...
   return ...;
|
+ maybe need_unlock(E);
   return ...;
)
// 

Signed-off-by: Grazvydas Ignotas 
---
  src/gallium/state_trackers/va/config.c  | 4 +++-
  src/gallium/state_trackers/va/image.c   | 4 +++-
  src/gallium/state_trackers/va/picture.c | 4 +++-
  3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/gallium/state_trackers/va/config.c 
b/src/gallium/state_trackers/va/config.c
index 25043d6..7bc031a 100644
--- a/src/gallium/state_trackers/va/config.c
+++ b/src/gallium/state_trackers/va/config.c
@@ -306,12 +306,14 @@ vlVaDestroyConfig(VADriverContextP ctx, VAConfigID 
config_id)
return VA_STATUS_ERROR_INVALID_CONTEXT;
  
 mtx_lock(>mutex);

 config = handle_table_get(drv->htab, config_id);
  
-   if (!config)

+   if (!config) {
+  mtx_unlock(>mutex);
return VA_STATUS_ERROR_INVALID_CONFIG;
+   }
  
 FREE(config);

 handle_table_remove(drv->htab, config_id);
 mtx_unlock(>mutex);
  
diff --git a/src/gallium/state_trackers/va/image.c b/src/gallium/state_trackers/va/image.c

index 86ae868..3f892c9 100644
--- a/src/gallium/state_trackers/va/image.c
+++ b/src/gallium/state_trackers/va/image.c
@@ -546,12 +546,14 @@ vlVaPutImage(VADriverContextP ctx, VASurfaceID surface, 
VAImageID image,
tex,
0,
PIPE_TRANSFER_WRITE |
PIPE_TRANSFER_DISCARD_RANGE,
_box, );
-if (map == NULL)
+if (map == NULL) {
+   mtx_unlock(>mutex);
 return VA_STATUS_ERROR_OPERATION_FAILED;
+}
  
  u_copy_nv12_from_yv12((const void * const*) data, pitches, i, j,

transfer->stride, tex->array_size,
map, dst_box.width, dst_box.height);
  pipe_transfer_unmap(drv->pipe, transfer);
diff --git a/src/gallium/state_trackers/va/picture.c 
b/src/gallium/state_trackers/va/picture.c
index 8951573..cfcf986 100644
--- a/src/gallium/state_trackers/va/picture.c
+++ b/src/gallium/state_trackers/va/picture.c
@@ -675,13 +675,15 @@ vlVaEndPicture(VADriverContextP ctx, VAContextID 
context_id)
  dst_rect.x1 = src_rect.x1 = surf->templat.width;
  dst_rect.y1 = src_rect.y1 = surf->templat.height;
  vl_compositor_yuv_deint_full(>cstate, >compositor,
   old_buf, surf->buffer,
   _rect, _rect, 
VL_COMPOSITOR_WEAVE);
- } else
+ } else {
  /* Can't convert from progressive to interlaced yet */
+mtx_unlock(>mutex);
  return VA_STATUS_ERROR_INVALID_SURFACE;
+ }
}
  
old_buf->destroy(old_buf);

context->target = surf->buffer;
 }


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

[Mesa-dev] [PATCH 1/2] st/va: release held locks in error paths

2018-01-15 Thread Grazvydas Ignotas 
Found with the help of following Coccinelle semantic patch:
// 
@@
expression E;
@@

  \(pthread_mutex_lock\|mtx_lock\|simple_mtx_lock\)(E)
  ...
(
  \(pthread_mutex_unlock\|mtx_unlock\|simple_mtx_unlock\)(E);
  ...
  return ...;
|
+ maybe need_unlock(E);
  return ...;
)
// 

Signed-off-by: Grazvydas Ignotas 
---
 src/gallium/state_trackers/va/config.c  | 4 +++-
 src/gallium/state_trackers/va/image.c   | 4 +++-
 src/gallium/state_trackers/va/picture.c | 4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/gallium/state_trackers/va/config.c 
b/src/gallium/state_trackers/va/config.c
index 25043d6..7bc031a 100644
--- a/src/gallium/state_trackers/va/config.c
+++ b/src/gallium/state_trackers/va/config.c
@@ -306,12 +306,14 @@ vlVaDestroyConfig(VADriverContextP ctx, VAConfigID 
config_id)
   return VA_STATUS_ERROR_INVALID_CONTEXT;
 
mtx_lock(>mutex);
config = handle_table_get(drv->htab, config_id);
 
-   if (!config)
+   if (!config) {
+  mtx_unlock(>mutex);
   return VA_STATUS_ERROR_INVALID_CONFIG;
+   }
 
FREE(config);
handle_table_remove(drv->htab, config_id);
mtx_unlock(>mutex);
 
diff --git a/src/gallium/state_trackers/va/image.c 
b/src/gallium/state_trackers/va/image.c
index 86ae868..3f892c9 100644
--- a/src/gallium/state_trackers/va/image.c
+++ b/src/gallium/state_trackers/va/image.c
@@ -546,12 +546,14 @@ vlVaPutImage(VADriverContextP ctx, VASurfaceID surface, 
VAImageID image,
   tex,
   0,
   PIPE_TRANSFER_WRITE |
   PIPE_TRANSFER_DISCARD_RANGE,
   _box, );
-if (map == NULL)
+if (map == NULL) {
+   mtx_unlock(>mutex);
return VA_STATUS_ERROR_OPERATION_FAILED;
+}
 
 u_copy_nv12_from_yv12((const void * const*) data, pitches, i, j,
   transfer->stride, tex->array_size,
   map, dst_box.width, dst_box.height);
 pipe_transfer_unmap(drv->pipe, transfer);
diff --git a/src/gallium/state_trackers/va/picture.c 
b/src/gallium/state_trackers/va/picture.c
index 8951573..cfcf986 100644
--- a/src/gallium/state_trackers/va/picture.c
+++ b/src/gallium/state_trackers/va/picture.c
@@ -675,13 +675,15 @@ vlVaEndPicture(VADriverContextP ctx, VAContextID 
context_id)
 dst_rect.x1 = src_rect.x1 = surf->templat.width;
 dst_rect.y1 = src_rect.y1 = surf->templat.height;
 vl_compositor_yuv_deint_full(>cstate, >compositor,
  old_buf, surf->buffer,
  _rect, _rect, 
VL_COMPOSITOR_WEAVE);
- } else
+ } else {
 /* Can't convert from progressive to interlaced yet */
+mtx_unlock(>mutex);
 return VA_STATUS_ERROR_INVALID_SURFACE;
+ }
   }
 
   old_buf->destroy(old_buf);
   context->target = surf->buffer;
}
-- 
2.7.4

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