This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8.
Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime
import/export callbacks". We have to do the same with qxl,
for the same reasons (it breaks DRI3).
Drop the WARN_ON_ONCE().
Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8
Signed-off-by: Gerd Hoffmann <[email protected]>
---
drivers/gpu/drm/qxl/qxl_drv.c | 4 ++++
drivers/gpu/drm/qxl/qxl_prime.c | 12 ++++++++++++
2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 578d867a81d5..f33e349c4ec5 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -255,10 +255,14 @@ static struct drm_driver qxl_driver = {
#if defined(CONFIG_DEBUG_FS)
.debugfs_init = qxl_debugfs_init,
#endif
+ .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
+ .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
.gem_prime_export = drm_gem_prime_export,
.gem_prime_import = drm_gem_prime_import,
.gem_prime_pin = qxl_gem_prime_pin,
.gem_prime_unpin = qxl_gem_prime_unpin,
+ .gem_prime_get_sg_table = qxl_gem_prime_get_sg_table,
+ .gem_prime_import_sg_table = qxl_gem_prime_import_sg_table,
.gem_prime_vmap = qxl_gem_prime_vmap,
.gem_prime_vunmap = qxl_gem_prime_vunmap,
.gem_prime_mmap = qxl_gem_prime_mmap,
diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
index 8b448eca1cd9..114653b471c6 100644
--- a/drivers/gpu/drm/qxl/qxl_prime.c
+++ b/drivers/gpu/drm/qxl/qxl_prime.c
@@ -42,6 +42,18 @@ void qxl_gem_prime_unpin(struct drm_gem_object *obj)
qxl_bo_unpin(bo);
}
+struct sg_table *qxl_gem_prime_get_sg_table(struct drm_gem_object *obj)
+{
+ return ERR_PTR(-ENOSYS);
+}
+
+struct drm_gem_object *qxl_gem_prime_import_sg_table(
+ struct drm_device *dev, struct dma_buf_attachment *attach,
+ struct sg_table *table)
+{
+ return ERR_PTR(-ENOSYS);
+}
+
void *qxl_gem_prime_vmap(struct drm_gem_object *obj)
{
struct qxl_bo *bo = gem_to_qxl_bo(obj);
--
2.18.1
On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <[email protected]> wrote:
>
> This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8.
>
> Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime
> import/export callbacks". We have to do the same with qxl,
> for the same reasons (it breaks DRI3).
>
> Drop the WARN_ON_ONCE().
>
> Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8
> Signed-off-by: Gerd Hoffmann <[email protected]>
Maybe we need some helpers for virtual drivers which only allow
self-reimport and nothing else at all? I think there's qxl, virgl,
vmwgfx and maybe also vbox one who could use this ... Just a quick
idea.
-Daniel
> ---
> drivers/gpu/drm/qxl/qxl_drv.c | 4 ++++
> drivers/gpu/drm/qxl/qxl_prime.c | 12 ++++++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index 578d867a81d5..f33e349c4ec5 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -255,10 +255,14 @@ static struct drm_driver qxl_driver = {
> #if defined(CONFIG_DEBUG_FS)
> .debugfs_init = qxl_debugfs_init,
> #endif
> + .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> + .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> .gem_prime_export = drm_gem_prime_export,
> .gem_prime_import = drm_gem_prime_import,
> .gem_prime_pin = qxl_gem_prime_pin,
> .gem_prime_unpin = qxl_gem_prime_unpin,
> + .gem_prime_get_sg_table = qxl_gem_prime_get_sg_table,
> + .gem_prime_import_sg_table = qxl_gem_prime_import_sg_table,
> .gem_prime_vmap = qxl_gem_prime_vmap,
> .gem_prime_vunmap = qxl_gem_prime_vunmap,
> .gem_prime_mmap = qxl_gem_prime_mmap,
> diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
> index 8b448eca1cd9..114653b471c6 100644
> --- a/drivers/gpu/drm/qxl/qxl_prime.c
> +++ b/drivers/gpu/drm/qxl/qxl_prime.c
> @@ -42,6 +42,18 @@ void qxl_gem_prime_unpin(struct drm_gem_object *obj)
> qxl_bo_unpin(bo);
> }
>
> +struct sg_table *qxl_gem_prime_get_sg_table(struct drm_gem_object *obj)
> +{
> + return ERR_PTR(-ENOSYS);
> +}
> +
> +struct drm_gem_object *qxl_gem_prime_import_sg_table(
> + struct drm_device *dev, struct dma_buf_attachment *attach,
> + struct sg_table *table)
> +{
> + return ERR_PTR(-ENOSYS);
> +}
> +
> void *qxl_gem_prime_vmap(struct drm_gem_object *obj)
> {
> struct qxl_bo *bo = gem_to_qxl_bo(obj);
> --
> 2.18.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On 4/26/19 4:21 PM, Daniel Vetter wrote:
> On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <[email protected]> wrote:
>> This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8.
>>
>> Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime
>> import/export callbacks". We have to do the same with qxl,
>> for the same reasons (it breaks DRI3).
>>
>> Drop the WARN_ON_ONCE().
>>
>> Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8
>> Signed-off-by: Gerd Hoffmann <[email protected]>
> Maybe we need some helpers for virtual drivers which only allow
> self-reimport and nothing else at all? I think there's qxl, virgl,
> vmwgfx and maybe also vbox one who could use this ... Just a quick
> idea.
> -Daniel
I think vmwgfx could, in theory, support the full range of operations,
at least for reasonably recent device versions. However, it wouldn't be
terribly efficient since the exported dma-buf sglist would basically be
a bounce-buffer.
/Thomas
>> ---
>> drivers/gpu/drm/qxl/qxl_drv.c | 4 ++++
>> drivers/gpu/drm/qxl/qxl_prime.c | 12 ++++++++++++
>> 2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
>> index 578d867a81d5..f33e349c4ec5 100644
>> --- a/drivers/gpu/drm/qxl/qxl_drv.c
>> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
>> @@ -255,10 +255,14 @@ static struct drm_driver qxl_driver = {
>> #if defined(CONFIG_DEBUG_FS)
>> .debugfs_init = qxl_debugfs_init,
>> #endif
>> + .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>> + .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> .gem_prime_export = drm_gem_prime_export,
>> .gem_prime_import = drm_gem_prime_import,
>> .gem_prime_pin = qxl_gem_prime_pin,
>> .gem_prime_unpin = qxl_gem_prime_unpin,
>> + .gem_prime_get_sg_table = qxl_gem_prime_get_sg_table,
>> + .gem_prime_import_sg_table = qxl_gem_prime_import_sg_table,
>> .gem_prime_vmap = qxl_gem_prime_vmap,
>> .gem_prime_vunmap = qxl_gem_prime_vunmap,
>> .gem_prime_mmap = qxl_gem_prime_mmap,
>> diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
>> index 8b448eca1cd9..114653b471c6 100644
>> --- a/drivers/gpu/drm/qxl/qxl_prime.c
>> +++ b/drivers/gpu/drm/qxl/qxl_prime.c
>> @@ -42,6 +42,18 @@ void qxl_gem_prime_unpin(struct drm_gem_object *obj)
>> qxl_bo_unpin(bo);
>> }
>>
>> +struct sg_table *qxl_gem_prime_get_sg_table(struct drm_gem_object *obj)
>> +{
>> + return ERR_PTR(-ENOSYS);
>> +}
>> +
>> +struct drm_gem_object *qxl_gem_prime_import_sg_table(
>> + struct drm_device *dev, struct dma_buf_attachment *attach,
>> + struct sg_table *table)
>> +{
>> + return ERR_PTR(-ENOSYS);
>> +}
>> +
>> void *qxl_gem_prime_vmap(struct drm_gem_object *obj)
>> {
>> struct qxl_bo *bo = gem_to_qxl_bo(obj);
>> --
>> 2.18.1
>>
>
On Fri, Apr 26, 2019 at 05:42:23PM +0200, Thomas Hellstrom wrote:
> On 4/26/19 4:21 PM, Daniel Vetter wrote:
> > On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <[email protected]> wrote:
> > > This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8.
> > >
> > > Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime
> > > import/export callbacks". We have to do the same with qxl,
> > > for the same reasons (it breaks DRI3).
> > >
> > > Drop the WARN_ON_ONCE().
> > >
> > > Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8
> > > Signed-off-by: Gerd Hoffmann <[email protected]>
> > Maybe we need some helpers for virtual drivers which only allow
> > self-reimport and nothing else at all? I think there's qxl, virgl,
> > vmwgfx and maybe also vbox one who could use this ... Just a quick
> > idea.
> > -Daniel
>
> I think vmwgfx could, in theory, support the full range of operations,
> at least for reasonably recent device versions. However, it wouldn't be
> terribly efficient since the exported dma-buf sglist would basically be a
> bounce-buffer.
Yeah not sure that makes sense to support really ...
-Daniel
>
> /Thomas
>
>
> > > ---
> > > drivers/gpu/drm/qxl/qxl_drv.c | 4 ++++
> > > drivers/gpu/drm/qxl/qxl_prime.c | 12 ++++++++++++
> > > 2 files changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> > > index 578d867a81d5..f33e349c4ec5 100644
> > > --- a/drivers/gpu/drm/qxl/qxl_drv.c
> > > +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> > > @@ -255,10 +255,14 @@ static struct drm_driver qxl_driver = {
> > > #if defined(CONFIG_DEBUG_FS)
> > > .debugfs_init = qxl_debugfs_init,
> > > #endif
> > > + .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> > > + .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> > > .gem_prime_export = drm_gem_prime_export,
> > > .gem_prime_import = drm_gem_prime_import,
> > > .gem_prime_pin = qxl_gem_prime_pin,
> > > .gem_prime_unpin = qxl_gem_prime_unpin,
> > > + .gem_prime_get_sg_table = qxl_gem_prime_get_sg_table,
> > > + .gem_prime_import_sg_table = qxl_gem_prime_import_sg_table,
> > > .gem_prime_vmap = qxl_gem_prime_vmap,
> > > .gem_prime_vunmap = qxl_gem_prime_vunmap,
> > > .gem_prime_mmap = qxl_gem_prime_mmap,
> > > diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
> > > index 8b448eca1cd9..114653b471c6 100644
> > > --- a/drivers/gpu/drm/qxl/qxl_prime.c
> > > +++ b/drivers/gpu/drm/qxl/qxl_prime.c
> > > @@ -42,6 +42,18 @@ void qxl_gem_prime_unpin(struct drm_gem_object *obj)
> > > qxl_bo_unpin(bo);
> > > }
> > >
> > > +struct sg_table *qxl_gem_prime_get_sg_table(struct drm_gem_object *obj)
> > > +{
> > > + return ERR_PTR(-ENOSYS);
> > > +}
> > > +
> > > +struct drm_gem_object *qxl_gem_prime_import_sg_table(
> > > + struct drm_device *dev, struct dma_buf_attachment *attach,
> > > + struct sg_table *table)
> > > +{
> > > + return ERR_PTR(-ENOSYS);
> > > +}
> > > +
> > > void *qxl_gem_prime_vmap(struct drm_gem_object *obj)
> > > {
> > > struct qxl_bo *bo = gem_to_qxl_bo(obj);
> > > --
> > > 2.18.1
> > >
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Fri, Apr 26, 2019 at 04:21:37PM +0200, Daniel Vetter wrote:
> On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <[email protected]> wrote:
> >
> > This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8.
> >
> > Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime
> > import/export callbacks". We have to do the same with qxl,
> > for the same reasons (it breaks DRI3).
> >
> > Drop the WARN_ON_ONCE().
> >
> > Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8
> > Signed-off-by: Gerd Hoffmann <[email protected]>
>
> Maybe we need some helpers for virtual drivers which only allow
> self-reimport and nothing else at all?
The current gem prime helpers handle this reasonable well,
I don't see a need for that.
More useful would be some way to signal this self-reimport capability
to userspace somehow. See DRM_PRIME_CAP_LOCAL patch.
Right now I have the choice to set DRM_PRIME_CAP_{IMPORT,EXPORT}, in
which case some userspace assumes it can do cross-driver export/import
and trips over that not working. Or I do not set
DRM_PRIME_CAP_{IMPORT,EXPORT}, which breaks DRI3 ...
> I think there's qxl, virgl,
export is implemented for virgl, and import should be possible too.
qxl/bochs is simliar to vmgfx, it could be done but dma-bufs would
basically be bounce buffers (need copy from/to vram) so probably not
worth the effort.
cheers,
Gerd
On Mon, Apr 29, 2019 at 9:54 AM Gerd Hoffmann <[email protected]> wrote:
>
> On Fri, Apr 26, 2019 at 04:21:37PM +0200, Daniel Vetter wrote:
> > On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <[email protected]> wrote:
> > >
> > > This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8.
> > >
> > > Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime
> > > import/export callbacks". We have to do the same with qxl,
> > > for the same reasons (it breaks DRI3).
> > >
> > > Drop the WARN_ON_ONCE().
> > >
> > > Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8
> > > Signed-off-by: Gerd Hoffmann <[email protected]>
> >
> > Maybe we need some helpers for virtual drivers which only allow
> > self-reimport and nothing else at all?
>
> The current gem prime helpers handle this reasonable well,
> I don't see a need for that.
>
> More useful would be some way to signal this self-reimport capability
> to userspace somehow. See DRM_PRIME_CAP_LOCAL patch.
Userspace is supposed to test whether import/export works for a
specific combo, not blindly assume it does and then keel over. I think
we need to fix that, not add more flags - there's lots of reasons why
a given pair of devices can't share buffers (e.g. all the contiguous
allocations stuff on socs).
> Right now I have the choice to set DRM_PRIME_CAP_{IMPORT,EXPORT}, in
> which case some userspace assumes it can do cross-driver export/import
> and trips over that not working. Or I do not set
> DRM_PRIME_CAP_{IMPORT,EXPORT}, which breaks DRI3 ...
Yeah that's not an option.
-Daniel
> > I think there's qxl, virgl,
>
> export is implemented for virgl, and import should be possible too.
>
> qxl/bochs is simliar to vmgfx, it could be done but dma-bufs would
> basically be bounce buffers (need copy from/to vram) so probably not
> worth the effort.
>
> cheers,
> Gerd
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi,
> > More useful would be some way to signal this self-reimport capability
> > to userspace somehow. See DRM_PRIME_CAP_LOCAL patch.
>
> Userspace is supposed to test whether import/export works for a
> specific combo, not blindly assume it does and then keel over. I think
> we need to fix that, not add more flags - there's lots of reasons why
> a given pair of devices can't share buffers (e.g. all the contiguous
> allocations stuff on socs).
Ok. Lets scratch the DRM_PRIME_CAP_LOCAL idea then and blame userspace
instead.
> > Right now I have the choice to set DRM_PRIME_CAP_{IMPORT,EXPORT}, in
> > which case some userspace assumes it can do cross-driver export/import
> > and trips over that not working. Or I do not set
> > DRM_PRIME_CAP_{IMPORT,EXPORT}, which breaks DRI3 ...
>
> Yeah that's not an option.
Good. Can I get an ack for this patch then, as it unbreaks DRI3 with qxl?
thanks,
Gerd
On Mon, Apr 29, 2019 at 4:38 PM Gerd Hoffmann <[email protected]> wrote:
>
> Hi,
>
> > > More useful would be some way to signal this self-reimport capability
> > > to userspace somehow. See DRM_PRIME_CAP_LOCAL patch.
> >
> > Userspace is supposed to test whether import/export works for a
> > specific combo, not blindly assume it does and then keel over. I think
> > we need to fix that, not add more flags - there's lots of reasons why
> > a given pair of devices can't share buffers (e.g. all the contiguous
> > allocations stuff on socs).
>
> Ok. Lets scratch the DRM_PRIME_CAP_LOCAL idea then and blame userspace
> instead.
>
> > > Right now I have the choice to set DRM_PRIME_CAP_{IMPORT,EXPORT}, in
> > > which case some userspace assumes it can do cross-driver export/import
> > > and trips over that not working. Or I do not set
> > > DRM_PRIME_CAP_{IMPORT,EXPORT}, which breaks DRI3 ...
> >
> > Yeah that's not an option.
>
> Good. Can I get an ack for this patch then, as it unbreaks DRI3 with qxl?
Oh sure, acked-by: me. Sorry forgot to supply that ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch