Re: [PATCH] drm/bridge: anx7625: Ensure bridge is suspended in disable()

[Doug Anderson]

Hi,

On Thu, Jan 18, 2024 at 9:30 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Wed, Jan 17, 2024 at 5:59 PM Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> wrote:
> >
> > Similar to commit 26db46bc9c67 ("drm/bridge: parade-ps8640: Ensure bridge
> > is suspended in .post_disable()"). Add a mutex to ensure that aux transfer
> > won't race with atomic_disable by holding the PM reference and prevent
> > the bridge from suspend.
> >
> > Also we need to use pm_runtime_put_sync_suspend() to suspend the bridge
> > instead of idle with pm_runtime_put_sync().
> >
> > Fixes: 3203e497eb76 ("drm/bridge: anx7625: Synchronously run runtime suspend.")
> > Fixes: adca62ec370c ("drm/bridge: anx7625: Support reading edid through aux channel")
> > Signed-off-by: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
> > Tested-by: Xuxin Xiong <xuxinxiong@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 7 ++++++-
> >  drivers/gpu/drm/bridge/analogix/anx7625.h | 2 ++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
>
> This looks good to me.
>
> Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>
> I guess this started showing up more because of commit 49ddab089611
> ("drm/panel-edp: use put_sync in unprepare"), right? Since that's a
> recent commit then that means there's probably a little more urgency
> in getting this landed. That being said, it feels like it would be
> most legit to allow this to hang out on the list for a few days before
> landing. I'll aim for early next week unless someone else has any
> comments.

Pushed to drm-misc-fixes.

4d5b7daa3c61 drm/bridge: anx7625: Ensure bridge is suspended in disable()


> I guess we should see if we need to do something similar for
> ti-sn65dsi86 too since I could imagine it having similar problems.

I tried this myself and I couldn't see any problems there. I also
spent some time thinking about it and it seemed fine. There should be
no fundamental reason why we'd have to necessarily power cycle the
bridge chip in this case. Presumably ps8640 needs it because the
embedded firmware on ps8640 is easy to confuse. I'm less familiar with
the anx bridge chip, but I could believe something similar is
happening there.

-Doug