| Message ID | 20220121130710.10382-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Reject bigjoiner if the pipe doesn't support it | expand |
On Fri, 21 Jan 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Check that our crtc can in fact be the bigjoiner master before > we let the modeset proceed with bigjoiner enabled. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 0964b2403e2d..36e547bd0cbe 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -4102,6 +4102,14 @@ static u8 bigjoiner_pipes(struct drm_i915_private *i915) > return 0; > } > > +static u8 bigjoiner_master_pipes(struct drm_i915_private *i915) > +{ > + u8 pipes = bigjoiner_pipes(i915); > + > + /* last pipe can not be master */ > + return pipes & (pipes >> 1); > +} > + > static bool transcoder_ddi_func_is_enabled(struct drm_i915_private *dev_priv, > enum transcoder cpu_transcoder) > { > @@ -7600,6 +7608,7 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, > struct intel_crtc_state *old_crtc_state, > struct intel_crtc_state *new_crtc_state) > { > + struct drm_i915_private *i915 = to_i915(state->base.dev); > struct intel_crtc_state *slave_crtc_state, *master_crtc_state; > struct intel_crtc *slave_crtc, *master_crtc; > > @@ -7615,6 +7624,13 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, > if (!new_crtc_state->bigjoiner) > return 0; > > + if ((bigjoiner_master_pipes(i915) & BIT(crtc->pipe)) == 0) { Feels like the check should be in intel_dsc_get_bigjoiner_{secondary,primary}. They already contain the check that the next/prev pipe exists, which (silly me) I thought was enough. BR, Jani. > + drm_dbg_kms(&i915->drm, > + "[CRTC:%d:%s] Bigjoiner not available on this pipe\n", > + crtc->base.base.id, crtc->base.name); > + return -EINVAL; > + } > + > slave_crtc = intel_dsc_get_bigjoiner_secondary(crtc); > if (!slave_crtc) { > DRM_DEBUG_KMS("[CRTC:%d:%s] Big joiner configuration requires "
On Fri, Jan 21, 2022 at 04:03:09PM +0200, Jani Nikula wrote: > On Fri, 21 Jan 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Check that our crtc can in fact be the bigjoiner master before > > we let the modeset proceed with bigjoiner enabled. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 0964b2403e2d..36e547bd0cbe 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -4102,6 +4102,14 @@ static u8 bigjoiner_pipes(struct drm_i915_private *i915) > > return 0; > > } > > > > +static u8 bigjoiner_master_pipes(struct drm_i915_private *i915) > > +{ > > + u8 pipes = bigjoiner_pipes(i915); > > + > > + /* last pipe can not be master */ > > + return pipes & (pipes >> 1); > > +} > > + > > static bool transcoder_ddi_func_is_enabled(struct drm_i915_private *dev_priv, > > enum transcoder cpu_transcoder) > > { > > @@ -7600,6 +7608,7 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, > > struct intel_crtc_state *old_crtc_state, > > struct intel_crtc_state *new_crtc_state) > > { > > + struct drm_i915_private *i915 = to_i915(state->base.dev); > > struct intel_crtc_state *slave_crtc_state, *master_crtc_state; > > struct intel_crtc *slave_crtc, *master_crtc; > > > > @@ -7615,6 +7624,13 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, > > if (!new_crtc_state->bigjoiner) > > return 0; > > > > + if ((bigjoiner_master_pipes(i915) & BIT(crtc->pipe)) == 0) { > > Feels like the check should be in > intel_dsc_get_bigjoiner_{secondary,primary}. The master pipe is selected by userspace. intel_dsc_get_bigjoiner_primary() is not relevant here. side note: A lot of that code really should be moved out of intel_vdsc.c since we also have the uncompressed joiner to consider. Should probably do a global s/bigjoiner/joiner/ as well since afaik "bigjoiner" refers specifically to the compressed joiner.
On Fri, Jan 21, 2022 at 05:55:24PM +0200, Ville Syrjälä wrote: > On Fri, Jan 21, 2022 at 04:03:09PM +0200, Jani Nikula wrote: > > On Fri, 21 Jan 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Check that our crtc can in fact be the bigjoiner master before > > > we let the modeset proceed with bigjoiner enabled. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index 0964b2403e2d..36e547bd0cbe 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -4102,6 +4102,14 @@ static u8 bigjoiner_pipes(struct drm_i915_private *i915) > > > return 0; > > > } > > > > > > +static u8 bigjoiner_master_pipes(struct drm_i915_private *i915) > > > +{ > > > + u8 pipes = bigjoiner_pipes(i915); > > > + > > > + /* last pipe can not be master */ > > > + return pipes & (pipes >> 1); > > > +} > > > + > > > static bool transcoder_ddi_func_is_enabled(struct drm_i915_private *dev_priv, > > > enum transcoder cpu_transcoder) > > > { > > > @@ -7600,6 +7608,7 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, > > > struct intel_crtc_state *old_crtc_state, > > > struct intel_crtc_state *new_crtc_state) > > > { > > > + struct drm_i915_private *i915 = to_i915(state->base.dev); > > > struct intel_crtc_state *slave_crtc_state, *master_crtc_state; > > > struct intel_crtc *slave_crtc, *master_crtc; > > > > > > @@ -7615,6 +7624,13 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, > > > if (!new_crtc_state->bigjoiner) > > > return 0; > > > > > > + if ((bigjoiner_master_pipes(i915) & BIT(crtc->pipe)) == 0) { > > > > Feels like the check should be in > > intel_dsc_get_bigjoiner_{secondary,primary}. > > The master pipe is selected by userspace. intel_dsc_get_bigjoiner_primary() > is not relevant here. Hmm. I guess the intel_dsc_get_bigjoiner_secondary()->NULL is in fact sufficient on current hardware since only pipe A can be non-joinable, and the dsc code already rejects the transcoder A case. Although it only claims to do that due to it not supporting dsc, but luckily that happens to be the same thing as no-joiner since there is no uncompressed joiner either. But to be 100% clear should probably do something along the lines proposed here. I'm actually think we nuke all these crtc pointers/etc. and just go for some kind of pipe bitmask based approach everywhere. So this probaly needs a bit more through all around.
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 0964b2403e2d..36e547bd0cbe 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -4102,6 +4102,14 @@ static u8 bigjoiner_pipes(struct drm_i915_private *i915) return 0; } +static u8 bigjoiner_master_pipes(struct drm_i915_private *i915) +{ + u8 pipes = bigjoiner_pipes(i915); + + /* last pipe can not be master */ + return pipes & (pipes >> 1); +} + static bool transcoder_ddi_func_is_enabled(struct drm_i915_private *dev_priv, enum transcoder cpu_transcoder) { @@ -7600,6 +7608,7 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, struct intel_crtc_state *old_crtc_state, struct intel_crtc_state *new_crtc_state) { + struct drm_i915_private *i915 = to_i915(state->base.dev); struct intel_crtc_state *slave_crtc_state, *master_crtc_state; struct intel_crtc *slave_crtc, *master_crtc; @@ -7615,6 +7624,13 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state, if (!new_crtc_state->bigjoiner) return 0; + if ((bigjoiner_master_pipes(i915) & BIT(crtc->pipe)) == 0) { + drm_dbg_kms(&i915->drm, + "[CRTC:%d:%s] Bigjoiner not available on this pipe\n", + crtc->base.base.id, crtc->base.name); + return -EINVAL; + } + slave_crtc = intel_dsc_get_bigjoiner_secondary(crtc); if (!slave_crtc) { DRM_DEBUG_KMS("[CRTC:%d:%s] Big joiner configuration requires "