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

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

[Krishna Manikandan]

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@codeaurora.org>

Changes in v2:
	- Remove NULL check from msm_pdev_shutdown (Stephen Boyd)
	- Change commit text to reflect when this issue
	  was uncovered (Sai Prakash Ranjan)
---
 drivers/gpu/drm/msm/msm_drv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index e4b750b..94e3963 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1322,6 +1322,13 @@ static int msm_pdev_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void msm_pdev_shutdown(struct platform_device *pdev)
+{
+	struct drm_device *drm = platform_get_drvdata(pdev);
+
+	drm_atomic_helper_shutdown(drm);
+}
+
 static const struct of_device_id dt_match[] = {
 	{ .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
 	{ .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
@@ -1334,6 +1341,7 @@ static int msm_pdev_remove(struct platform_device *pdev)
 static struct platform_driver msm_platform_driver = {
 	.probe      = msm_pdev_probe,
 	.remove     = msm_pdev_remove,
+	.shutdown   = msm_pdev_shutdown,
 	.driver     = {
 		.name   = "msm",
 		.of_match_table = dt_match,
-- 
1.9.1

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

[Emil Velikov]

Hi Krishna,

On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan <mkrishn@codeaurora.org> 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@codeaurora.org>
>
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.

-Emil

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

[Sai Prakash Ranjan]

Hi Emil,

On 2020-06-02 19:43, Emil Velikov wrote:
> Hi Krishna,
> 
> On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan 
> <mkrishn@codeaurora.org> 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@codeaurora.org>
>> 
> 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.

msm_pdev_shutdown()
  platform_drv_shutdown()
   device_shutdown()
    kernel_restart_prepare()
     kernel_restart()
      __arm64_sys_reboot()

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Emil Velikov]

On Tue, 2 Jun 2020 at 15:49, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> 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@codeaurora.org> 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@codeaurora.org>
> >>
> > 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

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

[Sai Prakash Ranjan]

Hi Emil,

On 2020-06-02 21:09, Emil Velikov wrote:
> On Tue, 2 Jun 2020 at 15:49, Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> 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@codeaurora.org> 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@codeaurora.org>
>> >>
>> > 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.

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

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@codeaurora.org> 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@codeaurora.org> 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@codeaurora.org> 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@codeaurora.org>
> >> >>
> >> > 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

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

[Sai Prakash Ranjan]

Hi,

On 2020-06-01 16:33, Krishna Manikandan 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@codeaurora.org>
> 
> Changes in v2:
> 	- Remove NULL check from msm_pdev_shutdown (Stephen Boyd)
> 	- Change commit text to reflect when this issue
> 	  was uncovered (Sai Prakash Ranjan)
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c 
> b/drivers/gpu/drm/msm/msm_drv.c
> index e4b750b..94e3963 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1322,6 +1322,13 @@ static int msm_pdev_remove(struct 
> platform_device *pdev)
>  	return 0;
>  }
> 
> +static void msm_pdev_shutdown(struct platform_device *pdev)
> +{
> +	struct drm_device *drm = platform_get_drvdata(pdev);
> +
> +	drm_atomic_helper_shutdown(drm);
> +}
> +
>  static const struct of_device_id dt_match[] = {
>  	{ .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
>  	{ .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
> @@ -1334,6 +1341,7 @@ static int msm_pdev_remove(struct platform_device 
> *pdev)
>  static struct platform_driver msm_platform_driver = {
>  	.probe      = msm_pdev_probe,
>  	.remove     = msm_pdev_remove,
> +	.shutdown   = msm_pdev_shutdown,
>  	.driver     = {
>  		.name   = "msm",
>  		.of_match_table = dt_match,

Any more comments on this patch?

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Rob Clark]

On Tue, Aug 18, 2020 at 3:03 AM Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Hi,
>
> On 2020-06-01 16:33, Krishna Manikandan 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@codeaurora.org>
> >
> > Changes in v2:
> >       - Remove NULL check from msm_pdev_shutdown (Stephen Boyd)
> >       - Change commit text to reflect when this issue
> >         was uncovered (Sai Prakash Ranjan)
> > ---
> >  drivers/gpu/drm/msm/msm_drv.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> > b/drivers/gpu/drm/msm/msm_drv.c
> > index e4b750b..94e3963 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -1322,6 +1322,13 @@ static int msm_pdev_remove(struct
> > platform_device *pdev)
> >       return 0;
> >  }
> >
> > +static void msm_pdev_shutdown(struct platform_device *pdev)
> > +{
> > +     struct drm_device *drm = platform_get_drvdata(pdev);
> > +
> > +     drm_atomic_helper_shutdown(drm);
> > +}
> > +
> >  static const struct of_device_id dt_match[] = {
> >       { .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
> >       { .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
> > @@ -1334,6 +1341,7 @@ static int msm_pdev_remove(struct platform_device
> > *pdev)
> >  static struct platform_driver msm_platform_driver = {
> >       .probe      = msm_pdev_probe,
> >       .remove     = msm_pdev_remove,
> > +     .shutdown   = msm_pdev_shutdown,
> >       .driver     = {
> >               .name   = "msm",
> >               .of_match_table = dt_match,
>
> Any more comments on this patch?

sorry, I managed to overlook this earlier.. I've pulled it in to msm-next

BR,
-R

> Thanks,
> Sai
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member
> of Code Aurora Forum, hosted by The Linux Foundation

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

[Sai Prakash Ranjan]

On 2020-08-18 20:42, Rob Clark wrote:
> On Tue, Aug 18, 2020 at 3:03 AM Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> wrote:
>> 
>> Hi,
>> 
>> On 2020-06-01 16:33, Krishna Manikandan 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@codeaurora.org>
>> >
>> > Changes in v2:
>> >       - Remove NULL check from msm_pdev_shutdown (Stephen Boyd)
>> >       - Change commit text to reflect when this issue
>> >         was uncovered (Sai Prakash Ranjan)
>> > ---
>> >  drivers/gpu/drm/msm/msm_drv.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/msm/msm_drv.c
>> > b/drivers/gpu/drm/msm/msm_drv.c
>> > index e4b750b..94e3963 100644
>> > --- a/drivers/gpu/drm/msm/msm_drv.c
>> > +++ b/drivers/gpu/drm/msm/msm_drv.c
>> > @@ -1322,6 +1322,13 @@ static int msm_pdev_remove(struct
>> > platform_device *pdev)
>> >       return 0;
>> >  }
>> >
>> > +static void msm_pdev_shutdown(struct platform_device *pdev)
>> > +{
>> > +     struct drm_device *drm = platform_get_drvdata(pdev);
>> > +
>> > +     drm_atomic_helper_shutdown(drm);
>> > +}
>> > +
>> >  static const struct of_device_id dt_match[] = {
>> >       { .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
>> >       { .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
>> > @@ -1334,6 +1341,7 @@ static int msm_pdev_remove(struct platform_device
>> > *pdev)
>> >  static struct platform_driver msm_platform_driver = {
>> >       .probe      = msm_pdev_probe,
>> >       .remove     = msm_pdev_remove,
>> > +     .shutdown   = msm_pdev_shutdown,
>> >       .driver     = {
>> >               .name   = "msm",
>> >               .of_match_table = dt_match,
>> 
>> Any more comments on this patch?
> 
> sorry, I managed to overlook this earlier.. I've pulled it in to 
> msm-next
> 

No problem, thanks Rob.

-Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation