[VDPAU] [PATCH 4/4] vdpau_wrapper: protect concurrent access to _imp_get_proc_address

[Rémi Denis-Courmont]

Le 2014-11-05 03:00, Aaron Plattner a écrit :
> On 10/29/2014 05:47 AM, Rémi Denis-Courmont wrote:
>> From: Rémi Denis-Courmont <remid at nvidia.com>
>>
>> The wrapper, as it's currently written, cannot cope with more than 
>> one
>> VdpGetProcAddress implementation. Luckily, this should hardly ever
>> happen.
>
> Does this ever actually happen?

I suppose it could conceivably happen on a dual-GPU system with both 
NVIDIA and VA-GL drivers. I have obviousky never seen it happen since it 
does not currently work.

Making that work would require significant revectoring of the wrapper. 
It would need to remap handles from the drivers, since different drivers 
can use overlapping ranges of handles. Then it would have to keep track 
of which driver which mapped handles belongs to.

> As you said, there can only ever be
> one _vdp_imp_device_create_x11_proc at the moment.  This is 
> protecting
> against an implementation that returns different gpa pointers, e.g.
> for different screens?  If so, that seems okay to me.

Well that would be one error case handled by the patch, which was not 
handled well before.

The more general problem is that the current code does not follow the 
memory model. That forbids a thread from writing to memory while another 
thread is reading it.

> Reviewed-by: Aaron Plattner <aplattner at nvidia.com>
>
>> This patch protects access to the _imp_get_proc_address variable to
>> conform to the memory model, and ensures that a single VDPAU
>> implementation is used - failing safe if not so.
>> ---
>>   src/vdpau_wrapper.c | 31 ++++++++++++++++++++++++-------
>>   1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/vdpau_wrapper.c b/src/vdpau_wrapper.c
>> index 7d4885d..196050b 100644
>> --- a/src/vdpau_wrapper.c
>> +++ b/src/vdpau_wrapper.c
>> @@ -526,6 +526,7 @@ VdpStatus vdp_device_create_x11(
>>   {
>>       static pthread_once_t once = PTHREAD_ONCE_INIT;
>>       static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
>> +    VdpGetProcAddress *gpa;
>>       VdpStatus status = VDP_STATUS_OK;
>>
>>       pthread_once(&once, init_fixes);
>> @@ -541,17 +542,33 @@ VdpStatus vdp_device_create_x11(
>>       if (status != VDP_STATUS_OK)
>>           return status;
>>
>> -    status = _vdp_imp_device_create_x11_proc(
>> -        display,
>> -        screen,
>> -        device,
>> -        &_imp_get_proc_address
>> -    );
>> +    status = _vdp_imp_device_create_x11_proc(display, screen, 
>> device, &gpa);
>>       if (status != VDP_STATUS_OK) {
>>           return status;
>>       }
>>
>>       *get_proc_address = vdp_wrapper_get_proc_address;
>>
>> -    return VDP_STATUS_OK;
>> +    pthread_mutex_lock(&lock);
>> +    if (_imp_get_proc_address != gpa) {
>> +        if (_imp_get_proc_address == NULL)
>> +            _imp_get_proc_address = gpa;
>> +        else
>> +        /* Currently the wrapper can only deal with one back-end.
>> +         * This should never happen, but better safe than sorry. */
>> +            status = VDP_STATUS_NO_IMPLEMENTATION;
>> +    }
>> +    pthread_mutex_unlock(&lock);
>> +
>> +    if (status != VDP_STATUS_OK) {
>> +        void *pv;
>> +
>> +        if (gpa(*device, VDP_FUNC_ID_DEVICE_DESTROY, &pv) == 
>> VDP_STATUS_OK) {
>> +            VdpDeviceDestroy *device_destroy = pv;
>> +
>> +            device_destroy(*device);
>> +        }
>> +    }
>> +
>> +    return status;
>>   }
>>

-- 
Rémi Denis-Courmont