Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

2022-12-02 Thread Noralf Trønnes 



Den 02.12.2022 12.50, skrev Thomas Zimmermann:
> 
>>>
>>> You use drm_gem_fb_vmap() in the other places but here you access the
>>> object directly (and in the next hunk), but again not so important since
>>> it goes away in a later patch.
>>
>> I'll update this patch to use drm_gem_fb_vmap() consistently.
> 
> And after looking at the impact and churn, I rather go with the existing
> code that initializes from the GEM DMA object.
> 
> Noralf, is there a reason why most of MIPI DBI uses DMA helpers? In
> terms of flexibility and resource consumption, wouldn't SHMEM helpers be
> a better fit?
> 

The SHMEM helper didn't exist at the time. The SPI subsystem doesn't
have an interface for scatter/gather transfers and DMA is needed in
order to run at full speed. SPI does convert an is_vmalloc_addr() buffer
to an sg list of pages in spi_map_buf() so it solves the missing
interface that way.

I have never tried to pass a shmem buffer to spi_sync() so I don't know
if it works, but I guess that it will work.

Bare in mind that theses buffers are at most 400k in size so I'm not
sure there's much to gain in term of resources at least.

Noralf.

Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

2022-12-02 Thread Noralf Trønnes 



Den 02.12.2022 12.27, skrev Thomas Zimmermann:
> Hi
> 
> Am 25.11.22 um 18:39 schrieb Noralf Trønnes:
>>
>>
>> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>>> Move the vmap/vunmap blocks from the inner fb_dirty helpers into the
>>> MIPI DBI update helpers. The function calls can result in waiting and/or
>>> processing overhead. Reduce the penalties by executing the functions
>>> once
>>> in the outer-most function of the pipe update.
>>>
>>> This change also prepares for MIPI DBI for shadow-plane helpers. With
>>> shadow-plane helpers, transfer source buffers are mapped into kernel
>>> address space automatically.
>>>
>>> Signed-off-by: Thomas Zimmermann 
>>> ---

>>> @@ -303,9 +291,6 @@ static void mipi_dbi_fb_dirty(struct
>>> drm_framebuffer *fb, struct drm_rect *rect)
>>>   if (ret)
>>>   drm_err_once(fb->dev, "Failed to update display %d\n", ret);
>>>   -    drm_gem_fb_vunmap(fb, map);
>>> -
>>> -err_drm_dev_exit:
>>>   drm_dev_exit(idx);
>>>   }
>>>   @@ -338,14 +323,27 @@ EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
>>>   void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
>>>     struct drm_plane_state *old_state)
>>>   {
>>> +    struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>>> +    struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>>
>> These should have been zeroed to avoid UBSAN complaint, but you'll
>> remove these later so not so important.
> 
> Will be fixed.
> 
> But do you know how these warnings happen? I went through the helpers
> before and changed them to only access the format-info's relevant
> planes. No unintialized memory access should be reported.
> 

It happens with imported buffers, iosys_map_clear() looks at the
uninitialized boolean variable ->is_iomem:

drm_gem_fb_vmap -> ... -> dma_buf_vmap -> iosys_map_clear

static inline void iosys_map_clear(struct iosys_map *map)
{
if (map->is_iomem) {
map->vaddr_iomem = NULL;
map->is_iomem = false;
} else {
map->vaddr = NULL;
}
}

Noralf.

Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

2022-12-02 Thread Thomas Zimmermann 




You use drm_gem_fb_vmap() in the other places but here you access the
object directly (and in the next hunk), but again not so important since
it goes away in a later patch.


I'll update this patch to use drm_gem_fb_vmap() consistently.


And after looking at the impact and churn, I rather go with the existing 
code that initializes from the GEM DMA object.


Noralf, is there a reason why most of MIPI DBI uses DMA helpers? In 
terms of flexibility and resource consumption, wouldn't SHMEM helpers be 
a better fit?


Best regards
Thomas





With the comments considered:

Reviewed-by: Noralf Trønnes 


Thanks.

Best regards
Thomas




  if (drm_atomic_helper_damage_merged(old_state, state, ))
-    st7586_fb_dirty(state->fb, );
+    st7586_fb_dirty(, fb, );
  }
  static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
@@ -176,6 +181,8 @@ static void st7586_pipe_enable(struct 
drm_simple_display_pipe *pipe,

  .y1 = 0,
  .y2 = fb->height,
  };
+    struct drm_gem_dma_object *dma_obj;
+    struct iosys_map src;
  int idx, ret;
  u8 addr_mode;
@@ -235,7 +242,10 @@ static void st7586_pipe_enable(struct 
drm_simple_display_pipe *pipe,

  msleep(100);
-    st7586_fb_dirty(fb, );
+    dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
+    iosys_map_set_vaddr(, dma_obj->vaddr);
+
+    st7586_fb_dirty(, fb, );
  mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
  out_exit:
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 8c4ea7956d61d..36ac8495566b0 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -13,9 +13,10 @@
  #include 
  struct drm_rect;
-struct spi_device;
  struct gpio_desc;
+struct iosys_map;
  struct regulator;
+struct spi_device;
  /**
   * struct mipi_dbi - MIPI DBI interface
@@ -176,8 +177,9 @@ int mipi_dbi_command_read(struct mipi_dbi *dbi, 
u8 cmd, u8 *val);
  int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, 
size_t len);
  int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const 
u8 *data,

    size_t len);
-int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
+int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct 
drm_framebuffer *fb,

    struct drm_rect *clip, bool swap);
+
  /**
   * mipi_dbi_command - MIPI DCS command with optional parameter(s)
   * @dbi: MIPI DBI structure




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


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

2022-12-02 Thread Thomas Zimmermann 

Hi

Am 25.11.22 um 18:39 schrieb Noralf Trønnes:



Den 21.11.2022 11.45, skrev Thomas Zimmermann:

Move the vmap/vunmap blocks from the inner fb_dirty helpers into the
MIPI DBI update helpers. The function calls can result in waiting and/or
processing overhead. Reduce the penalties by executing the functions once
in the outer-most function of the pipe update.

This change also prepares for MIPI DBI for shadow-plane helpers. With
shadow-plane helpers, transfer source buffers are mapped into kernel
address space automatically.

Signed-off-by: Thomas Zimmermann 
---
  drivers/gpu/drm/drm_mipi_dbi.c | 60 +++---
  drivers/gpu/drm/tiny/ili9225.c | 31 ++
  drivers/gpu/drm/tiny/st7586.c  | 28 +++-
  include/drm/drm_mipi_dbi.h |  6 ++--
  4 files changed, 81 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index a6ac565808765..40e59a3a6481e 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -192,6 +192,7 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
  /**
   * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
   * @dst: The destination buffer
+ * @src: The source buffer
   * @fb: The source framebuffer
   * @clip: Clipping rectangle of the area to be copied
   * @swap: When true, swap MSB/LSB of 16-bit values
@@ -199,12 +200,13 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
   * Returns:
   * Zero on success, negative error code on failure.
   */
-int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
+int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer 
*fb,
  struct drm_rect *clip, bool swap)
  {
struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
-   struct iosys_map map[DRM_FORMAT_MAX_PLANES];
-   struct iosys_map data[DRM_FORMAT_MAX_PLANES];
+   struct iosys_map data[DRM_FORMAT_MAX_PLANES] = {
+   *src,
+   };


I would prefer that you used src directly when calling the drm_fb_*
functions, data can be anything and to me it isn't clear what that
contains when I see it (ofc now I know, but next year I've forgotten).
And is the array necessary, this helper will only ever support one color
plane after all?


Will be fixed.




struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
int ret;
  
@@ -212,10 +214,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,

if (ret)
return ret;
  
-	ret = drm_gem_fb_vmap(fb, map, data);

-   if (ret)
-   goto out_drm_gem_fb_end_cpu_access;
-
switch (fb->format->format) {
case DRM_FORMAT_RGB565:
if (swap)
@@ -232,8 +230,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
ret = -EINVAL;
}
  
-	drm_gem_fb_vunmap(fb, map);

-out_drm_gem_fb_end_cpu_access:
drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
  
  	return ret;

@@ -257,10 +253,9 @@ static void mipi_dbi_set_window_address(struct 
mipi_dbi_dev *dbidev,
 ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
  }
  
-static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)

+static void mipi_dbi_fb_dirty(struct iosys_map *src, struct drm_framebuffer 
*fb,
+ struct drm_rect *rect)
  {
-   struct iosys_map map[DRM_FORMAT_MAX_PLANES];
-   struct iosys_map data[DRM_FORMAT_MAX_PLANES];
struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
unsigned int height = rect->y2 - rect->y1;
unsigned int width = rect->x2 - rect->x1;
@@ -270,16 +265,9 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, 
struct drm_rect *rect)
bool full;
void *tr;
  
-	if (WARN_ON(!fb))

-   return;
-
if (!drm_dev_enter(fb->dev, ))
return;
  
-	ret = drm_gem_fb_vmap(fb, map, data);

-   if (ret)
-   goto err_drm_dev_exit;
-
full = width == fb->width && height == fb->height;
  
  	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));

@@ -287,11 +275,11 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, 
struct drm_rect *rect)
if (!dbi->dc || !full || swap ||
fb->format->format == DRM_FORMAT_XRGB) {
tr = dbidev->tx_buf;
-   ret = mipi_dbi_buf_copy(dbidev->tx_buf, fb, rect, swap);
+   ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
if (ret)
goto err_msg;
} else {
-   tr = data[0].vaddr; /* TODO: Use mapping abstraction properly */
+   tr = src->vaddr; /* TODO: Use mapping abstraction properly */
}
  
  	mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,

@@ -303,9 +291,6 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, 
struct drm_rect *rect)
if (ret)

Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

2022-11-25 Thread Noralf Trønnes 



Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Move the vmap/vunmap blocks from the inner fb_dirty helpers into the
> MIPI DBI update helpers. The function calls can result in waiting and/or
> processing overhead. Reduce the penalties by executing the functions once
> in the outer-most function of the pipe update.
> 
> This change also prepares for MIPI DBI for shadow-plane helpers. With
> shadow-plane helpers, transfer source buffers are mapped into kernel
> address space automatically.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/drm_mipi_dbi.c | 60 +++---
>  drivers/gpu/drm/tiny/ili9225.c | 31 ++
>  drivers/gpu/drm/tiny/st7586.c  | 28 +++-
>  include/drm/drm_mipi_dbi.h |  6 ++--
>  4 files changed, 81 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> index a6ac565808765..40e59a3a6481e 100644
> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> @@ -192,6 +192,7 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>  /**
>   * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
>   * @dst: The destination buffer
> + * @src: The source buffer
>   * @fb: The source framebuffer
>   * @clip: Clipping rectangle of the area to be copied
>   * @swap: When true, swap MSB/LSB of 16-bit values
> @@ -199,12 +200,13 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>   * Returns:
>   * Zero on success, negative error code on failure.
>   */
> -int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
> +int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct 
> drm_framebuffer *fb,
> struct drm_rect *clip, bool swap)
>  {
>   struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
> - struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> - struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> + struct iosys_map data[DRM_FORMAT_MAX_PLANES] = {
> + *src,
> + };

I would prefer that you used src directly when calling the drm_fb_*
functions, data can be anything and to me it isn't clear what that
contains when I see it (ofc now I know, but next year I've forgotten).
And is the array necessary, this helper will only ever support one color
plane after all?

>   struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
>   int ret;
>  
> @@ -212,10 +214,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer 
> *fb,
>   if (ret)
>   return ret;
>  
> - ret = drm_gem_fb_vmap(fb, map, data);
> - if (ret)
> - goto out_drm_gem_fb_end_cpu_access;
> -
>   switch (fb->format->format) {
>   case DRM_FORMAT_RGB565:
>   if (swap)
> @@ -232,8 +230,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer 
> *fb,
>   ret = -EINVAL;
>   }
>  
> - drm_gem_fb_vunmap(fb, map);
> -out_drm_gem_fb_end_cpu_access:
>   drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>  
>   return ret;
> @@ -257,10 +253,9 @@ static void mipi_dbi_set_window_address(struct 
> mipi_dbi_dev *dbidev,
>ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
>  }
>  
> -static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect 
> *rect)
> +static void mipi_dbi_fb_dirty(struct iosys_map *src, struct drm_framebuffer 
> *fb,
> +   struct drm_rect *rect)
>  {
> - struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> - struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>   struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
>   unsigned int height = rect->y2 - rect->y1;
>   unsigned int width = rect->x2 - rect->x1;
> @@ -270,16 +265,9 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer 
> *fb, struct drm_rect *rect)
>   bool full;
>   void *tr;
>  
> - if (WARN_ON(!fb))
> - return;
> -
>   if (!drm_dev_enter(fb->dev, ))
>   return;
>  
> - ret = drm_gem_fb_vmap(fb, map, data);
> - if (ret)
> - goto err_drm_dev_exit;
> -
>   full = width == fb->width && height == fb->height;
>  
>   DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, 
> DRM_RECT_ARG(rect));
> @@ -287,11 +275,11 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer 
> *fb, struct drm_rect *rect)
>   if (!dbi->dc || !full || swap ||
>   fb->format->format == DRM_FORMAT_XRGB) {
>   tr = dbidev->tx_buf;
> - ret = mipi_dbi_buf_copy(dbidev->tx_buf, fb, rect, swap);
> + ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
>   if (ret)
>   goto err_msg;
>   } else {
> - tr = data[0].vaddr; /* TODO: Use mapping abstraction properly */
> + tr = src->vaddr; /* TODO: Use mapping abstraction properly */
>   }
>  
>   mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
> @@ -303,9 +291,6 @@ static void