Re: [Mesa-dev] [PATCH] st/vdpau: add device reference counting

Wed, 13 Aug 2014 08:52:21 -0700 

Am 13.08.2014 um 17:13 schrieb Ilia Mirkin:

On Wed, Aug 13, 2014 at 11:07 AM, Christian König
<deathsim...@vodafone.de> wrote:

From: Christian König <christian.koe...@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.koe...@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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to