Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220215235420.1284208-1-briannorris@chromium.org>
 <20220215155417.2.Ic15a2ef69c540aee8732703103e2cff51fb9c399@changeid> <6344d1828760d4d8625a87243fcc5f5b1096b9d4.camel@oss.nxp.com>
In-Reply-To: <6344d1828760d4d8625a87243fcc5f5b1096b9d4.camel@oss.nxp.com>
From:   Brian Norris <briannorris@chromium.org>
Date:   Mon, 28 Feb 2022 11:59:51 -0800
Message-ID: <CA+ASDXOFcmLO_UBfzZ37NmQ3i3n_=5XPcHa_7=OLFvg6xg=YHg@mail.gmail.com>
Subject: Re: [PATCH 2/2] drm/atomic: Force bridge self-refresh-exit on CRTC switch
To:     Liu Ying <victor.liu@oss.nxp.com>
Cc:     Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Robert Foss <robert.foss@linaro.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Jonas Karlman <jonas@kwiboo.se>,
        Linux Kernel <linux-kernel@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Sean Paul <seanpaul@chromium.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        stable <stable@vger.kernel.org>, Sean Paul <sean@poorly.run>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Liu,

On Mon, Feb 28, 2022 at 1:02 AM Liu Ying <victor.liu@oss.nxp.com> wrote:
> On Tue, 2022-02-15 at 15:54 -0800, Brian Norris wrote:
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1011,9 +1011,19 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
> >               return drm_atomic_crtc_effectively_active(old_state);
> >
> >       /*
> > -      * We need to run through the crtc_funcs->disable() function if the CRTC
> > -      * is currently on, if it's transitioning to self refresh mode, or if
> > -      * it's in self refresh mode and needs to be fully disabled.
> > +      * We need to disable bridge(s) and CRTC if we're transitioning out of
> > +      * self-refresh and changing CRTCs at the same time, because the
> > +      * bridge tracks self-refresh status via CRTC state.
> > +      */
> > +     if (old_state->self_refresh_active && new_state->enable &&
> > +         old_state->crtc != new_state->crtc)
> > +             return true;
>
> I think 'new_state->enable' should be changed to 'new_state->active',
> because 'active' is the one to enable/disable the CRTC while 'enable'
> reflects whether a mode blob is set to CRTC state.  The overall logic
> added above is ok to me. Let's see if others have any comments.

Thanks for the review, and good catch. This actually shows that most
of my development was before commit 69e630016ef4 ("drm/atomic: Check
new_crtc_state->active to determine if CRTC needs disable in self
refresh mode"). In fact, the "state->enable" condition was included
here mostly as a complement to the "!state->enable" condition that was
present previously, and I didn't adapt it properly upon rebase.

In practice, this portion of the condition is not needed at all; we
really want to exit PSR on CRTC-switch regardless of the new-CRTC
state. So rather than change "enable" to "active", I plan to remove it
entirely.

I'll give it some local tests and send v2 eventually.

Thanks,
Brian