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 <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.
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);
> > > > >
> > > > > - 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
