| Message ID | 20240322214801.319975-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/prime: Unbreak virtgpu dma-buf export | expand |
It also fixes importing virtgpu blobs into real hardware, for instance amdgpu for DRI_PRIME rendering. On Fri, Mar 22, 2024 at 2:48 PM Rob Clark <robdclark@gmail.com> wrote: > From: Rob Clark <robdclark@chromium.org> > > virtgpu "vram" GEM objects do not implement obj->get_sg_table(). But > they also don't use drm_gem_map_dma_buf(). In fact they may not even > have guest visible pages. But it is perfectly fine to export and share > with other virtual devices. > > Reported-by: Dominik Behr <dbehr@chromium.org> > Fixes: 207395da5a97 ("drm/prime: reject DMA-BUF attach when get_sg_table > is missing") > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > drivers/gpu/drm/drm_prime.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 7352bde299d5..64dd6276e828 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -582,7 +582,12 @@ int drm_gem_map_attach(struct dma_buf *dma_buf, > { > struct drm_gem_object *obj = dma_buf->priv; > > - if (!obj->funcs->get_sg_table) > + /* > + * drm_gem_map_dma_buf() requires obj->get_sg_table(), but drivers > + * that implement their own ->map_dma_buf() do not. > + */ > + if ((dma_buf->ops->map_dma_buf == drm_gem_map_dma_buf) && > + !obj->funcs->get_sg_table) > return -ENOSYS; > > return drm_gem_pin(obj); > -- > 2.44.0 > >
It also fixes importing virtgpu blobs into real hardware, for instance amdgpu for DRI_PRIME rendering. On Fri, Mar 22, 2024 at 2:48 PM Rob Clark <robdclark@gmail.com> wrote: > > From: Rob Clark <robdclark@chromium.org> > > virtgpu "vram" GEM objects do not implement obj->get_sg_table(). But > they also don't use drm_gem_map_dma_buf(). In fact they may not even > have guest visible pages. But it is perfectly fine to export and share > with other virtual devices. > > Reported-by: Dominik Behr <dbehr@chromium.org> > Fixes: 207395da5a97 ("drm/prime: reject DMA-BUF attach when get_sg_table is missing") > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > drivers/gpu/drm/drm_prime.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 7352bde299d5..64dd6276e828 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -582,7 +582,12 @@ int drm_gem_map_attach(struct dma_buf *dma_buf, > { > struct drm_gem_object *obj = dma_buf->priv; > > - if (!obj->funcs->get_sg_table) > + /* > + * drm_gem_map_dma_buf() requires obj->get_sg_table(), but drivers > + * that implement their own ->map_dma_buf() do not. > + */ > + if ((dma_buf->ops->map_dma_buf == drm_gem_map_dma_buf) && > + !obj->funcs->get_sg_table) > return -ENOSYS; > > return drm_gem_pin(obj); > -- > 2.44.0 >
This is actually a bit concerning.. importing a host page backed buffer without guest mapping into a passthru device probably doesn't work and should be rejected earlier. I do think we should relax the restriction (either taking my patch or reverting the commit it fixes) until we work this out properly (because the original patch is a regression), but importing a buffer without guest pages into a passthru device can't possibly work properly. Maybe it works by chance if the host buffer is mapped to the guest, but that is not guaranteed. BR, -R On Mon, Mar 25, 2024 at 3:35 PM Dominik Behr <dbehr@chromium.org> wrote: > > It also fixes importing virtgpu blobs into real hardware, for instance amdgpu for DRI_PRIME rendering. > > On Fri, Mar 22, 2024 at 2:48 PM Rob Clark <robdclark@gmail.com> wrote: >> >> From: Rob Clark <robdclark@chromium.org> >> >> virtgpu "vram" GEM objects do not implement obj->get_sg_table(). But >> they also don't use drm_gem_map_dma_buf(). In fact they may not even >> have guest visible pages. But it is perfectly fine to export and share >> with other virtual devices. >> >> Reported-by: Dominik Behr <dbehr@chromium.org> >> Fixes: 207395da5a97 ("drm/prime: reject DMA-BUF attach when get_sg_table is missing") >> Signed-off-by: Rob Clark <robdclark@chromium.org> >> --- >> drivers/gpu/drm/drm_prime.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 7352bde299d5..64dd6276e828 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -582,7 +582,12 @@ int drm_gem_map_attach(struct dma_buf *dma_buf, >> { >> struct drm_gem_object *obj = dma_buf->priv; >> >> - if (!obj->funcs->get_sg_table) >> + /* >> + * drm_gem_map_dma_buf() requires obj->get_sg_table(), but drivers >> + * that implement their own ->map_dma_buf() do not. >> + */ >> + if ((dma_buf->ops->map_dma_buf == drm_gem_map_dma_buf) && >> + !obj->funcs->get_sg_table) >> return -ENOSYS; >> >> return drm_gem_pin(obj); >> -- >> 2.44.0 >>
Makes sense to me!
Reviewed-by: Simon Ser <contact@emersion.fr>
It is mapped, via iommu interface (vfio-iommu) when map_dma is called. -- Dominik On Mon, Mar 25, 2024 at 7:38 PM Rob Clark <robdclark@gmail.com> wrote: > > This is actually a bit concerning.. importing a host page backed > buffer without guest mapping into a passthru device probably doesn't > work and should be rejected earlier. > > I do think we should relax the restriction (either taking my patch or > reverting the commit it fixes) until we work this out properly > (because the original patch is a regression), but importing a buffer > without guest pages into a passthru device can't possibly work > properly. Maybe it works by chance if the host buffer is mapped to > the guest, but that is not guaranteed. > > BR, > -R > > On Mon, Mar 25, 2024 at 3:35 PM Dominik Behr <dbehr@chromium.org> wrote: > > > > It also fixes importing virtgpu blobs into real hardware, for instance amdgpu for DRI_PRIME rendering. > > > > On Fri, Mar 22, 2024 at 2:48 PM Rob Clark <robdclark@gmail.com> wrote: > >> > >> From: Rob Clark <robdclark@chromium.org> > >> > >> virtgpu "vram" GEM objects do not implement obj->get_sg_table(). But > >> they also don't use drm_gem_map_dma_buf(). In fact they may not even > >> have guest visible pages. But it is perfectly fine to export and share > >> with other virtual devices. > >> > >> Reported-by: Dominik Behr <dbehr@chromium.org> > >> Fixes: 207395da5a97 ("drm/prime: reject DMA-BUF attach when get_sg_table is missing") > >> Signed-off-by: Rob Clark <robdclark@chromium.org> > >> --- > >> drivers/gpu/drm/drm_prime.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > >> index 7352bde299d5..64dd6276e828 100644 > >> --- a/drivers/gpu/drm/drm_prime.c > >> +++ b/drivers/gpu/drm/drm_prime.c > >> @@ -582,7 +582,12 @@ int drm_gem_map_attach(struct dma_buf *dma_buf, > >> { > >> struct drm_gem_object *obj = dma_buf->priv; > >> > >> - if (!obj->funcs->get_sg_table) > >> + /* > >> + * drm_gem_map_dma_buf() requires obj->get_sg_table(), but drivers > >> + * that implement their own ->map_dma_buf() do not. > >> + */ > >> + if ((dma_buf->ops->map_dma_buf == drm_gem_map_dma_buf) && > >> + !obj->funcs->get_sg_table) > >> return -ENOSYS; > >> > >> return drm_gem_pin(obj); > >> -- > >> 2.44.0 > >>
On Tue, Mar 26, 2024 at 2:15 AM Simon Ser <contact@emersion.fr> wrote: > > Makes sense to me! > > Reviewed-by: Simon Ser <contact@emersion.fr> Thanks.. any chance I could talk you into pushing to drm-misc-fixes? BR, -R
On Thursday, March 28th, 2024 at 19:47, Rob Clark <robdclark@gmail.com> wrote:
> any chance I could talk you into pushing to drm-misc-fixes?
Oh sorry, I thought you had access… Pushed with a minor edit to remove
unnecessary parentheses to make checkpatch happy!
On Thu, Mar 28, 2024 at 11:54 AM Simon Ser <contact@emersion.fr> wrote: > > On Thursday, March 28th, 2024 at 19:47, Rob Clark <robdclark@gmail.com> wrote: > > > any chance I could talk you into pushing to drm-misc-fixes? > > Oh sorry, I thought you had access… Pushed with a minor edit to remove > unnecessary parentheses to make checkpatch happy! Thanks! BR, -R
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7352bde299d5..64dd6276e828 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -582,7 +582,12 @@ int drm_gem_map_attach(struct dma_buf *dma_buf, { struct drm_gem_object *obj = dma_buf->priv; - if (!obj->funcs->get_sg_table) + /* + * drm_gem_map_dma_buf() requires obj->get_sg_table(), but drivers + * that implement their own ->map_dma_buf() do not. + */ + if ((dma_buf->ops->map_dma_buf == drm_gem_map_dma_buf) && + !obj->funcs->get_sg_table) return -ENOSYS; return drm_gem_pin(obj);