diff mbox series

[v6,07/14] drm/bridge: analogix_dp: Add support to get panel from the DP AUX bus

Message ID 20250123100747.1841357-8-damon.ding@rock-chips.com (mailing list archive)
State New
Headers show
Series Add eDP support for RK3588 | expand

Commit Message

Damon Ding Jan. 23, 2025, 10:07 a.m. UTC 
The main modification is moving the DP AUX initialization from function
analogix_dp_bind() to analogix_dp_probe(). In order to get the EDID of
eDP panel during probing, it is also needed to advance PM operaions to
ensure that eDP controller and phy are prepared for AUX transmission.

In addtion, add a new function analogix_dp_remove() to ensure symmetry
for PM operations.

Signed-off-by: Damon Ding <damon.ding@rock-chips.com>

---

Changes in v4:
- Use done_probing() to call drm_of_find_panel_or_bridge() and
  component_add() when getting panel from the DP AUX bus

Changes in v5:
- Advance PM operations to make eDP AUX work well

Changes in v6:
- Use devm_pm_runtime_enable() instead of devm_add_action_or_reset()
- Add a new function analogix_dp_remove() to ensure symmetry for PM
  operations
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 57 ++++++++++---------
 .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  4 ++
 include/drm/bridge/analogix_dp.h              |  1 +
 3 files changed, 34 insertions(+), 28 deletions(-)

Comments

Dmitry Baryshkov Jan. 23, 2025, 11:25 a.m. UTC | #1 
On Thu, Jan 23, 2025 at 06:07:40PM +0800, Damon Ding wrote:
> The main modification is moving the DP AUX initialization from function
> analogix_dp_bind() to analogix_dp_probe(). In order to get the EDID of
> eDP panel during probing, it is also needed to advance PM operaions to
> ensure that eDP controller and phy are prepared for AUX transmission.

This doesn't sound right. Per the documentation of
drm_dp_aux::transfer(), the device should power itself up if transfer()
is called when it is powered off. The caller must only ensure that the
panel is on.

Doug, what's your opinion?

> 
> In addtion, add a new function analogix_dp_remove() to ensure symmetry
> for PM operations.
> 
> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
> 
> ---
> 
> Changes in v4:
> - Use done_probing() to call drm_of_find_panel_or_bridge() and
>   component_add() when getting panel from the DP AUX bus
> 
> Changes in v5:
> - Advance PM operations to make eDP AUX work well
> 
> Changes in v6:
> - Use devm_pm_runtime_enable() instead of devm_add_action_or_reset()
> - Add a new function analogix_dp_remove() to ensure symmetry for PM
>   operations
> ---
>  .../drm/bridge/analogix/analogix_dp_core.c    | 57 ++++++++++---------
>  .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  4 ++
>  include/drm/bridge/analogix_dp.h              |  1 +
>  3 files changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 8251adfce2f9..30da8a14361e 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1658,14 +1658,42 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
>  	}
>  	disable_irq(dp->irq);
>  
> +	dp->aux.name = "DP-AUX";
> +	dp->aux.transfer = analogix_dpaux_transfer;
> +	dp->aux.dev = dp->dev;
> +	drm_dp_aux_init(&dp->aux);
> +
> +	if (IS_ENABLED(CONFIG_PM)) {
> +		pm_runtime_use_autosuspend(dp->dev);
> +		pm_runtime_set_autosuspend_delay(dp->dev, 100);
> +		ret = devm_pm_runtime_enable(dp->dev);
> +		if (ret)
> +			goto err_disable_pm_runtime;
> +	} else {
> +		ret = analogix_dp_resume(dp);
> +		if (ret)
> +			goto err_disable_clk;
> +	}
> +
>  	return dp;
>  
> +err_disable_pm_runtime:
> +	pm_runtime_dont_use_autosuspend(dp->dev);
>  err_disable_clk:
>  	clk_disable_unprepare(dp->clock);
>  	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL_GPL(analogix_dp_probe);
>  
> +void analogix_dp_remove(struct analogix_dp_device *dp)
> +{
> +	if (IS_ENABLED(CONFIG_PM))
> +		pm_runtime_dont_use_autosuspend(dp->dev);
> +	else
> +		analogix_dp_suspend(dp);
> +}
> +EXPORT_SYMBOL_GPL(analogix_dp_remove);
> +
>  int analogix_dp_suspend(struct analogix_dp_device *dp)
>  {
>  	phy_power_off(dp->phy);
> @@ -1708,25 +1736,12 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>  	dp->drm_dev = drm_dev;
>  	dp->encoder = dp->plat_data->encoder;
>  
> -	if (IS_ENABLED(CONFIG_PM)) {
> -		pm_runtime_use_autosuspend(dp->dev);
> -		pm_runtime_set_autosuspend_delay(dp->dev, 100);
> -		pm_runtime_enable(dp->dev);
> -	} else {
> -		ret = analogix_dp_resume(dp);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	dp->aux.name = "DP-AUX";
> -	dp->aux.transfer = analogix_dpaux_transfer;
> -	dp->aux.dev = dp->dev;
>  	dp->aux.drm_dev = drm_dev;
>  
>  	ret = drm_dp_aux_register(&dp->aux);
>  	if (ret) {
>  		DRM_ERROR("failed to register AUX (%d)\n", ret);
> -		goto err_disable_pm_runtime;
> +		return ret;
>  	}
>  
>  	ret = analogix_dp_create_bridge(drm_dev, dp);
> @@ -1739,13 +1754,6 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>  
>  err_unregister_aux:
>  	drm_dp_aux_unregister(&dp->aux);
> -err_disable_pm_runtime:
> -	if (IS_ENABLED(CONFIG_PM)) {
> -		pm_runtime_dont_use_autosuspend(dp->dev);
> -		pm_runtime_disable(dp->dev);
> -	} else {
> -		analogix_dp_suspend(dp);
> -	}
>  
>  	return ret;
>  }
> @@ -1762,13 +1770,6 @@ void analogix_dp_unbind(struct analogix_dp_device *dp)
>  	}
>  
>  	drm_dp_aux_unregister(&dp->aux);
> -
> -	if (IS_ENABLED(CONFIG_PM)) {
> -		pm_runtime_dont_use_autosuspend(dp->dev);
> -		pm_runtime_disable(dp->dev);
> -	} else {
> -		analogix_dp_suspend(dp);
> -	}
>  }
>  EXPORT_SYMBOL_GPL(analogix_dp_unbind);
>  
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index a63c2c867059..13f32aeea7ca 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -459,6 +459,10 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>  
>  static void rockchip_dp_remove(struct platform_device *pdev)
>  {
> +	struct rockchip_dp_device *dp = dev_get_drvdata(&pdev->dev);
> +
> +	analogix_dp_remove(dp->adp);
> +
>  	component_del(&pdev->dev, &rockchip_dp_component_ops);
>  }
>  
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index 1b119b547988..d9b721fd7201 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -43,6 +43,7 @@ int analogix_dp_suspend(struct analogix_dp_device *dp);
>  
>  struct analogix_dp_device *
>  analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data);
> +void analogix_dp_remove(struct analogix_dp_device *dp);
>  int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev);
>  void analogix_dp_unbind(struct analogix_dp_device *dp);
>  
> -- 
> 2.34.1
>
Doug Anderson Jan. 24, 2025, 3:13 a.m. UTC | #2 
Hi,

On Thu, Jan 23, 2025 at 3:25 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, Jan 23, 2025 at 06:07:40PM +0800, Damon Ding wrote:
> > The main modification is moving the DP AUX initialization from function
> > analogix_dp_bind() to analogix_dp_probe(). In order to get the EDID of
> > eDP panel during probing, it is also needed to advance PM operaions to

nit: s/operaions/operations


> > ensure that eDP controller and phy are prepared for AUX transmission.
>
> This doesn't sound right. Per the documentation of
> drm_dp_aux::transfer(), the device should power itself up if transfer()
> is called when it is powered off. The caller must only ensure that the
> panel is on.
>
> Doug, what's your opinion?

I think maybe the CL description is a bit confusing, but looking at
the patch I think that the general idea is correct. drm_dp_aux_init()
is expected to be called in probe() and not in bind(). ...and in order
for it to work then pm_runtime needs to be enabled at probe and not at
bind. So both of those two things that this patch does are (in my
opinion) correct.


> > In addtion, add a new function analogix_dp_remove() to ensure symmetry
> > for PM operations.
> >
> > Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
> >
> > ---
> >
> > Changes in v4:
> > - Use done_probing() to call drm_of_find_panel_or_bridge() and
> >   component_add() when getting panel from the DP AUX bus
> >
> > Changes in v5:
> > - Advance PM operations to make eDP AUX work well
> >
> > Changes in v6:
> > - Use devm_pm_runtime_enable() instead of devm_add_action_or_reset()
> > - Add a new function analogix_dp_remove() to ensure symmetry for PM
> >   operations
> > ---
> >  .../drm/bridge/analogix/analogix_dp_core.c    | 57 ++++++++++---------
> >  .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  4 ++
> >  include/drm/bridge/analogix_dp.h              |  1 +
> >  3 files changed, 34 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > index 8251adfce2f9..30da8a14361e 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > @@ -1658,14 +1658,42 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
> >       }
> >       disable_irq(dp->irq);

not related to your patch, but probably needs to be a prerequisite of
your patch: instead of calling disable_irq() here, you should OR in
"IRQF_NO_AUTOEN" to the "irq_flags" of devm_request_threaded_irq().
That not only closes a potential race condition but also makes all the
error handling much more robust.


> > +     dp->aux.name = "DP-AUX";
> > +     dp->aux.transfer = analogix_dpaux_transfer;
> > +     dp->aux.dev = dp->dev;
> > +     drm_dp_aux_init(&dp->aux);

FWIW: I would highly encourage you to (in a separate patch) add
wait_hpd_asserted() support here. It is deprecated to not implement
wait_hpd_asserted(). See the definition of "wait_hpd_asserted" in
"struct drm_dp_aux" if you're going to support eDP panels.


> > +     if (IS_ENABLED(CONFIG_PM)) {

Do we really truly need this? Is there anyone actually using this
driver without "CONFIG_PM", or can we just assume CONFIG_PM. For the
most part drivers I've interacted with just assume CONFIG_PM and
they're a lot simpler because of it. If there's truly a need then we
can keep this complexity, but I'd rather wait until there is a user.
Maybe you could add this as a dependency in the Kconfig if needed.


> > +             pm_runtime_use_autosuspend(dp->dev);
> > +             pm_runtime_set_autosuspend_delay(dp->dev, 100);
> > +             ret = devm_pm_runtime_enable(dp->dev);
> > +             if (ret)
> > +                     goto err_disable_pm_runtime;
> > +     } else {
> > +             ret = analogix_dp_resume(dp);
> > +             if (ret)
> > +                     goto err_disable_clk;

IMO: if analogix_dp_resume() succeeds, use devm_add_action_or_reset()
to have a function call analogix_dp_suspend(). Then you can keep using
"devm" for everything and totally get rid of the need for the
analogix_dp_remove() function.


> > +     }
> > +
> >       return dp;
> >
> > +err_disable_pm_runtime:
> > +     pm_runtime_dont_use_autosuspend(dp->dev);

You don't need to call pm_runtime_dont_use_autosuspend(). If you
enabled pm_runtime with devm_pm_runtime_enable() then it's documented
to handle calling pm_runtime_dont_use_autosuspend() for you. See the
kernel doc comment for devm_pm_runtime_enable(). So you can get rid of
this.


> >  err_disable_clk:
> >       clk_disable_unprepare(dp->clock);
> >       return ERR_PTR(ret);

Huh? Why would you call "err_disable_clk" here? The only thing that
enables the clock is analogix_dp_resume(), right? There's something
fishy here and it predates your patch. I suspect there were problems
in commit f37952339cc2 ("drm/bridge: analogix_dp: handle clock via
runtime PM"). You should fix that in a separate patch before yours.


> > +void analogix_dp_remove(struct analogix_dp_device *dp)
> > +{
> > +     if (IS_ENABLED(CONFIG_PM))
> > +             pm_runtime_dont_use_autosuspend(dp->dev);
> > +     else
> > +             analogix_dp_suspend(dp);
> > +}
> > +EXPORT_SYMBOL_GPL(analogix_dp_remove);

See above. Proper use of "devm" should mean you don't need a remove() function.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 8251adfce2f9..30da8a14361e 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1658,14 +1658,42 @@  analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
 	}
 	disable_irq(dp->irq);
 
+	dp->aux.name = "DP-AUX";
+	dp->aux.transfer = analogix_dpaux_transfer;
+	dp->aux.dev = dp->dev;
+	drm_dp_aux_init(&dp->aux);
+
+	if (IS_ENABLED(CONFIG_PM)) {
+		pm_runtime_use_autosuspend(dp->dev);
+		pm_runtime_set_autosuspend_delay(dp->dev, 100);
+		ret = devm_pm_runtime_enable(dp->dev);
+		if (ret)
+			goto err_disable_pm_runtime;
+	} else {
+		ret = analogix_dp_resume(dp);
+		if (ret)
+			goto err_disable_clk;
+	}
+
 	return dp;
 
+err_disable_pm_runtime:
+	pm_runtime_dont_use_autosuspend(dp->dev);
 err_disable_clk:
 	clk_disable_unprepare(dp->clock);
 	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(analogix_dp_probe);
 
+void analogix_dp_remove(struct analogix_dp_device *dp)
+{
+	if (IS_ENABLED(CONFIG_PM))
+		pm_runtime_dont_use_autosuspend(dp->dev);
+	else
+		analogix_dp_suspend(dp);
+}
+EXPORT_SYMBOL_GPL(analogix_dp_remove);
+
 int analogix_dp_suspend(struct analogix_dp_device *dp)
 {
 	phy_power_off(dp->phy);
@@ -1708,25 +1736,12 @@  int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
 	dp->drm_dev = drm_dev;
 	dp->encoder = dp->plat_data->encoder;
 
-	if (IS_ENABLED(CONFIG_PM)) {
-		pm_runtime_use_autosuspend(dp->dev);
-		pm_runtime_set_autosuspend_delay(dp->dev, 100);
-		pm_runtime_enable(dp->dev);
-	} else {
-		ret = analogix_dp_resume(dp);
-		if (ret)
-			return ret;
-	}
-
-	dp->aux.name = "DP-AUX";
-	dp->aux.transfer = analogix_dpaux_transfer;
-	dp->aux.dev = dp->dev;
 	dp->aux.drm_dev = drm_dev;
 
 	ret = drm_dp_aux_register(&dp->aux);
 	if (ret) {
 		DRM_ERROR("failed to register AUX (%d)\n", ret);
-		goto err_disable_pm_runtime;
+		return ret;
 	}
 
 	ret = analogix_dp_create_bridge(drm_dev, dp);
@@ -1739,13 +1754,6 @@  int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
 
 err_unregister_aux:
 	drm_dp_aux_unregister(&dp->aux);
-err_disable_pm_runtime:
-	if (IS_ENABLED(CONFIG_PM)) {
-		pm_runtime_dont_use_autosuspend(dp->dev);
-		pm_runtime_disable(dp->dev);
-	} else {
-		analogix_dp_suspend(dp);
-	}
 
 	return ret;
 }
@@ -1762,13 +1770,6 @@  void analogix_dp_unbind(struct analogix_dp_device *dp)
 	}
 
 	drm_dp_aux_unregister(&dp->aux);
-
-	if (IS_ENABLED(CONFIG_PM)) {
-		pm_runtime_dont_use_autosuspend(dp->dev);
-		pm_runtime_disable(dp->dev);
-	} else {
-		analogix_dp_suspend(dp);
-	}
 }
 EXPORT_SYMBOL_GPL(analogix_dp_unbind);
 
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index a63c2c867059..13f32aeea7ca 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -459,6 +459,10 @@  static int rockchip_dp_probe(struct platform_device *pdev)
 
 static void rockchip_dp_remove(struct platform_device *pdev)
 {
+	struct rockchip_dp_device *dp = dev_get_drvdata(&pdev->dev);
+
+	analogix_dp_remove(dp->adp);
+
 	component_del(&pdev->dev, &rockchip_dp_component_ops);
 }
 
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 1b119b547988..d9b721fd7201 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -43,6 +43,7 @@  int analogix_dp_suspend(struct analogix_dp_device *dp);
 
 struct analogix_dp_device *
 analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data);
+void analogix_dp_remove(struct analogix_dp_device *dp);
 int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev);
 void analogix_dp_unbind(struct analogix_dp_device *dp);