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

Commit Message

Thierry Reding Nov. 28, 2019, 3:37 p.m. UTC

From: Thierry Reding <[e-mail]>

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 <[e-mail]>
---
 drivers/gpu/drm/tegra/hub.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Daniel Vetter Nov. 29, 2019, 9:06 a.m. UTC | #1

On Thu, Nov 28, 2019 at 04:37:33PM +0100, Thierry Reding wrote:
> From: Thierry Reding <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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 <[e-mail]>
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 <[e-mail]>

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.

Cheers, Daniel

> -
>  	priv = drm_atomic_get_private_obj_state(state, &hub->base);
>  	if (IS_ERR(priv))
>  		return ERR_CAST(priv);
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Thierry Reding Nov. 29, 2019, 10:12 a.m. UTC | #2

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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 <[e-mail]>
> 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 <[e-mail]>

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

Daniel Vetter Nov. 29, 2019, 7:03 p.m. UTC | #3

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 <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > ---
> > >  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 <[e-mail]>
> > 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 <[e-mail]>
> 
> 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.

Cheers, Daniel

Thierry Reding Dec. 2, 2019, 3:08 p.m. UTC | #4

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 <[e-mail]>
> > > > 
> > > > 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 <[e-mail]>
> > > > ---
> > > >  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 <[e-mail]>
> > > 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 <[e-mail]>
> > 
> > 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

Patch

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));
-
 	priv = drm_atomic_get_private_obj_state(state, &hub->base);
 	if (IS_ERR(priv))
 		return ERR_CAST(priv);