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 <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().
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@xxxxxxxxxxxxxxxxxxxxx
> > > 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
