| Message ID | 20250326014902.379339-2-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] drm/virtio: Don't attach GEM to a non-created context in gem_object_open() | expand |
Hi Dmitry, > Subject: [PATCH v1 2/2] drm/virtio: Fix missed dmabuf unpinning in error path > of prepare_fb() > > Unpin imported dmabuf on fence allocation failure in prepare_fb(). > > Fixes: 4a696a2ee646 ("drm/virtio: Add prepare and cleanup routines for > imported dmabuf obj") > Cc: <stable@vger.kernel.org> # v6.14+ > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/virtio/virtgpu_plane.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c > b/drivers/gpu/drm/virtio/virtgpu_plane.c > index a6f5a78f436a..dc1d91639931 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_plane.c > +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c > @@ -342,6 +342,16 @@ static int virtio_gpu_prepare_imported_obj(struct > drm_plane *plane, > return ret; > } > > +static void virtio_gpu_cleanup_imported_obj(struct drm_gem_object *obj) > +{ > + struct dma_buf_attachment *attach = obj->import_attach; > + struct dma_resv *resv = attach->dmabuf->resv; > + > + dma_resv_lock(resv, NULL); > + dma_buf_unpin(attach); > + dma_resv_unlock(resv); > +} > + > static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane, > struct drm_plane_state *new_state) > { > @@ -376,23 +386,16 @@ static int virtio_gpu_plane_prepare_fb(struct > drm_plane *plane, > vgplane_st->fence = virtio_gpu_fence_alloc(vgdev, > vgdev->fence_drv.context, > 0); > - if (!vgplane_st->fence) > + if (!vgplane_st->fence) { > + if (obj->import_attach) > + virtio_gpu_cleanup_imported_obj(obj); I think checking for fence allocation failure before import would be much better. In other words, cleaning up the fence in case of any import errors would be much simpler IMO. Regardless, Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > return -ENOMEM; > + } > } > > return 0; > } > > -static void virtio_gpu_cleanup_imported_obj(struct drm_gem_object *obj) > -{ > - struct dma_buf_attachment *attach = obj->import_attach; > - struct dma_resv *resv = attach->dmabuf->resv; > - > - dma_resv_lock(resv, NULL); > - dma_buf_unpin(attach); > - dma_resv_unlock(resv); > -} > - > static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane, > struct drm_plane_state *state) > { > -- > 2.49.0 >
On 3/26/25 08:14, Kasireddy, Vivek wrote: ... >> static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane, >> struct drm_plane_state *new_state) >> { >> @@ -376,23 +386,16 @@ static int virtio_gpu_plane_prepare_fb(struct >> drm_plane *plane, >> vgplane_st->fence = virtio_gpu_fence_alloc(vgdev, >> vgdev->fence_drv.context, >> 0); >> - if (!vgplane_st->fence) >> + if (!vgplane_st->fence) { >> + if (obj->import_attach) >> + virtio_gpu_cleanup_imported_obj(obj); > I think checking for fence allocation failure before import would be much better. > In other words, cleaning up the fence in case of any import errors would be > much simpler IMO. > > Regardless, > Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com> Another question, why do we need this fencing for imported dmabuf? Fencing isn't done host/guest blobs in this code, while dmabuf is essentially a guest blob. Could you please clarify why this fence is needed? Maybe we shouldn't allocate fence in the first place for the dmabuf.
Hi Dmitry, > Subject: Re: [PATCH v1 2/2] drm/virtio: Fix missed dmabuf unpinning in error > path of prepare_fb() > > On 3/26/25 08:14, Kasireddy, Vivek wrote: > ... > >> static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane, > >> struct drm_plane_state *new_state) > >> { > >> @@ -376,23 +386,16 @@ static int virtio_gpu_plane_prepare_fb(struct > >> drm_plane *plane, > >> vgplane_st->fence = virtio_gpu_fence_alloc(vgdev, > >> vgdev->fence_drv.context, > >> 0); > >> - if (!vgplane_st->fence) > >> + if (!vgplane_st->fence) { > >> + if (obj->import_attach) > >> + virtio_gpu_cleanup_imported_obj(obj); > > I think checking for fence allocation failure before import would be much > better. > > In other words, cleaning up the fence in case of any import errors would be > > much simpler IMO. > > > > Regardless, > > Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > Another question, why do we need this fencing for imported dmabuf? > Fencing isn't done host/guest blobs in this code, while dmabuf is > essentially a guest blob. Could you please clarify why this fence is > needed? Maybe we shouldn't allocate fence in the first place for the dmabuf. At-least for the non-virgl use-cases (where the rendering is done in the Guest such as in passthrough), this Guest/Host synchronization fence serves two purposes: - It prevents the Guest from reusing/destroying the submitted buffer (Guest compositor FB) until the Host is done using it. Otherwise, the Guest compositor might render into this buffer at the same time while the Host is consuming it, leading to issues such as tearing/flickering. This problem is more noticeable in cases where the Guest compositor has only one backbuffer such as Xorg + dirtfyFb. - It also prevents the Guest compositor from rendering faster than the Host refresh rate. In other words, it just sets the upper bound in terms of the buffer submission rate as there is no point in going over the Host refresh rate, which would likely lead to wastage of GPU cycles and dropped frames. Therefore, this fence is really needed for Guest blobs including imported dmabufs. Thanks, Vivek > > -- > Best regards, > Dmitry
On 3/27/25 10:14, Kasireddy, Vivek wrote: >> Another question, why do we need this fencing for imported dmabuf? >> Fencing isn't done host/guest blobs in this code, while dmabuf is >> essentially a guest blob. Could you please clarify why this fence is >> needed? Maybe we shouldn't allocate fence in the first place for the dmabuf. > At-least for the non-virgl use-cases (where the rendering is done in the Guest such > as in passthrough), this Guest/Host synchronization fence serves two purposes: > - It prevents the Guest from reusing/destroying the submitted buffer (Guest compositor > FB) until the Host is done using it. Otherwise, the Guest compositor might render > into this buffer at the same time while the Host is consuming it, leading to issues > such as tearing/flickering. This problem is more noticeable in cases where the > Guest compositor has only one backbuffer such as Xorg + dirtfyFb. > > - It also prevents the Guest compositor from rendering faster than the Host refresh > rate. In other words, it just sets the upper bound in terms of the buffer submission > rate as there is no point in going over the Host refresh rate, which would likely > lead to wastage of GPU cycles and dropped frames. > > Therefore, this fence is really needed for Guest blobs including imported dmabufs. Thanks a lot
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c index a6f5a78f436a..dc1d91639931 100644 --- a/drivers/gpu/drm/virtio/virtgpu_plane.c +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c @@ -342,6 +342,16 @@ static int virtio_gpu_prepare_imported_obj(struct drm_plane *plane, return ret; } +static void virtio_gpu_cleanup_imported_obj(struct drm_gem_object *obj) +{ + struct dma_buf_attachment *attach = obj->import_attach; + struct dma_resv *resv = attach->dmabuf->resv; + + dma_resv_lock(resv, NULL); + dma_buf_unpin(attach); + dma_resv_unlock(resv); +} + static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane, struct drm_plane_state *new_state) { @@ -376,23 +386,16 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane, vgplane_st->fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, 0); - if (!vgplane_st->fence) + if (!vgplane_st->fence) { + if (obj->import_attach) + virtio_gpu_cleanup_imported_obj(obj); return -ENOMEM; + } } return 0; } -static void virtio_gpu_cleanup_imported_obj(struct drm_gem_object *obj) -{ - struct dma_buf_attachment *attach = obj->import_attach; - struct dma_resv *resv = attach->dmabuf->resv; - - dma_resv_lock(resv, NULL); - dma_buf_unpin(attach); - dma_resv_unlock(resv); -} - static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane, struct drm_plane_state *state) {
Unpin imported dmabuf on fence allocation failure in prepare_fb(). Fixes: 4a696a2ee646 ("drm/virtio: Add prepare and cleanup routines for imported dmabuf obj") Cc: <stable@vger.kernel.org> # v6.14+ Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> --- drivers/gpu/drm/virtio/virtgpu_plane.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)