Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references
On Mon, Dec 02, 2019 at 03:58:33PM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 09:20:25PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 29, 2019 at 11:44:12AM +0100, Thierry Reding wrote:
> > > On Fri, Nov 29, 2019 at 10:23:19AM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> > > > > From: Thierry Reding
> > > > >
> > > > > Ensure that a runtime PM reference is acquired each time the DPAUX
> > > > > registers are accessed. Otherwise the code may end up running without
> > > > > the controller being powered, out-of-reset or clocked in some corner
> > > > > cases, resulting in a crash.
> > > > >
> > > > > Signed-off-by: Thierry Reding
> > > >
> > > > On patches 4,5,7 in this series Acked-by: Daniel Vetter
> > > >
> > > >
> > > > On this one here I'm very confused.
> > > >
> > > > - Why do you drop the runtime pm between enable and disable? Is that
> > > > just
> > > > how the hw works, i.e. the pad config stays, just the registers go
> > > > away?
> > >
> > > Now you've made me doubt this. I don't think the pad configuration stays
> > > across runtime suspend/resume, so you're right, this shouldn't work.
> > > I'll need to go retest this one specifically.
> > >
> > > I had added these runtime PM references to ensure the device was
> > > properly configured at resume from suspend, but there ended up being an
> > > additional issue with the I2C driver that relies on this, so perhaps
> > > this may not be necessary in the end.
> > >
> > > > - I'm not seeing any locking between the different users (dp aux and
> > > > pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
> > > > make this easier (but I'm not super fond of that pattern from i2c).
> > >
> > > There should be no need to lock here. DP AUX transfers will only be used
> > > between drm_dp_aux_enable() and drm_dp_aux_disable().
> >
> > So dp aux vs. dp aux aside (that's the next point below), there's
> > guaranteed no one else can get at that pinctrl mux? Since the other
> > setting is labelled I2C I assumed there's an i2c controller hanging of it,
> > exposed to userspace, and userspace might probe that whenever it feels
> > like (similar to the issue below). But I don't know your hw, nor do I
> > really know pinctrl. Just looked a bit strange.
>
> Well technically anyone could get at the mux, but since it controls the
> pins of a single I2C controller, only that I2C controller should ever
> get its hands on the pinmux. Anything else would be an error in the DT.
>
> Now, the pins can also be used in AUX mode when the SOR is used in DP
> mode. However, since DP and HDMI are mutually exclusive, this is a board
> level decision, so in practice you're only ever going to see them used
> in either I2C or AUX mode. The "off" mode is used only for power saving
> when I2C or DPAUX don't use the pins.
Oh, so you don't support DP+ with some auto level shifter magic? At least
in other drivers DP+ is why you need to have a lock between dp aux and i2c
at runtime, because otherwise things go wrong if you reprobe DP while
userspace is probing the hdmi i2c side (or the other way round).
> Regarding the runtime PM references, it turns out that those are
> completely bogus because we already take a runtime PM reference at
> probe time. I'm going to drop this patch and look into fixing the other,
> real issues that you pointed out.
Ah that explains the confusion :-)
-Daniel
>
> Thierry
>
> >
> > Cheers, Daniel
> >
> >
> > > > - Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
> > > > callback, otherwise the various userspace interface (dp aux, but also
> > > > i2c on top of that) won't work. Some pre/post_transfer functions like
> > > > i2c has might be useful for stuff like this.
> > >
> > > I suppose it would be possible for someone to attempt to use those
> > > userspace interfaces outside of drm_dp_aux_enable()/drm_dp_aux_disable()
> > > and then the locking would be required.
> > >
> > > I'll look into that.
> > >
> > > Thierry
> > >
> > > >
> > > > Cheers, Daniel
> > > >
> > > > > ---
> > > > > drivers/gpu/drm/tegra/dpaux.c | 16 ++--
> > > > > 1 file changed, 14 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/tegra/dpaux.c
> > > > > b/drivers/gpu/drm/tegra/dpaux.c
> > > > > index 622cdf1ad246..4b2b86aed1a5 100644
> > > > > --- a/drivers/gpu/drm/tegra/dpaux.c
> > > > > +++ b/drivers/gpu/drm/tegra/dpaux.c
> > > > > @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct
> > > > > pinctrl_dev *pinctrl,
> > > > > unsigned int function, unsigned int
> > > > > group)
> > > > > {
> > > > > struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> > > > > + int err;
> > > > > +
> > > > > + pm_runtime_get_sync(dpaux->dev);
> > > > > + err = tegra_dpaux_pad_config(dpaux, function);
> > > > > + pm_runtime_put(dpaux->dev);
> >
Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references
On Fri, Nov 29, 2019 at 09:20:25PM +0100, Daniel Vetter wrote:
> On Fri, Nov 29, 2019 at 11:44:12AM +0100, Thierry Reding wrote:
> > On Fri, Nov 29, 2019 at 10:23:19AM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> > > > From: Thierry Reding
> > > >
> > > > Ensure that a runtime PM reference is acquired each time the DPAUX
> > > > registers are accessed. Otherwise the code may end up running without
> > > > the controller being powered, out-of-reset or clocked in some corner
> > > > cases, resulting in a crash.
> > > >
> > > > Signed-off-by: Thierry Reding
> > >
> > > On patches 4,5,7 in this series Acked-by: Daniel Vetter
> > >
> > >
> > > On this one here I'm very confused.
> > >
> > > - Why do you drop the runtime pm between enable and disable? Is that just
> > > how the hw works, i.e. the pad config stays, just the registers go away?
> >
> > Now you've made me doubt this. I don't think the pad configuration stays
> > across runtime suspend/resume, so you're right, this shouldn't work.
> > I'll need to go retest this one specifically.
> >
> > I had added these runtime PM references to ensure the device was
> > properly configured at resume from suspend, but there ended up being an
> > additional issue with the I2C driver that relies on this, so perhaps
> > this may not be necessary in the end.
> >
> > > - I'm not seeing any locking between the different users (dp aux and
> > > pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
> > > make this easier (but I'm not super fond of that pattern from i2c).
> >
> > There should be no need to lock here. DP AUX transfers will only be used
> > between drm_dp_aux_enable() and drm_dp_aux_disable().
>
> So dp aux vs. dp aux aside (that's the next point below), there's
> guaranteed no one else can get at that pinctrl mux? Since the other
> setting is labelled I2C I assumed there's an i2c controller hanging of it,
> exposed to userspace, and userspace might probe that whenever it feels
> like (similar to the issue below). But I don't know your hw, nor do I
> really know pinctrl. Just looked a bit strange.
Well technically anyone could get at the mux, but since it controls the
pins of a single I2C controller, only that I2C controller should ever
get its hands on the pinmux. Anything else would be an error in the DT.
Now, the pins can also be used in AUX mode when the SOR is used in DP
mode. However, since DP and HDMI are mutually exclusive, this is a board
level decision, so in practice you're only ever going to see them used
in either I2C or AUX mode. The "off" mode is used only for power saving
when I2C or DPAUX don't use the pins.
Regarding the runtime PM references, it turns out that those are
completely bogus because we already take a runtime PM reference at
probe time. I'm going to drop this patch and look into fixing the other,
real issues that you pointed out.
Thierry
>
> Cheers, Daniel
>
>
> > > - Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
> > > callback, otherwise the various userspace interface (dp aux, but also
> > > i2c on top of that) won't work. Some pre/post_transfer functions like
> > > i2c has might be useful for stuff like this.
> >
> > I suppose it would be possible for someone to attempt to use those
> > userspace interfaces outside of drm_dp_aux_enable()/drm_dp_aux_disable()
> > and then the locking would be required.
> >
> > I'll look into that.
> >
> > Thierry
> >
> > >
> > > Cheers, Daniel
> > >
> > > > ---
> > > > drivers/gpu/drm/tegra/dpaux.c | 16 ++--
> > > > 1 file changed, 14 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/tegra/dpaux.c
> > > > b/drivers/gpu/drm/tegra/dpaux.c
> > > > index 622cdf1ad246..4b2b86aed1a5 100644
> > > > --- a/drivers/gpu/drm/tegra/dpaux.c
> > > > +++ b/drivers/gpu/drm/tegra/dpaux.c
> > > > @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev
> > > > *pinctrl,
> > > >unsigned int function, unsigned int
> > > > group)
> > > > {
> > > > struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> > > > + int err;
> > > > +
> > > > + pm_runtime_get_sync(dpaux->dev);
> > > > + err = tegra_dpaux_pad_config(dpaux, function);
> > > > + pm_runtime_put(dpaux->dev);
> > > >
> > > > - return tegra_dpaux_pad_config(dpaux, function);
> > > > + return err;
> > > > }
> > > >
> > > > static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> > > > @@ -809,15 +814,22 @@ enum drm_connector_status
> > > > drm_dp_aux_detect(struct drm_dp_aux *aux)
> > > > int drm_dp_aux_enable(struct drm_dp_aux *aux)
> > > > {
> > > > struct tegra_dpaux *dpaux = to_dpaux(aux);
> > > > + int err;
> > > > +
> > > > + pm_runtime_get_sync(dpaux->dev);
> > > > + err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > > +
Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references
On Fri, Nov 29, 2019 at 11:44:12AM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 10:23:19AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding
> > >
> > > Ensure that a runtime PM reference is acquired each time the DPAUX
> > > registers are accessed. Otherwise the code may end up running without
> > > the controller being powered, out-of-reset or clocked in some corner
> > > cases, resulting in a crash.
> > >
> > > Signed-off-by: Thierry Reding
> >
> > On patches 4,5,7 in this series Acked-by: Daniel Vetter
> >
> >
> > On this one here I'm very confused.
> >
> > - Why do you drop the runtime pm between enable and disable? Is that just
> > how the hw works, i.e. the pad config stays, just the registers go away?
>
> Now you've made me doubt this. I don't think the pad configuration stays
> across runtime suspend/resume, so you're right, this shouldn't work.
> I'll need to go retest this one specifically.
>
> I had added these runtime PM references to ensure the device was
> properly configured at resume from suspend, but there ended up being an
> additional issue with the I2C driver that relies on this, so perhaps
> this may not be necessary in the end.
>
> > - I'm not seeing any locking between the different users (dp aux and
> > pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
> > make this easier (but I'm not super fond of that pattern from i2c).
>
> There should be no need to lock here. DP AUX transfers will only be used
> between drm_dp_aux_enable() and drm_dp_aux_disable().
So dp aux vs. dp aux aside (that's the next point below), there's
guaranteed no one else can get at that pinctrl mux? Since the other
setting is labelled I2C I assumed there's an i2c controller hanging of it,
exposed to userspace, and userspace might probe that whenever it feels
like (similar to the issue below). But I don't know your hw, nor do I
really know pinctrl. Just looked a bit strange.
Cheers, Daniel
> > - Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
> > callback, otherwise the various userspace interface (dp aux, but also
> > i2c on top of that) won't work. Some pre/post_transfer functions like
> > i2c has might be useful for stuff like this.
>
> I suppose it would be possible for someone to attempt to use those
> userspace interfaces outside of drm_dp_aux_enable()/drm_dp_aux_disable()
> and then the locking would be required.
>
> I'll look into that.
>
> Thierry
>
> >
> > Cheers, Daniel
> >
> > > ---
> > > drivers/gpu/drm/tegra/dpaux.c | 16 ++--
> > > 1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> > > index 622cdf1ad246..4b2b86aed1a5 100644
> > > --- a/drivers/gpu/drm/tegra/dpaux.c
> > > +++ b/drivers/gpu/drm/tegra/dpaux.c
> > > @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev
> > > *pinctrl,
> > > unsigned int function, unsigned int group)
> > > {
> > > struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> > > + int err;
> > > +
> > > + pm_runtime_get_sync(dpaux->dev);
> > > + err = tegra_dpaux_pad_config(dpaux, function);
> > > + pm_runtime_put(dpaux->dev);
> > >
> > > - return tegra_dpaux_pad_config(dpaux, function);
> > > + return err;
> > > }
> > >
> > > static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> > > @@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct
> > > drm_dp_aux *aux)
> > > int drm_dp_aux_enable(struct drm_dp_aux *aux)
> > > {
> > > struct tegra_dpaux *dpaux = to_dpaux(aux);
> > > + int err;
> > > +
> > > + pm_runtime_get_sync(dpaux->dev);
> > > + err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > + pm_runtime_put(dpaux->dev);
> > >
> > > - return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > + return err;
> > > }
> > >
> > > int drm_dp_aux_disable(struct drm_dp_aux *aux)
> > > {
> > > struct tegra_dpaux *dpaux = to_dpaux(aux);
> > >
> > > + pm_runtime_get_sync(dpaux->dev);
> > > tegra_dpaux_pad_power_down(dpaux);
> > > + pm_runtime_put(dpaux->dev);
> > >
> > > return 0;
> > > }
> > > --
> > > 2.23.0
> > >
> > > ___
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references
On Fri, Nov 29, 2019 at 10:23:19AM +0100, Daniel Vetter wrote:
> On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> > From: Thierry Reding
> >
> > Ensure that a runtime PM reference is acquired each time the DPAUX
> > registers are accessed. Otherwise the code may end up running without
> > the controller being powered, out-of-reset or clocked in some corner
> > cases, resulting in a crash.
> >
> > Signed-off-by: Thierry Reding
>
> On patches 4,5,7 in this series Acked-by: Daniel Vetter
>
>
> On this one here I'm very confused.
>
> - Why do you drop the runtime pm between enable and disable? Is that just
> how the hw works, i.e. the pad config stays, just the registers go away?
Now you've made me doubt this. I don't think the pad configuration stays
across runtime suspend/resume, so you're right, this shouldn't work.
I'll need to go retest this one specifically.
I had added these runtime PM references to ensure the device was
properly configured at resume from suspend, but there ended up being an
additional issue with the I2C driver that relies on this, so perhaps
this may not be necessary in the end.
> - I'm not seeing any locking between the different users (dp aux and
> pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
> make this easier (but I'm not super fond of that pattern from i2c).
There should be no need to lock here. DP AUX transfers will only be used
between drm_dp_aux_enable() and drm_dp_aux_disable().
> - Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
> callback, otherwise the various userspace interface (dp aux, but also
> i2c on top of that) won't work. Some pre/post_transfer functions like
> i2c has might be useful for stuff like this.
I suppose it would be possible for someone to attempt to use those
userspace interfaces outside of drm_dp_aux_enable()/drm_dp_aux_disable()
and then the locking would be required.
I'll look into that.
Thierry
>
> Cheers, Daniel
>
> > ---
> > drivers/gpu/drm/tegra/dpaux.c | 16 ++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> > index 622cdf1ad246..4b2b86aed1a5 100644
> > --- a/drivers/gpu/drm/tegra/dpaux.c
> > +++ b/drivers/gpu/drm/tegra/dpaux.c
> > @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev
> > *pinctrl,
> >unsigned int function, unsigned int group)
> > {
> > struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> > + int err;
> > +
> > + pm_runtime_get_sync(dpaux->dev);
> > + err = tegra_dpaux_pad_config(dpaux, function);
> > + pm_runtime_put(dpaux->dev);
> >
> > - return tegra_dpaux_pad_config(dpaux, function);
> > + return err;
> > }
> >
> > static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> > @@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct
> > drm_dp_aux *aux)
> > int drm_dp_aux_enable(struct drm_dp_aux *aux)
> > {
> > struct tegra_dpaux *dpaux = to_dpaux(aux);
> > + int err;
> > +
> > + pm_runtime_get_sync(dpaux->dev);
> > + err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > + pm_runtime_put(dpaux->dev);
> >
> > - return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > + return err;
> > }
> >
> > int drm_dp_aux_disable(struct drm_dp_aux *aux)
> > {
> > struct tegra_dpaux *dpaux = to_dpaux(aux);
> >
> > + pm_runtime_get_sync(dpaux->dev);
> > tegra_dpaux_pad_power_down(dpaux);
> > + pm_runtime_put(dpaux->dev);
> >
> > return 0;
> > }
> > --
> > 2.23.0
> >
> > ___
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references
On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> From: Thierry Reding
>
> Ensure that a runtime PM reference is acquired each time the DPAUX
> registers are accessed. Otherwise the code may end up running without
> the controller being powered, out-of-reset or clocked in some corner
> cases, resulting in a crash.
>
> Signed-off-by: Thierry Reding
On patches 4,5,7 in this series Acked-by: Daniel Vetter
On this one here I'm very confused.
- Why do you drop the runtime pm between enable and disable? Is that just
how the hw works, i.e. the pad config stays, just the registers go away?
- I'm not seeing any locking between the different users (dp aux and
pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
make this easier (but I'm not super fond of that pattern from i2c).
- Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
callback, otherwise the various userspace interface (dp aux, but also
i2c on top of that) won't work. Some pre/post_transfer functions like
i2c has might be useful for stuff like this.
Cheers, Daniel
> ---
> drivers/gpu/drm/tegra/dpaux.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> index 622cdf1ad246..4b2b86aed1a5 100644
> --- a/drivers/gpu/drm/tegra/dpaux.c
> +++ b/drivers/gpu/drm/tegra/dpaux.c
> @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev
> *pinctrl,
> unsigned int function, unsigned int group)
> {
> struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> + int err;
> +
> + pm_runtime_get_sync(dpaux->dev);
> + err = tegra_dpaux_pad_config(dpaux, function);
> + pm_runtime_put(dpaux->dev);
>
> - return tegra_dpaux_pad_config(dpaux, function);
> + return err;
> }
>
> static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> @@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct
> drm_dp_aux *aux)
> int drm_dp_aux_enable(struct drm_dp_aux *aux)
> {
> struct tegra_dpaux *dpaux = to_dpaux(aux);
> + int err;
> +
> + pm_runtime_get_sync(dpaux->dev);
> + err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> + pm_runtime_put(dpaux->dev);
>
> - return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> + return err;
> }
>
> int drm_dp_aux_disable(struct drm_dp_aux *aux)
> {
> struct tegra_dpaux *dpaux = to_dpaux(aux);
>
> + pm_runtime_get_sync(dpaux->dev);
> tegra_dpaux_pad_power_down(dpaux);
> + pm_runtime_put(dpaux->dev);
>
> return 0;
> }
> --
> 2.23.0
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
