[v3,5/7] drm: Validate encoder->possible_clones
diff mbox series

Message ID 20200211162208.16224-6-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • drm: Try to fix encoder possible_clones/crtc
Related show

Commit Message

Ville Syrjälä Feb. 11, 2020, 4:22 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Many drivers are populating encoder->possible_clones wrong. Let's
persuade them to get it right by adding some loud WARNs.

We'll cross check the bits between any two encoders. So either
both encoders can clone with the other, or neither can.

We'll also complain about effectively empty possible_clones, and
possible_clones containing bits for encoders that don't exist.

v2: encoder->possible_clones now includes the encoder itelf
v3: Move to drm_mode_config_validate() (Daniel)
    Document that you get a WARN when this is wrong (Daniel)
    Extract full_encoder_mask()

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_mode_config.c | 40 +++++++++++++++++++++++++++++++
 include/drm/drm_encoder.h         |  2 ++
 2 files changed, 42 insertions(+)

Comments

Daniel Vetter Feb. 11, 2020, 5:02 p.m. UTC | #1
On Tue, Feb 11, 2020 at 06:22:06PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Many drivers are populating encoder->possible_clones wrong. Let's
> persuade them to get it right by adding some loud WARNs.
> 
> We'll cross check the bits between any two encoders. So either
> both encoders can clone with the other, or neither can.
> 
> We'll also complain about effectively empty possible_clones, and
> possible_clones containing bits for encoders that don't exist.
> 
> v2: encoder->possible_clones now includes the encoder itelf
> v3: Move to drm_mode_config_validate() (Daniel)
>     Document that you get a WARN when this is wrong (Daniel)
>     Extract full_encoder_mask()
> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I wonder whether we should start to have some unit tests for stuff like
this, like set up broken driver, make sure we have a WARN in dmesg. But
ideally we'd do that with the mocking stuff Kunit hopefully has soon.

</idle musings>


> ---
>  drivers/gpu/drm/drm_mode_config.c | 40 +++++++++++++++++++++++++++++++
>  include/drm/drm_encoder.h         |  2 ++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 75e357c7e84d..afc91447293a 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -533,6 +533,17 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_mode_config_cleanup);
>  
> +static u32 full_encoder_mask(struct drm_device *dev)
> +{
> +	struct drm_encoder *encoder;
> +	u32 encoder_mask = 0;
> +
> +	drm_for_each_encoder(encoder, dev)
> +		encoder_mask |= drm_encoder_mask(encoder);
> +
> +	return encoder_mask;
> +}
> +
>  /*
>   * For some reason we want the encoder itself included in
>   * possible_clones. Make life easy for drivers by allowing them
> @@ -544,10 +555,39 @@ static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
>  		encoder->possible_clones = drm_encoder_mask(encoder);
>  }
>  
> +static void validate_encoder_possible_clones(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	u32 encoder_mask = full_encoder_mask(dev);
> +	struct drm_encoder *other;
> +
> +	drm_for_each_encoder(other, dev) {
> +		WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
> +		     !(other->possible_clones & drm_encoder_mask(encoder)),

Bikeshed: !! as canonical "make this a bool value" might be slightly
clearer, but whatever.

> +		     "possible_clones mismatch: "
> +		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
> +		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
> +		     encoder->base.id, encoder->name,
> +		     drm_encoder_mask(encoder), encoder->possible_clones,
> +		     other->base.id, other->name,
> +		     drm_encoder_mask(other), other->possible_clones);
> +	}
> +
> +	WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
> +	     (encoder->possible_clones & ~encoder_mask) != 0,
> +	     "Bogus possible_clones: "
> +	     "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
> +	     encoder->base.id, encoder->name,
> +	     encoder->possible_clones, encoder_mask);
> +}

Since it's next to each another double-checking that the fixup did add the
self-clone is probably too much :-)

> +
>  void drm_mode_config_validate(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
>  
>  	drm_for_each_encoder(encoder, dev)
>  		fixup_encoder_possible_clones(encoder);
> +
> +	drm_for_each_encoder(encoder, dev)
> +		validate_encoder_possible_clones(encoder);

>  }
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 22d6cdf729f1..3741963b9587 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -163,6 +163,8 @@ struct drm_encoder {
>  	 * any cloning it can leave @possible_clones set to 0. The core will
>  	 * automagically fix this up by setting the bit for the encoder itself.
>  	 *
> +	 * You will get a WARN if you get this wrong in the driver.

Nice.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +	 *
>  	 * Note that since encoder objects can't be hotplugged the assigned indices
>  	 * are stable and hence known before registering all objects.
>  	 */
> -- 
> 2.24.1
>
Ville Syrjälä Feb. 11, 2020, 5:13 p.m. UTC | #2
On Tue, Feb 11, 2020 at 06:02:33PM +0100, Daniel Vetter wrote:
> On Tue, Feb 11, 2020 at 06:22:06PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Many drivers are populating encoder->possible_clones wrong. Let's
> > persuade them to get it right by adding some loud WARNs.
> > 
> > We'll cross check the bits between any two encoders. So either
> > both encoders can clone with the other, or neither can.
> > 
> > We'll also complain about effectively empty possible_clones, and
> > possible_clones containing bits for encoders that don't exist.
> > 
> > v2: encoder->possible_clones now includes the encoder itelf
> > v3: Move to drm_mode_config_validate() (Daniel)
> >     Document that you get a WARN when this is wrong (Daniel)
> >     Extract full_encoder_mask()
> > 
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I wonder whether we should start to have some unit tests for stuff like
> this, like set up broken driver, make sure we have a WARN in dmesg. But
> ideally we'd do that with the mocking stuff Kunit hopefully has soon.
> 
> </idle musings>
> 
> 
> > ---
> >  drivers/gpu/drm/drm_mode_config.c | 40 +++++++++++++++++++++++++++++++
> >  include/drm/drm_encoder.h         |  2 ++
> >  2 files changed, 42 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 75e357c7e84d..afc91447293a 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -533,6 +533,17 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> >  }
> >  EXPORT_SYMBOL(drm_mode_config_cleanup);
> >  
> > +static u32 full_encoder_mask(struct drm_device *dev)
> > +{
> > +	struct drm_encoder *encoder;
> > +	u32 encoder_mask = 0;
> > +
> > +	drm_for_each_encoder(encoder, dev)
> > +		encoder_mask |= drm_encoder_mask(encoder);
> > +
> > +	return encoder_mask;
> > +}
> > +
> >  /*
> >   * For some reason we want the encoder itself included in
> >   * possible_clones. Make life easy for drivers by allowing them
> > @@ -544,10 +555,39 @@ static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
> >  		encoder->possible_clones = drm_encoder_mask(encoder);
> >  }
> >  
> > +static void validate_encoder_possible_clones(struct drm_encoder *encoder)
> > +{
> > +	struct drm_device *dev = encoder->dev;
> > +	u32 encoder_mask = full_encoder_mask(dev);
> > +	struct drm_encoder *other;
> > +
> > +	drm_for_each_encoder(other, dev) {
> > +		WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
> > +		     !(other->possible_clones & drm_encoder_mask(encoder)),
> 
> Bikeshed: !! as canonical "make this a bool value" might be slightly
> clearer, but whatever.

Can repaint.

> 
> > +		     "possible_clones mismatch: "
> > +		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
> > +		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
> > +		     encoder->base.id, encoder->name,
> > +		     drm_encoder_mask(encoder), encoder->possible_clones,
> > +		     other->base.id, other->name,
> > +		     drm_encoder_mask(other), other->possible_clones);
> > +	}
> > +
> > +	WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
> > +	     (encoder->possible_clones & ~encoder_mask) != 0,
> > +	     "Bogus possible_clones: "
> > +	     "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
> > +	     encoder->base.id, encoder->name,
> > +	     encoder->possible_clones, encoder_mask);
> > +}
> 
> Since it's next to each another double-checking that the fixup did add the
> self-clone is probably too much :-)

I changed the fixup to be just
if (possible_clones == 0)
	possible_clones = drm_encoder_mask();

So if the driver tries to set it up but fails and forgets the
encoder itself this WARN will still trip.

> 
> > +
> >  void drm_mode_config_validate(struct drm_device *dev)
> >  {
> >  	struct drm_encoder *encoder;
> >  
> >  	drm_for_each_encoder(encoder, dev)
> >  		fixup_encoder_possible_clones(encoder);
> > +
> > +	drm_for_each_encoder(encoder, dev)
> > +		validate_encoder_possible_clones(encoder);
> 
> >  }
> > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > index 22d6cdf729f1..3741963b9587 100644
> > --- a/include/drm/drm_encoder.h
> > +++ b/include/drm/drm_encoder.h
> > @@ -163,6 +163,8 @@ struct drm_encoder {
> >  	 * any cloning it can leave @possible_clones set to 0. The core will
> >  	 * automagically fix this up by setting the bit for the encoder itself.
> >  	 *
> > +	 * You will get a WARN if you get this wrong in the driver.
> 
> Nice.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > +	 *
> >  	 * Note that since encoder objects can't be hotplugged the assigned indices
> >  	 * are stable and hence known before registering all objects.
> >  	 */
> > -- 
> > 2.24.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter Feb. 12, 2020, 8:56 a.m. UTC | #3
On Tue, Feb 11, 2020 at 07:13:31PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 11, 2020 at 06:02:33PM +0100, Daniel Vetter wrote:
> > On Tue, Feb 11, 2020 at 06:22:06PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Many drivers are populating encoder->possible_clones wrong. Let's
> > > persuade them to get it right by adding some loud WARNs.
> > > 
> > > We'll cross check the bits between any two encoders. So either
> > > both encoders can clone with the other, or neither can.
> > > 
> > > We'll also complain about effectively empty possible_clones, and
> > > possible_clones containing bits for encoders that don't exist.
> > > 
> > > v2: encoder->possible_clones now includes the encoder itelf
> > > v3: Move to drm_mode_config_validate() (Daniel)
> > >     Document that you get a WARN when this is wrong (Daniel)
> > >     Extract full_encoder_mask()
> > > 
> > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I wonder whether we should start to have some unit tests for stuff like
> > this, like set up broken driver, make sure we have a WARN in dmesg. But
> > ideally we'd do that with the mocking stuff Kunit hopefully has soon.
> > 
> > </idle musings>
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/drm_mode_config.c | 40 +++++++++++++++++++++++++++++++
> > >  include/drm/drm_encoder.h         |  2 ++
> > >  2 files changed, 42 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index 75e357c7e84d..afc91447293a 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -533,6 +533,17 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> > >  }
> > >  EXPORT_SYMBOL(drm_mode_config_cleanup);
> > >  
> > > +static u32 full_encoder_mask(struct drm_device *dev)
> > > +{
> > > +	struct drm_encoder *encoder;
> > > +	u32 encoder_mask = 0;
> > > +
> > > +	drm_for_each_encoder(encoder, dev)
> > > +		encoder_mask |= drm_encoder_mask(encoder);
> > > +
> > > +	return encoder_mask;
> > > +}
> > > +
> > >  /*
> > >   * For some reason we want the encoder itself included in
> > >   * possible_clones. Make life easy for drivers by allowing them
> > > @@ -544,10 +555,39 @@ static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
> > >  		encoder->possible_clones = drm_encoder_mask(encoder);
> > >  }
> > >  
> > > +static void validate_encoder_possible_clones(struct drm_encoder *encoder)
> > > +{
> > > +	struct drm_device *dev = encoder->dev;
> > > +	u32 encoder_mask = full_encoder_mask(dev);
> > > +	struct drm_encoder *other;
> > > +
> > > +	drm_for_each_encoder(other, dev) {
> > > +		WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
> > > +		     !(other->possible_clones & drm_encoder_mask(encoder)),
> > 
> > Bikeshed: !! as canonical "make this a bool value" might be slightly
> > clearer, but whatever.
> 
> Can repaint.
> 
> > 
> > > +		     "possible_clones mismatch: "
> > > +		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
> > > +		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
> > > +		     encoder->base.id, encoder->name,
> > > +		     drm_encoder_mask(encoder), encoder->possible_clones,
> > > +		     other->base.id, other->name,
> > > +		     drm_encoder_mask(other), other->possible_clones);
> > > +	}
> > > +
> > > +	WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
> > > +	     (encoder->possible_clones & ~encoder_mask) != 0,
> > > +	     "Bogus possible_clones: "
> > > +	     "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
> > > +	     encoder->base.id, encoder->name,
> > > +	     encoder->possible_clones, encoder_mask);
> > > +}
> > 
> > Since it's next to each another double-checking that the fixup did add the
> > self-clone is probably too much :-)
> 
> I changed the fixup to be just
> if (possible_clones == 0)
> 	possible_clones = drm_encoder_mask();
> 
> So if the driver tries to set it up but fails and forgets the
> encoder itself this WARN will still trip.

Duh, I was just blind, everything is looking good.
-Daniel

> 
> > 
> > > +
> > >  void drm_mode_config_validate(struct drm_device *dev)
> > >  {
> > >  	struct drm_encoder *encoder;
> > >  
> > >  	drm_for_each_encoder(encoder, dev)
> > >  		fixup_encoder_possible_clones(encoder);
> > > +
> > > +	drm_for_each_encoder(encoder, dev)
> > > +		validate_encoder_possible_clones(encoder);
> > 
> > >  }
> > > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > > index 22d6cdf729f1..3741963b9587 100644
> > > --- a/include/drm/drm_encoder.h
> > > +++ b/include/drm/drm_encoder.h
> > > @@ -163,6 +163,8 @@ struct drm_encoder {
> > >  	 * any cloning it can leave @possible_clones set to 0. The core will
> > >  	 * automagically fix this up by setting the bit for the encoder itself.
> > >  	 *
> > > +	 * You will get a WARN if you get this wrong in the driver.
> > 
> > Nice.
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > > +	 *
> > >  	 * Note that since encoder objects can't be hotplugged the assigned indices
> > >  	 * are stable and hence known before registering all objects.
> > >  	 */
> > > -- 
> > > 2.24.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 75e357c7e84d..afc91447293a 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -533,6 +533,17 @@  void drm_mode_config_cleanup(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_config_cleanup);
 
+static u32 full_encoder_mask(struct drm_device *dev)
+{
+	struct drm_encoder *encoder;
+	u32 encoder_mask = 0;
+
+	drm_for_each_encoder(encoder, dev)
+		encoder_mask |= drm_encoder_mask(encoder);
+
+	return encoder_mask;
+}
+
 /*
  * For some reason we want the encoder itself included in
  * possible_clones. Make life easy for drivers by allowing them
@@ -544,10 +555,39 @@  static void fixup_encoder_possible_clones(struct drm_encoder *encoder)
 		encoder->possible_clones = drm_encoder_mask(encoder);
 }
 
+static void validate_encoder_possible_clones(struct drm_encoder *encoder)
+{
+	struct drm_device *dev = encoder->dev;
+	u32 encoder_mask = full_encoder_mask(dev);
+	struct drm_encoder *other;
+
+	drm_for_each_encoder(other, dev) {
+		WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
+		     !(other->possible_clones & drm_encoder_mask(encoder)),
+		     "possible_clones mismatch: "
+		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
+		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
+		     encoder->base.id, encoder->name,
+		     drm_encoder_mask(encoder), encoder->possible_clones,
+		     other->base.id, other->name,
+		     drm_encoder_mask(other), other->possible_clones);
+	}
+
+	WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
+	     (encoder->possible_clones & ~encoder_mask) != 0,
+	     "Bogus possible_clones: "
+	     "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
+	     encoder->base.id, encoder->name,
+	     encoder->possible_clones, encoder_mask);
+}
+
 void drm_mode_config_validate(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
 
 	drm_for_each_encoder(encoder, dev)
 		fixup_encoder_possible_clones(encoder);
+
+	drm_for_each_encoder(encoder, dev)
+		validate_encoder_possible_clones(encoder);
 }
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 22d6cdf729f1..3741963b9587 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -163,6 +163,8 @@  struct drm_encoder {
 	 * any cloning it can leave @possible_clones set to 0. The core will
 	 * automagically fix this up by setting the bit for the encoder itself.
 	 *
+	 * You will get a WARN if you get this wrong in the driver.
+	 *
 	 * Note that since encoder objects can't be hotplugged the assigned indices
 	 * are stable and hence known before registering all objects.
 	 */