[regression] HDMI breakage just before poweroff

[regression] HDMI breakage just before poweroff

[Vicente Bergas]

Hello,
on rk3399-sapphire there is a missing feature: the poweroff command is
unable to poweroff the system and the display stays on.
When the system is halted, power needs to be removed manually.

With Linux kernel 4.16, when reaching the halted state, the HDMI console
shows the last messages before halting.

With 4.17.0-rc3, when reaching the halted state, the HDMI console
shows colorful static noise.

Regards,
  Vicente.

Re: [regression] HDMI breakage just before poweroff

[JeffyChen]

Hi Vincente,

Thanks for your mail.

On 05/03/2018 03:36 AM, Vicente Bergas wrote:
> Hello,
> on rk3399-sapphire there is a missing feature: the poweroff command is
> unable to poweroff the system and the display stays on.
> When the system is halted, power needs to be removed manually.
according to the dtsi, the rk3399-sapphire is using rk808 pimc right?

i think the power off flow would be:
1/ rk808 set the pm_power_off callback:
drivers/mfd/rk808.c
         switch (rk808->variant) {
         case RK805_ID:
	...
                 pm_pwroff_fn = rk805_device_shutdown;
	...

         pm_off = of_property_read_bool(np,
                                 "rockchip,system-power-controller");
         if (pm_off && !pm_power_off) {
                 rk808_i2c_client = client;
                 pm_power_off = pm_pwroff_fn;
         }

2/ the poweroff command would call kernel_power_off:
kernel/reboot.c:
void kernel_power_off(void)
{
...
         machine_power_off();
}

3/ arm64's machine_power_off() would call pm_power_off to let rk808 
cutoff the power.

>
> With Linux kernel 4.16, when reaching the halted state, the HDMI console
> shows the last messages before halting.
if it ends up halted, maybe something wrong during setup the pm_power_off?
kernel/reboot.c:
         /* Instead of trying to make the power_off code look like
          * halt when pm_power_off is not set do it the easy way.
          */
         if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
                 cmd = LINUX_REBOOT_CMD_HALT;

>
> With 4.17.0-rc3, when reaching the halted state, the HDMI console
> shows colorful static noise.
>
we've added a shutdown() to the iommu driver:
https://patchwork.kernel.org/patch/10230817/

any chance related?

> Regards,
>    Vicente.
>
>
>

Re: [regression] HDMI breakage just before poweroff

[Robin Murphy]

On 03/05/18 04:51, JeffyChen wrote:
> Hi Vincente,
> 
> Thanks for your mail.
> 
> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
>> Hello,
>> on rk3399-sapphire there is a missing feature: the poweroff command is
>> unable to poweroff the system and the display stays on.
>> When the system is halted, power needs to be removed manually.
> according to the dtsi, the rk3399-sapphire is using rk808 pimc right?
> 
> i think the power off flow would be:
> 1/ rk808 set the pm_power_off callback:
> drivers/mfd/rk808.c
>          switch (rk808->variant) {
>          case RK805_ID:
>      ...
>                  pm_pwroff_fn = rk805_device_shutdown;
>      ...
> 
>          pm_off = of_property_read_bool(np,
>                                  "rockchip,system-power-controller");
>          if (pm_off && !pm_power_off) {
>                  rk808_i2c_client = client;
>                  pm_power_off = pm_pwroff_fn;
>          }
> 
> 2/ the poweroff command would call kernel_power_off:
> kernel/reboot.c:
> void kernel_power_off(void)
> {
> ...
>          machine_power_off();
> }
> 
> 3/ arm64's machine_power_off() would call pm_power_off to let rk808 
> cutoff the power.

Doesn't PSCI's pm_power_off take precedence, though? This sounds similar 
to what my RK3328 does - I looked into that briefly a while ago, and 
from what I remember it appeared that the firmware SYSTEM_OFF handler 
tries to turn off the PMIC by driving the SLEEP pin, but the kernel 
driver never actually configures it into the correct mode for that to 
work. Thus the box just ends up spinning in the firmware until you pull 
the plug manually.

>> With Linux kernel 4.16, when reaching the halted state, the HDMI console
>> shows the last messages before halting.
> if it ends up halted, maybe something wrong during setup the pm_power_off?
> kernel/reboot.c:
>          /* Instead of trying to make the power_off code look like
>           * halt when pm_power_off is not set do it the easy way.
>           */
>          if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
>                  cmd = LINUX_REBOOT_CMD_HALT;
> 
>>
>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
>> shows colorful static noise.
>>
> we've added a shutdown() to the iommu driver:
> https://patchwork.kernel.org/patch/10230817/
> 
> any chance related?

For sure - 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. Between the 
RK3399 memory map and the way the IOVA allocator works, that probably 
means it's reading from all over the peripherals region, which, er, 
isn't ideal.

Robin.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [regression] HDMI breakage just before poweroff

[JeffyChen]

Hi Robin,

On 05/03/2018 08:14 PM, Robin Murphy wrote:
> On 03/05/18 04:51, JeffyChen wrote:
>> Hi Vincente,
>>
>> Thanks for your mail.
>>
>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
>>> Hello,
>>> on rk3399-sapphire there is a missing feature: the poweroff command is
>>> unable to poweroff the system and the display stays on.
>>> When the system is halted, power needs to be removed manually.
>> according to the dtsi, the rk3399-sapphire is using rk808 pimc right?
>>
>> i think the power off flow would be:
>> 1/ rk808 set the pm_power_off callback:
>> drivers/mfd/rk808.c
>>          switch (rk808->variant) {
>>          case RK805_ID:
>>      ...
>>                  pm_pwroff_fn = rk805_device_shutdown;
>>      ...
>>
>>          pm_off = of_property_read_bool(np,
>>                                  "rockchip,system-power-controller");
>>          if (pm_off && !pm_power_off) {
>>                  rk808_i2c_client = client;
>>                  pm_power_off = pm_pwroff_fn;
>>          }
>>
>> 2/ the poweroff command would call kernel_power_off:
>> kernel/reboot.c:
>> void kernel_power_off(void)
>> {
>> ...
>>          machine_power_off();
>> }
>>
>> 3/ arm64's machine_power_off() would call pm_power_off to let rk808
>> cutoff the power.
>
> Doesn't PSCI's pm_power_off take precedence, though? This sounds similar
> to what my RK3328 does - I looked into that briefly a while ago, and
> from what I remember it appeared that the firmware SYSTEM_OFF handler
> tries to turn off the PMIC by driving the SLEEP pin, but the kernel
> driver never actually configures it into the correct mode for that to
> work. Thus the box just ends up spinning in the firmware until you pull
> the plug manually.

hmmm, right, it should be psci's power off.

and i checked the BSP kernel, the rk808 driver switched to use 
pm_power_off_prepare and syscore_ops->shutdown there

>
>>> With Linux kernel 4.16, when reaching the halted state, the HDMI console
>>> shows the last messages before halting.
>> if it ends up halted, maybe something wrong during setup the
>> pm_power_off?
>> kernel/reboot.c:
>>          /* Instead of trying to make the power_off code look like
>>           * halt when pm_power_off is not set do it the easy way.
>>           */
>>          if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
>>                  cmd = LINUX_REBOOT_CMD_HALT;
>>
>>>
>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
>>> shows colorful static noise.
>>>
>> we've added a shutdown() to the iommu driver:
>> https://patchwork.kernel.org/patch/10230817/
>>
>> any chance related?
>
> For sure - 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. Between the
> RK3399 memory map and the way the IOVA allocator works, that probably
> means it's reading from all over the peripherals region, which, er,
> isn't ideal.
>
> Robin.
>
>
>

Re: [regression] HDMI breakage just before poweroff

[Vicente Bergas]

Hi,

On Thu, May 3, 2018 at 5:10 PM, JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> Hi Robin,
>
>
> On 05/03/2018 08:14 PM, Robin Murphy wrote:
>>
>> On 03/05/18 04:51, JeffyChen wrote:
>>>
>>> Hi Vincente,
>>>
>>> Thanks for your mail.
>>>
>>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
>>>>
>>>> Hello,
>>>> on rk3399-sapphire there is a missing feature: the poweroff command is
>>>> unable to poweroff the system and the display stays on.
>>>> When the system is halted, power needs to be removed manually.
>>>
>>> according to the dtsi, the rk3399-sapphire is using rk808 pimc right?
>>>
>>> i think the power off flow would be:
>>> 1/ rk808 set the pm_power_off callback:
>>> drivers/mfd/rk808.c
>>>          switch (rk808->variant) {
>>>          case RK805_ID:
>>>      ...
>>>                  pm_pwroff_fn = rk805_device_shutdown;
>>>      ...
>>>
>>>          pm_off = of_property_read_bool(np,
>>>                                  "rockchip,system-power-controller");
>>>          if (pm_off && !pm_power_off) {
>>>                  rk808_i2c_client = client;
>>>                  pm_power_off = pm_pwroff_fn;
>>>          }
>>>
>>> 2/ the poweroff command would call kernel_power_off:
>>> kernel/reboot.c:
>>> void kernel_power_off(void)
>>> {
>>> ...
>>>          machine_power_off();
>>> }
>>>
>>> 3/ arm64's machine_power_off() would call pm_power_off to let rk808
>>> cutoff the power.
>>
>>
>> Doesn't PSCI's pm_power_off take precedence, though? This sounds similar
>> to what my RK3328 does - I looked into that briefly a while ago, and
>> from what I remember it appeared that the firmware SYSTEM_OFF handler
>> tries to turn off the PMIC by driving the SLEEP pin, but the kernel
>> driver never actually configures it into the correct mode for that to
>> work. Thus the box just ends up spinning in the firmware until you pull
>> the plug manually.
>
>
> hmmm, right, it should be psci's power off.
>
> and i checked the BSP kernel, the rk808 driver switched to use
> pm_power_off_prepare and syscore_ops->shutdown there
>

PSCI? that is provided by the ATF (ARM Trusted Firmware), right?
The Sapphire board this issue appears on has:
 1.- As first stage loader the SPL (Secondary Program Loader) from rkbin [1].
 2.- As second stage loader the the ATF from rkbin [1].
 3.- As third stage loader mainline u-boot.

I would strongly like to use open source and preferably mainline SPL
and ATF, but had no success when trying a few months back.

Regards,
  Vicente.

[1] https://github.com/rockchip-linux/rkbin

>
>>
>>>> With Linux kernel 4.16, when reaching the halted state, the HDMI console
>>>> shows the last messages before halting.
>>>
>>> if it ends up halted, maybe something wrong during setup the
>>> pm_power_off?
>>> kernel/reboot.c:
>>>          /* Instead of trying to make the power_off code look like
>>>           * halt when pm_power_off is not set do it the easy way.
>>>           */
>>>          if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
>>>                  cmd = LINUX_REBOOT_CMD_HALT;
>>>
>>>>
>>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
>>>> shows colorful static noise.
>>>>
>>> we've added a shutdown() to the iommu driver:
>>> https://patchwork.kernel.org/patch/10230817/
>>>
>>> any chance related?
>>
>>
>> For sure - 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. Between the
>> RK3399 memory map and the way the IOVA allocator works, that probably
>> means it's reading from all over the peripherals region, which, er,
>> isn't ideal.
>>
>> Robin.
>>
>>
>>
>
>

Re: [regression] HDMI breakage just before poweroff

[Vicente Bergas]

On Thu, May 3, 2018 at 2:14 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> On 03/05/18 04:51, JeffyChen wrote:
>>
>> Hi Vincente,
>>
>> Thanks for your mail.
>>
>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
>>>
>>> Hello,
>>> on rk3399-sapphire there is a missing feature: the poweroff command is
>>> unable to poweroff the system and the display stays on.
>>> When the system is halted, power needs to be removed manually.
>>
>> according to the dtsi, the rk3399-sapphire is using rk808 pimc right?
>>
>> i think the power off flow would be:
>> 1/ rk808 set the pm_power_off callback:
>> drivers/mfd/rk808.c
>>          switch (rk808->variant) {
>>          case RK805_ID:
>>      ...
>>                  pm_pwroff_fn = rk805_device_shutdown;
>>      ...
>>
>>          pm_off = of_property_read_bool(np,
>>                                  "rockchip,system-power-controller");
>>          if (pm_off && !pm_power_off) {
>>                  rk808_i2c_client = client;
>>                  pm_power_off = pm_pwroff_fn;
>>          }
>>
>> 2/ the poweroff command would call kernel_power_off:
>> kernel/reboot.c:
>> void kernel_power_off(void)
>> {
>> ...
>>          machine_power_off();
>> }
>>
>> 3/ arm64's machine_power_off() would call pm_power_off to let rk808 cutoff
>> the power.
>
>
> Doesn't PSCI's pm_power_off take precedence, though? This sounds similar to
> what my RK3328 does - I looked into that briefly a while ago, and from what
> I remember it appeared that the firmware SYSTEM_OFF handler tries to turn
> off the PMIC by driving the SLEEP pin, but the kernel driver never actually
> configures it into the correct mode for that to work. Thus the box just ends
> up spinning in the firmware until you pull the plug manually.
>
>>> With Linux kernel 4.16, when reaching the halted state, the HDMI console
>>> shows the last messages before halting.
>>
>> if it ends up halted, maybe something wrong during setup the pm_power_off?
>> kernel/reboot.c:
>>          /* Instead of trying to make the power_off code look like
>>           * halt when pm_power_off is not set do it the easy way.
>>           */
>>          if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
>>                  cmd = LINUX_REBOOT_CMD_HALT;
>>
>>>
>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
>>> shows colorful static noise.
>>>
>> we've added a shutdown() to the iommu driver:
>> https://patchwork.kernel.org/patch/10230817/
>>
>> any chance related?
>
>
> For sure - 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. Between the RK3399 memory
> map and the way the IOVA allocator works, that probably means it's reading
> from all over the peripherals region, which, er, isn't ideal.
>
> Robin.

I can confirm that
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1269,7 +1264,6 @@ MODULE_DEVICE_TABLE(of, rk_iommu_dt_ids);

 static struct platform_driver rk_iommu_driver = {
     .probe = rk_iommu_probe,
-    .shutdown = rk_iommu_shutdown,
     .driver = {
            .name = "rk_iommu",
            .of_match_table = rk_iommu_dt_ids,

fixes the HDMI issue.

Regards,
  Vicente.

Re: [regression] HDMI breakage just before poweroff

[Vicente Bergas]

On Thu, May 3, 2018 at 2:14 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> On 03/05/18 04:51, JeffyChen wrote:
>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
[snip]
>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
>>> shows colorful static noise.
>>>
>> we've added a shutdown() to the iommu driver:
>> https://patchwork.kernel.org/patch/10230817/
>>
>> any chance related?
>
> For sure - 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. Between the RK3399 memory
> map and the way the IOVA allocator works, that probably means it's reading
> from all over the peripherals region, which, er, isn't ideal.

Hi,
just wondering if there has been any progress on that front?

Regards,
  Vicente.

Re: [regression] HDMI breakage just before poweroff

[JeffyChen]

Hi Vicente,

On 06/12/2018 06:04 AM, Vicente Bergas wrote:
> On Thu, May 3, 2018 at 2:14 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
>> On 03/05/18 04:51, JeffyChen wrote:
>>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
> [snip]
>>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
>>>> shows colorful static noise.
>>>>
>>> we've added a shutdown() to the iommu driver:
>>> https://patchwork.kernel.org/patch/10230817/
>>>
>>> any chance related?
>>
>> For sure - 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. Between the RK3399 memory
>> map and the way the IOVA allocator works, that probably means it's reading
>> from all over the peripherals region, which, er, isn't ideal.
>
> Hi,
> just wondering if there has been any progress on that front?
>

maybe we can add .shutdown() to rockchip_drm_drv too? (maybe just call 
drm_atomic_helper_shutdown())

could you help to try this hack:

+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -451,6 +451,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_remove,


> Regards,
>    Vicente.
>
>
>

Re: [regression] HDMI breakage just before poweroff

[Marc Zyngier]

On 12/06/18 04:52, JeffyChen wrote:
> Hi Vicente,
> 
> On 06/12/2018 06:04 AM, Vicente Bergas wrote:
>> On Thu, May 3, 2018 at 2:14 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
>>> On 03/05/18 04:51, JeffyChen wrote:
>>>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
>> [snip]
>>>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
>>>>> shows colorful static noise.
>>>>>
>>>> we've added a shutdown() to the iommu driver:
>>>> https://patchwork.kernel.org/patch/10230817/
>>>>
>>>> any chance related?
>>>
>>> For sure - 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. Between the RK3399 memory
>>> map and the way the IOVA allocator works, that probably means it's reading
>>> from all over the peripherals region, which, er, isn't ideal.
>>
>> Hi,
>> just wondering if there has been any progress on that front?
>>
> 
> maybe we can add .shutdown() to rockchip_drm_drv too? (maybe just call 
> drm_atomic_helper_shutdown())
> 
> could you help to try this hack:
> 
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -451,6 +451,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_remove,
> 

Is there any mechanism guaranteeing the ordering of shutdown between VOP
and IOMMU?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [regression] HDMI breakage just before poweroff

[JeffyChen]

Hi Marc,

On 06/12/2018 04:54 PM, Marc Zyngier wrote:
> On 12/06/18 04:52, JeffyChen wrote:
>> Hi Vicente,
>>
>> On 06/12/2018 06:04 AM, Vicente Bergas wrote:
>>> On Thu, May 3, 2018 at 2:14 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
>>>> On 03/05/18 04:51, JeffyChen wrote:
>>>>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
>>> [snip]
>>>>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
>>>>>> shows colorful static noise.
>>>>>>
>>>>> we've added a shutdown() to the iommu driver:
>>>>> https://patchwork.kernel.org/patch/10230817/
>>>>>
>>>>> any chance related?
>>>>
>>>> For sure - 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. Between the RK3399 memory
>>>> map and the way the IOVA allocator works, that probably means it's reading
>>>> from all over the peripherals region, which, er, isn't ideal.
>>>
>>> Hi,
>>> just wondering if there has been any progress on that front?
>>>
>>
>> maybe we can add .shutdown() to rockchip_drm_drv too? (maybe just call
>> drm_atomic_helper_shutdown())
>>
>> could you help to try this hack:
>>
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> @@ -451,6 +451,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_remove,
>>
>
> Is there any mechanism guaranteeing the ordering of shutdown between VOP
> and IOMMU?

it seems like the device_shutdown() will walk the devices_kset->list 
backward.

and the devices_kset->list's order is based on the probe 
order(drivers/base/dd.c -> really_probe() -> devices_kset_move_last())

and we are using of_iommu for rockchip-iommu, which would make sure 
iommu probed before iommu master, so the vop iommu would be
shutdown after vop

>
> Thanks,
>
> 	M.
>

Re: [regression] HDMI breakage just before poweroff

[Tomasz Figa]

On Tue, Jun 12, 2018 at 7:27 PM JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>
> Hi Marc,
>
> On 06/12/2018 04:54 PM, Marc Zyngier wrote:
> > On 12/06/18 04:52, JeffyChen wrote:
> >> Hi Vicente,
> >>
> >> On 06/12/2018 06:04 AM, Vicente Bergas wrote:
> >>> On Thu, May 3, 2018 at 2:14 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> >>>> On 03/05/18 04:51, JeffyChen wrote:
> >>>>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
> >>> [snip]
> >>>>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
> >>>>>> shows colorful static noise.
> >>>>>>
> >>>>> we've added a shutdown() to the iommu driver:
> >>>>> https://patchwork.kernel.org/patch/10230817/
> >>>>>
> >>>>> any chance related?
> >>>>
> >>>> For sure - 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. Between the RK3399 memory
> >>>> map and the way the IOVA allocator works, that probably means it's reading
> >>>> from all over the peripherals region, which, er, isn't ideal.
> >>>
> >>> Hi,
> >>> just wondering if there has been any progress on that front?
> >>>
> >>
> >> maybe we can add .shutdown() to rockchip_drm_drv too? (maybe just call
> >> drm_atomic_helper_shutdown())
> >>
> >> could you help to try this hack:
> >>
> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> >> @@ -451,6 +451,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_remove,
> >>
> >
> > Is there any mechanism guaranteeing the ordering of shutdown between VOP
> > and IOMMU?
>
> it seems like the device_shutdown() will walk the devices_kset->list
> backward.
>
> and the devices_kset->list's order is based on the probe
> order(drivers/base/dd.c -> really_probe() -> devices_kset_move_last())
>
> and we are using of_iommu for rockchip-iommu, which would make sure
> iommu probed before iommu master, so the vop iommu would be
> shutdown after vop

Rather than shutting down the IOMMU, shouldn't we shut down all the
respective master? The latter would automatically imply shutting down
the IOMMUs, so we could remove the shutdown callback from the IOMMU
driver.

Best regards,
Tomasz

Re: [regression] HDMI breakage just before poweroff

[Marc Zyngier]

On Tue, 12 Jun 2018 11:30:49 +0100,
Tomasz Figa <tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> 
> On Tue, Jun 12, 2018 at 7:27 PM JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> >
> > Hi Marc,
> >
> > On 06/12/2018 04:54 PM, Marc Zyngier wrote:
> > > On 12/06/18 04:52, JeffyChen wrote:
> > >> Hi Vicente,
> > >>
> > >> On 06/12/2018 06:04 AM, Vicente Bergas wrote:
> > >>> On Thu, May 3, 2018 at 2:14 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> > >>>> On 03/05/18 04:51, JeffyChen wrote:
> > >>>>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
> > >>> [snip]
> > >>>>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
> > >>>>>> shows colorful static noise.
> > >>>>>>
> > >>>>> we've added a shutdown() to the iommu driver:
> > >>>>> https://patchwork.kernel.org/patch/10230817/
> > >>>>>
> > >>>>> any chance related?
> > >>>>
> > >>>> For sure - 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. Between the RK3399 memory
> > >>>> map and the way the IOVA allocator works, that probably means it's reading
> > >>>> from all over the peripherals region, which, er, isn't ideal.
> > >>>
> > >>> Hi,
> > >>> just wondering if there has been any progress on that front?
> > >>>
> > >>
> > >> maybe we can add .shutdown() to rockchip_drm_drv too? (maybe just call
> > >> drm_atomic_helper_shutdown())
> > >>
> > >> could you help to try this hack:
> > >>
> > >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > >> @@ -451,6 +451,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_remove,
> > >>
> > >
> > > Is there any mechanism guaranteeing the ordering of shutdown between VOP
> > > and IOMMU?
> >
> > it seems like the device_shutdown() will walk the devices_kset->list
> > backward.
> >
> > and the devices_kset->list's order is based on the probe
> > order(drivers/base/dd.c -> really_probe() -> devices_kset_move_last())
> >
> > and we are using of_iommu for rockchip-iommu, which would make sure
> > iommu probed before iommu master, so the vop iommu would be
> > shutdown after vop
> 
> Rather than shutting down the IOMMU, shouldn't we shut down all the
> respective master? The latter would automatically imply shutting down
> the IOMMUs, so we could remove the shutdown callback from the IOMMU
> driver.

As long as you can definitely ensure that you cannot have a active
IOMMU by the time you hit reboot/halt/kexec, that should work. But
experience seems to indicate that this is not a universal truth.

I'd be more confident if we had some form of fallback that would work
in the kexec/kdump use case.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [regression] HDMI breakage just before poweroff

[Vicente Bergas]

On Tue, Jun 12, 2018 at 1:02 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On Tue, 12 Jun 2018 11:30:49 +0100,
> Tomasz Figa <tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>
>> On Tue, Jun 12, 2018 at 7:27 PM JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>> >
>> > Hi Marc,
>> >
>> > On 06/12/2018 04:54 PM, Marc Zyngier wrote:
>> > > On 12/06/18 04:52, JeffyChen wrote:
>> > >> Hi Vicente,
>> > >>
>> > >> On 06/12/2018 06:04 AM, Vicente Bergas wrote:
>> > >>> On Thu, May 3, 2018 at 2:14 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
>> > >>>> On 03/05/18 04:51, JeffyChen wrote:
>> > >>>>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
>> > >>> [snip]
>> > >>>>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
>> > >>>>>> shows colorful static noise.
>> > >>>>>>
>> > >>>>> we've added a shutdown() to the iommu driver:
>> > >>>>> https://patchwork.kernel.org/patch/10230817/
>> > >>>>>
>> > >>>>> any chance related?
>> > >>>>
>> > >>>> For sure - 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. Between the RK3399 memory
>> > >>>> map and the way the IOVA allocator works, that probably means it's reading
>> > >>>> from all over the peripherals region, which, er, isn't ideal.
>> > >>>
>> > >>> Hi,
>> > >>> just wondering if there has been any progress on that front?
>> > >>>
>> > >>
>> > >> maybe we can add .shutdown() to rockchip_drm_drv too? (maybe just call
>> > >> drm_atomic_helper_shutdown())
>> > >>
>> > >> could you help to try this hack:
>> > >>
>> > >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> > >> @@ -451,6 +451,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_remove,
>> > >>
>> > >
>> > > Is there any mechanism guaranteeing the ordering of shutdown between VOP
>> > > and IOMMU?
>> >
>> > it seems like the device_shutdown() will walk the devices_kset->list
>> > backward.
>> >
>> > and the devices_kset->list's order is based on the probe
>> > order(drivers/base/dd.c -> really_probe() -> devices_kset_move_last())
>> >
>> > and we are using of_iommu for rockchip-iommu, which would make sure
>> > iommu probed before iommu master, so the vop iommu would be
>> > shutdown after vop
>>
>> Rather than shutting down the IOMMU, shouldn't we shut down all the
>> respective master? The latter would automatically imply shutting down
>> the IOMMUs, so we could remove the shutdown callback from the IOMMU
>> driver.
>
> As long as you can definitely ensure that you cannot have a active
> IOMMU by the time you hit reboot/halt/kexec, that should work. But
> experience seems to indicate that this is not a universal truth.
>
> I'd be more confident if we had some form of fallback that would work
> in the kexec/kdump use case.
>
> Thanks,
>
>         M.
>
> --
> Jazz is not dead, it just smell funny.

Hi,
just tested it.
There was an issue because of 'incompatible pointer type',
see the proper patch at the bottom.

It seems to do what it is expected to do, that is, it shuts down the display.
I am not sure that this is what is wanted.
When the system is in the halted state, should not it show the last messages?
For this, the display needs to be operational.

Regards,
  Vicente.

--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -451,6 +451,7 @@
 static struct platform_driver rockchip_drm_platform_driver = {
     .probe = rockchip_drm_platform_probe,
     .remove = rockchip_drm_platform_remove,
+    .shutdown = (void (*)(struct platform_device
*))rockchip_drm_platform_remove,
     .driver = {
         .name = "rockchip-drm",
         .of_match_table = rockchip_drm_dt_ids,

Re: [regression] HDMI breakage just before poweroff

[Marc Zyngier]

On 13/06/18 08:15, Vicente Bergas wrote:
> On Tue, Jun 12, 2018 at 1:02 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>> On Tue, 12 Jun 2018 11:30:49 +0100,
>> Tomasz Figa <tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>>
>>> On Tue, Jun 12, 2018 at 7:27 PM JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>>>>
>>>> Hi Marc,
>>>>
>>>> On 06/12/2018 04:54 PM, Marc Zyngier wrote:
>>>>> On 12/06/18 04:52, JeffyChen wrote:
>>>>>> Hi Vicente,
>>>>>>
>>>>>> On 06/12/2018 06:04 AM, Vicente Bergas wrote:
>>>>>>> On Thu, May 3, 2018 at 2:14 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
>>>>>>>> On 03/05/18 04:51, JeffyChen wrote:
>>>>>>>>> On 05/03/2018 03:36 AM, Vicente Bergas wrote:
>>>>>>> [snip]
>>>>>>>>>> With 4.17.0-rc3, when reaching the halted state, the HDMI console
>>>>>>>>>> shows colorful static noise.
>>>>>>>>>>
>>>>>>>>> we've added a shutdown() to the iommu driver:
>>>>>>>>> https://patchwork.kernel.org/patch/10230817/
>>>>>>>>>
>>>>>>>>> any chance related?
>>>>>>>>
>>>>>>>> For sure - 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. Between the RK3399 memory
>>>>>>>> map and the way the IOVA allocator works, that probably means it's reading
>>>>>>>> from all over the peripherals region, which, er, isn't ideal.
>>>>>>>
>>>>>>> Hi,
>>>>>>> just wondering if there has been any progress on that front?
>>>>>>>
>>>>>>
>>>>>> maybe we can add .shutdown() to rockchip_drm_drv too? (maybe just call
>>>>>> drm_atomic_helper_shutdown())
>>>>>>
>>>>>> could you help to try this hack:
>>>>>>
>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>>>>> @@ -451,6 +451,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_remove,
>>>>>>
>>>>>
>>>>> Is there any mechanism guaranteeing the ordering of shutdown between VOP
>>>>> and IOMMU?
>>>>
>>>> it seems like the device_shutdown() will walk the devices_kset->list
>>>> backward.
>>>>
>>>> and the devices_kset->list's order is based on the probe
>>>> order(drivers/base/dd.c -> really_probe() -> devices_kset_move_last())
>>>>
>>>> and we are using of_iommu for rockchip-iommu, which would make sure
>>>> iommu probed before iommu master, so the vop iommu would be
>>>> shutdown after vop
>>>
>>> Rather than shutting down the IOMMU, shouldn't we shut down all the
>>> respective master? The latter would automatically imply shutting down
>>> the IOMMUs, so we could remove the shutdown callback from the IOMMU
>>> driver.
>>
>> As long as you can definitely ensure that you cannot have a active
>> IOMMU by the time you hit reboot/halt/kexec, that should work. But
>> experience seems to indicate that this is not a universal truth.
>>
>> I'd be more confident if we had some form of fallback that would work
>> in the kexec/kdump use case.
>>
>> Thanks,
>>
>>         M.
>>
>> --
>> Jazz is not dead, it just smell funny.
> 
> Hi,
> just tested it.
> There was an issue because of 'incompatible pointer type',
> see the proper patch at the bottom.
> 
> It seems to do what it is expected to do, that is, it shuts down the display.
> I am not sure that this is what is wanted.
> When the system is in the halted state, should not it show the last messages?
> For this, the display needs to be operational.

Well, we can't have everything, unfortunately. We definitely need to
stop the IOMMU in order to kexec safely, which is an important use-case.

A possibility would be to switch to a directly mapped buffer on
shutdown, but I'm not sure how we can enforce that at this stage.

> 
> Regards,
>   Vicente.
> 
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -451,6 +451,7 @@
>  static struct platform_driver rockchip_drm_platform_driver = {
>      .probe = rockchip_drm_platform_probe,
>      .remove = rockchip_drm_platform_remove,
> +    .shutdown = (void (*)(struct platform_device
> *))rockchip_drm_platform_remove,

No, please... :-( Provide a wrapper that returns void instead.

>      .driver = {
>          .name = "rockchip-drm",
>          .of_match_table = rockchip_drm_dt_ids,
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [regression] HDMI breakage just before poweroff

[JeffyChen]

Hi Marc & Vicente,

On 06/13/2018 06:26 PM, Marc Zyngier wrote:
>> >
>> >--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> >+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>> >@@ -451,6 +451,7 @@
>> >  static struct platform_driver rockchip_drm_platform_driver = {
>> >      .probe = rockchip_drm_platform_probe,
>> >      .remove = rockchip_drm_platform_remove,
>> >+    .shutdown = (void (*)(struct platform_device
>> >*))rockchip_drm_platform_remove,
> No, please...:-(  Provide a wrapper that returns void instead.

hmmm, this is just a test only hack, i am thinking maybe 
drm_atomic_helper_shutdown is enough for shutdown() ?


something like:
+++ 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);
+}
+

>

Re: [regression] HDMI breakage just before poweroff

[Vicente Bergas]

On Wed, Jun 13, 2018 at 12:46 PM, JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> Hi Marc & Vicente,
>
> On 06/13/2018 06:26 PM, Marc Zyngier wrote:
>>>
>>> >
>>> >--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>> >+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>> >@@ -451,6 +451,7 @@
>>> >  static struct platform_driver rockchip_drm_platform_driver = {
>>> >      .probe = rockchip_drm_platform_probe,
>>> >      .remove = rockchip_drm_platform_remove,
>>> >+    .shutdown = (void (*)(struct platform_device
>>> >*))rockchip_drm_platform_remove,
>>
>> No, please...:-(  Provide a wrapper that returns void instead.
>
>
> hmmm, this is just a test only hack, i am thinking maybe
> drm_atomic_helper_shutdown is enough for shutdown() ?
>
>
> something like:
> +++ 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);
> +}
> +
>
Hi,
I can confirm that rockchip_drm_platform_shutdown works as fine as
rockchip_drm_platform_remove.
Just a note: The first time I noticed this error was because it
appeared on display, now that the display is off, are we sure the
error is fixed?
Also, could there be other peripherals that need to be powered off
before the iommu?
For curiosity: why is the iommu disabled before kexec instead of
resetting it after kexec?

Regards,
  Vicente.

Re: [regression] HDMI breakage just before poweroff

[Marc Zyngier]

On 15/06/18 17:39, Vicente Bergas wrote:
> On Wed, Jun 13, 2018 at 12:46 PM, JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>> Hi Marc & Vicente,
>>
>> On 06/13/2018 06:26 PM, Marc Zyngier wrote:
>>>>
>>>>>
>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>>>> @@ -451,6 +451,7 @@
>>>>>  static struct platform_driver rockchip_drm_platform_driver = {
>>>>>      .probe = rockchip_drm_platform_probe,
>>>>>      .remove = rockchip_drm_platform_remove,
>>>>> +    .shutdown = (void (*)(struct platform_device
>>>>> *))rockchip_drm_platform_remove,
>>>
>>> No, please...:-(  Provide a wrapper that returns void instead.
>>
>>
>> hmmm, this is just a test only hack, i am thinking maybe
>> drm_atomic_helper_shutdown is enough for shutdown() ?
>>
>>
>> something like:
>> +++ 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);
>> +}
>> +
>>
> Hi,
> I can confirm that rockchip_drm_platform_shutdown works as fine as
> rockchip_drm_platform_remove.
> Just a note: The first time I noticed this error was because it
> appeared on display, now that the display is off, are we sure the
> error is fixed?
> Also, could there be other peripherals that need to be powered off
> before the iommu?
> For curiosity: why is the iommu disabled before kexec instead of
> resetting it after kexec?

Because by the time you're in the new kernel, you've corrupted the page
tables, accessed peripheral address space, and set the system on fire.

In general, having a DMA master active across something like kexec is a
pretty fatal issue. And when your boot loader is a Linux kernel using
kexec, you're really insisting that kexec works correctly.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [regression] HDMI breakage just before poweroff

[Vicente Bergas]

On Fri, Jun 15, 2018 at 6:51 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On 15/06/18 17:39, Vicente Bergas wrote:
>> On Wed, Jun 13, 2018 at 12:46 PM, JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>>> Hi Marc & Vicente,
>>>
>>> On 06/13/2018 06:26 PM, Marc Zyngier wrote:
>>>>>
>>>>>>
>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>>>>> @@ -451,6 +451,7 @@
>>>>>>  static struct platform_driver rockchip_drm_platform_driver = {
>>>>>>      .probe = rockchip_drm_platform_probe,
>>>>>>      .remove = rockchip_drm_platform_remove,
>>>>>> +    .shutdown = (void (*)(struct platform_device
>>>>>> *))rockchip_drm_platform_remove,
>>>>
>>>> No, please...:-(  Provide a wrapper that returns void instead.
>>>
>>>
>>> hmmm, this is just a test only hack, i am thinking maybe
>>> drm_atomic_helper_shutdown is enough for shutdown() ?
>>>
>>>
>>> something like:
>>> +++ 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);
>>> +}
>>> +
>>>
>> Hi,
>> I can confirm that rockchip_drm_platform_shutdown works as fine as
>> rockchip_drm_platform_remove.
>> Just a note: The first time I noticed this error was because it
>> appeared on display, now that the display is off, are we sure the
>> error is fixed?
>> Also, could there be other peripherals that need to be powered off
>> before the iommu?
>> For curiosity: why is the iommu disabled before kexec instead of
>> resetting it after kexec?
>
> Because by the time you're in the new kernel, you've corrupted the page
> tables, accessed peripheral address space, and set the system on fire.
>
> In general, having a DMA master active across something like kexec is a
> pretty fatal issue. And when your boot loader is a Linux kernel using
> kexec, you're really insisting that kexec works correctly.

Is it feasible to place the display memory at a virtual address that
matches the physical address?

Re: [regression] HDMI breakage just before poweroff

[Marc Zyngier]

On Fri, 15 Jun 2018 19:46:50 +0100,
Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Fri, Jun 15, 2018 at 6:51 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> > On 15/06/18 17:39, Vicente Bergas wrote:
> >> On Wed, Jun 13, 2018 at 12:46 PM, JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> >>> Hi Marc & Vicente,
> >>>
> >>> On 06/13/2018 06:26 PM, Marc Zyngier wrote:

[...]

> >> I can confirm that rockchip_drm_platform_shutdown works as fine as
> >> rockchip_drm_platform_remove.
> >> Just a note: The first time I noticed this error was because it
> >> appeared on display, now that the display is off, are we sure the
> >> error is fixed?
> >> Also, could there be other peripherals that need to be powered off
> >> before the iommu?
> >> For curiosity: why is the iommu disabled before kexec instead of
> >> resetting it after kexec?
> >
> > Because by the time you're in the new kernel, you've corrupted the page
> > tables, accessed peripheral address space, and set the system on fire.
> >
> > In general, having a DMA master active across something like kexec is a
> > pretty fatal issue. And when your boot loader is a Linux kernel using
> > kexec, you're really insisting that kexec works correctly.
> 
> Is it feasible to place the display memory at a virtual address that
> matches the physical address?

That depends on the addressing capability of the end-point, and on the
kernel ability to allocate a large, contiguous range of physical
memory. I'm not sure it is worth the hassle just to get the display
working all the way through shutdown (knowing that the most likely
event after that is a power off).

	M.

-- 
Jazz is not dead, it just smell funny.

[PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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);
+}
+
 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,
-- 
2.18.0

Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Marc Zyngier]

Hi Vicente,

On Sun,  5 Aug 2018 16:09:11 +0200
Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Vicente Bergas]

On Sun, Aug 5, 2018 at 6:50 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Vicente,
>
> On Sun,  5 Aug 2018 16:09:11 +0200
> Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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.

Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Marc Zyngier]

On Sun, 05 Aug 2018 18:38:18 +0100,
Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Sun, Aug 5, 2018 at 6:50 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> > Hi Vicente,
> >
> > On Sun,  5 Aug 2018 16:09:11 +0200
> > Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> >> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> >> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  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.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Heiko Stuebner]

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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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

Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Vicente Bergas]

Hi Heiko, Jeffy, Marc,

On Tue, Aug 7, 2018 at 2:44 PM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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
>
>

Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Heiko Stuebner]

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 <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> >> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> >> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  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

Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Vicente Bergas]

On Tue, Aug 7, 2018 at 6:07 PM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> >> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> >> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> ---
>> >>  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 <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
tag.
As said, i only tested that on shutdown, the hdmi output
is also shut down instead of showing random noise.

Regards,
  Vicente.

Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Marc Zyngier]

On Tue, 7 Aug 2018 18:20:15 +0200
Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Aug 7, 2018 at 6:07 PM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> >> >> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> >> >> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> >> ---
> >> >>  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 <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 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.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Heiko Stuebner]

Am Sonntag, 9. September 2018, 15:43:00 CEST schrieb Marc Zyngier:
> On Tue, 7 Aug 2018 18:20:15 +0200
> Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > On Tue, Aug 7, 2018 at 6:07 PM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> > >> >> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> > >> >> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > >> >> ---
> > >> >>  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 <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 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

Re: [PATCH] drm/rockchip: shutdown drm subsystem on shutdown

[Marc Zyngier]

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 <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> On Tue, Aug 7, 2018 at 6:07 PM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>>>> Suggested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>>>>>>> Signed-off-by: Vicente Bergas <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>>> ---
>>>>>>>  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 <vicencb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> 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.
-- 
Jazz is not dead. It just smells funny...