drm/rockchip: shutdown drm subsystem on shutdown

Commit Message

Vicente Bergas Aug. 5, 2018, 2:09 p.m. UTC

As explained by Robin Murphy:
> the IOMMU shutdown disables paging, so if the VOP is still
> scanning out then that will result in whatever IOVAs it was using now going
> straight out onto the bus as physical addresses.

Suggested-by: JeffyChen <[e-mail]>
Suggested-by: Robin Murphy <[e-mail]>
Signed-off-by: Vicente Bergas <[e-mail]>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Marc Zyngier Aug. 5, 2018, 4:50 p.m. UTC | #1

Hi Vicente,

On Sun,  5 Aug 2018 16:09:11 +0200
Vicente Bergas <[e-mail]> wrote:

> As explained by Robin Murphy:
> > the IOMMU shutdown disables paging, so if the VOP is still
> > scanning out then that will result in whatever IOVAs it was using now going
> > straight out onto the bus as physical addresses.  
> 
> Suggested-by: JeffyChen <[e-mail]>
> Suggested-by: Robin Murphy <[e-mail]>
> Signed-off-by: Vicente Bergas <[e-mail]>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index f814d37b1db2..00a06768edb2 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
> +{
> +	struct drm_device *drm = platform_get_drvdata(pdev);
> +
> +	if (drm)
> +		drm_atomic_helper_shutdown(drm);

I'm not completely sure this is the right thing to do here. We want
shutdown to teardown the whole thing, not just the DRM context.

The driver already calls drm_atomic_helper_shutdown as part of the
unbind sequence, which looks very much like what we're trying to
achieve here.

I've already posted a patch[1] which calls the .remove handler,
ensuring that the whole infrastructure gets torn down at shutdown time.

Thanks,

	M.

[1] https://www.spinics.net/lists/arm-kernel/msg670229.html

Vicente Bergas Aug. 5, 2018, 5:38 p.m. UTC | #2

On Sun, Aug 5, 2018 at 6:50 PM, Marc Zyngier <[e-mail]> wrote:
> Hi Vicente,
>
> On Sun,  5 Aug 2018 16:09:11 +0200
> Vicente Bergas <[e-mail]> wrote:
>
>> As explained by Robin Murphy:
>> > the IOMMU shutdown disables paging, so if the VOP is still
>> > scanning out then that will result in whatever IOVAs it was using now going
>> > straight out onto the bus as physical addresses.
>>
>> Suggested-by: JeffyChen <[e-mail]>
>> Suggested-by: Robin Murphy <[e-mail]>
>> Signed-off-by: Vicente Bergas <[e-mail]>
>> ---
>>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> index f814d37b1db2..00a06768edb2 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
>>       return 0;
>>  }
>>
>> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
>> +{
>> +     struct drm_device *drm = platform_get_drvdata(pdev);
>> +
>> +     if (drm)
>> +             drm_atomic_helper_shutdown(drm);
>
> I'm not completely sure this is the right thing to do here. We want
> shutdown to teardown the whole thing, not just the DRM context.
>
> The driver already calls drm_atomic_helper_shutdown as part of the
> unbind sequence, which looks very much like what we're trying to
> achieve here.

OK, but maybe it is calling drm_atomic_helper_shutdown too late
because this patch makes a difference and does indeed fix the issue.
Anyways, please, apply your version if you have reasons to think it
is better suited to do the task.

>
> I've already posted a patch[1] which calls the .remove handler,
> ensuring that the whole infrastructure gets torn down at shutdown time.

That is funny: after months of having reported the issue, we both
sent a patch to fix it in less than 3 hours difference!

Regards,
  Vicente.

>
> Thanks,
>
>         M.
>
> [1] https://www.spinics.net/lists/arm-kernel/msg670229.html
> --
> Without deviation from the norm, progress is not possible.

Marc Zyngier Aug. 5, 2018, 6:23 p.m. UTC | #3

On Sun, 05 Aug 2018 18:38:18 +0100,
Vicente Bergas <[e-mail]> wrote:
> 
> On Sun, Aug 5, 2018 at 6:50 PM, Marc Zyngier <[e-mail]> wrote:
> > Hi Vicente,
> >
> > On Sun,  5 Aug 2018 16:09:11 +0200
> > Vicente Bergas <[e-mail]> wrote:
> >
> >> As explained by Robin Murphy:
> >> > the IOMMU shutdown disables paging, so if the VOP is still
> >> > scanning out then that will result in whatever IOVAs it was using now going
> >> > straight out onto the bus as physical addresses.
> >>
> >> Suggested-by: JeffyChen <[e-mail]>
> >> Suggested-by: Robin Murphy <[e-mail]>
> >> Signed-off-by: Vicente Bergas <[e-mail]>
> >> ---
> >>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> index f814d37b1db2..00a06768edb2 100644
> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
> >>       return 0;
> >>  }
> >>
> >> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
> >> +{
> >> +     struct drm_device *drm = platform_get_drvdata(pdev);
> >> +
> >> +     if (drm)
> >> +             drm_atomic_helper_shutdown(drm);
> >
> > I'm not completely sure this is the right thing to do here. We want
> > shutdown to teardown the whole thing, not just the DRM context.
> >
> > The driver already calls drm_atomic_helper_shutdown as part of the
> > unbind sequence, which looks very much like what we're trying to
> > achieve here.
> 
> OK, but maybe it is calling drm_atomic_helper_shutdown too late
> because this patch makes a difference and does indeed fix the issue.

I'm not saying it doesn't fix your issue. All I'm saying is that we
may want something that is closer to a full remove than just a shallow
DRM teardown. At the moment, unbind is simply *never* called.

> Anyways, please, apply your version if you have reasons to think it
> is better suited to do the task.

That would be for the DRM maintainers to decide. For that particular
subsystem, I'm just a random contributor.

> > I've already posted a patch[1] which calls the .remove handler,
> > ensuring that the whole infrastructure gets torn down at shutdown time.
> 
> That is funny: after months of having reported the issue, we both
> sent a patch to fix it in less than 3 hours difference!

I was hoping you'd do that earlier, but given that 4.18 is just a week
away and that we still don't have a proper fix for this, I've taken
the matter into my own hands. Without this, I cannot reliably use
kexec anymore, which is just the same as not being able to boot at
all.

Thanks,

	M.

Heiko Stübner Aug. 7, 2018, 12:44 p.m. UTC | #4

Hi Vicente,

Am Sonntag, 5. August 2018, 16:09:11 CEST schrieb Vicente Bergas:
> As explained by Robin Murphy:
> > the IOMMU shutdown disables paging, so if the VOP is still
> > scanning out then that will result in whatever IOVAs it was using now going
> > straight out onto the bus as physical addresses.
> 
> Suggested-by: JeffyChen <[e-mail]>
> Suggested-by: Robin Murphy <[e-mail]>
> Signed-off-by: Vicente Bergas <[e-mail]>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index f814d37b1db2..00a06768edb2 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
> +{
> +	struct drm_device *drm = platform_get_drvdata(pdev);
> +
> +	if (drm)
> +		drm_atomic_helper_shutdown(drm);

I tend to side with Marc's more drastic approach, especially as this one
should also nicely unbind the encoders used. Are you ok with us going
with Marc's patch or do you have concerns?

Providing a Tested-by tag would also be great ;-)


Thanks
Heiko

Vicente Bergas Aug. 7, 2018, 4:05 p.m. UTC | #5

Hi Heiko, Jeffy, Marc,

On Tue, Aug 7, 2018 at 2:44 PM, Heiko Stuebner <[e-mail]> wrote:
> Hi Vicente,
>
> Am Sonntag, 5. August 2018, 16:09:11 CEST schrieb Vicente Bergas:
>> As explained by Robin Murphy:
>> > the IOMMU shutdown disables paging, so if the VOP is still
>> > scanning out then that will result in whatever IOVAs it was using now going
>> > straight out onto the bus as physical addresses.
>>
>> Suggested-by: JeffyChen <[e-mail]>
>> Suggested-by: Robin Murphy <[e-mail]>
>> Signed-off-by: Vicente Bergas <[e-mail]>
>> ---
>>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> index f814d37b1db2..00a06768edb2 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
>>       return 0;
>>  }
>>
>> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
>> +{
>> +     struct drm_device *drm = platform_get_drvdata(pdev);
>> +
>> +     if (drm)
>> +             drm_atomic_helper_shutdown(drm);
>
> I tend to side with Marc's more drastic approach, especially as this one
> should also nicely unbind the encoders used. Are you ok with us going
> with Marc's patch or do you have concerns?

The patch i posted comes from Jeffy, as is, no modifications.
So, if he has no concerns about it, then it is also fine for me.

>
> Providing a Tested-by tag would also be great ;-)

OK, i'll reply to his patch with a Tested-by tag, but i was only
aware of this issue affecting hdmi on power-off, so, the only testing
performed was checking only this. I have done no kexec-related test.

Only one issue related to this: Marc, how can i reply to your patch
if i was not a recipient?

Regards,
  Vicente.

>
>
> Thanks
> Heiko
>
>

Heiko Stübner Aug. 7, 2018, 4:07 p.m. UTC | #6

Am Dienstag, 7. August 2018, 18:05:13 CEST schrieb Vicente Bergas:
> Hi Heiko, Jeffy, Marc,
> 
> On Tue, Aug 7, 2018 at 2:44 PM, Heiko Stuebner <[e-mail]> wrote:
> > Hi Vicente,
> >
> > Am Sonntag, 5. August 2018, 16:09:11 CEST schrieb Vicente Bergas:
> >> As explained by Robin Murphy:
> >> > the IOMMU shutdown disables paging, so if the VOP is still
> >> > scanning out then that will result in whatever IOVAs it was using now going
> >> > straight out onto the bus as physical addresses.
> >>
> >> Suggested-by: JeffyChen <[e-mail]>
> >> Suggested-by: Robin Murphy <[e-mail]>
> >> Signed-off-by: Vicente Bergas <[e-mail]>
> >> ---
> >>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> index f814d37b1db2..00a06768edb2 100644
> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
> >>       return 0;
> >>  }
> >>
> >> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
> >> +{
> >> +     struct drm_device *drm = platform_get_drvdata(pdev);
> >> +
> >> +     if (drm)
> >> +             drm_atomic_helper_shutdown(drm);
> >
> > I tend to side with Marc's more drastic approach, especially as this one
> > should also nicely unbind the encoders used. Are you ok with us going
> > with Marc's patch or do you have concerns?
> 
> The patch i posted comes from Jeffy, as is, no modifications.
> So, if he has no concerns about it, then it is also fine for me.
> 
> >
> > Providing a Tested-by tag would also be great ;-)
> 
> OK, i'll reply to his patch with a Tested-by tag, but i was only
> aware of this issue affecting hdmi on power-off, so, the only testing
> performed was checking only this. I have done no kexec-related test.
> 
> Only one issue related to this: Marc, how can i reply to your patch
> if i was not a recipient?

You can also just post it here. Together with Sandy I'm carrying the
drm-maintainer hat, so I'm probably the one that applies either one
of the patches and can pick up a tag from here as well :-D


Heiko

Vicente Bergas Aug. 7, 2018, 4:20 p.m. UTC | #7

On Tue, Aug 7, 2018 at 6:07 PM, Heiko Stuebner <[e-mail]> wrote:
> Am Dienstag, 7. August 2018, 18:05:13 CEST schrieb Vicente Bergas:
>> Hi Heiko, Jeffy, Marc,
>>
>> On Tue, Aug 7, 2018 at 2:44 PM, Heiko Stuebner <[e-mail]> wrote:
>> > Hi Vicente,
>> >
>> > Am Sonntag, 5. August 2018, 16:09:11 CEST schrieb Vicente Bergas:
>> >> As explained by Robin Murphy:
>> >> > the IOMMU shutdown disables paging, so if the VOP is still
>> >> > scanning out then that will result in whatever IOVAs it was using now going
>> >> > straight out onto the bus as physical addresses.
>> >>
>> >> Suggested-by: JeffyChen <[e-mail]>
>> >> Suggested-by: Robin Murphy <[e-mail]>
>> >> Signed-off-by: Vicente Bergas <[e-mail]>
>> >> ---
>> >>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
>> >>  1 file changed, 9 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> >> index f814d37b1db2..00a06768edb2 100644
>> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> >> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
>> >>       return 0;
>> >>  }
>> >>
>> >> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
>> >> +{
>> >> +     struct drm_device *drm = platform_get_drvdata(pdev);
>> >> +
>> >> +     if (drm)
>> >> +             drm_atomic_helper_shutdown(drm);
>> >
>> > I tend to side with Marc's more drastic approach, especially as this one
>> > should also nicely unbind the encoders used. Are you ok with us going
>> > with Marc's patch or do you have concerns?
>>
>> The patch i posted comes from Jeffy, as is, no modifications.
>> So, if he has no concerns about it, then it is also fine for me.
>>
>> >
>> > Providing a Tested-by tag would also be great ;-)
>>
>> OK, i'll reply to his patch with a Tested-by tag, but i was only
>> aware of this issue affecting hdmi on power-off, so, the only testing
>> performed was checking only this. I have done no kexec-related test.
>>
>> Only one issue related to this: Marc, how can i reply to your patch
>> if i was not a recipient?
>
> You can also just post it here. Together with Sandy I'm carrying the
> drm-maintainer hat, so I'm probably the one that applies either one
> of the patches and can pick up a tag from here as well :-D
>
>
> Heiko

OK, perfect, so, for this patch:
https://www.spinics.net/lists/arm-kernel/msg670229.html
here is my
Tested-by: Vicente Bergas <[e-mail]>
tag.
As said, i only tested that on shutdown, the hdmi output
is also shut down instead of showing random noise.

Regards,
  Vicente.

Marc Zyngier Sept. 9, 2018, 1:43 p.m. UTC | #8

On Tue, 7 Aug 2018 18:20:15 +0200
Vicente Bergas <[e-mail]> wrote:

> On Tue, Aug 7, 2018 at 6:07 PM, Heiko Stuebner <[e-mail]> wrote:
> > Am Dienstag, 7. August 2018, 18:05:13 CEST schrieb Vicente Bergas:  
> >> Hi Heiko, Jeffy, Marc,
> >>
> >> On Tue, Aug 7, 2018 at 2:44 PM, Heiko Stuebner <[e-mail]> wrote:  
> >> > Hi Vicente,
> >> >
> >> > Am Sonntag, 5. August 2018, 16:09:11 CEST schrieb Vicente Bergas:  
> >> >> As explained by Robin Murphy:  
> >> >> > the IOMMU shutdown disables paging, so if the VOP is still
> >> >> > scanning out then that will result in whatever IOVAs it was using now going
> >> >> > straight out onto the bus as physical addresses.  
> >> >>
> >> >> Suggested-by: JeffyChen <[e-mail]>
> >> >> Suggested-by: Robin Murphy <[e-mail]>
> >> >> Signed-off-by: Vicente Bergas <[e-mail]>
> >> >> ---
> >> >>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
> >> >>  1 file changed, 9 insertions(+)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> >> index f814d37b1db2..00a06768edb2 100644
> >> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> >> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
> >> >>       return 0;
> >> >>  }
> >> >>
> >> >> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
> >> >> +{
> >> >> +     struct drm_device *drm = platform_get_drvdata(pdev);
> >> >> +
> >> >> +     if (drm)
> >> >> +             drm_atomic_helper_shutdown(drm);  
> >> >
> >> > I tend to side with Marc's more drastic approach, especially as this one
> >> > should also nicely unbind the encoders used. Are you ok with us going
> >> > with Marc's patch or do you have concerns?  
> >>
> >> The patch i posted comes from Jeffy, as is, no modifications.
> >> So, if he has no concerns about it, then it is also fine for me.
> >>  
> >> >
> >> > Providing a Tested-by tag would also be great ;-)  
> >>
> >> OK, i'll reply to his patch with a Tested-by tag, but i was only
> >> aware of this issue affecting hdmi on power-off, so, the only testing
> >> performed was checking only this. I have done no kexec-related test.
> >>
> >> Only one issue related to this: Marc, how can i reply to your patch
> >> if i was not a recipient?  
> >
> > You can also just post it here. Together with Sandy I'm carrying the
> > drm-maintainer hat, so I'm probably the one that applies either one
> > of the patches and can pick up a tag from here as well :-D
> >
> >
> > Heiko  
> 
> OK, perfect, so, for this patch:
> https://www.spinics.net/lists/arm-kernel/msg670229.html
> here is my
> Tested-by: Vicente Bergas <[e-mail]>
> tag.
> As said, i only tested that on shutdown, the hdmi output
> is also shut down instead of showing random noise.

Any update on this patch? I was hoping to see it in 4.18, but so far
nothing has happened.

Thanks,

	M.

Heiko Stübner Sept. 10, 2018, 9:08 a.m. UTC | #9

Am Sonntag, 9. September 2018, 15:43:00 CEST schrieb Marc Zyngier:
> On Tue, 7 Aug 2018 18:20:15 +0200
> Vicente Bergas <[e-mail]> wrote:
> 
> > On Tue, Aug 7, 2018 at 6:07 PM, Heiko Stuebner <[e-mail]> wrote:
> > > Am Dienstag, 7. August 2018, 18:05:13 CEST schrieb Vicente Bergas:  
> > >> Hi Heiko, Jeffy, Marc,
> > >>
> > >> On Tue, Aug 7, 2018 at 2:44 PM, Heiko Stuebner <[e-mail]> wrote:  
> > >> > Hi Vicente,
> > >> >
> > >> > Am Sonntag, 5. August 2018, 16:09:11 CEST schrieb Vicente Bergas:  
> > >> >> As explained by Robin Murphy:  
> > >> >> > the IOMMU shutdown disables paging, so if the VOP is still
> > >> >> > scanning out then that will result in whatever IOVAs it was using now going
> > >> >> > straight out onto the bus as physical addresses.  
> > >> >>
> > >> >> Suggested-by: JeffyChen <[e-mail]>
> > >> >> Suggested-by: Robin Murphy <[e-mail]>
> > >> >> Signed-off-by: Vicente Bergas <[e-mail]>
> > >> >> ---
> > >> >>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
> > >> >>  1 file changed, 9 insertions(+)
> > >> >>
> > >> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > >> >> index f814d37b1db2..00a06768edb2 100644
> > >> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > >> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > >> >> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
> > >> >>       return 0;
> > >> >>  }
> > >> >>
> > >> >> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
> > >> >> +{
> > >> >> +     struct drm_device *drm = platform_get_drvdata(pdev);
> > >> >> +
> > >> >> +     if (drm)
> > >> >> +             drm_atomic_helper_shutdown(drm);  
> > >> >
> > >> > I tend to side with Marc's more drastic approach, especially as this one
> > >> > should also nicely unbind the encoders used. Are you ok with us going
> > >> > with Marc's patch or do you have concerns?  
> > >>
> > >> The patch i posted comes from Jeffy, as is, no modifications.
> > >> So, if he has no concerns about it, then it is also fine for me.
> > >>  
> > >> >
> > >> > Providing a Tested-by tag would also be great ;-)  
> > >>
> > >> OK, i'll reply to his patch with a Tested-by tag, but i was only
> > >> aware of this issue affecting hdmi on power-off, so, the only testing
> > >> performed was checking only this. I have done no kexec-related test.
> > >>
> > >> Only one issue related to this: Marc, how can i reply to your patch
> > >> if i was not a recipient?  
> > >
> > > You can also just post it here. Together with Sandy I'm carrying the
> > > drm-maintainer hat, so I'm probably the one that applies either one
> > > of the patches and can pick up a tag from here as well :-D
> > >
> > >
> > > Heiko  
> > 
> > OK, perfect, so, for this patch:
> > https://www.spinics.net/lists/arm-kernel/msg670229.html
> > here is my
> > Tested-by: Vicente Bergas <[e-mail]>
> > tag.
> > As said, i only tested that on shutdown, the hdmi output
> > is also shut down instead of showing random noise.
> 
> Any update on this patch? I was hoping to see it in 4.18, but so far
> nothing has happened.

it somehow slipped through my grasp, but I've applied it now.
With a stable tag added so it should also trickle down to some
stable kernels hopefully.


Sorry for the holdup
Heiko

Marc Zyngier Sept. 10, 2018, 9:57 a.m. UTC | #10

On 10/09/18 10:08, Heiko Stuebner wrote:
> Am Sonntag, 9. September 2018, 15:43:00 CEST schrieb Marc Zyngier:
>> On Tue, 7 Aug 2018 18:20:15 +0200
>> Vicente Bergas <[e-mail]> wrote:
>>
>>> On Tue, Aug 7, 2018 at 6:07 PM, Heiko Stuebner <[e-mail]> wrote:
>>>> Am Dienstag, 7. August 2018, 18:05:13 CEST schrieb Vicente Bergas:  
>>>>> Hi Heiko, Jeffy, Marc,
>>>>>
>>>>> On Tue, Aug 7, 2018 at 2:44 PM, Heiko Stuebner <[e-mail]> wrote:  
>>>>>> Hi Vicente,
>>>>>>
>>>>>> Am Sonntag, 5. August 2018, 16:09:11 CEST schrieb Vicente Bergas:  
>>>>>>> As explained by Robin Murphy:  
>>>>>>>> the IOMMU shutdown disables paging, so if the VOP is still
>>>>>>>> scanning out then that will result in whatever IOVAs it was using now going
>>>>>>>> straight out onto the bus as physical addresses.  
>>>>>>>
>>>>>>> Suggested-by: JeffyChen <[e-mail]>
>>>>>>> Suggested-by: Robin Murphy <[e-mail]>
>>>>>>> Signed-off-by: Vicente Bergas <[e-mail]>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 9 +++++++++
>>>>>>>  1 file changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>>>>>> index f814d37b1db2..00a06768edb2 100644
>>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>>>>>> @@ -442,6 +442,14 @@ static int rockchip_drm_platform_remove(struct platform_device *pdev)
>>>>>>>       return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> +static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> +     struct drm_device *drm = platform_get_drvdata(pdev);
>>>>>>> +
>>>>>>> +     if (drm)
>>>>>>> +             drm_atomic_helper_shutdown(drm);  
>>>>>>
>>>>>> I tend to side with Marc's more drastic approach, especially as this one
>>>>>> should also nicely unbind the encoders used. Are you ok with us going
>>>>>> with Marc's patch or do you have concerns?  
>>>>>
>>>>> The patch i posted comes from Jeffy, as is, no modifications.
>>>>> So, if he has no concerns about it, then it is also fine for me.
>>>>>  
>>>>>>
>>>>>> Providing a Tested-by tag would also be great ;-)  
>>>>>
>>>>> OK, i'll reply to his patch with a Tested-by tag, but i was only
>>>>> aware of this issue affecting hdmi on power-off, so, the only testing
>>>>> performed was checking only this. I have done no kexec-related test.
>>>>>
>>>>> Only one issue related to this: Marc, how can i reply to your patch
>>>>> if i was not a recipient?  
>>>>
>>>> You can also just post it here. Together with Sandy I'm carrying the
>>>> drm-maintainer hat, so I'm probably the one that applies either one
>>>> of the patches and can pick up a tag from here as well :-D
>>>>
>>>>
>>>> Heiko  
>>>
>>> OK, perfect, so, for this patch:
>>> https://www.spinics.net/lists/arm-kernel/msg670229.html
>>> here is my
>>> Tested-by: Vicente Bergas <[e-mail]>
>>> tag.
>>> As said, i only tested that on shutdown, the hdmi output
>>> is also shut down instead of showing random noise.
>>
>> Any update on this patch? I was hoping to see it in 4.18, but so far
>> nothing has happened.
> 
> it somehow slipped through my grasp, but I've applied it now.
> With a stable tag added so it should also trickle down to some
> stable kernels hopefully.
> 
> 
> Sorry for the holdup

No worries, and thanks for the stable tag! :-)

	M.

Patch

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f814d37b1db2..00a06768edb2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -442,6 +442,14 @@  static int rockchip_drm_platform_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
+{
+	struct drm_device *drm = platform_get_drvdata(pdev);
+
+	if (drm)
+		drm_atomic_helper_shutdown(drm);
+}
+
 static const struct of_device_id rockchip_drm_dt_ids[] = {
 	{ .compatible = "rockchip,display-subsystem", },
 	{ /* sentinel */ },
@@ -451,6 +459,7 @@  MODULE_DEVICE_TABLE(of, rockchip_drm_dt_ids);
 static struct platform_driver rockchip_drm_platform_driver = {
 	.probe = rockchip_drm_platform_probe,
 	.remove = rockchip_drm_platform_remove,
+	.shutdown = rockchip_drm_platform_shutdown,
 	.driver = {
 		.name = "rockchip-drm",
 		.of_match_table = rockchip_drm_dt_ids,