diff mbox

[10/14] drm/i915: Populate possible_crtcs correctly

Message ID 20180615164925.28971-11-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä June 15, 2018, 4:49 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Don't advertize non-exisiting crtcs in the encoder possible_crtcs
bitmask.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Dhinakaran Pandiyan June 22, 2018, 1:26 a.m. UTC | #1
On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Don't advertize non-exisiting crtcs in the encoder possible_crtcs
> bitmask.
> 
How do we end up advertising non-existing CRTCs? encoder->crtc_mask
seems to be populated in the encoder init functions based on possible
pipes. Do you mean pipe and crtc->index can potentially differ?


> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index b095899d68a9..3fa9da714403 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13959,6 +13959,20 @@ static int intel_encoder_clones(struct
> intel_encoder *encoder)
>  	return index_mask;
>  }
>  
> +static int intel_encoder_crtcs(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct intel_crtc *crtc;
> +	int index_mask = 0;
> +
> +	for_each_intel_crtc(dev, crtc) {
> +		if (encoder->crtc_mask & BIT(crtc->pipe))
> +			index_mask |= drm_crtc_mask(&crtc->base);
> +	}
> +
> +	return index_mask;
> +}
> +
>  static bool has_edp_a(struct drm_i915_private *dev_priv)
>  {
>  	if (!IS_MOBILE(dev_priv))
> @@ -14211,7 +14225,8 @@ static void intel_setup_outputs(struct
> drm_i915_private *dev_priv)
>  	intel_psr_init(dev_priv);
>  
>  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> -		encoder->base.possible_crtcs = encoder->crtc_mask;
> +		encoder->base.possible_crtcs =
> +			intel_encoder_crtcs(encoder);
>  		encoder->base.possible_clones =
>  			intel_encoder_clones(encoder);
>  	}
Ville Syrjälä June 25, 2018, 11:10 a.m. UTC | #2
On Thu, Jun 21, 2018 at 06:26:04PM -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Don't advertize non-exisiting crtcs in the encoder possible_crtcs
> > bitmask.
> > 
> How do we end up advertising non-existing CRTCs? encoder->crtc_mask
> seems to be populated in the encoder init functions based on possible
> pipes. Do you mean pipe and crtc->index can potentially differ?

No. Just that we may sometimes set BIT(PIPE_C) in crtc_mask when
there are only two pipes on the device.

> 
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index b095899d68a9..3fa9da714403 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13959,6 +13959,20 @@ static int intel_encoder_clones(struct
> > intel_encoder *encoder)
> >  	return index_mask;
> >  }
> >  
> > +static int intel_encoder_crtcs(struct intel_encoder *encoder)
> > +{
> > +	struct drm_device *dev = encoder->base.dev;
> > +	struct intel_crtc *crtc;
> > +	int index_mask = 0;
> > +
> > +	for_each_intel_crtc(dev, crtc) {
> > +		if (encoder->crtc_mask & BIT(crtc->pipe))
> > +			index_mask |= drm_crtc_mask(&crtc->base);
> > +	}
> > +
> > +	return index_mask;
> > +}
> > +
> >  static bool has_edp_a(struct drm_i915_private *dev_priv)
> >  {
> >  	if (!IS_MOBILE(dev_priv))
> > @@ -14211,7 +14225,8 @@ static void intel_setup_outputs(struct
> > drm_i915_private *dev_priv)
> >  	intel_psr_init(dev_priv);
> >  
> >  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> > -		encoder->base.possible_crtcs = encoder->crtc_mask;
> > +		encoder->base.possible_crtcs =
> > +			intel_encoder_crtcs(encoder);
> >  		encoder->base.possible_clones =
> >  			intel_encoder_clones(encoder);
> >  	}
Dhinakaran Pandiyan June 25, 2018, 6:37 p.m. UTC | #3
On Mon, 2018-06-25 at 14:10 +0300, Ville Syrjälä wrote:
> On Thu, Jun 21, 2018 at 06:26:04PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Fri, 2018-06-15 at 19:49 +0300, Ville Syrjala wrote:
> > > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Don't advertize non-exisiting crtcs in the encoder possible_crtcs
> > > bitmask.
> > > 
> > How do we end up advertising non-existing CRTCs? encoder->crtc_mask
> > seems to be populated in the encoder init functions based on
> > possible
> > pipes. Do you mean pipe and crtc->index can potentially differ?
> No. Just that we may sometimes set BIT(PIPE_C) in crtc_mask when
> there are only two pipes on the device.
> 
Got it, thanks.
Since crtc_mask seems to be initialized based on the platform, we
should probably warn if we find out a bit is set incorrectly.

With or without that, this patch is
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>



> > 
> > 
> > 
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index b095899d68a9..3fa9da714403 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13959,6 +13959,20 @@ static int intel_encoder_clones(struct
> > > intel_encoder *encoder)
> > >  	return index_mask;
> > >  }
> > >  
> > > +static int intel_encoder_crtcs(struct intel_encoder *encoder)
> > > +{
> > > +	struct drm_device *dev = encoder->base.dev;
> > > +	struct intel_crtc *crtc;
> > > +	int index_mask = 0;
> > > +
> > > +	for_each_intel_crtc(dev, crtc) {
> > > +		if (encoder->crtc_mask & BIT(crtc->pipe))
> > > +			index_mask |= drm_crtc_mask(&crtc-
> > > >base);
> > > +	}
> > > +
> > > +	return index_mask;
> > > +}
> > > +
> > >  static bool has_edp_a(struct drm_i915_private *dev_priv)
> > >  {
> > >  	if (!IS_MOBILE(dev_priv))
> > > @@ -14211,7 +14225,8 @@ static void intel_setup_outputs(struct
> > > drm_i915_private *dev_priv)
> > >  	intel_psr_init(dev_priv);
> > >  
> > >  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> > > -		encoder->base.possible_crtcs = encoder-
> > > >crtc_mask;
> > > +		encoder->base.possible_crtcs =
> > > +			intel_encoder_crtcs(encoder);
> > >  		encoder->base.possible_clones =
> > >  			intel_encoder_clones(encoder);
> > >  	}
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b095899d68a9..3fa9da714403 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13959,6 +13959,20 @@  static int intel_encoder_clones(struct intel_encoder *encoder)
 	return index_mask;
 }
 
+static int intel_encoder_crtcs(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct intel_crtc *crtc;
+	int index_mask = 0;
+
+	for_each_intel_crtc(dev, crtc) {
+		if (encoder->crtc_mask & BIT(crtc->pipe))
+			index_mask |= drm_crtc_mask(&crtc->base);
+	}
+
+	return index_mask;
+}
+
 static bool has_edp_a(struct drm_i915_private *dev_priv)
 {
 	if (!IS_MOBILE(dev_priv))
@@ -14211,7 +14225,8 @@  static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 	intel_psr_init(dev_priv);
 
 	for_each_intel_encoder(&dev_priv->drm, encoder) {
-		encoder->base.possible_crtcs = encoder->crtc_mask;
+		encoder->base.possible_crtcs =
+			intel_encoder_crtcs(encoder);
 		encoder->base.possible_clones =
 			intel_encoder_clones(encoder);
 	}