Re: [PATCH 01/12] drm/udl: Restore display mode on resume

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 07, 2022 at 07:51:05AM +0200, Takashi Iwai wrote:
> On Tue, 06 Sep 2022 22:06:55 +0200,
> Daniel Vetter wrote:
> > 
> > On Tue, Aug 16, 2022 at 05:36:44PM +0200, Takashi Iwai wrote:
> > > From: Thomas Zimmermann <tzimmermann@suse.de>
> > > 
> > > Restore the display mode whne resuming from suspend. Currently, the
> > > display remains dark.
> > > 
> > > On resume, the CRTC's mode does not change, but the 'active' flag
> > > changes to 'true'. Taking this into account when considering a mode
> > > switch restores the display mode.
> > > 
> > > The bug is reproducable by using Gnome with udl and observing the
> > > adapter's suspend/resume behavior.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > 
> > This patch isn't great and incomplete, see
> > 
> > https://lore.kernel.org/dri-devel/YxegiQFAv+OWjjqE@phenom.ffwll.local/
> > 
> > You need cc: stable and fixes: 997d33c35618 and actually just remove the
> > entire check :-)
> 
> OK, then is something like below?
> 
> I already submitted v2 yesterday (as I overlooked your reply), so I'll
> respin v3 with this (and your ack) if that's OK.
> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] drm/udl: Restore display mode on resume
> 
> Restore the display mode whne resuming from suspend. Currently, the
> display remains dark.
> 
> On resume, the CRTC's mode does not change, but the 'active' flag
> changes to 'true'. Taking this into account when considering a mode
> switch restores the display mode.
> 
> The bug is reproducable by using Gnome with udl and observing the
> adapter's suspend/resume behavior.
> 
> Actually, the whole check added in udl_simple_display_pipe_enable()
> about the crtc_state->mode_changed was bogus.  We should drop the
> whole check and always apply the mode change in this function.
> 
> [ tiwai -- Drop the mode_changed check entirely instead, per Daniel's
>   suggestion ]
> 
> Fixes: 997d33c35618 ("drm/udl: Inline DPMS code into CRTC enable and disable functions")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Yeah I think that's the right one. But please check that it still fixes
the bug you're seeing :-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/udl/udl_modeset.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 169110d8fc2e..34ce5b43c5db 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -382,9 +382,6 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height);
>  
> -	if (!crtc_state->mode_changed)
> -		return;
> -
>  	/* enable display */
>  	udl_crtc_write_mode_to_hw(crtc);
>  }
> -- 
> 2.35.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch