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 <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.
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);
> > > > + 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
Attachment:
signature.asc
Description: PGP signature
