Re: [PATCH 1/3] drm/prime: forward begin_cpu_access callback to drivers

[Daniel Vetter]

On Wed, Dec 20, 2017 at 08:54:33PM +0100, Christian König wrote:
> Am 20.12.2017 um 20:43 schrieb Daniel Vetter:
> > On Wed, Dec 20, 2017 at 6:20 PM, Li, Samuel  wrote:
> > > Ping... can someone please review this patch?
> > Might be simpler to implement your own dma-buf backend instead of
> > going through the drm_prime midlayer. That was mostly written to give
> > nvidia a set of non-EXPORT_GPL symbols to support dma-buf. Or
> > something like that.
> 
> Ah, that explains that. Well the alternative Sam suggest was to export most
> of the functions implementing that.

Yeah that sounds like a good idea, makes it feel less midlayer-y.

> But then we have a hard time having a common detection logic if a DMA-buf
> has a GEM object behind it or not.

Hm, why do you need that? I guess you'll care whether it's an
amdgpu-exported one, and you can still do that by checking against your
own dma_buf_ops table. We do the same in i915, without using the core
drm_prime helpers.

> > Also don't expect people to look at patches when CI bots spot issues.
> 
> That is either a false positive or a one liner.

Hey it's holiday seasons, leave me some cheap excuses pls :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/3] drm/prime: forward begin_cpu_access callback to drivers

[Li, Samuel]

Ping... can someone please review this patch?

Samuel Li



> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Samuel Li
> Sent: Friday, December 15, 2017 11:28 AM
> To: dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org
> Cc: Koenig, Christian 
> Subject: [PATCH 1/3] drm/prime: forward begin_cpu_access callback to
> drivers
> 
> From: Christian König 
> 
> Allow drivers to implement their own begin_cpu_access callback.
> 
> Change-Id: I97709b42b9351a04ee7e01106107a87bc56ea258
> Signed-off-by: Christian König 
> ---
>  drivers/gpu/drm/drm_prime.c | 13 +
>  include/drm/drm_drv.h   |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 8de93a2..b4b0e64 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -346,6 +346,18 @@ void drm_gem_dmabuf_release(struct dma_buf
> *dma_buf)  }  EXPORT_SYMBOL(drm_gem_dmabuf_release);
> 
> +static int drm_gem_dmabuf_begin_cpu_access(struct dma_buf *dma_buf,
> + enum dma_data_direction
> direction) {
> + struct drm_gem_object *obj = dma_buf->priv;
> + struct drm_device *dev = obj->dev;
> +
> + if (!dev->driver->gem_prime_begin_cpu_access)
> + return 0;
> +
> + return dev->driver->gem_prime_begin_cpu_access(obj, direction); }
> +
>  static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)  {
>   struct drm_gem_object *obj = dma_buf->priv; @@ -403,6 +415,7
> @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>   .map_dma_buf = drm_gem_map_dma_buf,
>   .unmap_dma_buf = drm_gem_unmap_dma_buf,
>   .release = drm_gem_dmabuf_release,
> + .begin_cpu_access = drm_gem_dmabuf_begin_cpu_access,
>   .map = drm_gem_dmabuf_kmap,
>   .map_atomic = drm_gem_dmabuf_kmap_atomic,
>   .unmap = drm_gem_dmabuf_kunmap,
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index
> 412e83a..1fbf298 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -475,6 +475,8 @@ struct drm_driver {
>   struct drm_device *dev,
>   struct dma_buf_attachment *attach,
>   struct sg_table *sgt);
> + int (*gem_prime_begin_cpu_access)(struct drm_gem_object *obj,
> +enum dma_data_direction
> direction);
>   void *(*gem_prime_vmap)(struct drm_gem_object *obj);
>   void (*gem_prime_vunmap)(struct drm_gem_object *obj, void
> *vaddr);
>   int (*gem_prime_mmap)(struct drm_gem_object *obj,
> --
> 2.7.4
> 
> ___
> amd-gfx mailing list
> amd-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/prime: forward begin_cpu_access callback to drivers

[kbuild test robot]

Hi Christian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.15-rc3 next-20171215]
[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/Samuel-Li/drm-prime-forward-begin_cpu_access-callback-to-drivers/20171216-125056
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: i386-randconfig-a0-201750 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/drm_drv.c:36:0:
>> include/drm/drm_drv.h:494:14: warning: 'enum dma_data_direction' declared 
>> inside parameter list
enum dma_data_direction direction);
 ^
>> include/drm/drm_drv.h:494:14: warning: its scope is only this definition or 
>> declaration, which is probably not what you want

vim +494 include/drm/drm_drv.h

59  
60  /**
61   * struct drm_driver - DRM driver structure
62   *
63   * This structure represent the common code for a family of cards. 
There will
64   * one drm_device for each card present in this family. It contains 
lots of
65   * vfunc entries, and a pile of those probably should be moved to more
66   * appropriate places like _mode_config_funcs or into a new 
operations
67   * structure for GEM drivers.
68   */
69  struct drm_driver {
70  /**
71   * @load:
72   *
73   * Backward-compatible driver callback to complete
74   * initialization steps after the driver is registered.  For
75   * this reason, may suffer from race conditions and its use is
76   * deprecated for new drivers.  It is therefore only supported
77   * for existing drivers not yet converted to the new scheme.
78   * See drm_dev_init() and drm_dev_register() for proper and
79   * race-free way to set up a  drm_device.
80   *
81   * This is deprecated, do not use!
82   *
83   * Returns:
84   *
85   * Zero on success, non-zero value on failure.
86   */
87  int (*load) (struct drm_device *, unsigned long flags);
88  
89  /**
90   * @open:
91   *
92   * Driver callback when a new  drm_file is opened. 
Useful for
93   * setting up driver-private data structures like buffer 
allocators,
94   * execution contexts or similar things. Such driver-private 
resources
95   * must be released again in @postclose.
96   *
97   * Since the display/modeset side of DRM can only be owned by 
exactly
98   * one  drm_file (see _file.is_master and 
_device.master)
99   * there should never be a need to set up any modeset related 
resources
   100   * in this callback. Doing so would be a driver design bug.
   101   *
   102   * Returns:
   103   *
   104   * 0 on success, a negative error code on failure, which will be
   105   * promoted to userspace as the result of the open() system 
call.
   106   */
   107  int (*open) (struct drm_device *, struct drm_file *);
   108  
   109  /**
   110   * @postclose:
   111   *
   112   * One of the driver callbacks when a new  drm_file is 
closed.
   113   * Useful for tearing down driver-private data structures 
allocated in
   114   * @open like buffer allocators, execution contexts or similar 
things.
   115   *
   116   * Since the display/modeset side of DRM can only be owned by 
exactly
   117   * one  drm_file (see _file.is_master and 
_device.master)
   118   * there should never be a need to tear down any modeset related
   119   * resources in this callback. Doing so would be a driver 
design bug.
   120   */
   121  void (*postclose) (struct drm_device *, struct drm_file *);
   122  
   123  /**
   124   * @lastclose:
   125   *
   126   * Called when the last  drm_file has been closed and 
there's
   127   * currently no userspace client for the  drm_device.
   128   *
   129   * Modern drivers should only use this to force-restore the 
fbdev
   130   * framebuffer using 
drm_fb_helper_restore_fbdev_mode_unlocked().
   131   * Anything else would indicate there's something seriously 
wrong.
   132   * Modern drivers can also use this to execute delayed power 
switching
   133   * state changes, e.g. in conjunction with the 
:ref:`vga_switcheroo`
   134   * infrastructure.
   135   *
   

Re: [PATCH 1/3] drm/prime: forward begin_cpu_access callback to drivers

[kbuild test robot]

Hi Christian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.15-rc3 next-20171215]
[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/Samuel-Li/drm-prime-forward-begin_cpu_access-callback-to-drivers/20171216-125056
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: i386-randconfig-x016-201750 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/drm_drv.c:36:0:
>> include/drm/drm_drv.h:494:14: warning: 'enum dma_data_direction' declared 
>> inside parameter list will not be visible outside of this definition or 
>> declaration
enum dma_data_direction direction);
 ^~

vim +494 include/drm/drm_drv.h

59  
60  /**
61   * struct drm_driver - DRM driver structure
62   *
63   * This structure represent the common code for a family of cards. 
There will
64   * one drm_device for each card present in this family. It contains 
lots of
65   * vfunc entries, and a pile of those probably should be moved to more
66   * appropriate places like _mode_config_funcs or into a new 
operations
67   * structure for GEM drivers.
68   */
69  struct drm_driver {
70  /**
71   * @load:
72   *
73   * Backward-compatible driver callback to complete
74   * initialization steps after the driver is registered.  For
75   * this reason, may suffer from race conditions and its use is
76   * deprecated for new drivers.  It is therefore only supported
77   * for existing drivers not yet converted to the new scheme.
78   * See drm_dev_init() and drm_dev_register() for proper and
79   * race-free way to set up a  drm_device.
80   *
81   * This is deprecated, do not use!
82   *
83   * Returns:
84   *
85   * Zero on success, non-zero value on failure.
86   */
87  int (*load) (struct drm_device *, unsigned long flags);
88  
89  /**
90   * @open:
91   *
92   * Driver callback when a new  drm_file is opened. 
Useful for
93   * setting up driver-private data structures like buffer 
allocators,
94   * execution contexts or similar things. Such driver-private 
resources
95   * must be released again in @postclose.
96   *
97   * Since the display/modeset side of DRM can only be owned by 
exactly
98   * one  drm_file (see _file.is_master and 
_device.master)
99   * there should never be a need to set up any modeset related 
resources
   100   * in this callback. Doing so would be a driver design bug.
   101   *
   102   * Returns:
   103   *
   104   * 0 on success, a negative error code on failure, which will be
   105   * promoted to userspace as the result of the open() system 
call.
   106   */
   107  int (*open) (struct drm_device *, struct drm_file *);
   108  
   109  /**
   110   * @postclose:
   111   *
   112   * One of the driver callbacks when a new  drm_file is 
closed.
   113   * Useful for tearing down driver-private data structures 
allocated in
   114   * @open like buffer allocators, execution contexts or similar 
things.
   115   *
   116   * Since the display/modeset side of DRM can only be owned by 
exactly
   117   * one  drm_file (see _file.is_master and 
_device.master)
   118   * there should never be a need to tear down any modeset related
   119   * resources in this callback. Doing so would be a driver 
design bug.
   120   */
   121  void (*postclose) (struct drm_device *, struct drm_file *);
   122  
   123  /**
   124   * @lastclose:
   125   *
   126   * Called when the last  drm_file has been closed and 
there's
   127   * currently no userspace client for the  drm_device.
   128   *
   129   * Modern drivers should only use this to force-restore the 
fbdev
   130   * framebuffer using 
drm_fb_helper_restore_fbdev_mode_unlocked().
   131   * Anything else would indicate there's something seriously 
wrong.
   132   * Modern drivers can also use this to execute delayed power 
switching
   133   * state changes, e.g. in conjunction with the 
:ref:`vga_switcheroo`
   134   * infrastructure.
   135   *
   136   * This is called after