Re: [Intel-gfx] [PATCH v4] drm/i915/fbdev: Implement fb_dirty for intel custom fb helper

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Mon, 2023-01-23 at 12:38 +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 23.01.23 um 08:44 schrieb Jouni Högander:
> > After disconnecting damage worker from update logic it's left to
> > fbdev
> > emulation implementation to have fb_dirty function. Currently intel
> > fbdev doesn't have it. This is causing problems to features (PSR,
> > FBC,
> > DRRS) relying on dirty callback.
> > 
> > Implement simple fb_dirty callback to deliver notifications about
> > updates
> > in fb console.
> > 
> > v4: Add proper Fixes tag and modify commit message
> > v3: Check damage clip
> > v2: Improved commit message and added Fixes tag
> > 
> > Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker from
> > update logic")
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_fbdev.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > index 19f3b5d92a55..d39db8050c69 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > @@ -321,8 +321,20 @@ static int intelfb_create(struct drm_fb_helper
> > *helper,
> >         return ret;
> >   }
> >   
> > +static int intelfb_dirty(struct drm_fb_helper *helper, struct
> > drm_clip_rect *clip)
> > +{
> > +       if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
> > +               return 0;
> > +
> > +       if (helper->fb->funcs->dirty)
> > +               return helper->fb->funcs->dirty(helper->fb, NULL,
> > 0, 0, clip, 1);
> 
> Didn't I nack this approach already? You shouldn't set fb_dirty. The 
> better solution was to implement i915-specific helpers for write, 
> fillarea, copyarea and blit. Those should call intelfb_dirty()
> directly 
> after performing the output.  And IIRC you already implemented this.

I have implemented such thing. I didn't took it as a nack back then.
Last comment from you was:

"if you go with fb_dirty, please implement the clipping 
test in your callback."

and v3 was prepared to address that comment. My thinking was that this
is only for fb console, but Ville Syrjälä commented that there is more
than console so I sticked in this approach.

So you think I should just drop idea of setting dirty callback and
implement those i915-specific helpers?

> 
> Best regards
> Thomas
> 
> > +
> > +       return 0;
> > +}
> > +
> >   static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
> >         .fb_probe = intelfb_create,
> > +       .fb_dirty = intelfb_dirty,
> >   };
> >   
> >   static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
> 
> -- 
> 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