[07/11] drm/vc4: hdmi: Disable connector status polling during suspend

Suspend of VC4 HDMI will likely triggers a warning from
vc4_hdmi_connector_detect_ctx() during poll of connector status.
The power management will prevent the resume and keep the relevant
power domain disabled.

Since there is no reason to poll the connector status during
suspend, the polling should be disabled during this.

It not possible to use drm_mode_config_helper_suspend() here,
because the callbacks might be called during bind phase and not all
components are fully initialized.

Link: https://lore.kernel.org/dri-devel/7003512d-7303-4f41-b0d6-a8af5bf8e497@gmx.net/
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

--
2.34.1

Hi,

On Sun, Jun 30, 2024 at 05:36:48PM GMT, Stefan Wahren wrote:
> Suspend of VC4 HDMI will likely triggers a warning from
> vc4_hdmi_connector_detect_ctx() during poll of connector status.
> The power management will prevent the resume and keep the relevant
> power domain disabled.
> 
> Since there is no reason to poll the connector status during
> suspend, the polling should be disabled during this.
> 
> It not possible to use drm_mode_config_helper_suspend() here,
> because the callbacks might be called during bind phase and not all
> components are fully initialized.
> 
> Link: https://lore.kernel.org/dri-devel/7003512d-7303-4f41-b0d6-a8af5bf8e497@gmx.net/
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index b3a42b709718..e80495cea6ac 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct drm_device *drm,
>  static int vc4_hdmi_runtime_suspend(struct device *dev)
>  {
>  	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> +	struct drm_device *drm = vc4_hdmi->connector.dev;
> +
> +	/*
> +	 * Don't disable polling if it was never initialized
> +	 */
> +	if (drm && drm->mode_config.poll_enabled)
> +		drm_kms_helper_poll_disable(drm);

Does it make sense to add it to runtime_suspend?

What if the board boots without a display connected, and only after a
while one is connected? Wouldn't that prevent the driver from detecting
it?

Maxime

Hi Stefan,

> Suspend of VC4 HDMI will likely triggers a warning from
> vc4_hdmi_connector_detect_ctx() during poll of connector status.
> The power management will prevent the resume and keep the relevant
> power domain disabled.
>
> Since there is no reason to poll the connector status during
> suspend, the polling should be disabled during this.

What about HDMI-CEC? I don't know well enough how CEC integrates at
this level but CEC can wake up the device over HDMI from a TV display
for example so if this affects that, while it's maybe not required for
first pass, I know the rpi is used in a lot of media use cases so the
ability to wake up via CEC would certainly be welcomed.

> It not possible to use drm_mode_config_helper_suspend() here,
> because the callbacks might be called during bind phase and not all
> components are fully initialized.
>
> Link: https://lore.kernel.org/dri-devel/7003512d-7303-4f41-b0d6-a8af5bf8e497@gmx.net/
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index b3a42b709718..e80495cea6ac 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct drm_device *drm,
>  static int vc4_hdmi_runtime_suspend(struct device *dev)
>  {
>         struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> +       struct drm_device *drm = vc4_hdmi->connector.dev;
> +
> +       /*
> +        * Don't disable polling if it was never initialized
> +        */
> +       if (drm && drm->mode_config.poll_enabled)
> +               drm_kms_helper_poll_disable(drm);
>
>         clk_disable_unprepare(vc4_hdmi->hsm_clock);
>
> @@ -3115,6 +3122,7 @@ static int vc4_hdmi_runtime_suspend(struct device *dev)
>  static int vc4_hdmi_runtime_resume(struct device *dev)
>  {
>         struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> +       struct drm_device *drm = vc4_hdmi->connector.dev;
>         unsigned long __maybe_unused flags;
>         u32 __maybe_unused value;
>         unsigned long rate;
> @@ -3159,6 +3167,9 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
>         }
>  #endif
>
> +       if (drm && drm->mode_config.poll_enabled)
> +               drm_kms_helper_poll_enable(drm);
> +
>         return 0;
>
>  err_disable_clk:
> --
> 2.34.1
>

Am 03.07.24 um 12:28 schrieb Stefan Wahren:
> Hi Maxime,
>
> Am 02.07.24 um 15:48 schrieb Maxime Ripard:
>> Hi,
>>
>> On Sun, Jun 30, 2024 at 05:36:48PM GMT, Stefan Wahren wrote:
>>> Suspend of VC4 HDMI will likely triggers a warning from
>>> vc4_hdmi_connector_detect_ctx() during poll of connector status.
>>> The power management will prevent the resume and keep the relevant
>>> power domain disabled.
>>>
>>> Since there is no reason to poll the connector status during
>>> suspend, the polling should be disabled during this.
>>>
>>> It not possible to use drm_mode_config_helper_suspend() here,
>>> because the callbacks might be called during bind phase and not all
>>> components are fully initialized.
>>>
>>> Link:
>>> https://lore.kernel.org/dri-devel/7003512d-7303-4f41-b0d6-a8af5bf8e497@gmx.net/
>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>>> ---
>>>   drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> index b3a42b709718..e80495cea6ac 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct
>>> drm_device *drm,
>>>   static int vc4_hdmi_runtime_suspend(struct device *dev)
>>>   {
>>>       struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
>>> +    struct drm_device *drm = vc4_hdmi->connector.dev;
>>> +
>>> +    /*
>>> +     * Don't disable polling if it was never initialized
>>> +     */
>>> +    if (drm && drm->mode_config.poll_enabled)
>>> +        drm_kms_helper_poll_disable(drm);
>> Does it make sense to add it to runtime_suspend?
> i saw that other drm drivers used drm_mode_config_helper_suspend() in
> the RUNTIME_PM_OPS. But i agree, it should be better moved to
> SYSTEM_SLEEP_PM_OPS.
>> What if the board boots without a display connected, and only after a
>> while one is connected? Wouldn't that prevent the driver from detecting
>> it?
> tbh I noticed that HDMI re-detection didn't worked in my setup
> 6.10-rcX before this series. I need to investigate ...
Okay, this patch breaks definitely HDMI re-detection. So please ignore
this patch. Sorry, about this mess.

There is another minor issue which already exists before that cause the
following log entry on HDMI reconnect:

[   74.078745] vc4-drm soc:gpu: [drm] User-defined mode not supported:
"1920x1200": 60 154000 1920 1968 2000 2080 1200 1203 1209 1235 0x68 0x9

But in this case HDMI works.

Regards
>>
>> Maxime
>

Hi Stefan

On Wed, 3 Jul 2024 at 16:32, Stefan Wahren <wahrenst@gmx.net> wrote:
>
> Am 03.07.24 um 12:28 schrieb Stefan Wahren:
> > Hi Maxime,
> >
> > Am 02.07.24 um 15:48 schrieb Maxime Ripard:
> >> Hi,
> >>
> >> On Sun, Jun 30, 2024 at 05:36:48PM GMT, Stefan Wahren wrote:
> >>> Suspend of VC4 HDMI will likely triggers a warning from
> >>> vc4_hdmi_connector_detect_ctx() during poll of connector status.
> >>> The power management will prevent the resume and keep the relevant
> >>> power domain disabled.
> >>>
> >>> Since there is no reason to poll the connector status during
> >>> suspend, the polling should be disabled during this.
> >>>
> >>> It not possible to use drm_mode_config_helper_suspend() here,
> >>> because the callbacks might be called during bind phase and not all
> >>> components are fully initialized.
> >>>
> >>> Link:
> >>> https://lore.kernel.org/dri-devel/7003512d-7303-4f41-b0d6-a8af5bf8e497@gmx.net/
> >>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> >>> ---
> >>>   drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++++++++++
> >>>   1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c
> >>> b/drivers/gpu/drm/vc4/vc4_hdmi.c
> >>> index b3a42b709718..e80495cea6ac 100644
> >>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> >>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> >>> @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct
> >>> drm_device *drm,
> >>>   static int vc4_hdmi_runtime_suspend(struct device *dev)
> >>>   {
> >>>       struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> >>> +    struct drm_device *drm = vc4_hdmi->connector.dev;
> >>> +
> >>> +    /*
> >>> +     * Don't disable polling if it was never initialized
> >>> +     */
> >>> +    if (drm && drm->mode_config.poll_enabled)
> >>> +        drm_kms_helper_poll_disable(drm);
> >> Does it make sense to add it to runtime_suspend?
> > i saw that other drm drivers used drm_mode_config_helper_suspend() in
> > the RUNTIME_PM_OPS. But i agree, it should be better moved to
> > SYSTEM_SLEEP_PM_OPS.
> >> What if the board boots without a display connected, and only after a
> >> while one is connected? Wouldn't that prevent the driver from detecting
> >> it?
> > tbh I noticed that HDMI re-detection didn't worked in my setup
> > 6.10-rcX before this series. I need to investigate ...
> Okay, this patch breaks definitely HDMI re-detection. So please ignore
> this patch. Sorry, about this mess.
>
> There is another minor issue which already exists before that cause the
> following log entry on HDMI reconnect:
>
> [   74.078745] vc4-drm soc:gpu: [drm] User-defined mode not supported:
> "1920x1200": 60 154000 1920 1968 2000 2080 1200 1203 1209 1235 0x68 0x9
>
> But in this case HDMI works.

That's saying the mode specified on the kernel command line via
"video=" is invalid. All other modes enumerated from the EDID should
still be present.

I don't immediately see anything wrong with the mode - it's just DMT
mode 0x44 aka 1920x1200@60Hz with reduced blanking. 154MHz clock is
less than the 162MHz limit for Pi0-3 so should be supported.
Setting the module parameter drm.debug to 0x4 should give you the
extra debug of the reason the mode was rejected via the log message at
[1].

At a guess the firmware has inserted the video= line based on the mode
it configured. Whilst that was useful, we've moved away from that now
by setting "disable_fw_kms_setup=1" in config.txt.

  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_modes.c#L1815-L1816

> Regards
> >>
> >> Maxime
> >
>

Hi Peter,

Am 02.07.24 um 22:02 schrieb Peter Robinson:
> Hi Stefan,
>
>> Suspend of VC4 HDMI will likely triggers a warning from
>> vc4_hdmi_connector_detect_ctx() during poll of connector status.
>> The power management will prevent the resume and keep the relevant
>> power domain disabled.
>>
>> Since there is no reason to poll the connector status during
>> suspend, the polling should be disabled during this.
> What about HDMI-CEC? I don't know well enough how CEC integrates at
> this level but CEC can wake up the device over HDMI from a TV display
> for example so if this affects that, while it's maybe not required for
> first pass, I know the rpi is used in a lot of media use cases so the
> ability to wake up via CEC would certainly be welcomed.
unfortunately i don't know much about HDMI-CEC, too. The only thing I
noticed was that I currently cannot configure vc4 as a wakeup source.