On Fri, Nov 29, 2019 at 10:06:43AM +0100, Daniel Vetter wrote:
> On Thu, Nov 28, 2019 at 04:37:33PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> >
> > I have no recollection why that check is there, but it seems to trigger
> > all the time, so remove it. Everything works fine without.
> >
> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/tegra/hub.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c
> > index 6aca0fd5a8e5..e56c0f7d3a13 100644
> > --- a/drivers/gpu/drm/tegra/hub.c
> > +++ b/drivers/gpu/drm/tegra/hub.c
> > @@ -615,11 +615,8 @@ static struct tegra_display_hub_state *
> > tegra_display_hub_get_state(struct tegra_display_hub *hub,
> > struct drm_atomic_state *state)
> > {
> > - struct drm_device *drm = dev_get_drvdata(hub->client.parent);
> > struct drm_private_state *priv;
> >
> > - WARN_ON(!drm_modeset_is_locked(&drm->mode_config.connection_mutex));
>
> I suspect copypasta from the mst private state stuff, which relied on this
> lock to protect it. Except your code never bothered to grab that lock (or
> any other) so was technically broken until we added generic locking in
>
> commit b962a12050a387e4bbf3a48745afe1d29d396b0d
> Author: Rob Clark <robdclark@xxxxxxxxx>
> Date: Mon Oct 22 14:31:22 2018 +0200
>
> drm/atomic: integrate modeset lock with private objects
>
> Hence this is now ok to drop, originally it wasnt.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
Great, thanks for pointing that out. I'll update the commit message with
that explanation.
> Aside: You're single-thread all your atomic updates on the hub->lock,
> which might not be what you want. At least updates to separate crtc should
> go through in parallel. Usual way to fix this is to add a
> tegra_crtc_state->hub_changed that your earlier code sets, and then you
> walk the crtc states in the atomic commit (only those, not all, otherwise
> you just rebuild that global lock again), and then only grab the hub state
> when you need to update something.
I'm confused. Where do you see hub->lock? Did you mean wgrp->lock?
Thierry
Attachment:
signature.asc
Description: PGP signature
