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 <tred...@nvidia.com> > > > > > > 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 <tred...@nvidia.com> > > > > On patches 4,5,7 in this series Acked-by: Daniel Vetter > > <daniel.vet...@ffwll.ch> > > > > 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