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 <treding@xxxxxxxxxx>
> >
> > 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 <treding@xxxxxxxxxx>
>
> On patches 4,5,7 in this series Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Attachment:
signature.asc
Description: PGP signature
