[PATCH 0/2] drm/fb-helper: Documentation update for defio

[PATCH 0/2] drm/fb-helper: Documentation update for defio

[Thomas Zimmermann]

The current documentation wrt. defio support needs an update. There
are no more users of drm_fb_helper_defio_init(), so we can remove this
interface.

Thomas Zimmermann (2):
  drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs
  drm/todo: Clarify situation around fbdev and defio

 Documentation/gpu/todo.rst      |  8 ++---
 drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
 include/drm/drm_fb_helper.h     |  1 -
 3 files changed, 17 insertions(+), 53 deletions(-)

--
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs

[Thomas Zimmermann]

There are no users of drm_fb_helper_defio_init(), so we can remove
it. The documenation around defio support is a bit misleading and
should mention compatibility issues with SHMEM helpers. Clarify this.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
 include/drm/drm_fb_helper.h     |  1 -
 2 files changed, 13 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index b75ae8555baf..8ebeccdeed23 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
  *
  * Drivers that support a dumb buffer with a virtual address and mmap support,
  * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
+ * It will automatically set up deferred I/O if the driver requires a shadow
+ * buffer.
  *
- * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
- * down by calling drm_fb_helper_fbdev_teardown().
+ * For other drivers, setup fbdev emulation by calling
+ * drm_fb_helper_fbdev_setup() and tear it down by calling
+ * drm_fb_helper_fbdev_teardown().
  *
  * At runtime drivers should restore the fbdev console by using
  * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
@@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
  * always run in process context since the fb_*() function could be running in
  * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
  * callback it will also schedule dirty_work with the damage collected from the
- * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup
- * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
+ * mmap page writes.
+ *
+ * Deferred I/O is not compatible with SHMEM. Such drivers should request an
+ * fbdev shadow buffer and call drm_fbdev_generic_setup() instead.
  */
 
 static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
@@ -679,49 +684,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
 }
 EXPORT_SYMBOL(drm_fb_helper_deferred_io);
 
-/**
- * drm_fb_helper_defio_init - fbdev deferred I/O initialization
- * @fb_helper: driver-allocated fbdev helper
- *
- * This function allocates &fb_deferred_io, sets callback to
- * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
- * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
- * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
- *
- * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
- * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
- * affect other instances of that &fb_ops.
- *
- * Returns:
- * 0 on success or a negative error code on failure.
- */
-int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
-{
-	struct fb_info *info = fb_helper->fbdev;
-	struct fb_deferred_io *fbdefio;
-	struct fb_ops *fbops;
-
-	fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
-	fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
-	if (!fbdefio || !fbops) {
-		kfree(fbdefio);
-		kfree(fbops);
-		return -ENOMEM;
-	}
-
-	info->fbdefio = fbdefio;
-	fbdefio->delay = msecs_to_jiffies(50);
-	fbdefio->deferred_io = drm_fb_helper_deferred_io;
-
-	*fbops = *info->fbops;
-	info->fbops = fbops;
-
-	fb_deferred_io_init(info);
-
-	return 0;
-}
-EXPORT_SYMBOL(drm_fb_helper_defio_init);
-
 /**
  * drm_fb_helper_sys_read - wrapper around fb_sys_read
  * @info: fb_info struct pointer
@@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
  *
  * Drivers that set the dirty callback on their framebuffer will get a shadow
  * fbdev buffer that is blitted onto the real buffer. This is done in order to
- * make deferred I/O work with all kinds of buffers.
+ * make deferred I/O work with all kinds of buffers. A shadow buffer can be
+ * requested explicitly by setting struct drm_mode_config.prefer_shadow or
+ * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
+ * required to use generic fbdev emulation with SHMEM helpers.
  *
  * This function is safe to call even when there are no connectors present.
  * Setup will be retried on the next hotplug event.
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 8dcc012ccbc8..2338e9f94a03 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -235,7 +235,6 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
 
 void drm_fb_helper_deferred_io(struct fb_info *info,
 			       struct list_head *pagelist);
-int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
 
 ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
 			       size_t count, loff_t *ppos);
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/todo: Clarify situation around fbdev and defio

[Thomas Zimmermann]

The TODO item is misleading and makes it seem that fbdev emulation
cannot be used with SHMEM. Rephrase the text to describe the current
situation more correctly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 Documentation/gpu/todo.rst | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 73c51b5a0997..6792fa9b6b6b 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -206,10 +206,10 @@ Generic fbdev defio support
 ---------------------------
 
 The defio support code in the fbdev core has some very specific requirements,
-which means drivers need to have a special framebuffer for fbdev. Which prevents
-us from using the generic fbdev emulation code everywhere. The main issue is
-that it uses some fields in struct page itself, which breaks shmem gem objects
-(and other things).
+which means drivers need to have a special framebuffer for fbdev. The main
+issue is that it uses some fields in struct page itself, which breaks shmem
+gem objects (and other things). To support defio, affected drivers require
+the use of a shadow buffer, which may add CPU and memory overhead.
 
 Possible solution would be to write our own defio mmap code in the drm fbdev
 emulation. It would need to fully wrap the existing mmap ops, forwarding
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs

[Noralf Trønnes]

Den 25.10.2019 11.27, skrev Thomas Zimmermann:
> There are no users of drm_fb_helper_defio_init(), so we can remove
> it. The documenation around defio support is a bit misleading and
> should mention compatibility issues with SHMEM helpers. Clarify this.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
>  include/drm/drm_fb_helper.h     |  1 -
>  2 files changed, 13 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index b75ae8555baf..8ebeccdeed23 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>   *
>   * Drivers that support a dumb buffer with a virtual address and mmap support,
>   * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
> + * It will automatically set up deferred I/O if the driver requires a shadow
> + * buffer.
>   *
> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
> - * down by calling drm_fb_helper_fbdev_teardown().
> + * For other drivers, setup fbdev emulation by calling
> + * drm_fb_helper_fbdev_setup() and tear it down by calling
> + * drm_fb_helper_fbdev_teardown().
>   *
>   * At runtime drivers should restore the fbdev console by using
>   * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>   * always run in process context since the fb_*() function could be running in
>   * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
>   * callback it will also schedule dirty_work with the damage collected from the
> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup
> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
> + * mmap page writes.
> + *
> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an
> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead.
>   */
>  
>  static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
> @@ -679,49 +684,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
>  }
>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>  
> -/**
> - * drm_fb_helper_defio_init - fbdev deferred I/O initialization
> - * @fb_helper: driver-allocated fbdev helper
> - *
> - * This function allocates &fb_deferred_io, sets callback to
> - * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
> - * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
> - *
> - * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
> - * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
> - * affect other instances of that &fb_ops.
> - *
> - * Returns:
> - * 0 on success or a negative error code on failure.
> - */
> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> -{
> -	struct fb_info *info = fb_helper->fbdev;
> -	struct fb_deferred_io *fbdefio;
> -	struct fb_ops *fbops;
> -
> -	fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> -	fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> -	if (!fbdefio || !fbops) {
> -		kfree(fbdefio);
> -		kfree(fbops);
> -		return -ENOMEM;
> -	}
> -
> -	info->fbdefio = fbdefio;
> -	fbdefio->delay = msecs_to_jiffies(50);
> -	fbdefio->deferred_io = drm_fb_helper_deferred_io;
> -
> -	*fbops = *info->fbops;
> -	info->fbops = fbops;
> -
> -	fb_deferred_io_init(info);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(drm_fb_helper_defio_init);
> -

With this gone you can remove the defio cleanup part from
drm_fb_helper_fbdev_teardown() as well.

And I see that there's no users left of that function (the same with
*_seup()). Would be nice if you just removed them. I made them before I
embarked on the generic fbdev solution. I think it's better to try and
make the generic emulation usable for "everyone" and avoid the need for
drivers to have to do their own special stuff. Ofc speeding up the defio
code to avoid the shadow buffering would aid in that as Daniel have
talked about.

Either way you choose, this patch is:

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

>  /**
>   * drm_fb_helper_sys_read - wrapper around fb_sys_read
>   * @info: fb_info struct pointer
> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>   *
>   * Drivers that set the dirty callback on their framebuffer will get a shadow
>   * fbdev buffer that is blitted onto the real buffer. This is done in order to
> - * make deferred I/O work with all kinds of buffers.
> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be
> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or
> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
> + * required to use generic fbdev emulation with SHMEM helpers.
>   *
>   * This function is safe to call even when there are no connectors present.
>   * Setup will be retried on the next hotplug event.
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 8dcc012ccbc8..2338e9f94a03 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -235,7 +235,6 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>  
>  void drm_fb_helper_deferred_io(struct fb_info *info,
>  			       struct list_head *pagelist);
> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
>  
>  ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
>  			       size_t count, loff_t *ppos);
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/todo: Clarify situation around fbdev and defio

[Noralf Trønnes]

Den 25.10.2019 11.27, skrev Thomas Zimmermann:
> The TODO item is misleading and makes it seem that fbdev emulation
> cannot be used with SHMEM. Rephrase the text to describe the current
> situation more correctly.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  Documentation/gpu/todo.rst | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 73c51b5a0997..6792fa9b6b6b 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -206,10 +206,10 @@ Generic fbdev defio support
>  ---------------------------
>  
>  The defio support code in the fbdev core has some very specific requirements,
> -which means drivers need to have a special framebuffer for fbdev. Which prevents
> -us from using the generic fbdev emulation code everywhere. The main issue is
> -that it uses some fields in struct page itself, which breaks shmem gem objects
> -(and other things).
> +which means drivers need to have a special framebuffer for fbdev. The main
> +issue is that it uses some fields in struct page itself, which breaks shmem
> +gem objects (and other things). To support defio, affected drivers require
> +the use of a shadow buffer, which may add CPU and memory overhead.
>  

Ah yes, there's a todo on this. Look good to me:

Acked-by: Noralf Trønnes <noralf@tronnes.org>

>  Possible solution would be to write our own defio mmap code in the drm fbdev
>  emulation. It would need to fully wrap the existing mmap ops, forwarding
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 7082 bytes --]

Hi

Am 25.10.19 um 17:46 schrieb Noralf Trønnes:
> 
> 
> Den 25.10.2019 11.27, skrev Thomas Zimmermann:
>> There are no users of drm_fb_helper_defio_init(), so we can remove
>> it. The documenation around defio support is a bit misleading and
>> should mention compatibility issues with SHMEM helpers. Clarify this.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
>>  include/drm/drm_fb_helper.h     |  1 -
>>  2 files changed, 13 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index b75ae8555baf..8ebeccdeed23 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>>   *
>>   * Drivers that support a dumb buffer with a virtual address and mmap support,
>>   * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
>> + * It will automatically set up deferred I/O if the driver requires a shadow
>> + * buffer.
>>   *
>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
>> - * down by calling drm_fb_helper_fbdev_teardown().
>> + * For other drivers, setup fbdev emulation by calling
>> + * drm_fb_helper_fbdev_setup() and tear it down by calling
>> + * drm_fb_helper_fbdev_teardown().
>>   *
>>   * At runtime drivers should restore the fbdev console by using
>>   * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>>   * always run in process context since the fb_*() function could be running in
>>   * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
>>   * callback it will also schedule dirty_work with the damage collected from the
>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup
>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
>> + * mmap page writes.
>> + *
>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an
>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead.
>>   */
>>  
>>  static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
>> @@ -679,49 +684,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
>>  }
>>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>>  
>> -/**
>> - * drm_fb_helper_defio_init - fbdev deferred I/O initialization
>> - * @fb_helper: driver-allocated fbdev helper
>> - *
>> - * This function allocates &fb_deferred_io, sets callback to
>> - * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
>> - * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
>> - *
>> - * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
>> - * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
>> - * affect other instances of that &fb_ops.
>> - *
>> - * Returns:
>> - * 0 on success or a negative error code on failure.
>> - */
>> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
>> -{
>> -	struct fb_info *info = fb_helper->fbdev;
>> -	struct fb_deferred_io *fbdefio;
>> -	struct fb_ops *fbops;
>> -
>> -	fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
>> -	fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
>> -	if (!fbdefio || !fbops) {
>> -		kfree(fbdefio);
>> -		kfree(fbops);
>> -		return -ENOMEM;
>> -	}
>> -
>> -	info->fbdefio = fbdefio;
>> -	fbdefio->delay = msecs_to_jiffies(50);
>> -	fbdefio->deferred_io = drm_fb_helper_deferred_io;
>> -
>> -	*fbops = *info->fbops;
>> -	info->fbops = fbops;
>> -
>> -	fb_deferred_io_init(info);
>> -
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL(drm_fb_helper_defio_init);
>> -
> 
> With this gone you can remove the defio cleanup part from
> drm_fb_helper_fbdev_teardown() as well.
> 
> And I see that there's no users left of that function (the same with
> *_seup()). Would be nice if you just removed them. I made them before I
> embarked on the generic fbdev solution. I think it's better to try and
> make the generic emulation usable for "everyone" and avoid the need for
> drivers to have to do their own special stuff. 

The last user was vboxvideo, which I converted ~2 weeks ago. I haven't
removed them yet, as there's a TODO item to convert drivers over to them.

From a quick 'git grep':

Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could
probably be converted over to generic fbdev. That's hibmc (I have some
untested patches for it), msm, omapdrm, tegra, and udl (currently being
converted).

Only nouveau and gma500 have some form of HW acceleration.

The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are
these strictly required, or can they be made available within generic fbdev?

Best regards
Thomas

> code to avoid the shadow buffering would aid in that as Daniel have
> talked about.
> 
> Either way you choose, this patch is:
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> 
>>  /**
>>   * drm_fb_helper_sys_read - wrapper around fb_sys_read
>>   * @info: fb_info struct pointer
>> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>>   *
>>   * Drivers that set the dirty callback on their framebuffer will get a shadow
>>   * fbdev buffer that is blitted onto the real buffer. This is done in order to
>> - * make deferred I/O work with all kinds of buffers.
>> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be
>> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or
>> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
>> + * required to use generic fbdev emulation with SHMEM helpers.
>>   *
>>   * This function is safe to call even when there are no connectors present.
>>   * Setup will be retried on the next hotplug event.
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index 8dcc012ccbc8..2338e9f94a03 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -235,7 +235,6 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>>  
>>  void drm_fb_helper_deferred_io(struct fb_info *info,
>>  			       struct list_head *pagelist);
>> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
>>  
>>  ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
>>  			       size_t count, loff_t *ppos);
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs

[Daniel Vetter]

On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 25.10.19 um 17:46 schrieb Noralf Trønnes:
> >
> >
> > Den 25.10.2019 11.27, skrev Thomas Zimmermann:
> >> There are no users of drm_fb_helper_defio_init(), so we can remove
> >> it. The documenation around defio support is a bit misleading and
> >> should mention compatibility issues with SHMEM helpers. Clarify this.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >>  drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
> >>  include/drm/drm_fb_helper.h     |  1 -
> >>  2 files changed, 13 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >> index b75ae8555baf..8ebeccdeed23 100644
> >> --- a/drivers/gpu/drm/drm_fb_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
> >>   *
> >>   * Drivers that support a dumb buffer with a virtual address and mmap support,
> >>   * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
> >> + * It will automatically set up deferred I/O if the driver requires a shadow
> >> + * buffer.
> >>   *
> >> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
> >> - * down by calling drm_fb_helper_fbdev_teardown().
> >> + * For other drivers, setup fbdev emulation by calling
> >> + * drm_fb_helper_fbdev_setup() and tear it down by calling
> >> + * drm_fb_helper_fbdev_teardown().
> >>   *
> >>   * At runtime drivers should restore the fbdev console by using
> >>   * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
> >> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
> >>   * always run in process context since the fb_*() function could be running in
> >>   * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
> >>   * callback it will also schedule dirty_work with the damage collected from the
> >> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup
> >> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
> >> + * mmap page writes.
> >> + *
> >> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an
> >> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead.
> >>   */
> >>
> >>  static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
> >> @@ -679,49 +684,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
> >>  }
> >>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> >>
> >> -/**
> >> - * drm_fb_helper_defio_init - fbdev deferred I/O initialization
> >> - * @fb_helper: driver-allocated fbdev helper
> >> - *
> >> - * This function allocates &fb_deferred_io, sets callback to
> >> - * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
> >> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
> >> - * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
> >> - *
> >> - * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
> >> - * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
> >> - * affect other instances of that &fb_ops.
> >> - *
> >> - * Returns:
> >> - * 0 on success or a negative error code on failure.
> >> - */
> >> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> >> -{
> >> -    struct fb_info *info = fb_helper->fbdev;
> >> -    struct fb_deferred_io *fbdefio;
> >> -    struct fb_ops *fbops;
> >> -
> >> -    fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> >> -    fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> >> -    if (!fbdefio || !fbops) {
> >> -            kfree(fbdefio);
> >> -            kfree(fbops);
> >> -            return -ENOMEM;
> >> -    }
> >> -
> >> -    info->fbdefio = fbdefio;
> >> -    fbdefio->delay = msecs_to_jiffies(50);
> >> -    fbdefio->deferred_io = drm_fb_helper_deferred_io;
> >> -
> >> -    *fbops = *info->fbops;
> >> -    info->fbops = fbops;
> >> -
> >> -    fb_deferred_io_init(info);
> >> -
> >> -    return 0;
> >> -}
> >> -EXPORT_SYMBOL(drm_fb_helper_defio_init);
> >> -
> >
> > With this gone you can remove the defio cleanup part from
> > drm_fb_helper_fbdev_teardown() as well.
> >
> > And I see that there's no users left of that function (the same with
> > *_seup()). Would be nice if you just removed them. I made them before I
> > embarked on the generic fbdev solution. I think it's better to try and
> > make the generic emulation usable for "everyone" and avoid the need for
> > drivers to have to do their own special stuff.
>
> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't
> removed them yet, as there's a TODO item to convert drivers over to them.
>
> From a quick 'git grep':
>
> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could
> probably be converted over to generic fbdev. That's hibmc (I have some
> untested patches for it), msm, omapdrm, tegra, and udl (currently being
> converted).
>
> Only nouveau and gma500 have some form of HW acceleration.
>
> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are
> these strictly required, or can they be made available within generic fbdev?

Take a pile of digging through a few fb horrors, but this boils down to:

*sys* functions operate on an fb that works like normal system memory.

*cfb* functions operate on void __iomem * instead. Which makes a huge
difference on some architectures, but afaiui neither x86 nor arm care.
On ppc it seems to make a difference sometimes.

So for shmem we should use *sys* functions, for vram *cfb*.

And that leads me to realizing that drm_gem_vram_vmap has the totally
wrong type, it should be void __iomem *. There's this fancy is_iomem
parameter for ttm_kmap_obj_virtual that should help you figure out the
right type, but only nouveau bothers to implement this correctly.

I'm honestly not sure whether we should care.
-Daniel

>
> Best regards
> Thomas
>
> > code to avoid the shadow buffering would aid in that as Daniel have
> > talked about.
> >
> > Either way you choose, this patch is:
> >
> > Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> >
> >>  /**
> >>   * drm_fb_helper_sys_read - wrapper around fb_sys_read
> >>   * @info: fb_info struct pointer
> >> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
> >>   *
> >>   * Drivers that set the dirty callback on their framebuffer will get a shadow
> >>   * fbdev buffer that is blitted onto the real buffer. This is done in order to
> >> - * make deferred I/O work with all kinds of buffers.
> >> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be
> >> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or
> >> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
> >> + * required to use generic fbdev emulation with SHMEM helpers.
> >>   *
> >>   * This function is safe to call even when there are no connectors present.
> >>   * Setup will be retried on the next hotplug event.
> >> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> >> index 8dcc012ccbc8..2338e9f94a03 100644
> >> --- a/include/drm/drm_fb_helper.h
> >> +++ b/include/drm/drm_fb_helper.h
> >> @@ -235,7 +235,6 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
> >>
> >>  void drm_fb_helper_deferred_io(struct fb_info *info,
> >>                             struct list_head *pagelist);
> >> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
> >>
> >>  ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
> >>                             size_t count, loff_t *ppos);
> >>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 9159 bytes --]

Hi

Am 25.10.19 um 20:54 schrieb Daniel Vetter:
> On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 25.10.19 um 17:46 schrieb Noralf Trønnes:
>>>
>>>
>>> Den 25.10.2019 11.27, skrev Thomas Zimmermann:
>>>> There are no users of drm_fb_helper_defio_init(), so we can remove
>>>> it. The documenation around defio support is a bit misleading and
>>>> should mention compatibility issues with SHMEM helpers. Clarify this.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>  drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
>>>>  include/drm/drm_fb_helper.h     |  1 -
>>>>  2 files changed, 13 insertions(+), 49 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>> index b75ae8555baf..8ebeccdeed23 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>>>>   *
>>>>   * Drivers that support a dumb buffer with a virtual address and mmap support,
>>>>   * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
>>>> + * It will automatically set up deferred I/O if the driver requires a shadow
>>>> + * buffer.
>>>>   *
>>>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
>>>> - * down by calling drm_fb_helper_fbdev_teardown().
>>>> + * For other drivers, setup fbdev emulation by calling
>>>> + * drm_fb_helper_fbdev_setup() and tear it down by calling
>>>> + * drm_fb_helper_fbdev_teardown().
>>>>   *
>>>>   * At runtime drivers should restore the fbdev console by using
>>>>   * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
>>>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>>>>   * always run in process context since the fb_*() function could be running in
>>>>   * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
>>>>   * callback it will also schedule dirty_work with the damage collected from the
>>>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup
>>>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
>>>> + * mmap page writes.
>>>> + *
>>>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an
>>>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead.
>>>>   */
>>>>
>>>>  static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
>>>> @@ -679,49 +684,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
>>>>  }
>>>>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>>>>
>>>> -/**
>>>> - * drm_fb_helper_defio_init - fbdev deferred I/O initialization
>>>> - * @fb_helper: driver-allocated fbdev helper
>>>> - *
>>>> - * This function allocates &fb_deferred_io, sets callback to
>>>> - * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
>>>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
>>>> - * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
>>>> - *
>>>> - * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
>>>> - * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
>>>> - * affect other instances of that &fb_ops.
>>>> - *
>>>> - * Returns:
>>>> - * 0 on success or a negative error code on failure.
>>>> - */
>>>> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
>>>> -{
>>>> -    struct fb_info *info = fb_helper->fbdev;
>>>> -    struct fb_deferred_io *fbdefio;
>>>> -    struct fb_ops *fbops;
>>>> -
>>>> -    fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
>>>> -    fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
>>>> -    if (!fbdefio || !fbops) {
>>>> -            kfree(fbdefio);
>>>> -            kfree(fbops);
>>>> -            return -ENOMEM;
>>>> -    }
>>>> -
>>>> -    info->fbdefio = fbdefio;
>>>> -    fbdefio->delay = msecs_to_jiffies(50);
>>>> -    fbdefio->deferred_io = drm_fb_helper_deferred_io;
>>>> -
>>>> -    *fbops = *info->fbops;
>>>> -    info->fbops = fbops;
>>>> -
>>>> -    fb_deferred_io_init(info);
>>>> -
>>>> -    return 0;
>>>> -}
>>>> -EXPORT_SYMBOL(drm_fb_helper_defio_init);
>>>> -
>>>
>>> With this gone you can remove the defio cleanup part from
>>> drm_fb_helper_fbdev_teardown() as well.
>>>
>>> And I see that there's no users left of that function (the same with
>>> *_seup()). Would be nice if you just removed them. I made them before I
>>> embarked on the generic fbdev solution. I think it's better to try and
>>> make the generic emulation usable for "everyone" and avoid the need for
>>> drivers to have to do their own special stuff.
>>
>> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't
>> removed them yet, as there's a TODO item to convert drivers over to them.
>>
>> From a quick 'git grep':
>>
>> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could
>> probably be converted over to generic fbdev. That's hibmc (I have some
>> untested patches for it), msm, omapdrm, tegra, and udl (currently being
>> converted).
>>
>> Only nouveau and gma500 have some form of HW acceleration.
>>
>> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are
>> these strictly required, or can they be made available within generic fbdev?
> 
> Take a pile of digging through a few fb horrors, but this boils down to:
> 
> *sys* functions operate on an fb that works like normal system memory.
> 
> *cfb* functions operate on void __iomem * instead. Which makes a huge
> difference on some architectures, but afaiui neither x86 nor arm care.
> On ppc it seems to make a difference sometimes.
> 
> So for shmem we should use *sys* functions, for vram *cfb*.
> 
> And that leads me to realizing that drm_gem_vram_vmap has the totally
> wrong type, it should be void __iomem *. There's this fancy is_iomem
> parameter for ttm_kmap_obj_virtual that should help you figure out the
> right type, but only nouveau bothers to implement this correctly.
> 
> I'm honestly not sure whether we should care.

I remember getting an eamil about this from some automated test system.
I haven't had time to change it and it's apparently not urgent.

If we really want to fix the problem, we'd have to propagage is_iomem
through the DRM core; probably touching every vmap callback. OTOH this
might not be a bad thing. With is_iomem available in the generic fbdev
emulation, it could select between sys and cfb functions and support
drivers with cfb-based fbdev as well.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>> code to avoid the shadow buffering would aid in that as Daniel have
>>> talked about.
>>>
>>> Either way you choose, this patch is:
>>>
>>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
>>>
>>>>  /**
>>>>   * drm_fb_helper_sys_read - wrapper around fb_sys_read
>>>>   * @info: fb_info struct pointer
>>>> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>>>>   *
>>>>   * Drivers that set the dirty callback on their framebuffer will get a shadow
>>>>   * fbdev buffer that is blitted onto the real buffer. This is done in order to
>>>> - * make deferred I/O work with all kinds of buffers.
>>>> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be
>>>> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or
>>>> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
>>>> + * required to use generic fbdev emulation with SHMEM helpers.
>>>>   *
>>>>   * This function is safe to call even when there are no connectors present.
>>>>   * Setup will be retried on the next hotplug event.
>>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>>>> index 8dcc012ccbc8..2338e9f94a03 100644
>>>> --- a/include/drm/drm_fb_helper.h
>>>> +++ b/include/drm/drm_fb_helper.h
>>>> @@ -235,7 +235,6 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>>>>
>>>>  void drm_fb_helper_deferred_io(struct fb_info *info,
>>>>                             struct list_head *pagelist);
>>>> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
>>>>
>>>>  ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
>>>>                             size_t count, loff_t *ppos);
>>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs

[Daniel Vetter]

On Mon, Oct 28, 2019 at 09:13:47AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 25.10.19 um 20:54 schrieb Daniel Vetter:
> > On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> Hi
> >>
> >> Am 25.10.19 um 17:46 schrieb Noralf Trønnes:
> >>>
> >>>
> >>> Den 25.10.2019 11.27, skrev Thomas Zimmermann:
> >>>> There are no users of drm_fb_helper_defio_init(), so we can remove
> >>>> it. The documenation around defio support is a bit misleading and
> >>>> should mention compatibility issues with SHMEM helpers. Clarify this.
> >>>>
> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>> ---
> >>>>  drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
> >>>>  include/drm/drm_fb_helper.h     |  1 -
> >>>>  2 files changed, 13 insertions(+), 49 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>>> index b75ae8555baf..8ebeccdeed23 100644
> >>>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
> >>>>   *
> >>>>   * Drivers that support a dumb buffer with a virtual address and mmap support,
> >>>>   * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
> >>>> + * It will automatically set up deferred I/O if the driver requires a shadow
> >>>> + * buffer.
> >>>>   *
> >>>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
> >>>> - * down by calling drm_fb_helper_fbdev_teardown().
> >>>> + * For other drivers, setup fbdev emulation by calling
> >>>> + * drm_fb_helper_fbdev_setup() and tear it down by calling
> >>>> + * drm_fb_helper_fbdev_teardown().
> >>>>   *
> >>>>   * At runtime drivers should restore the fbdev console by using
> >>>>   * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
> >>>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
> >>>>   * always run in process context since the fb_*() function could be running in
> >>>>   * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
> >>>>   * callback it will also schedule dirty_work with the damage collected from the
> >>>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup
> >>>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
> >>>> + * mmap page writes.
> >>>> + *
> >>>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an
> >>>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead.
> >>>>   */
> >>>>
> >>>>  static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
> >>>> @@ -679,49 +684,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
> >>>>  }
> >>>>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> >>>>
> >>>> -/**
> >>>> - * drm_fb_helper_defio_init - fbdev deferred I/O initialization
> >>>> - * @fb_helper: driver-allocated fbdev helper
> >>>> - *
> >>>> - * This function allocates &fb_deferred_io, sets callback to
> >>>> - * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
> >>>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
> >>>> - * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
> >>>> - *
> >>>> - * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
> >>>> - * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
> >>>> - * affect other instances of that &fb_ops.
> >>>> - *
> >>>> - * Returns:
> >>>> - * 0 on success or a negative error code on failure.
> >>>> - */
> >>>> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> >>>> -{
> >>>> -    struct fb_info *info = fb_helper->fbdev;
> >>>> -    struct fb_deferred_io *fbdefio;
> >>>> -    struct fb_ops *fbops;
> >>>> -
> >>>> -    fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> >>>> -    fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> >>>> -    if (!fbdefio || !fbops) {
> >>>> -            kfree(fbdefio);
> >>>> -            kfree(fbops);
> >>>> -            return -ENOMEM;
> >>>> -    }
> >>>> -
> >>>> -    info->fbdefio = fbdefio;
> >>>> -    fbdefio->delay = msecs_to_jiffies(50);
> >>>> -    fbdefio->deferred_io = drm_fb_helper_deferred_io;
> >>>> -
> >>>> -    *fbops = *info->fbops;
> >>>> -    info->fbops = fbops;
> >>>> -
> >>>> -    fb_deferred_io_init(info);
> >>>> -
> >>>> -    return 0;
> >>>> -}
> >>>> -EXPORT_SYMBOL(drm_fb_helper_defio_init);
> >>>> -
> >>>
> >>> With this gone you can remove the defio cleanup part from
> >>> drm_fb_helper_fbdev_teardown() as well.
> >>>
> >>> And I see that there's no users left of that function (the same with
> >>> *_seup()). Would be nice if you just removed them. I made them before I
> >>> embarked on the generic fbdev solution. I think it's better to try and
> >>> make the generic emulation usable for "everyone" and avoid the need for
> >>> drivers to have to do their own special stuff.
> >>
> >> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't
> >> removed them yet, as there's a TODO item to convert drivers over to them.
> >>
> >> From a quick 'git grep':
> >>
> >> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could
> >> probably be converted over to generic fbdev. That's hibmc (I have some
> >> untested patches for it), msm, omapdrm, tegra, and udl (currently being
> >> converted).
> >>
> >> Only nouveau and gma500 have some form of HW acceleration.
> >>
> >> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are
> >> these strictly required, or can they be made available within generic fbdev?
> > 
> > Take a pile of digging through a few fb horrors, but this boils down to:
> > 
> > *sys* functions operate on an fb that works like normal system memory.
> > 
> > *cfb* functions operate on void __iomem * instead. Which makes a huge
> > difference on some architectures, but afaiui neither x86 nor arm care.
> > On ppc it seems to make a difference sometimes.
> > 
> > So for shmem we should use *sys* functions, for vram *cfb*.
> > 
> > And that leads me to realizing that drm_gem_vram_vmap has the totally
> > wrong type, it should be void __iomem *. There's this fancy is_iomem
> > parameter for ttm_kmap_obj_virtual that should help you figure out the
> > right type, but only nouveau bothers to implement this correctly.
> > 
> > I'm honestly not sure whether we should care.
> 
> I remember getting an eamil about this from some automated test system.
> I haven't had time to change it and it's apparently not urgent.
> 
> If we really want to fix the problem, we'd have to propagage is_iomem
> through the DRM core; probably touching every vmap callback. OTOH this
> might not be a bad thing. With is_iomem available in the generic fbdev
> emulation, it could select between sys and cfb functions and support
> drivers with cfb-based fbdev as well.

is_iomem doesn't work, it's hack, since the change is in the function
prototypes/signatures. So we'd need to make sure we have 2 sets of kernel
mapping functions for everything, plus 2 sets of anything that accesses
through the kernel.

It's a huge task, and I'm really not sure we have any architecture we care
about this ...
-Daniel

> 
> Best regards
> Thomas
> 
> > -Daniel
> > 
> >>
> >> Best regards
> >> Thomas
> >>
> >>> code to avoid the shadow buffering would aid in that as Daniel have
> >>> talked about.
> >>>
> >>> Either way you choose, this patch is:
> >>>
> >>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> >>>
> >>>>  /**
> >>>>   * drm_fb_helper_sys_read - wrapper around fb_sys_read
> >>>>   * @info: fb_info struct pointer
> >>>> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
> >>>>   *
> >>>>   * Drivers that set the dirty callback on their framebuffer will get a shadow
> >>>>   * fbdev buffer that is blitted onto the real buffer. This is done in order to
> >>>> - * make deferred I/O work with all kinds of buffers.
> >>>> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be
> >>>> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or
> >>>> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
> >>>> + * required to use generic fbdev emulation with SHMEM helpers.
> >>>>   *
> >>>>   * This function is safe to call even when there are no connectors present.
> >>>>   * Setup will be retried on the next hotplug event.
> >>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> >>>> index 8dcc012ccbc8..2338e9f94a03 100644
> >>>> --- a/include/drm/drm_fb_helper.h
> >>>> +++ b/include/drm/drm_fb_helper.h
> >>>> @@ -235,7 +235,6 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
> >>>>
> >>>>  void drm_fb_helper_deferred_io(struct fb_info *info,
> >>>>                             struct list_head *pagelist);
> >>>> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
> >>>>
> >>>>  ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
> >>>>                             size_t count, loff_t *ppos);
> >>>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> >>
> >> --
> >> Thomas Zimmermann
> >> Graphics Driver Developer
> >> SUSE Software Solutions Germany GmbH
> >> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> (HRB 36809, AG Nürnberg)
> >> Geschäftsführer: Felix Imendörffer
> >>
> > 
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
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/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 11340 bytes --]

Hi Daniel

Am 04.11.19 um 10:55 schrieb Daniel Vetter:
> On Mon, Oct 28, 2019 at 09:13:47AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 25.10.19 um 20:54 schrieb Daniel Vetter:
>>> On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>
>>>> Hi
>>>>
>>>> Am 25.10.19 um 17:46 schrieb Noralf Trønnes:
>>>>>
>>>>>
>>>>> Den 25.10.2019 11.27, skrev Thomas Zimmermann:
>>>>>> There are no users of drm_fb_helper_defio_init(), so we can remove
>>>>>> it. The documenation around defio support is a bit misleading and
>>>>>> should mention compatibility issues with SHMEM helpers. Clarify this.
>>>>>>
>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> ---
>>>>>>  drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
>>>>>>  include/drm/drm_fb_helper.h     |  1 -
>>>>>>  2 files changed, 13 insertions(+), 49 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>>>> index b75ae8555baf..8ebeccdeed23 100644
>>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>>>>>>   *
>>>>>>   * Drivers that support a dumb buffer with a virtual address and mmap support,
>>>>>>   * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
>>>>>> + * It will automatically set up deferred I/O if the driver requires a shadow
>>>>>> + * buffer.
>>>>>>   *
>>>>>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
>>>>>> - * down by calling drm_fb_helper_fbdev_teardown().
>>>>>> + * For other drivers, setup fbdev emulation by calling
>>>>>> + * drm_fb_helper_fbdev_setup() and tear it down by calling
>>>>>> + * drm_fb_helper_fbdev_teardown().
>>>>>>   *
>>>>>>   * At runtime drivers should restore the fbdev console by using
>>>>>>   * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
>>>>>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>>>>>>   * always run in process context since the fb_*() function could be running in
>>>>>>   * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
>>>>>>   * callback it will also schedule dirty_work with the damage collected from the
>>>>>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup
>>>>>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
>>>>>> + * mmap page writes.
>>>>>> + *
>>>>>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an
>>>>>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead.
>>>>>>   */
>>>>>>
>>>>>>  static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
>>>>>> @@ -679,49 +684,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
>>>>>>  }
>>>>>>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>>>>>>
>>>>>> -/**
>>>>>> - * drm_fb_helper_defio_init - fbdev deferred I/O initialization
>>>>>> - * @fb_helper: driver-allocated fbdev helper
>>>>>> - *
>>>>>> - * This function allocates &fb_deferred_io, sets callback to
>>>>>> - * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
>>>>>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
>>>>>> - * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
>>>>>> - *
>>>>>> - * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
>>>>>> - * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
>>>>>> - * affect other instances of that &fb_ops.
>>>>>> - *
>>>>>> - * Returns:
>>>>>> - * 0 on success or a negative error code on failure.
>>>>>> - */
>>>>>> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
>>>>>> -{
>>>>>> -    struct fb_info *info = fb_helper->fbdev;
>>>>>> -    struct fb_deferred_io *fbdefio;
>>>>>> -    struct fb_ops *fbops;
>>>>>> -
>>>>>> -    fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
>>>>>> -    fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
>>>>>> -    if (!fbdefio || !fbops) {
>>>>>> -            kfree(fbdefio);
>>>>>> -            kfree(fbops);
>>>>>> -            return -ENOMEM;
>>>>>> -    }
>>>>>> -
>>>>>> -    info->fbdefio = fbdefio;
>>>>>> -    fbdefio->delay = msecs_to_jiffies(50);
>>>>>> -    fbdefio->deferred_io = drm_fb_helper_deferred_io;
>>>>>> -
>>>>>> -    *fbops = *info->fbops;
>>>>>> -    info->fbops = fbops;
>>>>>> -
>>>>>> -    fb_deferred_io_init(info);
>>>>>> -
>>>>>> -    return 0;
>>>>>> -}
>>>>>> -EXPORT_SYMBOL(drm_fb_helper_defio_init);
>>>>>> -
>>>>>
>>>>> With this gone you can remove the defio cleanup part from
>>>>> drm_fb_helper_fbdev_teardown() as well.
>>>>>
>>>>> And I see that there's no users left of that function (the same with
>>>>> *_seup()). Would be nice if you just removed them. I made them before I
>>>>> embarked on the generic fbdev solution. I think it's better to try and
>>>>> make the generic emulation usable for "everyone" and avoid the need for
>>>>> drivers to have to do their own special stuff.
>>>>
>>>> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't
>>>> removed them yet, as there's a TODO item to convert drivers over to them.
>>>>
>>>> From a quick 'git grep':
>>>>
>>>> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could
>>>> probably be converted over to generic fbdev. That's hibmc (I have some
>>>> untested patches for it), msm, omapdrm, tegra, and udl (currently being
>>>> converted).
>>>>
>>>> Only nouveau and gma500 have some form of HW acceleration.
>>>>
>>>> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are
>>>> these strictly required, or can they be made available within generic fbdev?
>>>
>>> Take a pile of digging through a few fb horrors, but this boils down to:
>>>
>>> *sys* functions operate on an fb that works like normal system memory.
>>>
>>> *cfb* functions operate on void __iomem * instead. Which makes a huge
>>> difference on some architectures, but afaiui neither x86 nor arm care.
>>> On ppc it seems to make a difference sometimes.
>>>
>>> So for shmem we should use *sys* functions, for vram *cfb*.
>>>
>>> And that leads me to realizing that drm_gem_vram_vmap has the totally
>>> wrong type, it should be void __iomem *. There's this fancy is_iomem
>>> parameter for ttm_kmap_obj_virtual that should help you figure out the
>>> right type, but only nouveau bothers to implement this correctly.
>>>
>>> I'm honestly not sure whether we should care.
>>
>> I remember getting an eamil about this from some automated test system.
>> I haven't had time to change it and it's apparently not urgent.
>>
>> If we really want to fix the problem, we'd have to propagage is_iomem
>> through the DRM core; probably touching every vmap callback. OTOH this
>> might not be a bad thing. With is_iomem available in the generic fbdev
>> emulation, it could select between sys and cfb functions and support
>> drivers with cfb-based fbdev as well.
> 
> is_iomem doesn't work, it's hack, since the change is in the function
> prototypes/signatures. So we'd need to make sure we have 2 sets of kernel
> mapping functions for everything, plus 2 sets of anything that accesses
> through the kernel.
> 
> It's a huge task, and I'm really not sure we have any architecture we care
> about this ...

But the caller of vmap() doesn't know if it is I/O memory. It has to get
this information via vmap() from the memory manager. Having two
individual vmap() functions (and picking the correct one) would be quite
a burden to the caller.

But what's wrong with casting the returned address to void __iomem* if
is_iomem is true?

To get an idea of how well this works, I made a patchset to propagate
is_iomem through all the vmap() interfaces in DRM. I found that most
drivers' memory managers are SHMEM- or CMA-based and don't have to
bother. The rest is based on TTM, which already returns the correct
value for is_iomem. I didn't modify dma-buf interfaces and simply
assumed 'false' here.

The final patches modify the fbdev emulation to use either sys or cfb
functions, depending on the value of is_iomem. Admittedly, I don't have
the hardware where it makes a differences, but the change wasn't that
hard either.

I can probably post the patchset later this week for RFC.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>> -Daniel
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>> code to avoid the shadow buffering would aid in that as Daniel have
>>>>> talked about.
>>>>>
>>>>> Either way you choose, this patch is:
>>>>>
>>>>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>
>>>>>>  /**
>>>>>>   * drm_fb_helper_sys_read - wrapper around fb_sys_read
>>>>>>   * @info: fb_info struct pointer
>>>>>> @@ -2356,7 +2318,10 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>>>>>>   *
>>>>>>   * Drivers that set the dirty callback on their framebuffer will get a shadow
>>>>>>   * fbdev buffer that is blitted onto the real buffer. This is done in order to
>>>>>> - * make deferred I/O work with all kinds of buffers.
>>>>>> + * make deferred I/O work with all kinds of buffers. A shadow buffer can be
>>>>>> + * requested explicitly by setting struct drm_mode_config.prefer_shadow or
>>>>>> + * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
>>>>>> + * required to use generic fbdev emulation with SHMEM helpers.
>>>>>>   *
>>>>>>   * This function is safe to call even when there are no connectors present.
>>>>>>   * Setup will be retried on the next hotplug event.
>>>>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>>>>>> index 8dcc012ccbc8..2338e9f94a03 100644
>>>>>> --- a/include/drm/drm_fb_helper.h
>>>>>> +++ b/include/drm/drm_fb_helper.h
>>>>>> @@ -235,7 +235,6 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
>>>>>>
>>>>>>  void drm_fb_helper_deferred_io(struct fb_info *info,
>>>>>>                             struct list_head *pagelist);
>>>>>> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper);
>>>>>>
>>>>>>  ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
>>>>>>                             size_t count, loff_t *ppos);
>>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>>>
>>>> --
>>>> Thomas Zimmermann
>>>> Graphics Driver Developer
>>>> SUSE Software Solutions Germany GmbH
>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>> (HRB 36809, AG Nürnberg)
>>>> Geschäftsführer: Felix Imendörffer
>>>>
>>>
>>>
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs

[Daniel Vetter]

On Mon, Nov 04, 2019 at 07:48:35PM +0100, Thomas Zimmermann wrote:
> Hi Daniel
> 
> Am 04.11.19 um 10:55 schrieb Daniel Vetter:
> > On Mon, Oct 28, 2019 at 09:13:47AM +0100, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 25.10.19 um 20:54 schrieb Daniel Vetter:
> >>> On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>>
> >>>> Hi
> >>>>
> >>>> Am 25.10.19 um 17:46 schrieb Noralf Trønnes:
> >>>>>
> >>>>>
> >>>>> Den 25.10.2019 11.27, skrev Thomas Zimmermann:
> >>>>>> There are no users of drm_fb_helper_defio_init(), so we can remove
> >>>>>> it. The documenation around defio support is a bit misleading and
> >>>>>> should mention compatibility issues with SHMEM helpers. Clarify this.
> >>>>>>
> >>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
> >>>>>>  include/drm/drm_fb_helper.h     |  1 -
> >>>>>>  2 files changed, 13 insertions(+), 49 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>>>>> index b75ae8555baf..8ebeccdeed23 100644
> >>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>>>>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
> >>>>>>   *
> >>>>>>   * Drivers that support a dumb buffer with a virtual address and mmap support,
> >>>>>>   * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
> >>>>>> + * It will automatically set up deferred I/O if the driver requires a shadow
> >>>>>> + * buffer.
> >>>>>>   *
> >>>>>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
> >>>>>> - * down by calling drm_fb_helper_fbdev_teardown().
> >>>>>> + * For other drivers, setup fbdev emulation by calling
> >>>>>> + * drm_fb_helper_fbdev_setup() and tear it down by calling
> >>>>>> + * drm_fb_helper_fbdev_teardown().
> >>>>>>   *
> >>>>>>   * At runtime drivers should restore the fbdev console by using
> >>>>>>   * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
> >>>>>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
> >>>>>>   * always run in process context since the fb_*() function could be running in
> >>>>>>   * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
> >>>>>>   * callback it will also schedule dirty_work with the damage collected from the
> >>>>>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup
> >>>>>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
> >>>>>> + * mmap page writes.
> >>>>>> + *
> >>>>>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an
> >>>>>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead.
> >>>>>>   */
> >>>>>>
> >>>>>>  static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
> >>>>>> @@ -679,49 +684,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
> >>>>>>  }
> >>>>>>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> >>>>>>
> >>>>>> -/**
> >>>>>> - * drm_fb_helper_defio_init - fbdev deferred I/O initialization
> >>>>>> - * @fb_helper: driver-allocated fbdev helper
> >>>>>> - *
> >>>>>> - * This function allocates &fb_deferred_io, sets callback to
> >>>>>> - * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
> >>>>>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
> >>>>>> - * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
> >>>>>> - *
> >>>>>> - * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
> >>>>>> - * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
> >>>>>> - * affect other instances of that &fb_ops.
> >>>>>> - *
> >>>>>> - * Returns:
> >>>>>> - * 0 on success or a negative error code on failure.
> >>>>>> - */
> >>>>>> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
> >>>>>> -{
> >>>>>> -    struct fb_info *info = fb_helper->fbdev;
> >>>>>> -    struct fb_deferred_io *fbdefio;
> >>>>>> -    struct fb_ops *fbops;
> >>>>>> -
> >>>>>> -    fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> >>>>>> -    fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> >>>>>> -    if (!fbdefio || !fbops) {
> >>>>>> -            kfree(fbdefio);
> >>>>>> -            kfree(fbops);
> >>>>>> -            return -ENOMEM;
> >>>>>> -    }
> >>>>>> -
> >>>>>> -    info->fbdefio = fbdefio;
> >>>>>> -    fbdefio->delay = msecs_to_jiffies(50);
> >>>>>> -    fbdefio->deferred_io = drm_fb_helper_deferred_io;
> >>>>>> -
> >>>>>> -    *fbops = *info->fbops;
> >>>>>> -    info->fbops = fbops;
> >>>>>> -
> >>>>>> -    fb_deferred_io_init(info);
> >>>>>> -
> >>>>>> -    return 0;
> >>>>>> -}
> >>>>>> -EXPORT_SYMBOL(drm_fb_helper_defio_init);
> >>>>>> -
> >>>>>
> >>>>> With this gone you can remove the defio cleanup part from
> >>>>> drm_fb_helper_fbdev_teardown() as well.
> >>>>>
> >>>>> And I see that there's no users left of that function (the same with
> >>>>> *_seup()). Would be nice if you just removed them. I made them before I
> >>>>> embarked on the generic fbdev solution. I think it's better to try and
> >>>>> make the generic emulation usable for "everyone" and avoid the need for
> >>>>> drivers to have to do their own special stuff.
> >>>>
> >>>> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't
> >>>> removed them yet, as there's a TODO item to convert drivers over to them.
> >>>>
> >>>> From a quick 'git grep':
> >>>>
> >>>> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could
> >>>> probably be converted over to generic fbdev. That's hibmc (I have some
> >>>> untested patches for it), msm, omapdrm, tegra, and udl (currently being
> >>>> converted).
> >>>>
> >>>> Only nouveau and gma500 have some form of HW acceleration.
> >>>>
> >>>> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are
> >>>> these strictly required, or can they be made available within generic fbdev?
> >>>
> >>> Take a pile of digging through a few fb horrors, but this boils down to:
> >>>
> >>> *sys* functions operate on an fb that works like normal system memory.
> >>>
> >>> *cfb* functions operate on void __iomem * instead. Which makes a huge
> >>> difference on some architectures, but afaiui neither x86 nor arm care.
> >>> On ppc it seems to make a difference sometimes.
> >>>
> >>> So for shmem we should use *sys* functions, for vram *cfb*.
> >>>
> >>> And that leads me to realizing that drm_gem_vram_vmap has the totally
> >>> wrong type, it should be void __iomem *. There's this fancy is_iomem
> >>> parameter for ttm_kmap_obj_virtual that should help you figure out the
> >>> right type, but only nouveau bothers to implement this correctly.
> >>>
> >>> I'm honestly not sure whether we should care.
> >>
> >> I remember getting an eamil about this from some automated test system.
> >> I haven't had time to change it and it's apparently not urgent.
> >>
> >> If we really want to fix the problem, we'd have to propagage is_iomem
> >> through the DRM core; probably touching every vmap callback. OTOH this
> >> might not be a bad thing. With is_iomem available in the generic fbdev
> >> emulation, it could select between sys and cfb functions and support
> >> drivers with cfb-based fbdev as well.
> > 
> > is_iomem doesn't work, it's hack, since the change is in the function
> > prototypes/signatures. So we'd need to make sure we have 2 sets of kernel
> > mapping functions for everything, plus 2 sets of anything that accesses
> > through the kernel.
> > 
> > It's a huge task, and I'm really not sure we have any architecture we care
> > about this ...
> 
> But the caller of vmap() doesn't know if it is I/O memory. It has to get
> this information via vmap() from the memory manager. Having two
> individual vmap() functions (and picking the correct one) would be quite
> a burden to the caller.
> 
> But what's wrong with casting the returned address to void __iomem* if
> is_iomem is true?
> 
> To get an idea of how well this works, I made a patchset to propagate
> is_iomem through all the vmap() interfaces in DRM. I found that most
> drivers' memory managers are SHMEM- or CMA-based and don't have to
> bother. The rest is based on TTM, which already returns the correct
> value for is_iomem. I didn't modify dma-buf interfaces and simply
> assumed 'false' here.
> 
> The final patches modify the fbdev emulation to use either sys or cfb
> functions, depending on the value of is_iomem. Admittedly, I don't have
> the hardware where it makes a differences, but the change wasn't that
> hard either.
> 
> I can probably post the patchset later this week for RFC.

The big reason behind __iomem is that you can use sparse to make sure you
got it right. With is_iomem and the explicit cast, you'll lose that
information. Which means no one will get it right (viz entire current drm
except nouveau).

That's why I think the 2 paths would be a lot nicer, and callers would
need to first call the system memory mmap, then the iomem mmap, until they
have a non-NULL pointer. Since they need the duplicated code anyway.

Other option would be we do an entire new pointer like the below:

struct opaque_buffer_ptr {
	union { void * smem; void * __iomem iomem ; };
	bool is_iomem;
};

And then an entire new set of functions that deals in this new primitive.

But unloading the is_iomem explicitly on drivers, expecting them to get it
right without the help or sparse, seems futile.

Also, all of this is _huuuuuuuuge_ amounts of work, and I'm still not sure
where we need it.
-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/2] drm/fb-helper: Remove drm_fb_helper_defio_init() and update docs

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 10388 bytes --]

Hi

Am 05.11.19 um 10:30 schrieb Daniel Vetter:
> On Mon, Nov 04, 2019 at 07:48:35PM +0100, Thomas Zimmermann wrote:
>> Hi Daniel
>>
>> Am 04.11.19 um 10:55 schrieb Daniel Vetter:
>>> On Mon, Oct 28, 2019 at 09:13:47AM +0100, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 25.10.19 um 20:54 schrieb Daniel Vetter:
>>>>> On Fri, Oct 25, 2019 at 7:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> Am 25.10.19 um 17:46 schrieb Noralf Trønnes:
>>>>>>>
>>>>>>>
>>>>>>> Den 25.10.2019 11.27, skrev Thomas Zimmermann:
>>>>>>>> There are no users of drm_fb_helper_defio_init(), so we can remove
>>>>>>>> it. The documenation around defio support is a bit misleading and
>>>>>>>> should mention compatibility issues with SHMEM helpers. Clarify this.
>>>>>>>>
>>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/drm_fb_helper.c | 61 +++++++--------------------------
>>>>>>>>  include/drm/drm_fb_helper.h     |  1 -
>>>>>>>>  2 files changed, 13 insertions(+), 49 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>>>>>> index b75ae8555baf..8ebeccdeed23 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>>>>> @@ -92,9 +92,12 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>>>>>>>>   *
>>>>>>>>   * Drivers that support a dumb buffer with a virtual address and mmap support,
>>>>>>>>   * should try out the generic fbdev emulation using drm_fbdev_generic_setup().
>>>>>>>> + * It will automatically set up deferred I/O if the driver requires a shadow
>>>>>>>> + * buffer.
>>>>>>>>   *
>>>>>>>> - * Setup fbdev emulation by calling drm_fb_helper_fbdev_setup() and tear it
>>>>>>>> - * down by calling drm_fb_helper_fbdev_teardown().
>>>>>>>> + * For other drivers, setup fbdev emulation by calling
>>>>>>>> + * drm_fb_helper_fbdev_setup() and tear it down by calling
>>>>>>>> + * drm_fb_helper_fbdev_teardown().
>>>>>>>>   *
>>>>>>>>   * At runtime drivers should restore the fbdev console by using
>>>>>>>>   * drm_fb_helper_lastclose() as their &drm_driver.lastclose callback.
>>>>>>>> @@ -127,8 +130,10 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>>>>>>>>   * always run in process context since the fb_*() function could be running in
>>>>>>>>   * atomic context. If drm_fb_helper_deferred_io() is used as the deferred_io
>>>>>>>>   * callback it will also schedule dirty_work with the damage collected from the
>>>>>>>> - * mmap page writes. Drivers can use drm_fb_helper_defio_init() to setup
>>>>>>>> - * deferred I/O (coupled with drm_fb_helper_fbdev_teardown()).
>>>>>>>> + * mmap page writes.
>>>>>>>> + *
>>>>>>>> + * Deferred I/O is not compatible with SHMEM. Such drivers should request an
>>>>>>>> + * fbdev shadow buffer and call drm_fbdev_generic_setup() instead.
>>>>>>>>   */
>>>>>>>>
>>>>>>>>  static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
>>>>>>>> @@ -679,49 +684,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
>>>>>>>>  }
>>>>>>>>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>>>>>>>>
>>>>>>>> -/**
>>>>>>>> - * drm_fb_helper_defio_init - fbdev deferred I/O initialization
>>>>>>>> - * @fb_helper: driver-allocated fbdev helper
>>>>>>>> - *
>>>>>>>> - * This function allocates &fb_deferred_io, sets callback to
>>>>>>>> - * drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
>>>>>>>> - * It should be called from the &drm_fb_helper_funcs->fb_probe callback.
>>>>>>>> - * drm_fb_helper_fbdev_teardown() cleans up deferred I/O.
>>>>>>>> - *
>>>>>>>> - * NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
>>>>>>>> - * because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
>>>>>>>> - * affect other instances of that &fb_ops.
>>>>>>>> - *
>>>>>>>> - * Returns:
>>>>>>>> - * 0 on success or a negative error code on failure.
>>>>>>>> - */
>>>>>>>> -int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper)
>>>>>>>> -{
>>>>>>>> -    struct fb_info *info = fb_helper->fbdev;
>>>>>>>> -    struct fb_deferred_io *fbdefio;
>>>>>>>> -    struct fb_ops *fbops;
>>>>>>>> -
>>>>>>>> -    fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
>>>>>>>> -    fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
>>>>>>>> -    if (!fbdefio || !fbops) {
>>>>>>>> -            kfree(fbdefio);
>>>>>>>> -            kfree(fbops);
>>>>>>>> -            return -ENOMEM;
>>>>>>>> -    }
>>>>>>>> -
>>>>>>>> -    info->fbdefio = fbdefio;
>>>>>>>> -    fbdefio->delay = msecs_to_jiffies(50);
>>>>>>>> -    fbdefio->deferred_io = drm_fb_helper_deferred_io;
>>>>>>>> -
>>>>>>>> -    *fbops = *info->fbops;
>>>>>>>> -    info->fbops = fbops;
>>>>>>>> -
>>>>>>>> -    fb_deferred_io_init(info);
>>>>>>>> -
>>>>>>>> -    return 0;
>>>>>>>> -}
>>>>>>>> -EXPORT_SYMBOL(drm_fb_helper_defio_init);
>>>>>>>> -
>>>>>>>
>>>>>>> With this gone you can remove the defio cleanup part from
>>>>>>> drm_fb_helper_fbdev_teardown() as well.
>>>>>>>
>>>>>>> And I see that there's no users left of that function (the same with
>>>>>>> *_seup()). Would be nice if you just removed them. I made them before I
>>>>>>> embarked on the generic fbdev solution. I think it's better to try and
>>>>>>> make the generic emulation usable for "everyone" and avoid the need for
>>>>>>> drivers to have to do their own special stuff.
>>>>>>
>>>>>> The last user was vboxvideo, which I converted ~2 weeks ago. I haven't
>>>>>> removed them yet, as there's a TODO item to convert drivers over to them.
>>>>>>
>>>>>> From a quick 'git grep':
>>>>>>
>>>>>> Most drivers that uses drm_fb_helper_sys_*() in its fb_ops could
>>>>>> probably be converted over to generic fbdev. That's hibmc (I have some
>>>>>> untested patches for it), msm, omapdrm, tegra, and udl (currently being
>>>>>> converted).
>>>>>>
>>>>>> Only nouveau and gma500 have some form of HW acceleration.
>>>>>>
>>>>>> The rest of the drivers (10) uses drm_fb_helper_cfb_*() functions. Are
>>>>>> these strictly required, or can they be made available within generic fbdev?
>>>>>
>>>>> Take a pile of digging through a few fb horrors, but this boils down to:
>>>>>
>>>>> *sys* functions operate on an fb that works like normal system memory.
>>>>>
>>>>> *cfb* functions operate on void __iomem * instead. Which makes a huge
>>>>> difference on some architectures, but afaiui neither x86 nor arm care.
>>>>> On ppc it seems to make a difference sometimes.
>>>>>
>>>>> So for shmem we should use *sys* functions, for vram *cfb*.
>>>>>
>>>>> And that leads me to realizing that drm_gem_vram_vmap has the totally
>>>>> wrong type, it should be void __iomem *. There's this fancy is_iomem
>>>>> parameter for ttm_kmap_obj_virtual that should help you figure out the
>>>>> right type, but only nouveau bothers to implement this correctly.
>>>>>
>>>>> I'm honestly not sure whether we should care.
>>>>
>>>> I remember getting an eamil about this from some automated test system.
>>>> I haven't had time to change it and it's apparently not urgent.
>>>>
>>>> If we really want to fix the problem, we'd have to propagage is_iomem
>>>> through the DRM core; probably touching every vmap callback. OTOH this
>>>> might not be a bad thing. With is_iomem available in the generic fbdev
>>>> emulation, it could select between sys and cfb functions and support
>>>> drivers with cfb-based fbdev as well.
>>>
>>> is_iomem doesn't work, it's hack, since the change is in the function
>>> prototypes/signatures. So we'd need to make sure we have 2 sets of kernel
>>> mapping functions for everything, plus 2 sets of anything that accesses
>>> through the kernel.
>>>
>>> It's a huge task, and I'm really not sure we have any architecture we care
>>> about this ...
>>
>> But the caller of vmap() doesn't know if it is I/O memory. It has to get
>> this information via vmap() from the memory manager. Having two
>> individual vmap() functions (and picking the correct one) would be quite
>> a burden to the caller.
>>
>> But what's wrong with casting the returned address to void __iomem* if
>> is_iomem is true?
>>
>> To get an idea of how well this works, I made a patchset to propagate
>> is_iomem through all the vmap() interfaces in DRM. I found that most
>> drivers' memory managers are SHMEM- or CMA-based and don't have to
>> bother. The rest is based on TTM, which already returns the correct
>> value for is_iomem. I didn't modify dma-buf interfaces and simply
>> assumed 'false' here.
>>
>> The final patches modify the fbdev emulation to use either sys or cfb
>> functions, depending on the value of is_iomem. Admittedly, I don't have
>> the hardware where it makes a differences, but the change wasn't that
>> hard either.
>>
>> I can probably post the patchset later this week for RFC.
> 
> The big reason behind __iomem is that you can use sparse to make sure you
> got it right. With is_iomem and the explicit cast, you'll lose that
> information. Which means no one will get it right (viz entire current drm
> except nouveau).
> 
> That's why I think the 2 paths would be a lot nicer, and callers would
> need to first call the system memory mmap, then the iomem mmap, until they
> have a non-NULL pointer. Since they need the duplicated code anyway.
> 
> Other option would be we do an entire new pointer like the below:
> 
> struct opaque_buffer_ptr {
> 	union { void * smem; void * __iomem iomem ; };
> 	bool is_iomem;
> };
> 
> And then an entire new set of functions that deals in this new primitive.
> 
> But unloading the is_iomem explicitly on drivers, expecting them to get it
> right without the help or sparse, seems futile.
> 
> Also, all of this is _huuuuuuuuge_ amounts of work, and I'm still not sure
> where we need it.

I'm telling you that I have the patches written, and you say it's too
much work? That makes no sense. :)

Let my post the patches that I have and continue discussing from there.

Best regards
Thomas


> -Daniel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel