Re: [PATCH 1/9] drm/tegra: hub: Remove bogus connection mutex check

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3419 bytes --]

On Fri, Nov 29, 2019 at 08:03:09PM +0100, Daniel Vetter wrote:
> On Fri, Nov 29, 2019 at 11:12:55AM +0100, Thierry Reding wrote:
> > 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@nvidia.com>
> > > > 
> > > > 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@nvidia.com>
> > > > ---
> > > >  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@gmail.com>
> > > 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@ffwll.ch>
> > 
> > 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?
> 
> struct tegra_display_hub->base.lock I have no idea what wgrp->lock is
> protecting - the functions seem to be only called from driver load/cleanup
> code, and that is single-threaded. If I'm not missing anything then
> wgrp->lock does nothing for you.

This is currently single-threaded, but the idea was to make window group
assignment more dynamic. I currently always enable window groups because
there's no good place to dynamically do that. Once I figure that out the
lock would become necessary, so I'd prefer to leave it in place as sort
of a reminder that I need to actually worry about the locking.

I'll have to look into the hub->base.lock, I don't recall exactly what I
thought at the time and it seems like I didn't leave enough comments in
the code to quickly refresh my memory...

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel