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 14:29 +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 23.01.23 um 13:20 schrieb Hogander, Jouni:
> > 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?
> 
> Ah OK, so I remembered incorrectly.
> 
> A few things have changed since my original comment and I worked on 
> fbdev helper a bit.  The thing is that fb_dirty will likely go away
> at 
> some point (together with the rest of drm_fb_helper_funcs). IOW at
> some 
> point, you'll need those i915 functions anyways. Not using fb_dirty
> now 
> will safe that work later on.

Ok, I will modify the patch accordingly.

> 
> Best regards
> Thomas
> 
> > 
> > > 
> > > 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
> > 
> 
> -- 
> 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