Re: [PATCH v2 18/21] drm/fb_helper: Minimize damage-helper overhead

[Javier Martinez Canillas]

On 10/24/22 13:19, Thomas Zimmermann wrote:
> Pull the test for fb_dirty into the caller to avoid extra work
> if no callback has been set. In this case no damage handling is
> required and no damage area needs to be computed. Print a warning
> if the damage worker runs without getting an fb_dirty callback.
> 
> Signed-off-by: Thomas Zimmermann 
> ---

Reviewed-by: Javier Martinez Canillas 

But I've a trivial comment below:

>  drivers/gpu/drm/drm_fb_helper.c | 90 ++---
>  1 file changed, 60 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 836523aef6a27..fbc5c5445fdb0 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -449,12 +449,13 @@ static int drm_fb_helper_damage_blit(struct 
> drm_fb_helper *fb_helper,
>  static void drm_fb_helper_damage_work(struct work_struct *work)
>  {
>   struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper, 
> damage_work);
> + struct drm_device *dev = helper->dev;

You removed this in patch #15, maybe just leaving it in that patch if you
plan to use it again here?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH v2 18/21] drm/fb_helper: Minimize damage-helper overhead

[Thomas Zimmermann]

Pull the test for fb_dirty into the caller to avoid extra work
if no callback has been set. In this case no damage handling is
required and no damage area needs to be computed. Print a warning
if the damage worker runs without getting an fb_dirty callback.

Signed-off-by: Thomas Zimmermann 
---
 drivers/gpu/drm/drm_fb_helper.c | 90 ++---
 1 file changed, 60 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 836523aef6a27..fbc5c5445fdb0 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -449,12 +449,13 @@ static int drm_fb_helper_damage_blit(struct drm_fb_helper 
*fb_helper,
 static void drm_fb_helper_damage_work(struct work_struct *work)
 {
struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper, 
damage_work);
+   struct drm_device *dev = helper->dev;
struct drm_clip_rect *clip = &helper->damage_clip;
struct drm_clip_rect clip_copy;
unsigned long flags;
int ret;
 
-   if (!helper->funcs->fb_dirty)
+   if (drm_WARN_ON_ONCE(dev, !helper->funcs->fb_dirty))
return;
 
spin_lock_irqsave(&helper->damage_lock, flags);
@@ -659,16 +660,12 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 }
 EXPORT_SYMBOL(drm_fb_helper_fini);
 
-static void drm_fb_helper_damage(struct fb_info *info, u32 x, u32 y,
+static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
 u32 width, u32 height)
 {
-   struct drm_fb_helper *helper = info->par;
struct drm_clip_rect *clip = &helper->damage_clip;
unsigned long flags;
 
-   if (!helper->funcs->fb_dirty)
-   return;
-
spin_lock_irqsave(&helper->damage_lock, flags);
clip->x1 = min_t(u32, clip->x1, x);
clip->y1 = min_t(u32, clip->y1, y);
@@ -718,6 +715,7 @@ static void drm_fb_helper_memory_range_to_clip(struct 
fb_info *info, off_t off,
  */
 void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head 
*pagereflist)
 {
+   struct drm_fb_helper *helper = info->par;
unsigned long start, end, min_off, max_off;
struct fb_deferred_io_pageref *pageref;
struct drm_rect damage_area;
@@ -733,17 +731,19 @@ void drm_fb_helper_deferred_io(struct fb_info *info, 
struct list_head *pagerefli
if (min_off >= max_off)
return;
 
-   /*
-* As we can only track pages, we might reach beyond the end
-* of the screen and account for non-existing scanlines. Hence,
-* keep the covered memory area within the screen buffer.
-*/
-   max_off = min(max_off, info->screen_size);
+   if (helper->funcs->fb_dirty) {
+   /*
+* As we can only track pages, we might reach beyond the end
+* of the screen and account for non-existing scanlines. Hence,
+* keep the covered memory area within the screen buffer.
+*/
+   max_off = min(max_off, info->screen_size);
 
-   drm_fb_helper_memory_range_to_clip(info, min_off, max_off - min_off, 
&damage_area);
-   drm_fb_helper_damage(info, damage_area.x1, damage_area.y1,
-drm_rect_width(&damage_area),
-drm_rect_height(&damage_area));
+   drm_fb_helper_memory_range_to_clip(info, min_off, max_off - 
min_off, &damage_area);
+   drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
+drm_rect_width(&damage_area),
+drm_rect_height(&damage_area));
+   }
 }
 EXPORT_SYMBOL(drm_fb_helper_deferred_io);
 
@@ -877,6 +877,7 @@ static ssize_t drm_fb_helper_write_screen_buffer(struct 
fb_info *info, const cha
 ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
size_t count, loff_t *ppos)
 {
+   struct drm_fb_helper *helper = info->par;
loff_t pos = *ppos;
ssize_t ret;
struct drm_rect damage_area;
@@ -885,10 +886,12 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, 
const char __user *buf,
if (ret <= 0)
return ret;
 
-   drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
-   drm_fb_helper_damage(info, damage_area.x1, damage_area.y1,
-drm_rect_width(&damage_area),
-drm_rect_height(&damage_area));
+   if (helper->funcs->fb_dirty) {
+   drm_fb_helper_memory_range_to_clip(info, pos, ret, 
&damage_area);
+   drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
+drm_rect_width(&damage_area),
+drm_rect_height(&damage_area));
+   }
 
return ret;
 }
@@ -904,8 +907,12 @@ EXPORT_SYMBOL(drm_fb_helper_sys_write);
 void