[Mesa-dev] [PATCH] st/vdpau: add device reference counting
Ilia Mirkin
imirkin at alum.mit.edu
Wed Aug 13 13:04:51 PDT 2014
On Wed, Aug 13, 2014 at 11:51 AM, Christian König
<deathsimple at vodafone.de> wrote:
> 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.
Makes sense. This change looks fine, but I didn't actually do an
in-depth check that you made sure that you hit *every* place that
needed it. So this gets my
Acked-by: Ilia Mirkin <imirkin at alum.mit.edu>
>
> 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