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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YxjOdtQmC/z73ImI@phenom.ffwll.local>
Date:   Wed, 7 Sep 2022 19:01:42 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Takashi Iwai <tiwai@...e.de>
Cc:     Daniel Vetter <daniel@...ll.ch>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 01/12] drm/udl: Restore display mode on resume

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@...e.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@...e.de>
> > > Signed-off-by: Takashi Iwai <tiwai@...e.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@...e.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@...r.kernel.org>
> Signed-off-by: Thomas Zimmermann <tzimmermann@...e.de>
> Suggested-by: Daniel Vetter <daniel.vetter@...ll.ch>
> Signed-off-by: Takashi Iwai <tiwai@...e.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@...ll.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