[Mesa-dev] [PATCH] st/vdpau: add device reference counting
Christian König
deathsimple at vodafone.de
Wed Aug 13 08:51:24 PDT 2014
Am 13.08.2014 um 17:13 schrieb Ilia Mirkin:
> On Wed, Aug 13, 2014 at 11:07 AM, Christian König
> <deathsimple at vodafone.de> wrote:
>> From: Christian König <christian.koenig at amd.com>
>>
>> This fixes an issue with flash where it tries to destroy a decoder
>> after already destroying the device associated with the decoder.
>>
>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=82517
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
> Have you considered the opposite approach -- tearing everything down
> when the device is torn down, instead of keeping the device alive
> longer?
Yeah, considered that as well but refcounting of the device was just
easier to implement.
I mostly think that this is just a workaround for a buggy application
and I don't want anything like this in the driver if it isn't
lightweight and/or necessary.
> Any idea what NVIDIA does? (Do the VDPAU docs say anything
> about this? I don't see it anywhere.)
I briefly remember reading about that, but couldn't find it of hand any
more.
> The reason I bring it up is that now an application that doesn't
> destroy everything about a device will end up leaking it, which may be
> a heavier object to leak than just surfaces or something.
As long as it doesn't crash with this approach and the VDPAU spec
doesn't mandates something else I would like to stay with just
refcounting the device.
Trying to work around all buggy applications in the driver is usually a
hopeless effort and only encourages application developers to not fix
bugs any more.
Thanks for the comment,
Christian.
>
>> ---
>> src/gallium/state_trackers/vdpau/bitmap.c | 4 +++-
>> src/gallium/state_trackers/vdpau/decode.c | 4 +++-
>> src/gallium/state_trackers/vdpau/device.c | 21 ++++++++++++++++-----
>> src/gallium/state_trackers/vdpau/mixer.c | 4 +++-
>> src/gallium/state_trackers/vdpau/output.c | 4 +++-
>> src/gallium/state_trackers/vdpau/presentation.c | 4 +++-
>> src/gallium/state_trackers/vdpau/surface.c | 4 +++-
>> src/gallium/state_trackers/vdpau/vdpau_private.h | 12 ++++++++++++
>> 8 files changed, 46 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/vdpau/bitmap.c b/src/gallium/state_trackers/vdpau/bitmap.c
>> index a068921..97a4287 100644
>> --- a/src/gallium/state_trackers/vdpau/bitmap.c
>> +++ b/src/gallium/state_trackers/vdpau/bitmap.c
>> @@ -67,7 +67,7 @@ vlVdpBitmapSurfaceCreate(VdpDevice device,
>> if (!vlsurface)
>> return VDP_STATUS_RESOURCES;
>>
>> - vlsurface->device = dev;
>> + DeviceReference(&vlsurface->device, dev);
>>
>> memset(&res_tmpl, 0, sizeof(res_tmpl));
>> res_tmpl.target = PIPE_TEXTURE_2D;
>> @@ -117,6 +117,7 @@ err_sampler:
>> pipe_sampler_view_reference(&vlsurface->sampler_view, NULL);
>> err_unlock:
>> pipe_mutex_unlock(dev->mutex);
>> + DeviceReference(&vlsurface->device, NULL);
>> FREE(vlsurface);
>> return ret;
>> }
>> @@ -138,6 +139,7 @@ vlVdpBitmapSurfaceDestroy(VdpBitmapSurface surface)
>> pipe_mutex_unlock(vlsurface->device->mutex);
>>
>> vlRemoveDataHTAB(surface);
>> + DeviceReference(&vlsurface->device, NULL);
>> FREE(vlsurface);
>>
>> return VDP_STATUS_OK;
>> diff --git a/src/gallium/state_trackers/vdpau/decode.c b/src/gallium/state_trackers/vdpau/decode.c
>> index 1e5f81e..767d311 100644
>> --- a/src/gallium/state_trackers/vdpau/decode.c
>> +++ b/src/gallium/state_trackers/vdpau/decode.c
>> @@ -110,7 +110,7 @@ vlVdpDecoderCreate(VdpDevice device,
>> return VDP_STATUS_RESOURCES;
>> }
>>
>> - vldecoder->device = dev;
>> + DeviceReference(&vldecoder->device, dev);
>>
>> templat.entrypoint = PIPE_VIDEO_ENTRYPOINT_BITSTREAM;
>> templat.chroma_format = PIPE_VIDEO_CHROMA_FORMAT_420;
>> @@ -141,6 +141,7 @@ error_handle:
>>
>> error_decoder:
>> pipe_mutex_unlock(dev->mutex);
>> + DeviceReference(&vldecoder->device, NULL);
>> FREE(vldecoder);
>> return ret;
>> }
>> @@ -163,6 +164,7 @@ vlVdpDecoderDestroy(VdpDecoder decoder)
>> pipe_mutex_destroy(vldecoder->mutex);
>>
>> vlRemoveDataHTAB(decoder);
>> + DeviceReference(&vldecoder->device, NULL);
>> FREE(vldecoder);
>>
>> return VDP_STATUS_OK;
>> diff --git a/src/gallium/state_trackers/vdpau/device.c b/src/gallium/state_trackers/vdpau/device.c
>> index 0cdda73..9c5ec60 100644
>> --- a/src/gallium/state_trackers/vdpau/device.c
>> +++ b/src/gallium/state_trackers/vdpau/device.c
>> @@ -59,6 +59,8 @@ vdp_imp_device_create_x11(Display *display, int screen, VdpDevice *device,
>> goto no_dev;
>> }
>>
>> + pipe_reference_init(&dev->reference, 1);
>> +
>> dev->vscreen = vl_screen_create(display, screen);
>> if (!dev->vscreen) {
>> ret = VDP_STATUS_RESOURCES;
>> @@ -124,7 +126,7 @@ vlVdpPresentationQueueTargetCreateX11(VdpDevice device, Drawable drawable,
>> if (!pqt)
>> return VDP_STATUS_RESOURCES;
>>
>> - pqt->device = dev;
>> + DeviceReference(&pqt->device, dev);
>> pqt->drawable = drawable;
>>
>> *target = vlAddDataHTAB(pqt);
>> @@ -153,6 +155,7 @@ vlVdpPresentationQueueTargetDestroy(VdpPresentationQueueTarget presentation_queu
>> return VDP_STATUS_INVALID_HANDLE;
>>
>> vlRemoveDataHTAB(presentation_queue_target);
>> + DeviceReference(&pqt->device, NULL);
>> FREE(pqt);
>>
>> return VDP_STATUS_OK;
>> @@ -168,16 +171,24 @@ vlVdpDeviceDestroy(VdpDevice device)
>> if (!dev)
>> return VDP_STATUS_INVALID_HANDLE;
>>
>> + vlRemoveDataHTAB(device);
>> + DeviceReference(&dev, NULL);
>> +
>> + return VDP_STATUS_OK;
>> +}
>> +
>> +/**
>> + * Free a VdpDevice.
>> + */
>> +void
>> +vlVdpDeviceFree(vlVdpDevice *dev)
>> +{
>> pipe_mutex_destroy(dev->mutex);
>> vl_compositor_cleanup(&dev->compositor);
>> dev->context->destroy(dev->context);
>> vl_screen_destroy(dev->vscreen);
>> -
>> - vlRemoveDataHTAB(device);
>> FREE(dev);
>> vlDestroyHTAB();
>> -
>> - return VDP_STATUS_OK;
>> }
>>
>> /**
>> diff --git a/src/gallium/state_trackers/vdpau/mixer.c b/src/gallium/state_trackers/vdpau/mixer.c
>> index e6bfb8c..a724aa5 100644
>> --- a/src/gallium/state_trackers/vdpau/mixer.c
>> +++ b/src/gallium/state_trackers/vdpau/mixer.c
>> @@ -60,7 +60,7 @@ vlVdpVideoMixerCreate(VdpDevice device,
>> if (!vmixer)
>> return VDP_STATUS_RESOURCES;
>>
>> - vmixer->device = dev;
>> + DeviceReference(&vmixer->device, dev);
>>
>> pipe_mutex_lock(dev->mutex);
>>
>> @@ -160,6 +160,7 @@ no_params:
>> no_handle:
>> vl_compositor_cleanup_state(&vmixer->cstate);
>> pipe_mutex_unlock(dev->mutex);
>> + DeviceReference(&vmixer->device, NULL);
>> FREE(vmixer);
>> return ret;
>> }
>> @@ -199,6 +200,7 @@ vlVdpVideoMixerDestroy(VdpVideoMixer mixer)
>> FREE(vmixer->sharpness.filter);
>> }
>> pipe_mutex_unlock(vmixer->device->mutex);
>> + DeviceReference(&vmixer->device, NULL);
>>
>> FREE(vmixer);
>>
>> diff --git a/src/gallium/state_trackers/vdpau/output.c b/src/gallium/state_trackers/vdpau/output.c
>> index 457f678..caae50f 100644
>> --- a/src/gallium/state_trackers/vdpau/output.c
>> +++ b/src/gallium/state_trackers/vdpau/output.c
>> @@ -69,7 +69,7 @@ vlVdpOutputSurfaceCreate(VdpDevice device,
>> if (!vlsurface)
>> return VDP_STATUS_RESOURCES;
>>
>> - vlsurface->device = dev;
>> + DeviceReference(&vlsurface->device, dev);
>>
>> memset(&res_tmpl, 0, sizeof(res_tmpl));
>>
>> @@ -120,6 +120,7 @@ err_resource:
>> pipe_resource_reference(&res, NULL);
>> err_unlock:
>> pipe_mutex_unlock(dev->mutex);
>> + DeviceReference(&vlsurface->device, NULL);
>> FREE(vlsurface);
>> return VDP_STATUS_ERROR;
>> }
>> @@ -149,6 +150,7 @@ vlVdpOutputSurfaceDestroy(VdpOutputSurface surface)
>> pipe_mutex_unlock(vlsurface->device->mutex);
>>
>> vlRemoveDataHTAB(surface);
>> + DeviceReference(&vlsurface->device, NULL);
>> FREE(vlsurface);
>>
>> return VDP_STATUS_OK;
>> diff --git a/src/gallium/state_trackers/vdpau/presentation.c b/src/gallium/state_trackers/vdpau/presentation.c
>> index cb6cb38..7f8dbed 100644
>> --- a/src/gallium/state_trackers/vdpau/presentation.c
>> +++ b/src/gallium/state_trackers/vdpau/presentation.c
>> @@ -62,7 +62,7 @@ vlVdpPresentationQueueCreate(VdpDevice device,
>> if (!pq)
>> return VDP_STATUS_RESOURCES;
>>
>> - pq->device = dev;
>> + DeviceReference(&pq->device, dev);
>> pq->drawable = pqt->drawable;
>>
>> pipe_mutex_lock(dev->mutex);
>> @@ -83,6 +83,7 @@ vlVdpPresentationQueueCreate(VdpDevice device,
>>
>> no_handle:
>> no_compositor:
>> + DeviceReference(&pq->device, NULL);
>> FREE(pq);
>> return ret;
>> }
>> @@ -104,6 +105,7 @@ vlVdpPresentationQueueDestroy(VdpPresentationQueue presentation_queue)
>> pipe_mutex_unlock(pq->device->mutex);
>>
>> vlRemoveDataHTAB(presentation_queue);
>> + DeviceReference(&pq->device, NULL);
>> FREE(pq);
>>
>> return VDP_STATUS_OK;
>> diff --git a/src/gallium/state_trackers/vdpau/surface.c b/src/gallium/state_trackers/vdpau/surface.c
>> index 0d9f2b0..1932cdd 100644
>> --- a/src/gallium/state_trackers/vdpau/surface.c
>> +++ b/src/gallium/state_trackers/vdpau/surface.c
>> @@ -74,7 +74,7 @@ vlVdpVideoSurfaceCreate(VdpDevice device, VdpChromaType chroma_type,
>> goto inv_device;
>> }
>>
>> - p_surf->device = dev;
>> + DeviceReference(&p_surf->device, dev);
>> pipe = dev->context;
>>
>> pipe_mutex_lock(dev->mutex);
>> @@ -115,6 +115,7 @@ no_handle:
>> p_surf->video_buffer->destroy(p_surf->video_buffer);
>>
>> inv_device:
>> + DeviceReference(&p_surf->device, NULL);
>> FREE(p_surf);
>>
>> no_res:
>> @@ -140,6 +141,7 @@ vlVdpVideoSurfaceDestroy(VdpVideoSurface surface)
>> pipe_mutex_unlock(p_surf->device->mutex);
>>
>> vlRemoveDataHTAB(surface);
>> + DeviceReference(&p_surf->device, NULL);
>> FREE(p_surf);
>>
>> return VDP_STATUS_OK;
>> diff --git a/src/gallium/state_trackers/vdpau/vdpau_private.h b/src/gallium/state_trackers/vdpau/vdpau_private.h
>> index ce6852b..65f8e47 100644
>> --- a/src/gallium/state_trackers/vdpau/vdpau_private.h
>> +++ b/src/gallium/state_trackers/vdpau/vdpau_private.h
>> @@ -344,6 +344,7 @@ CheckSurfaceParams(struct pipe_screen *screen,
>>
>> typedef struct
>> {
>> + struct pipe_reference reference;
>> struct vl_screen *vscreen;
>> struct pipe_context *context;
>> struct vl_compositor compositor;
>> @@ -453,6 +454,7 @@ void vlVdpSave4DelayedRendering(vlVdpDevice *dev, VdpOutputSurface surface, stru
>> /* Internal function pointers */
>> VdpGetErrorString vlVdpGetErrorString;
>> VdpDeviceDestroy vlVdpDeviceDestroy;
>> +void vlVdpDeviceFree(vlVdpDevice *dev);
>> VdpGetProcAddress vlVdpGetProcAddress;
>> VdpGetApiVersion vlVdpGetApiVersion;
>> VdpGetInformationString vlVdpGetInformationString;
>> @@ -542,4 +544,14 @@ static inline void VDPAU_MSG(unsigned int level, const char *fmt, ...)
>> }
>> }
>>
>> +static inline void
>> +DeviceReference(vlVdpDevice **ptr, vlVdpDevice *dev)
>> +{
>> + vlVdpDevice *old_dev = *ptr;
>> +
>> + if (pipe_reference(&(*ptr)->reference, &dev->reference))
>> + vlVdpDeviceFree(old_dev);
>> + *ptr = dev;
>> +}
>> +
>> #endif /* VDPAU_PRIVATE_H */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list