| Message ID | 20180615164925.28971-11-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > }
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); > > }
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 --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); }