| Message ID | 20221129124302.291759-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/fbdev: Implement fb_dirty for intel custom fb helper | expand |
On Tue, Nov 29, 2022 at 02:43:02PM +0200, Jouni Högander wrote: > After splitting generic drm_fb_helper into it's own file it's left to > helper implementation to have fb_dirty function. Currently intel > fb doesn't have it. This is causing problems when PSR is enabled. > > Implement simple fb_dirty callback to deliver notifications to psr > about updates in fb console. Just found this regression myself after being baffled why the vt console was inoperable right after the driver gets loaded. It's also not just psr, but also fbc that is having issues. Needs a fixes + cc:stable tags. > > 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 | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > index 5575d7abdc09..7c7fba3fe69e 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > @@ -328,8 +328,17 @@ 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) > +{ The original thing had some kind of "is this rect actually visible?" check here. Does anyone know why it was there, and if so maybe it should go back to the higher level function so everyone doens't need to add it back in? > + 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
On Tue, Dec 20, 2022 at 07:53:06PM +0200, Ville Syrjälä wrote: > On Tue, Nov 29, 2022 at 02:43:02PM +0200, Jouni Högander wrote: > > After splitting generic drm_fb_helper into it's own file it's left to > > helper implementation to have fb_dirty function. Currently intel > > fb doesn't have it. This is causing problems when PSR is enabled. > > > > Implement simple fb_dirty callback to deliver notifications to psr > > about updates in fb console. > > Just found this regression myself after being baffled why the > vt console was inoperable right after the driver gets loaded. > > It's also not just psr, but also fbc that is having issues. > > Needs a fixes + cc:stable tags. Actually looks like it didn't make it into 6.1 so I guess no cc:stable needed. > > > > > 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 | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > > index 5575d7abdc09..7c7fba3fe69e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > @@ -328,8 +328,17 @@ 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) > > +{ > > The original thing had some kind of "is this rect actually visible?" > check here. Does anyone know why it was there, and if so maybe it should > go back to the higher level function so everyone doens't need to add it > back in? > > > + 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 > > -- > Ville Syrjälä > Intel
On Tue, 2022-12-20 at 19:58 +0200, Ville Syrjälä wrote: > On Tue, Dec 20, 2022 at 07:53:06PM +0200, Ville Syrjälä wrote: > > On Tue, Nov 29, 2022 at 02:43:02PM +0200, Jouni Högander wrote: > > > After splitting generic drm_fb_helper into it's own file it's > > > left to > > > helper implementation to have fb_dirty function. Currently intel > > > fb doesn't have it. This is causing problems when PSR is enabled. > > > > > > Implement simple fb_dirty callback to deliver notifications to > > > psr > > > about updates in fb console. > > > > Just found this regression myself after being baffled why the > > vt console was inoperable right after the driver gets loaded. > > > > It's also not just psr, but also fbc that is having issues. > > > > Needs a fixes + cc:stable tags. > > Actually looks like it didn't make it into 6.1 so I guess > no cc:stable needed. Added Fixes tag. > > > > > > > > > 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 | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c > > > b/drivers/gpu/drm/i915/display/intel_fbdev.c > > > index 5575d7abdc09..7c7fba3fe69e 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > > @@ -328,8 +328,17 @@ 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) > > > +{ > > > > The original thing had some kind of "is this rect actually > > visible?" > > check here. Does anyone know why it was there, and if so maybe it > > should > > go back to the higher level function so everyone doens't need to > > add it > > back in? I'll guess you mean this one ?: // snip /* Call damage handlers only if necessary */ if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2)) return 0; // snip I will create separate patch for this. > > > > > + 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 > > > > -- > > Ville Syrjälä > > Intel > BR, Jouni Högander
Hi Am 29.11.22 um 13:43 schrieb Jouni Högander: > After splitting generic drm_fb_helper into it's own file it's left to > helper implementation to have fb_dirty function. Currently intel > fb doesn't have it. This is causing problems when PSR is enabled. > > Implement simple fb_dirty callback to deliver notifications to psr > about updates in fb console. I'm a bit confused about i915's use of fb_dirty here. How is this supposed to interact with mmap? i915 doesn't use deferred I/O so fbdev mmap will never call fb_dirty if userspace writes to mmap'ed pages. Is this only required for the kernel's graphics console? Best regards Thomas > > 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 | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > index 5575d7abdc09..7c7fba3fe69e 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > @@ -328,8 +328,17 @@ 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 (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)
On Wed, 2022-12-21 at 11:49 +0100, Thomas Zimmermann wrote: > Hi > > Am 29.11.22 um 13:43 schrieb Jouni Högander: > > After splitting generic drm_fb_helper into it's own file it's left > > to > > helper implementation to have fb_dirty function. Currently intel > > fb doesn't have it. This is causing problems when PSR is enabled. > > > > Implement simple fb_dirty callback to deliver notifications to psr > > about updates in fb console. > > I'm a bit confused about i915's use of fb_dirty here. How is this > supposed to interact with mmap? i915 doesn't use deferred I/O so > fbdev > mmap will never call fb_dirty if userspace writes to mmap'ed pages. > Is > this only required for the kernel's graphics console? Yes, this fix is targeted for kernel's graphics console. Please check new set I just sent. > Best regards > Thomas > > > > > 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 | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c > > b/drivers/gpu/drm/i915/display/intel_fbdev.c > > index 5575d7abdc09..7c7fba3fe69e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > @@ -328,8 +328,17 @@ 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 (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) > > -- > 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 BR, Jouni Högander
On Wed, Dec 21, 2022 at 11:49:59AM +0100, Thomas Zimmermann wrote: > Hi > > Am 29.11.22 um 13:43 schrieb Jouni Högander: > > After splitting generic drm_fb_helper into it's own file it's left to > > helper implementation to have fb_dirty function. Currently intel > > fb doesn't have it. This is causing problems when PSR is enabled. > > > > Implement simple fb_dirty callback to deliver notifications to psr > > about updates in fb console. > > I'm a bit confused about i915's use of fb_dirty here. How is this > supposed to interact with mmap? i915 doesn't use deferred I/O so fbdev > mmap will never call fb_dirty if userspace writes to mmap'ed pages. Is > this only required for the kernel's graphics console? It's required for everything. mmap is presumably borked for the cases where we can't use any hw based damage tracking. > > Best regards > Thomas > > > > > 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 | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > > index 5575d7abdc09..7c7fba3fe69e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > > @@ -328,8 +328,17 @@ 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 (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) > > -- > 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 21.12.22 um 15:51 schrieb Ville Syrjälä: > On Wed, Dec 21, 2022 at 11:49:59AM +0100, Thomas Zimmermann wrote: >> Hi >> >> Am 29.11.22 um 13:43 schrieb Jouni Högander: >>> After splitting generic drm_fb_helper into it's own file it's left to >>> helper implementation to have fb_dirty function. Currently intel >>> fb doesn't have it. This is causing problems when PSR is enabled. >>> >>> Implement simple fb_dirty callback to deliver notifications to psr >>> about updates in fb console. >> >> I'm a bit confused about i915's use of fb_dirty here. How is this >> supposed to interact with mmap? i915 doesn't use deferred I/O so fbdev >> mmap will never call fb_dirty if userspace writes to mmap'ed pages. Is >> this only required for the kernel's graphics console? > > It's required for everything. mmap is presumably borked for > the cases where we can't use any hw based damage tracking. In this case, it would make sense to implement the update with fb_dirty (instead of the fb_ops I mentioned). For mmap you can use fbdev's deferred I/O. There's drm_fb_helper_deferrer_io() that tracks mmaped pages and regularly calls fb_dirty to let the driver do an update. Best regards Thomas > >> >> Best regards >> Thomas >> >>> >>> 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 | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c >>> index 5575d7abdc09..7c7fba3fe69e 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c >>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c >>> @@ -328,8 +328,17 @@ 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 (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) >> >> -- >> 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 Wed, Dec 21, 2022 at 04:08:13PM +0100, Thomas Zimmermann wrote: > Hi > > Am 21.12.22 um 15:51 schrieb Ville Syrjälä: > > On Wed, Dec 21, 2022 at 11:49:59AM +0100, Thomas Zimmermann wrote: > >> Hi > >> > >> Am 29.11.22 um 13:43 schrieb Jouni Högander: > >>> After splitting generic drm_fb_helper into it's own file it's left to > >>> helper implementation to have fb_dirty function. Currently intel > >>> fb doesn't have it. This is causing problems when PSR is enabled. > >>> > >>> Implement simple fb_dirty callback to deliver notifications to psr > >>> about updates in fb console. > >> > >> I'm a bit confused about i915's use of fb_dirty here. How is this > >> supposed to interact with mmap? i915 doesn't use deferred I/O so fbdev > >> mmap will never call fb_dirty if userspace writes to mmap'ed pages. Is > >> this only required for the kernel's graphics console? > > > > It's required for everything. mmap is presumably borked for > > the cases where we can't use any hw based damage tracking. > > In this case, it would make sense to implement the update with fb_dirty > (instead of the fb_ops I mentioned). > > For mmap you can use fbdev's deferred I/O. There's > drm_fb_helper_deferrer_io() that tracks mmaped pages and regularly calls > fb_dirty to let the driver do an update. Not sure we want the extra defio overhead for a feature no one is likely to ever need. If we actually cared about any of this we could perhaps just hook into .fb_mmap() and turn off any stuff that needs software dirty tracking while the buffer is mapped.
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c index 5575d7abdc09..7c7fba3fe69e 100644 --- a/drivers/gpu/drm/i915/display/intel_fbdev.c +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c @@ -328,8 +328,17 @@ 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 (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 splitting generic drm_fb_helper into it's own file it's left to helper implementation to have fb_dirty function. Currently intel fb doesn't have it. This is causing problems when PSR is enabled. Implement simple fb_dirty callback to deliver notifications to psr about updates in fb console. 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 | 9 +++++++++ 1 file changed, 9 insertions(+)