[PATCH 08/10] drm/fb-helper: Restore damage area upon errors

[Daniel Vetter]

On Tue, Nov 17, 2020 at 04:14:46PM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Nov 16, 2020 at 09:52:16PM +0100, Daniel Vetter wrote:
> > On Mon, Nov 16, 2020 at 09:04:35PM +0100, Thomas Zimmermann wrote:
> > > If the damage handling fails, restore the damage area. The next invocation
> > > of the damage worker will then perform the update.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> > > ---
> > >  drivers/gpu/drm/drm_fb_helper.c | 23 ++++++++++++++++++++---
> > >  1 file changed, 20 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 2e1a335bafd2..13b65dad2ca8 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -431,11 +431,28 @@ static void drm_fb_helper_damage_work(struct work_struct *work)
> > >  	if (helper->buffer) {
> > >  		ret = drm_fb_helper_damage_blit(helper, &clip_copy);
> > >  		if (ret)
> > > -			return;
> > > +			goto err;
> > >  	}
> > >  
> > > -	if (helper->fb->funcs->dirty)
> > > -		helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
> > > +	if (helper->fb->funcs->dirty) {
> > > +		ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
> > > +		if (ret)
> > > +			goto err;
> > 
> > I think this is unexpected enough that we should put some drm error
> > printing here I think.
> 
> Note, that the dirty framebuffer routines are used by the
> framebuffer console. Printing warnings means another line
> in the framebuffer console and thus another call to the
> dirty routines. Assuming the problem does not get solved
> magically the system will be busy printing warnings forever.
> I have been through that fun while working on the OMAP DSI
> command mode patches and I suggest to only do ratelimited
> logging in the code related to damage tracking.

Excellent point. Maybe _once is good enough even, perhaps with a dump of
the entire atomic state included for debugging (in case it's transient or
something like that). Past that there's nothing we can do anyway, and most
likely it's just hurting.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch