[v2,5/6] drm: Validate encoder->possible_clones
diff mbox series

Message ID 20200207135950.6655-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. 7, 2020, 1:59 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.

TODO: Or should we just silently filter out any bits for non-existing
encoders?

v2: encoder->possible_clones now includes the encoder itelf

Cc: 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_encoder.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Thomas Zimmermann Feb. 7, 2020, 2:25 p.m. UTC | #1
Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> 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.
> 
> TODO: Or should we just silently filter out any bits for non-existing
> encoders?

Please be noisy. Setting these masks incorrectly could be a bug.

> 
> v2: encoder->possible_clones now includes the encoder itelf
> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/drm_encoder.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index f761d9306028..bc2246f27e0d 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -79,6 +79,34 @@ static void fixup_possible_clones(struct drm_device *dev)
>  		encoder->possible_clones |= drm_encoder_mask(encoder);
>  }
>  
> +static void validate_possible_clones(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_encoder *other;
> +	u32 encoder_mask = 0;
> +
> +	drm_for_each_encoder(other, dev) {
> +		encoder_mask |= drm_encoder_mask(other);
> +
> +		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);
> +}
> +
>  int drm_encoder_register_all(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
> @@ -87,6 +115,8 @@ int drm_encoder_register_all(struct drm_device *dev)
>  	fixup_possible_clones(dev);
>  
>  	drm_for_each_encoder(encoder, dev) {
> +		validate_possible_clones(encoder);
> +
>  		if (encoder->funcs->late_register)
>  			ret = encoder->funcs->late_register(encoder);
>  		if (ret)
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index f761d9306028..bc2246f27e0d 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -79,6 +79,34 @@  static void fixup_possible_clones(struct drm_device *dev)
 		encoder->possible_clones |= drm_encoder_mask(encoder);
 }
 
+static void validate_possible_clones(struct drm_encoder *encoder)
+{
+	struct drm_device *dev = encoder->dev;
+	struct drm_encoder *other;
+	u32 encoder_mask = 0;
+
+	drm_for_each_encoder(other, dev) {
+		encoder_mask |= drm_encoder_mask(other);
+
+		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);
+}
+
 int drm_encoder_register_all(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
@@ -87,6 +115,8 @@  int drm_encoder_register_all(struct drm_device *dev)
 	fixup_possible_clones(dev);
 
 	drm_for_each_encoder(encoder, dev) {
+		validate_possible_clones(encoder);
+
 		if (encoder->funcs->late_register)
 			ret = encoder->funcs->late_register(encoder);
 		if (ret)