| Message ID | 20230123074437.475103-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] drm/i915/fbdev: Implement fb_dirty for intel custom fb helper | expand |
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. 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)
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
On Mon, 2023-01-23 at 14:20 +0200, Jouni Högander wrote: > 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. Meant to say "I haven't implemented such thing". Sorry for the typo. > 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 >
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. 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 >
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
On Mon, Jan 23, 2023 at 09:44:37AM +0200, Jouni Högander wrote: > 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> Pushed this to get the regression actually fixed. Thanks. > --- > 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); > + > + 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) > -- > 2.34.1
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); + + 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)
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(+)