Re: [v2] drm/msm: add shutdown support for display platform_driver

[Emil Velikov]

On Tue, 2 Jun 2020 at 17:10, Sai Prakash Ranjan
<saiprakash.ranjan@xxxxxxxxxxxxxx> wrote:
>
> Hi Emil,
>
> On 2020-06-02 21:09, Emil Velikov wrote:
> > On Tue, 2 Jun 2020 at 15:49, Sai Prakash Ranjan
> > <saiprakash.ranjan@xxxxxxxxxxxxxx> wrote:
> >>
> >> Hi Emil,
> >>
> >> On 2020-06-02 19:43, Emil Velikov wrote:
> >> > Hi Krishna,
> >> >
> >> > On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan
> >> > <mkrishn@xxxxxxxxxxxxxx> wrote:
> >> >>
> >> >> Define shutdown callback for display drm driver,
> >> >> so as to disable all the CRTCS when shutdown
> >> >> notification is received by the driver.
> >> >>
> >> >> This change will turn off the timing engine so
> >> >> that no display transactions are requested
> >> >> while mmu translations are getting disabled
> >> >> during reboot sequence.
> >> >>
> >> >> Signed-off-by: Krishna Manikandan <mkrishn@xxxxxxxxxxxxxx>
> >> >>
> >> > AFAICT atomics is setup in msm_drm_ops::bind and shutdown in
> >> > msm_drm_ops::unbind.
> >> >
> >> > Are you saying that unbind never triggers? If so, then we should
> >> > really fix that instead, since this patch seems more like a
> >> > workaround.
> >> >
> >>
> >> Which path do you suppose that the unbind should be called from,
> >> remove
> >> callback? Here we are talking about the drivers which are builtin,
> >> where
> >> remove callbacks are not called from the driver core during
> >> reboot/shutdown,
> >> instead shutdown callbacks are called which needs to be defined in
> >> order
> >> to
> >> trigger unbind. So AFAICS there is nothing to be fixed.
> >>
> > Interesting - in drm effectively only drm panels implement .shutdown.
> > So my naive assumption was that .remove was used implicitly by core,
> > as part of the shutdown process. Yet that's not the case, so it seems
> > that many drivers could use some fixes.
> >
> > Then again, that's an existing problem which is irrelevant for msm.
> > -Emil
>
> To give more context, we are actually targeting the clients/consumers
> of SMMU/IOMMU here because we have to make sure that before the supplier
> (SMMU) shuts down, its consumers/clients need to be shutdown properly.
> Now the ordering of this is taken care in the SMMU driver via
> device_link
> which makes sure that consumer shutdown callbacks are called first, but
> we
> need to define shutdown callbacks for all its consumers to make sure we
> actually shutdown the clients or else it would invite the crashes during
> reboot
> which in this case was seen for display.
>
Thank you very much for the extra details. I think other DRM drivers
will be safe as-is.

-Emil