diff mbox series

drm/vgem: create a render node for vgem

Message ID 20181026120647.7528-1-emil.l.velikov@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm/vgem: create a render node for vgem | expand

Commit Message

Emil Velikov Oct. 26, 2018, 12:06 p.m. UTC 
From: Emil Velikov <emil.velikov@collabora.com>

VGEM doesn't do anything modeset specific, so in a way exposing a
primary node is 'wrong'. At the same time, we extensively use if for
creating dumb buffers, fences, prime fd <> handle imports/exports.

To the point that we explicitly annotate the vgem fence ioctls as
DRM_RENDER_ALLOW and have an IGT test which opens the render node.

close(drm_open_driver_render(DRIVER_VGEM))

Better late than never, let's flip the switch.

Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/vgem/vgem_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Daniel Vetter Oct. 26, 2018, 1:40 p.m. UTC | #1 
On Fri, Oct 26, 2018 at 01:06:47PM +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> VGEM doesn't do anything modeset specific, so in a way exposing a
> primary node is 'wrong'. At the same time, we extensively use if for
> creating dumb buffers, fences, prime fd <> handle imports/exports.
> 
> To the point that we explicitly annotate the vgem fence ioctls as
> DRM_RENDER_ALLOW and have an IGT test which opens the render node.
> 
> close(drm_open_driver_render(DRIVER_VGEM))

Huh, I guess that test doesn't pass?

> Better late than never, let's flip the switch.
> 
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/vgem/vgem_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index f1f7ab9dcdbf..f1d1d9e2c82e 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -431,7 +431,8 @@ static void vgem_release(struct drm_device *dev)
>  }
>  
>  static struct drm_driver vgem_driver = {
> -	.driver_features		= DRIVER_GEM | DRIVER_PRIME,
> +	.driver_features		= DRIVER_GEM | DRIVER_PRIME |
> +					  DRIVER_RENDER;
>  	.release			= vgem_release,
>  	.open				= vgem_open,
>  	.postclose			= vgem_postclose,
> -- 
> 2.19.1
>
Chris Wilson Oct. 26, 2018, 2:40 p.m. UTC | #2 
Quoting Daniel Vetter (2018-10-26 14:40:36)
> On Fri, Oct 26, 2018 at 01:06:47PM +0100, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> > 
> > VGEM doesn't do anything modeset specific, so in a way exposing a
> > primary node is 'wrong'. At the same time, we extensively use if for
> > creating dumb buffers, fences, prime fd <> handle imports/exports.

What problem are you solving? Now if you suggest a way to only expose a
render node and not a master node, that might be more interesting.
-Chris
kernel test robot Oct. 26, 2018, 3:55 p.m. UTC | #3 
Hi Emil,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sof-driver-fuweitax/master]
[also build test ERROR on v4.19 next-20181019]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Emil-Velikov/drm-vgem-create-a-render-node-for-vgem/20181026-233734
base:   https://github.com/fuweitax/linux master
config: i386-randconfig-x077-201842 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/vgem/vgem_drv.c:436:21: error: expected '}' before ';' token
           DRIVER_RENDER;
                        ^
   drivers/gpu/drm/vgem/vgem_drv.c:424:13: warning: 'vgem_release' defined but not used [-Wunused-function]
    static void vgem_release(struct drm_device *dev)
                ^~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:401:12: warning: 'vgem_prime_mmap' defined but not used [-Wunused-function]
    static int vgem_prime_mmap(struct drm_gem_object *obj,
               ^~~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:393:13: warning: 'vgem_prime_vunmap' defined but not used [-Wunused-function]
    static void vgem_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
                ^~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:380:14: warning: 'vgem_prime_vmap' defined but not used [-Wunused-function]
    static void *vgem_prime_vmap(struct drm_gem_object *obj)
                 ^~~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:355:31: warning: 'vgem_prime_import_sg_table' defined but not used [-Wunused-function]
    static struct drm_gem_object *vgem_prime_import_sg_table(struct drm_device *dev,
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:347:31: warning: 'vgem_prime_import' defined but not used [-Wunused-function]
    static struct drm_gem_object* vgem_prime_import(struct drm_device *dev,
                                  ^~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:340:25: warning: 'vgem_prime_get_sg_table' defined but not used [-Wunused-function]
    static struct sg_table *vgem_prime_get_sg_table(struct drm_gem_object *obj)
                            ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:333:13: warning: 'vgem_prime_unpin' defined but not used [-Wunused-function]
    static void vgem_prime_unpin(struct drm_gem_object *obj)
                ^~~~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:315:12: warning: 'vgem_prime_pin' defined but not used [-Wunused-function]
    static int vgem_prime_pin(struct drm_gem_object *obj)
               ^~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:253:30: warning: 'vgem_ioctls' defined but not used [-Wunused-variable]
    static struct drm_ioctl_desc vgem_ioctls[] = {
                                 ^~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:227:12: warning: 'vgem_gem_dumb_map' defined but not used [-Wunused-function]
    static int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev,
               ^~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:204:12: warning: 'vgem_gem_dumb_create' defined but not used [-Wunused-function]
    static int vgem_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
               ^~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:145:13: warning: 'vgem_postclose' defined but not used [-Wunused-function]
    static void vgem_postclose(struct drm_device *dev, struct drm_file *file)
                ^~~~~~~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:125:12: warning: 'vgem_open' defined but not used [-Wunused-function]
    static int vgem_open(struct drm_device *dev, struct drm_file *file)
               ^~~~~~~~~
   drivers/gpu/drm/vgem/vgem_drv.c:50:13: warning: 'vgem_gem_free_object' defined but not used [-Wunused-function]
    static void vgem_gem_free_object(struct drm_gem_object *obj)
                ^~~~~~~~~~~~~~~~~~~~

vim +436 drivers/gpu/drm/vgem/vgem_drv.c

   433	
   434	static struct drm_driver vgem_driver = {
   435		.driver_features		= DRIVER_GEM | DRIVER_PRIME |
 > 436						  DRIVER_RENDER;
   437		.release			= vgem_release,
   438		.open				= vgem_open,
   439		.postclose			= vgem_postclose,
   440		.gem_free_object_unlocked	= vgem_gem_free_object,
   441		.gem_vm_ops			= &vgem_gem_vm_ops,
   442		.ioctls				= vgem_ioctls,
   443		.num_ioctls 			= ARRAY_SIZE(vgem_ioctls),
   444		.fops				= &vgem_driver_fops,
   445	
   446		.dumb_create			= vgem_gem_dumb_create,
   447		.dumb_map_offset		= vgem_gem_dumb_map,
   448	
   449		.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
   450		.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
   451		.gem_prime_pin = vgem_prime_pin,
   452		.gem_prime_unpin = vgem_prime_unpin,
   453		.gem_prime_import = vgem_prime_import,
   454		.gem_prime_export = drm_gem_prime_export,
   455		.gem_prime_import_sg_table = vgem_prime_import_sg_table,
   456		.gem_prime_get_sg_table = vgem_prime_get_sg_table,
   457		.gem_prime_vmap = vgem_prime_vmap,
   458		.gem_prime_vunmap = vgem_prime_vunmap,
   459		.gem_prime_mmap = vgem_prime_mmap,
   460	
   461		.name	= DRIVER_NAME,
   462		.desc	= DRIVER_DESC,
   463		.date	= DRIVER_DATE,
   464		.major	= DRIVER_MAJOR,
   465		.minor	= DRIVER_MINOR,
   466	};
   467	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Emil Velikov Oct. 29, 2018, noon UTC | #4 
On Fri, 26 Oct 2018 at 15:40, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Daniel Vetter (2018-10-26 14:40:36)
> > On Fri, Oct 26, 2018 at 01:06:47PM +0100, Emil Velikov wrote:
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > >
> > > VGEM doesn't do anything modeset specific, so in a way exposing a
> > > primary node is 'wrong'. At the same time, we extensively use if for
> > > creating dumb buffers, fences, prime fd <> handle imports/exports.
>
> What problem are you solving?
Expose a render node, since only the first instance using the card
node gets auth.

> Now if you suggest a way to only expose a
> render node and not a master node, that might be more interesting.

A couple of years ago, I've proposed a way to have render-only
drivers, yet it got shot down. Don't recall was it Daniel V, Dave or
someone else.
If you fancy going that route, be my guest.

Keep in mind that is orthogonal to the goal here.

Thanks
Emil
Emil Velikov Oct. 29, 2018, 12:03 p.m. UTC | #5 
On Fri, 26 Oct 2018 at 14:40, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Oct 26, 2018 at 01:06:47PM +0100, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > VGEM doesn't do anything modeset specific, so in a way exposing a
> > primary node is 'wrong'. At the same time, we extensively use if for
> > creating dumb buffers, fences, prime fd <> handle imports/exports.
> >
> > To the point that we explicitly annotate the vgem fence ioctls as
> > DRM_RENDER_ALLOW and have an IGT test which opens the render node.
> >
> > close(drm_open_driver_render(DRIVER_VGEM))
>
> Huh, I guess that test doesn't pass?
>
It does since the IGT code seems bonkers... Namely it silently falls
back to the primary/card node.
It seems to be for historical reasons... back in the days where render
nodes were still experimental and had to be explicitly enabled via
kernel arg.

I'll try and follow-up to address that.

> > Better late than never, let's flip the switch.
> >
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks. I'll give it a few more days for others to review/comment.

-Emil
Chris Wilson Oct. 29, 2018, 12:08 p.m. UTC | #6 
Quoting Emil Velikov (2018-10-29 12:00:17)
> On Fri, 26 Oct 2018 at 15:40, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Quoting Daniel Vetter (2018-10-26 14:40:36)
> > > On Fri, Oct 26, 2018 at 01:06:47PM +0100, Emil Velikov wrote:
> > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > >
> > > > VGEM doesn't do anything modeset specific, so in a way exposing a
> > > > primary node is 'wrong'. At the same time, we extensively use if for
> > > > creating dumb buffers, fences, prime fd <> handle imports/exports.
> >
> > What problem are you solving?
> Expose a render node, since only the first instance using the card
> node gets auth.

More accurately, the fence signal/attach do not need DRM_AUTH since they
are operating on resources private to the fd.
-Chris
Daniel Vetter Oct. 29, 2018, 1:51 p.m. UTC | #7 
On Mon, Oct 29, 2018 at 12:03:46PM +0000, Emil Velikov wrote:
> On Fri, 26 Oct 2018 at 14:40, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Fri, Oct 26, 2018 at 01:06:47PM +0100, Emil Velikov wrote:
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > >
> > > VGEM doesn't do anything modeset specific, so in a way exposing a
> > > primary node is 'wrong'. At the same time, we extensively use if for
> > > creating dumb buffers, fences, prime fd <> handle imports/exports.
> > >
> > > To the point that we explicitly annotate the vgem fence ioctls as
> > > DRM_RENDER_ALLOW and have an IGT test which opens the render node.
> > >
> > > close(drm_open_driver_render(DRIVER_VGEM))
> >
> > Huh, I guess that test doesn't pass?
> >
> It does since the IGT code seems bonkers... Namely it silently falls
> back to the primary/card node.
> It seems to be for historical reasons... back in the days where render
> nodes were still experimental and had to be explicitly enabled via
> kernel arg.
> 
> I'll try and follow-up to address that.

I guess enforcing this with igt is a bit harder, since not much with
render nodes is generic across drivers. Hence a lot of the tests are very
specific.

But anything we can increase the odds for people using render nodes is
good I guess, and if it's just using vgem as a driver copypaste template.
-Daniel

> 
> > > Better late than never, let's flip the switch.
> > >
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> >
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Thanks. I'll give it a few more days for others to review/comment.
> 
> -Emil
Chris Wilson Nov. 5, 2018, 3:30 p.m. UTC | #8 
Quoting kbuild test robot (2018-10-26 16:55:25)
> Hi Emil,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on sof-driver-fuweitax/master]
> [also build test ERROR on v4.19 next-20181019]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Emil-Velikov/drm-vgem-create-a-render-node-for-vgem/20181026-233734
> base:   https://github.com/fuweitax/linux master
> config: i386-randconfig-x077-201842 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
> >> drivers/gpu/drm/vgem/vgem_drv.c:436:21: error: expected '}' before ';' token
>            DRIVER_RENDER;

The build is broken, our CI is offline:

commit 3a6eb795641c0e140424a3e4f301188eb2fd6d63
Author:     Emil Velikov <emil.velikov@collabora.com>
AuthorDate: Fri Oct 26 13:06:47 2018 +0100
Commit:     Emil Velikov <emil.l.velikov@gmail.com>
CommitDate: Mon Nov 5 13:06:30 2018 +0000

    drm/vgem: create a render node for vgem

-Chris
Emil Velikov Nov. 5, 2018, 3:35 p.m. UTC | #9 
On Mon, 5 Nov 2018 at 15:30, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting kbuild test robot (2018-10-26 16:55:25)
> > Hi Emil,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on sof-driver-fuweitax/master]
> > [also build test ERROR on v4.19 next-20181019]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> > url:    https://github.com/0day-ci/linux/commits/Emil-Velikov/drm-vgem-create-a-render-node-for-vgem/20181026-233734
> > base:   https://github.com/fuweitax/linux master
> > config: i386-randconfig-x077-201842 (attached as .config)
> > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> > reproduce:
> >         # save the attached .config to linux build tree
> >         make ARCH=i386
> >
> > All errors (new ones prefixed by >>):
> >
> > >> drivers/gpu/drm/vgem/vgem_drv.c:436:21: error: expected '}' before ';' token
> >            DRIVER_RENDER;
>
> The build is broken, our CI is offline:
>
It should be back up, props to Imre.

-Emil

/me looks for brown paper bag
Daniel Vetter Nov. 6, 2018, 9:05 a.m. UTC | #10 
On Mon, Nov 05, 2018 at 03:35:54PM +0000, Emil Velikov wrote:
> On Mon, 5 Nov 2018 at 15:30, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Quoting kbuild test robot (2018-10-26 16:55:25)
> > > Hi Emil,
> > >
> > > Thank you for the patch! Yet something to improve:
> > >
> > > [auto build test ERROR on sof-driver-fuweitax/master]
> > > [also build test ERROR on v4.19 next-20181019]
> > > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > >
> > > url:    https://github.com/0day-ci/linux/commits/Emil-Velikov/drm-vgem-create-a-render-node-for-vgem/20181026-233734
> > > base:   https://github.com/fuweitax/linux master
> > > config: i386-randconfig-x077-201842 (attached as .config)
> > > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> > > reproduce:
> > >         # save the attached .config to linux build tree
> > >         make ARCH=i386
> > >
> > > All errors (new ones prefixed by >>):
> > >
> > > >> drivers/gpu/drm/vgem/vgem_drv.c:436:21: error: expected '}' before ';' token
> > >            DRIVER_RENDER;
> >
> > The build is broken, our CI is offline:
> >
> It should be back up, props to Imre.
> 
> -Emil
> 
> /me looks for brown paper bag

When pushing to drm-misc you're supposed to build-test the 3 defconfigs in
drm-rerere.
-Daniel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index f1f7ab9dcdbf..f1d1d9e2c82e 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -431,7 +431,8 @@  static void vgem_release(struct drm_device *dev)
 }
 
 static struct drm_driver vgem_driver = {
-	.driver_features		= DRIVER_GEM | DRIVER_PRIME,
+	.driver_features		= DRIVER_GEM | DRIVER_PRIME |
+					  DRIVER_RENDER;
 	.release			= vgem_release,
 	.open				= vgem_open,
 	.postclose			= vgem_postclose,