Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Abhinav Kumar]

Hi Doug

Sorry for the delayed response.

On 2/2/2023 2:46 PM, Doug Anderson wrote:
> Hi,
>
> On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > Hi Doug
> >
> > On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> > > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > > time") the error handling with regards to dsi_mgr_bridge_power_on()
> > > got a bit worse. Specifically if we failed to power the bridge on then
> > > nothing would really notice. The modeset function couldn't return an
> > > error and thus we'd blindly go forward and try to do the pre-enable.
> > >
> > > In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> > > for parade-ps8640") we added a special case to move the powerup back
> > > to pre-enable time for ps8640. When we did that, we didn't try to
> > > recover the old/better error handling just for ps8640.
> > >
> > > In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> > > at modeset") we've now moved the powering up back to exclusively being
> > > during pre-enable. That means we can add the better error handling
> > > back in, so let's do it. To do so we'll add a new function
> > > dsi_mgr_bridge_power_off() that's matches how errors were handled
> > > prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> > > modeset time").
> > >
> > > NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> > > should be calling it in dsi_mgr_bridge_post_disable(). That would make
> > > some sense, but doing so would change the current behavior and thus
> > > should be a separate patch. Specifically:
> > > * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
> > >     even in the slave-DSI case of bonded DSI. We'd need to add special
> > >     handling for this if it's truly needed.
> > > * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
> > >     midway through the poweroff.
> > > * dsi_mgr_bridge_post_disable() has a different order of some of the
> > >     poweroffs / IRQ disables.
> > > For now we'll leave dsi_mgr_bridge_post_disable() alone.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > > ---
> > >
> > > Changes in v2:
> > > - ("More properly handle errors...") new for v2.
> > >
> > >    drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
> > >    1 file changed, 26 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > index 2197a54b9b96..28b8012a21f2 100644
> > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > > @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
> > >        }
> > >    }
> > >
> > > -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> > > +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> > >    {
> > >        int id = dsi_mgr_bridge_get_id(bridge);
> > >        struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> > > @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> > >        if (is_bonded_dsi && msm_dsi1)
> > >                msm_dsi_host_enable_irq(msm_dsi1->host);
> > >
> > > -     return;
> > > +     return 0;
> > >
> > >    host1_on_fail:
> > >        msm_dsi_host_power_off(host);
> > >    host_on_fail:
> > >        dsi_mgr_phy_disable(id);
> > >    phy_en_fail:
> > > -     return;
> > > +     return ret;
> > > +}
> > > +
> > > +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
> > > +{
> > > +     int id = dsi_mgr_bridge_get_id(bridge);
> > > +     struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> > > +     struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
> > > +     struct mipi_dsi_host *host = msm_dsi->host;
> > > +     bool is_bonded_dsi = IS_BONDED_DSI();
> > > +
> > > +     msm_dsi_host_disable_irq(host);
> > > +     if (is_bonded_dsi && msm_dsi1) {
> > > +             msm_dsi_host_disable_irq(msm_dsi1->host);
> > > +             msm_dsi_host_power_off(msm_dsi1->host);
> > > +     }
> >
> > The order of disabling the IRQs should be opposite of how they were enabled.
> >
> > So while enabling it was DSI0 and then DSI1.
> >
> > Hence while disabling it should be DSI1 and then DSI0.
> >
> > So the order here should be
> >
> > DSI1 irq disable
> > DSI0 irq disable
> > DSI1 host power off
> > DSI0 host power off
>
> Right. Normally you want to go opposite. I guess a few points, though:
>
> 1. As talked about in the commit message, the order I have matches the
> order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host
> powerup to modeset time").
>
> 2. I'd be curious if it matters. The order you request means we need
> to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a
> big deal if it's important, it's nice not to have to do so.
>
> 3. As talked about in the commit message, eventually we should
> probably resolve this order with the order of things in
> dsi_mgr_bridge_post_disable(), which is yet a different ordering.
> Ideally this resolution would be done by someone who actually has
> proper documentation of the hardware and how it's supposed to work
> (AKA not me).
>
> So my preference would be to either land or drop ${SUBJECT} patch
> (either is fine with me) and then someone at Qualcomm could then take
> over further cleanup.

I do think the ordering matters but you are right, this change brings 
back the ordering we had before so lets handle the re-ordering of all 
places in a separate change. I am okay with this change to go-in, hence

Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>

What is the plan to land the patches?

2 & 3 go in msm-next but 1 goes in drm-misc?

Thanks

Abhinav


> -Doug