| Message ID | 20210514153711.2359617-16-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Another batch of reviewed XeLPD / ADL-P patches | expand |
On Fri, 14 May 2021, Matt Roper <matthew.d.roper@intel.com> wrote: > From: Animesh Manna <animesh.manna@intel.com> > > Respective bit for master or slave to be set for uncompressed > bigjoiner in dss_ctl1 register. I was looking at the changes here due to a static checker complaint, and I think there are a number of issues here. Some more serious than others, and some predate the patch. Comments inline. > Cc: Manasi Navare <manasi.d.navare@intel.com> > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > Signed-off-by: Clinton Taylor <Clinton.A.Taylor@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 6 +++ > drivers/gpu/drm/i915/display/intel_vdsc.c | 40 +++++++++++++++++++- > drivers/gpu/drm/i915/display/intel_vdsc.h | 2 + > drivers/gpu/drm/i915/i915_reg.h | 2 + > 4 files changed, 49 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index b5fd721137d3..422b59ebf6dc 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -3411,6 +3411,7 @@ static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state, > const struct intel_crtc_state *crtc_state) > { > struct intel_crtc *master = to_intel_crtc(crtc_state->uapi.crtc); > + struct drm_i915_private *dev_priv = to_i915(master->base.dev); > struct intel_crtc_state *master_crtc_state; > struct drm_connector_state *conn_state; > struct drm_connector *conn; > @@ -3444,6 +3445,9 @@ static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state, > /* and DSC on slave */ > intel_dsc_enable(NULL, crtc_state); > } > + > + if (DISPLAY_VER(dev_priv) >= 13) > + intel_uncompressed_joiner_enable(crtc_state); > } > > static void hsw_crtc_enable(struct intel_atomic_state *state, > @@ -6250,6 +6254,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc, > } > > intel_dsc_get_config(pipe_config); > + if (DISPLAY_VER(dev_priv) >= 13 && !pipe_config->dsc.compression_enable) > + intel_uncompressed_joiner_get_config(pipe_config); > > if (!active) { > /* bigjoiner slave doesn't enable transcoder */ > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c > index adcd6752f919..efc3184d8315 100644 > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c > @@ -1021,6 +1021,22 @@ static i915_reg_t dss_ctl2_reg(const struct intel_crtc_state *crtc_state) > return is_pipe_dsc(crtc_state) ? ICL_PIPE_DSS_CTL2(pipe) : DSS_CTL2; > } > > +void intel_uncompressed_joiner_enable(const struct intel_crtc_state *crtc_state) Naming. Basically for any new function, the function name prefix should match the file name. intel_vdsc.[ch] should have functions prefixed intel_vdsc_*(). This is where we're headed to increase clarity. intel_uncompressed_*() is something completely different. Granted, here we already have intel_dsc_*() in intel_vdsc.c. We should probably stick with intel_dsc_*(). A possible function or file rename is not out of the question, but that's a separate matter. > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + u32 dss_ctl1_val = 0; > + > + if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { > + if (crtc_state->bigjoiner_slave) > + dss_ctl1_val |= UNCOMPRESSED_JOINER_SLAVE; > + else > + dss_ctl1_val |= UNCOMPRESSED_JOINER_MASTER; > + > + intel_de_write(dev_priv, dss_ctl1_reg(crtc_state), dss_ctl1_val); > + } > +} > + > void intel_dsc_enable(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state) > { > @@ -1060,13 +1076,35 @@ void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state) > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > - if (!old_crtc_state->dsc.compression_enable) > + if (!(old_crtc_state->dsc.compression_enable && > + old_crtc_state->bigjoiner)) This fails to disable compression if we only have compression but no bigjoiner, which is the more common case! See also: https://gitlab.freedesktop.org/drm/intel/-/issues/3537 https://patchwork.freedesktop.org/patch/msgid/20210603065356.15435-1-vandita.kulkarni@intel.com > return; > > intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0); > intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0); > } > > +void intel_uncompressed_joiner_get_config(struct intel_crtc_state *crtc_state) > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + u32 dss_ctl1; > + > + dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg(crtc_state)); > + if (dss_ctl1 & UNCOMPRESSED_JOINER_MASTER) { > + crtc_state->bigjoiner = true; > + if (!WARN_ON(INTEL_NUM_PIPES(dev_priv) == crtc->pipe + 1)) > + crtc_state->bigjoiner_linked_crtc = > + intel_get_crtc_for_pipe(dev_priv, crtc->pipe + 1); > + } else if (dss_ctl1 & UNCOMPRESSED_JOINER_SLAVE) { > + crtc_state->bigjoiner = true; > + crtc_state->bigjoiner_slave = true; > + if (!WARN_ON(crtc->pipe == PIPE_A)) > + crtc_state->bigjoiner_linked_crtc = > + intel_get_crtc_for_pipe(dev_priv, crtc->pipe - 1); > + } Nitpick: This duplicates a bunch of logic for figuring out master/slave. The static checker warning was about crtc->pipe + 1 usage. Since INTEL_NUM_PIPES() looks at the hamming weight of i915->pipe_mask, the checker has a hard time figuring out it does not overflow i915->pipe_to_crtc_mapping[] in intel_get_crtc_for_pipe(). So here in intel_vdsc.c the checks are for overflowing/underflowing the pipe range. In intel_get_crtc_for_pipe() there's a check for the pipe actually existing - the pipe numbering might not be contiguous. Superficially the static checker warning is bogus, as in we won't overflow anything. However, deep down there are issues in the consistency of the checks and how to handle non-contigouous pipe numbering. Indeed, this does not *need* the number. We should figure out the next *crtc*, not the next pipe *number*, which may or may not be pipe + 1. BR, Jani. > +} > + > void intel_dsc_get_config(struct intel_crtc_state *crtc_state) > { > struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.h b/drivers/gpu/drm/i915/display/intel_vdsc.h > index 65d301c23580..fe4d45561253 100644 > --- a/drivers/gpu/drm/i915/display/intel_vdsc.h > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h > @@ -12,11 +12,13 @@ struct intel_encoder; > struct intel_crtc_state; > > bool intel_dsc_source_support(const struct intel_crtc_state *crtc_state); > +void intel_uncompressed_joiner_enable(const struct intel_crtc_state *crtc_state); > void intel_dsc_enable(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state); > void intel_dsc_disable(const struct intel_crtc_state *crtc_state); > int intel_dsc_compute_params(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config); > +void intel_uncompressed_joiner_get_config(struct intel_crtc_state *crtc_state); > void intel_dsc_get_config(struct intel_crtc_state *crtc_state); > enum intel_display_power_domain > intel_dsc_power_domain(const struct intel_crtc_state *crtc_state); > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 31bc413dbba1..dd6e0bae9573 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -11493,6 +11493,8 @@ enum skl_power_gate { > #define SPLITTER_CONFIGURATION_MASK REG_GENMASK(26, 25) > #define SPLITTER_CONFIGURATION_2_SEGMENT REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 0) > #define SPLITTER_CONFIGURATION_4_SEGMENT REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 1) > +#define UNCOMPRESSED_JOINER_MASTER (1 << 21) > +#define UNCOMPRESSED_JOINER_SLAVE (1 << 20) > > #define _ICL_PIPE_DSS_CTL2_PB 0x78204 > #define _ICL_PIPE_DSS_CTL2_PC 0x78404
On Thu, 03 Jun 2021, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Fri, 14 May 2021, Matt Roper <matthew.d.roper@intel.com> wrote: >> From: Animesh Manna <animesh.manna@intel.com> >> >> Respective bit for master or slave to be set for uncompressed >> bigjoiner in dss_ctl1 register. > > I was looking at the changes here due to a static checker complaint, and > I think there are a number of issues here. Some more serious than > others, and some predate the patch. > > Comments inline. > >> Cc: Manasi Navare <manasi.d.navare@intel.com> >> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >> Signed-off-by: Clinton Taylor <Clinton.A.Taylor@intel.com> >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 6 +++ >> drivers/gpu/drm/i915/display/intel_vdsc.c | 40 +++++++++++++++++++- >> drivers/gpu/drm/i915/display/intel_vdsc.h | 2 + >> drivers/gpu/drm/i915/i915_reg.h | 2 + >> 4 files changed, 49 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index b5fd721137d3..422b59ebf6dc 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -3411,6 +3411,7 @@ static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state, >> const struct intel_crtc_state *crtc_state) >> { >> struct intel_crtc *master = to_intel_crtc(crtc_state->uapi.crtc); >> + struct drm_i915_private *dev_priv = to_i915(master->base.dev); >> struct intel_crtc_state *master_crtc_state; >> struct drm_connector_state *conn_state; >> struct drm_connector *conn; >> @@ -3444,6 +3445,9 @@ static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state, >> /* and DSC on slave */ >> intel_dsc_enable(NULL, crtc_state); >> } >> + >> + if (DISPLAY_VER(dev_priv) >= 13) I don't think we should add these checks here. Make sure the crtc_state only has the relevant stuff enabled if the platform supports it. Don't duplicate the checks. >> + intel_uncompressed_joiner_enable(crtc_state); As this is always called after intel_dsc_enable(), I think it would make sense to move this within intel_dsc_enable(). >> } >> >> static void hsw_crtc_enable(struct intel_atomic_state *state, >> @@ -6250,6 +6254,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc, >> } >> >> intel_dsc_get_config(pipe_config); >> + if (DISPLAY_VER(dev_priv) >= 13 && !pipe_config->dsc.compression_enable) >> + intel_uncompressed_joiner_get_config(pipe_config); As this is always called after intel_dsc_get_config(), I think it would make sense to move this within intel_dsc_get_config. >> >> if (!active) { >> /* bigjoiner slave doesn't enable transcoder */ >> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c >> index adcd6752f919..efc3184d8315 100644 >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c >> @@ -1021,6 +1021,22 @@ static i915_reg_t dss_ctl2_reg(const struct intel_crtc_state *crtc_state) >> return is_pipe_dsc(crtc_state) ? ICL_PIPE_DSS_CTL2(pipe) : DSS_CTL2; >> } >> >> +void intel_uncompressed_joiner_enable(const struct intel_crtc_state *crtc_state) > > Naming. Basically for any new function, the function name prefix should > match the file name. intel_vdsc.[ch] should have functions prefixed > intel_vdsc_*(). This is where we're headed to increase clarity. > > intel_uncompressed_*() is something completely different. > > Granted, here we already have intel_dsc_*() in intel_vdsc.c. We should > probably stick with intel_dsc_*(). A possible function or file rename is > not out of the question, but that's a separate matter. > >> +{ >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> + u32 dss_ctl1_val = 0; >> + >> + if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { >> + if (crtc_state->bigjoiner_slave) >> + dss_ctl1_val |= UNCOMPRESSED_JOINER_SLAVE; >> + else >> + dss_ctl1_val |= UNCOMPRESSED_JOINER_MASTER; >> + >> + intel_de_write(dev_priv, dss_ctl1_reg(crtc_state), dss_ctl1_val); >> + } >> +} >> + >> void intel_dsc_enable(struct intel_encoder *encoder, >> const struct intel_crtc_state *crtc_state) >> { >> @@ -1060,13 +1076,35 @@ void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state) >> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> >> - if (!old_crtc_state->dsc.compression_enable) >> + if (!(old_crtc_state->dsc.compression_enable && >> + old_crtc_state->bigjoiner)) > > This fails to disable compression if we only have compression but no > bigjoiner, which is the more common case! > > See also: > > https://gitlab.freedesktop.org/drm/intel/-/issues/3537 > https://patchwork.freedesktop.org/patch/msgid/20210603065356.15435-1-vandita.kulkarni@intel.com > >> return; >> >> intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0); >> intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0); >> } >> >> +void intel_uncompressed_joiner_get_config(struct intel_crtc_state *crtc_state) >> +{ >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> + u32 dss_ctl1; >> + >> + dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg(crtc_state)); You can't read this without holding the power domain. Since this is always called after intel_dsc_get_config(), I think it would make sense to move this reading there. >> + if (dss_ctl1 & UNCOMPRESSED_JOINER_MASTER) { >> + crtc_state->bigjoiner = true; >> + if (!WARN_ON(INTEL_NUM_PIPES(dev_priv) == crtc->pipe + 1)) >> + crtc_state->bigjoiner_linked_crtc = >> + intel_get_crtc_for_pipe(dev_priv, crtc->pipe + 1); >> + } else if (dss_ctl1 & UNCOMPRESSED_JOINER_SLAVE) { >> + crtc_state->bigjoiner = true; >> + crtc_state->bigjoiner_slave = true; >> + if (!WARN_ON(crtc->pipe == PIPE_A)) >> + crtc_state->bigjoiner_linked_crtc = >> + intel_get_crtc_for_pipe(dev_priv, crtc->pipe - 1); >> + } > > Nitpick: This duplicates a bunch of logic for figuring out master/slave. > > The static checker warning was about crtc->pipe + 1 usage. Since > INTEL_NUM_PIPES() looks at the hamming weight of i915->pipe_mask, the > checker has a hard time figuring out it does not overflow > i915->pipe_to_crtc_mapping[] in intel_get_crtc_for_pipe(). > > So here in intel_vdsc.c the checks are for overflowing/underflowing the > pipe range. In intel_get_crtc_for_pipe() there's a check for the pipe > actually existing - the pipe numbering might not be contiguous. > > Superficially the static checker warning is bogus, as in we won't > overflow anything. However, deep down there are issues in the > consistency of the checks and how to handle non-contigouous pipe > numbering. > > Indeed, this does not *need* the number. We should figure out the next > *crtc*, not the next pipe *number*, which may or may not be pipe + 1. > > > BR, > Jani. > >> +} >> + >> void intel_dsc_get_config(struct intel_crtc_state *crtc_state) >> { >> struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; >> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.h b/drivers/gpu/drm/i915/display/intel_vdsc.h >> index 65d301c23580..fe4d45561253 100644 >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.h >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h >> @@ -12,11 +12,13 @@ struct intel_encoder; >> struct intel_crtc_state; >> >> bool intel_dsc_source_support(const struct intel_crtc_state *crtc_state); >> +void intel_uncompressed_joiner_enable(const struct intel_crtc_state *crtc_state); >> void intel_dsc_enable(struct intel_encoder *encoder, >> const struct intel_crtc_state *crtc_state); >> void intel_dsc_disable(const struct intel_crtc_state *crtc_state); >> int intel_dsc_compute_params(struct intel_encoder *encoder, >> struct intel_crtc_state *pipe_config); >> +void intel_uncompressed_joiner_get_config(struct intel_crtc_state *crtc_state); >> void intel_dsc_get_config(struct intel_crtc_state *crtc_state); >> enum intel_display_power_domain >> intel_dsc_power_domain(const struct intel_crtc_state *crtc_state); >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 31bc413dbba1..dd6e0bae9573 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -11493,6 +11493,8 @@ enum skl_power_gate { >> #define SPLITTER_CONFIGURATION_MASK REG_GENMASK(26, 25) >> #define SPLITTER_CONFIGURATION_2_SEGMENT REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 0) >> #define SPLITTER_CONFIGURATION_4_SEGMENT REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 1) >> +#define UNCOMPRESSED_JOINER_MASTER (1 << 21) >> +#define UNCOMPRESSED_JOINER_SLAVE (1 << 20) >> >> #define _ICL_PIPE_DSS_CTL2_PB 0x78204 >> #define _ICL_PIPE_DSS_CTL2_PC 0x78404
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Thursday, June 3, 2021 3:10 PM > To: Roper, Matthew D <matthew.d.roper@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Manna, Animesh <animesh.manna@intel.com>; Navare, Manasi D > <manasi.d.navare@intel.com>; Kulkarni, Vandita <vandita.kulkarni@intel.com> > Subject: Re: [Intel-gfx] [CI 15/19] drm/i915/bigjoiner: atomic commit changes > for uncompressed joiner > > On Fri, 14 May 2021, Matt Roper <matthew.d.roper@intel.com> wrote: > > From: Animesh Manna <animesh.manna@intel.com> > > > > Respective bit for master or slave to be set for uncompressed > > bigjoiner in dss_ctl1 register. > > I was looking at the changes here due to a static checker complaint, and I think > there are a number of issues here. Some more serious than others, and some > predate the patch. > > Comments inline. > > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > > Signed-off-by: Clinton Taylor <Clinton.A.Taylor@intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 6 +++ > > drivers/gpu/drm/i915/display/intel_vdsc.c | 40 +++++++++++++++++++- > > drivers/gpu/drm/i915/display/intel_vdsc.h | 2 + > > drivers/gpu/drm/i915/i915_reg.h | 2 + > > 4 files changed, 49 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index b5fd721137d3..422b59ebf6dc 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -3411,6 +3411,7 @@ static void icl_ddi_bigjoiner_pre_enable(struct > intel_atomic_state *state, > > const struct intel_crtc_state > *crtc_state) { > > struct intel_crtc *master = to_intel_crtc(crtc_state->uapi.crtc); > > + struct drm_i915_private *dev_priv = to_i915(master->base.dev); > > struct intel_crtc_state *master_crtc_state; > > struct drm_connector_state *conn_state; > > struct drm_connector *conn; > > @@ -3444,6 +3445,9 @@ static void icl_ddi_bigjoiner_pre_enable(struct > intel_atomic_state *state, > > /* and DSC on slave */ > > intel_dsc_enable(NULL, crtc_state); > > } > > + > > + if (DISPLAY_VER(dev_priv) >= 13) > > + intel_uncompressed_joiner_enable(crtc_state); > > } > > > > static void hsw_crtc_enable(struct intel_atomic_state *state, @@ > > -6250,6 +6254,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc, > > } > > > > intel_dsc_get_config(pipe_config); > > + if (DISPLAY_VER(dev_priv) >= 13 && !pipe_config- > >dsc.compression_enable) > > + intel_uncompressed_joiner_get_config(pipe_config); > > > > if (!active) { > > /* bigjoiner slave doesn't enable transcoder */ diff --git > > a/drivers/gpu/drm/i915/display/intel_vdsc.c > > b/drivers/gpu/drm/i915/display/intel_vdsc.c > > index adcd6752f919..efc3184d8315 100644 > > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c > > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c > > @@ -1021,6 +1021,22 @@ static i915_reg_t dss_ctl2_reg(const struct > intel_crtc_state *crtc_state) > > return is_pipe_dsc(crtc_state) ? ICL_PIPE_DSS_CTL2(pipe) : DSS_CTL2; > > } > > > > +void intel_uncompressed_joiner_enable(const struct intel_crtc_state > > +*crtc_state) > > Naming. Basically for any new function, the function name prefix should match > the file name. intel_vdsc.[ch] should have functions prefixed intel_vdsc_*(). This > is where we're headed to increase clarity. > > intel_uncompressed_*() is something completely different. > > Granted, here we already have intel_dsc_*() in intel_vdsc.c. We should probably > stick with intel_dsc_*(). A possible function or file rename is not out of the > question, but that's a separate matter. As there is not separate register for uncompressed joiner, using bitfield of dsc register only the function name can be changed to intel_dsc_uncompressed_joiner_enable(). > > > +{ > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + u32 dss_ctl1_val = 0; > > + > > + if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { > > + if (crtc_state->bigjoiner_slave) > > + dss_ctl1_val |= UNCOMPRESSED_JOINER_SLAVE; > > + else > > + dss_ctl1_val |= UNCOMPRESSED_JOINER_MASTER; > > + > > + intel_de_write(dev_priv, dss_ctl1_reg(crtc_state), dss_ctl1_val); > > + } > > +} > > + > > void intel_dsc_enable(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state) { @@ -1060,13 > > +1076,35 @@ void intel_dsc_disable(const struct intel_crtc_state > *old_crtc_state) > > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > > > - if (!old_crtc_state->dsc.compression_enable) > > + if (!(old_crtc_state->dsc.compression_enable && > > + old_crtc_state->bigjoiner)) > > This fails to disable compression if we only have compression but no bigjoiner, > which is the more common case! > > See also: > > https://gitlab.freedesktop.org/drm/intel/-/issues/3537 > https://patchwork.freedesktop.org/patch/msgid/20210603065356.15435-1- > vandita.kulkarni@intel.com > We may need to remove both the condition check. In uncompressed bigjoiner, compression_enable flag will be 0 and may not clear the bit of dss_ctl1_reg. So can we remove both the checks, hoping it will not harm reseting the register even if it is not set. > > return; > > > > intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0); > > intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0); } > > > > +void intel_uncompressed_joiner_get_config(struct intel_crtc_state > > +*crtc_state) { > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + u32 dss_ctl1; > > + > > + dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg(crtc_state)); > > + if (dss_ctl1 & UNCOMPRESSED_JOINER_MASTER) { > > + crtc_state->bigjoiner = true; > > + if (!WARN_ON(INTEL_NUM_PIPES(dev_priv) == crtc->pipe + 1)) > > + crtc_state->bigjoiner_linked_crtc = > > + intel_get_crtc_for_pipe(dev_priv, crtc->pipe + > 1); > > + } else if (dss_ctl1 & UNCOMPRESSED_JOINER_SLAVE) { > > + crtc_state->bigjoiner = true; > > + crtc_state->bigjoiner_slave = true; > > + if (!WARN_ON(crtc->pipe == PIPE_A)) > > + crtc_state->bigjoiner_linked_crtc = > > + intel_get_crtc_for_pipe(dev_priv, crtc->pipe - > 1); > > + } > > Nitpick: This duplicates a bunch of logic for figuring out master/slave. > > The static checker warning was about crtc->pipe + 1 usage. Since > INTEL_NUM_PIPES() looks at the hamming weight of i915->pipe_mask, the > checker has a hard time figuring out it does not overflow > i915->pipe_to_crtc_mapping[] in intel_get_crtc_for_pipe(). > > So here in intel_vdsc.c the checks are for overflowing/underflowing the pipe > range. In intel_get_crtc_for_pipe() there's a check for the pipe actually existing - > the pipe numbering might not be contiguous. > > Superficially the static checker warning is bogus, as in we won't overflow > anything. However, deep down there are issues in the consistency of the checks > and how to handle non-contigouous pipe numbering. > > Indeed, this does not *need* the number. We should figure out the next *crtc*, > not the next pipe *number*, which may or may not be pipe + 1. dss_ctl1 value is used to identify master or slave crtc. If needed WARN_ON check can be removed... but pipe enum values like PIPE_A/B/C/D is used to get the master/slave crtc and always bigjoiner is possible with two adjacent pipes. Regards, Animesh > > > BR, > Jani. > > > +} > > + > > void intel_dsc_get_config(struct intel_crtc_state *crtc_state) { > > struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; diff > > --git a/drivers/gpu/drm/i915/display/intel_vdsc.h > > b/drivers/gpu/drm/i915/display/intel_vdsc.h > > index 65d301c23580..fe4d45561253 100644 > > --- a/drivers/gpu/drm/i915/display/intel_vdsc.h > > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h > > @@ -12,11 +12,13 @@ struct intel_encoder; struct intel_crtc_state; > > > > bool intel_dsc_source_support(const struct intel_crtc_state > > *crtc_state); > > +void intel_uncompressed_joiner_enable(const struct intel_crtc_state > > +*crtc_state); > > void intel_dsc_enable(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state); void > > intel_dsc_disable(const struct intel_crtc_state *crtc_state); int > > intel_dsc_compute_params(struct intel_encoder *encoder, > > struct intel_crtc_state *pipe_config); > > +void intel_uncompressed_joiner_get_config(struct intel_crtc_state > > +*crtc_state); > > void intel_dsc_get_config(struct intel_crtc_state *crtc_state); enum > > intel_display_power_domain intel_dsc_power_domain(const struct > > intel_crtc_state *crtc_state); diff --git > > a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 31bc413dbba1..dd6e0bae9573 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -11493,6 +11493,8 @@ enum skl_power_gate { > > #define SPLITTER_CONFIGURATION_MASK REG_GENMASK(26, 25) > > #define SPLITTER_CONFIGURATION_2_SEGMENT > REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 0) > > #define SPLITTER_CONFIGURATION_4_SEGMENT > REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 1) > > +#define UNCOMPRESSED_JOINER_MASTER (1 << 21) > > +#define UNCOMPRESSED_JOINER_SLAVE (1 << 20) > > > > #define _ICL_PIPE_DSS_CTL2_PB 0x78204 > > #define _ICL_PIPE_DSS_CTL2_PC 0x78404 > > -- > Jani Nikula, Intel Open Source Graphics Center
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Thursday, June 3, 2021 6:03 PM > To: Roper, Matthew D <matthew.d.roper@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Manna, Animesh <animesh.manna@intel.com>; Navare, Manasi D > <manasi.d.navare@intel.com>; Kulkarni, Vandita <vandita.kulkarni@intel.com> > Subject: Re: [Intel-gfx] [CI 15/19] drm/i915/bigjoiner: atomic commit changes > for uncompressed joiner > > On Thu, 03 Jun 2021, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Fri, 14 May 2021, Matt Roper <matthew.d.roper@intel.com> wrote: > >> From: Animesh Manna <animesh.manna@intel.com> > >> > >> Respective bit for master or slave to be set for uncompressed > >> bigjoiner in dss_ctl1 register. > > > > I was looking at the changes here due to a static checker complaint, > > and I think there are a number of issues here. Some more serious than > > others, and some predate the patch. > > > > Comments inline. > > > >> Cc: Manasi Navare <manasi.d.navare@intel.com> > >> Signed-off-by: Animesh Manna <animesh.manna@intel.com> > >> Signed-off-by: Clinton Taylor <Clinton.A.Taylor@intel.com> > >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > >> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_display.c | 6 +++ > >> drivers/gpu/drm/i915/display/intel_vdsc.c | 40 +++++++++++++++++++- > >> drivers/gpu/drm/i915/display/intel_vdsc.h | 2 + > >> drivers/gpu/drm/i915/i915_reg.h | 2 + > >> 4 files changed, 49 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c > >> b/drivers/gpu/drm/i915/display/intel_display.c > >> index b5fd721137d3..422b59ebf6dc 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display.c > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >> @@ -3411,6 +3411,7 @@ static void icl_ddi_bigjoiner_pre_enable(struct > intel_atomic_state *state, > >> const struct intel_crtc_state > *crtc_state) { > >> struct intel_crtc *master = to_intel_crtc(crtc_state->uapi.crtc); > >> + struct drm_i915_private *dev_priv = to_i915(master->base.dev); > >> struct intel_crtc_state *master_crtc_state; > >> struct drm_connector_state *conn_state; > >> struct drm_connector *conn; > >> @@ -3444,6 +3445,9 @@ static void icl_ddi_bigjoiner_pre_enable(struct > intel_atomic_state *state, > >> /* and DSC on slave */ > >> intel_dsc_enable(NULL, crtc_state); > >> } > >> + > >> + if (DISPLAY_VER(dev_priv) >= 13) > > I don't think we should add these checks here. Make sure the crtc_state only has > the relevant stuff enabled if the platform supports it. Don't duplicate the checks. Agree. > > >> + intel_uncompressed_joiner_enable(crtc_state); > > As this is always called after intel_dsc_enable(), I think it would make sense to > move this within intel_dsc_enable(). Agree. > > >> } > >> > >> static void hsw_crtc_enable(struct intel_atomic_state *state, @@ > >> -6250,6 +6254,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc, > >> } > >> > >> intel_dsc_get_config(pipe_config); > >> + if (DISPLAY_VER(dev_priv) >= 13 && !pipe_config- > >dsc.compression_enable) > >> + intel_uncompressed_joiner_get_config(pipe_config); > > As this is always called after intel_dsc_get_config(), I think it would make sense > to move this within intel_dsc_get_config. > > >> > >> if (!active) { > >> /* bigjoiner slave doesn't enable transcoder */ diff --git > >> a/drivers/gpu/drm/i915/display/intel_vdsc.c > >> b/drivers/gpu/drm/i915/display/intel_vdsc.c > >> index adcd6752f919..efc3184d8315 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c > >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c > >> @@ -1021,6 +1021,22 @@ static i915_reg_t dss_ctl2_reg(const struct > intel_crtc_state *crtc_state) > >> return is_pipe_dsc(crtc_state) ? ICL_PIPE_DSS_CTL2(pipe) : > >> DSS_CTL2; } > >> > >> +void intel_uncompressed_joiner_enable(const struct intel_crtc_state > >> +*crtc_state) > > > > Naming. Basically for any new function, the function name prefix > > should match the file name. intel_vdsc.[ch] should have functions > > prefixed intel_vdsc_*(). This is where we're headed to increase clarity. > > > > intel_uncompressed_*() is something completely different. > > > > Granted, here we already have intel_dsc_*() in intel_vdsc.c. We should > > probably stick with intel_dsc_*(). A possible function or file rename > > is not out of the question, but that's a separate matter. > > > >> +{ > >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >> + u32 dss_ctl1_val = 0; > >> + > >> + if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { > >> + if (crtc_state->bigjoiner_slave) > >> + dss_ctl1_val |= UNCOMPRESSED_JOINER_SLAVE; > >> + else > >> + dss_ctl1_val |= UNCOMPRESSED_JOINER_MASTER; > >> + > >> + intel_de_write(dev_priv, dss_ctl1_reg(crtc_state), dss_ctl1_val); > >> + } > >> +} > >> + > >> void intel_dsc_enable(struct intel_encoder *encoder, > >> const struct intel_crtc_state *crtc_state) { @@ -1060,13 > >> +1076,35 @@ void intel_dsc_disable(const struct intel_crtc_state > *old_crtc_state) > >> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); > >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >> > >> - if (!old_crtc_state->dsc.compression_enable) > >> + if (!(old_crtc_state->dsc.compression_enable && > >> + old_crtc_state->bigjoiner)) > > > > This fails to disable compression if we only have compression but no > > bigjoiner, which is the more common case! > > > > See also: > > > > https://gitlab.freedesktop.org/drm/intel/-/issues/3537 > > https://patchwork.freedesktop.org/patch/msgid/20210603065356.15435-1-v > > andita.kulkarni@intel.com > > > >> return; > >> > >> intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0); > >> intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0); } > >> > >> +void intel_uncompressed_joiner_get_config(struct intel_crtc_state > >> +*crtc_state) { > >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >> + u32 dss_ctl1; > >> + > >> + dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg(crtc_state)); > > You can't read this without holding the power domain. > > Since this is always called after intel_dsc_get_config(), I think it would make > sense to move this reading there. Ok. Earlier the plan is to have separate functions for uncompressed joiner, but as you suggested it can be done in the same functions of compressed bigjoiner code. Thanks Jani for review. Regards, Animesh > > >> + if (dss_ctl1 & UNCOMPRESSED_JOINER_MASTER) { > >> + crtc_state->bigjoiner = true; > >> + if (!WARN_ON(INTEL_NUM_PIPES(dev_priv) == crtc->pipe + 1)) > >> + crtc_state->bigjoiner_linked_crtc = > >> + intel_get_crtc_for_pipe(dev_priv, crtc->pipe + > 1); > >> + } else if (dss_ctl1 & UNCOMPRESSED_JOINER_SLAVE) { > >> + crtc_state->bigjoiner = true; > >> + crtc_state->bigjoiner_slave = true; > >> + if (!WARN_ON(crtc->pipe == PIPE_A)) > >> + crtc_state->bigjoiner_linked_crtc = > >> + intel_get_crtc_for_pipe(dev_priv, crtc->pipe - > 1); > >> + } > > > > Nitpick: This duplicates a bunch of logic for figuring out master/slave. > > > > The static checker warning was about crtc->pipe + 1 usage. Since > > INTEL_NUM_PIPES() looks at the hamming weight of i915->pipe_mask, the > > checker has a hard time figuring out it does not overflow > > i915->pipe_to_crtc_mapping[] in intel_get_crtc_for_pipe(). > > > > So here in intel_vdsc.c the checks are for overflowing/underflowing > > the pipe range. In intel_get_crtc_for_pipe() there's a check for the > > pipe actually existing - the pipe numbering might not be contiguous. > > > > Superficially the static checker warning is bogus, as in we won't > > overflow anything. However, deep down there are issues in the > > consistency of the checks and how to handle non-contigouous pipe > > numbering. > > > > Indeed, this does not *need* the number. We should figure out the next > > *crtc*, not the next pipe *number*, which may or may not be pipe + 1. > > > > > > BR, > > Jani. > > > >> +} > >> + > >> void intel_dsc_get_config(struct intel_crtc_state *crtc_state) { > >> struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; diff > >> --git a/drivers/gpu/drm/i915/display/intel_vdsc.h > >> b/drivers/gpu/drm/i915/display/intel_vdsc.h > >> index 65d301c23580..fe4d45561253 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.h > >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h > >> @@ -12,11 +12,13 @@ struct intel_encoder; struct intel_crtc_state; > >> > >> bool intel_dsc_source_support(const struct intel_crtc_state > >> *crtc_state); > >> +void intel_uncompressed_joiner_enable(const struct intel_crtc_state > >> +*crtc_state); > >> void intel_dsc_enable(struct intel_encoder *encoder, > >> const struct intel_crtc_state *crtc_state); void > >> intel_dsc_disable(const struct intel_crtc_state *crtc_state); int > >> intel_dsc_compute_params(struct intel_encoder *encoder, > >> struct intel_crtc_state *pipe_config); > >> +void intel_uncompressed_joiner_get_config(struct intel_crtc_state > >> +*crtc_state); > >> void intel_dsc_get_config(struct intel_crtc_state *crtc_state); > >> enum intel_display_power_domain intel_dsc_power_domain(const struct > >> intel_crtc_state *crtc_state); diff --git > >> a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> index 31bc413dbba1..dd6e0bae9573 100644 > >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> @@ -11493,6 +11493,8 @@ enum skl_power_gate { > >> #define SPLITTER_CONFIGURATION_MASK REG_GENMASK(26, 25) > >> #define SPLITTER_CONFIGURATION_2_SEGMENT > REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 0) > >> #define SPLITTER_CONFIGURATION_4_SEGMENT > REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 1) > >> +#define UNCOMPRESSED_JOINER_MASTER (1 << 21) > >> +#define UNCOMPRESSED_JOINER_SLAVE (1 << 20) > >> > >> #define _ICL_PIPE_DSS_CTL2_PB 0x78204 > >> #define _ICL_PIPE_DSS_CTL2_PC 0x78404 > > -- > Jani Nikula, Intel Open Source Graphics Center
On Thu, 03 Jun 2021, "Manna, Animesh" <animesh.manna@intel.com> wrote: >> -----Original Message----- >> From: Jani Nikula <jani.nikula@linux.intel.com> >> Sent: Thursday, June 3, 2021 3:10 PM >> To: Roper, Matthew D <matthew.d.roper@intel.com>; intel- >> gfx@lists.freedesktop.org >> Cc: Manna, Animesh <animesh.manna@intel.com>; Navare, Manasi D >> <manasi.d.navare@intel.com>; Kulkarni, Vandita <vandita.kulkarni@intel.com> >> Subject: Re: [Intel-gfx] [CI 15/19] drm/i915/bigjoiner: atomic commit changes >> for uncompressed joiner >> >> On Fri, 14 May 2021, Matt Roper <matthew.d.roper@intel.com> wrote: >> > From: Animesh Manna <animesh.manna@intel.com> >> > >> > Respective bit for master or slave to be set for uncompressed >> > bigjoiner in dss_ctl1 register. >> >> I was looking at the changes here due to a static checker complaint, and I think >> there are a number of issues here. Some more serious than others, and some >> predate the patch. >> >> Comments inline. >> >> > Cc: Manasi Navare <manasi.d.navare@intel.com> >> > Signed-off-by: Animesh Manna <animesh.manna@intel.com> >> > Signed-off-by: Clinton Taylor <Clinton.A.Taylor@intel.com> >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_display.c | 6 +++ >> > drivers/gpu/drm/i915/display/intel_vdsc.c | 40 +++++++++++++++++++- >> > drivers/gpu/drm/i915/display/intel_vdsc.h | 2 + >> > drivers/gpu/drm/i915/i915_reg.h | 2 + >> > 4 files changed, 49 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c >> > b/drivers/gpu/drm/i915/display/intel_display.c >> > index b5fd721137d3..422b59ebf6dc 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_display.c >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c >> > @@ -3411,6 +3411,7 @@ static void icl_ddi_bigjoiner_pre_enable(struct >> intel_atomic_state *state, >> > const struct intel_crtc_state >> *crtc_state) { >> > struct intel_crtc *master = to_intel_crtc(crtc_state->uapi.crtc); >> > + struct drm_i915_private *dev_priv = to_i915(master->base.dev); >> > struct intel_crtc_state *master_crtc_state; >> > struct drm_connector_state *conn_state; >> > struct drm_connector *conn; >> > @@ -3444,6 +3445,9 @@ static void icl_ddi_bigjoiner_pre_enable(struct >> intel_atomic_state *state, >> > /* and DSC on slave */ >> > intel_dsc_enable(NULL, crtc_state); >> > } >> > + >> > + if (DISPLAY_VER(dev_priv) >= 13) >> > + intel_uncompressed_joiner_enable(crtc_state); >> > } >> > >> > static void hsw_crtc_enable(struct intel_atomic_state *state, @@ >> > -6250,6 +6254,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc, >> > } >> > >> > intel_dsc_get_config(pipe_config); >> > + if (DISPLAY_VER(dev_priv) >= 13 && !pipe_config- >> >dsc.compression_enable) >> > + intel_uncompressed_joiner_get_config(pipe_config); >> > >> > if (!active) { >> > /* bigjoiner slave doesn't enable transcoder */ diff --git >> > a/drivers/gpu/drm/i915/display/intel_vdsc.c >> > b/drivers/gpu/drm/i915/display/intel_vdsc.c >> > index adcd6752f919..efc3184d8315 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c >> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c >> > @@ -1021,6 +1021,22 @@ static i915_reg_t dss_ctl2_reg(const struct >> intel_crtc_state *crtc_state) >> > return is_pipe_dsc(crtc_state) ? ICL_PIPE_DSS_CTL2(pipe) : DSS_CTL2; >> > } >> > >> > +void intel_uncompressed_joiner_enable(const struct intel_crtc_state >> > +*crtc_state) >> >> Naming. Basically for any new function, the function name prefix should match >> the file name. intel_vdsc.[ch] should have functions prefixed intel_vdsc_*(). This >> is where we're headed to increase clarity. >> >> intel_uncompressed_*() is something completely different. >> >> Granted, here we already have intel_dsc_*() in intel_vdsc.c. We should probably >> stick with intel_dsc_*(). A possible function or file rename is not out of the >> question, but that's a separate matter. > > As there is not separate register for uncompressed joiner, using bitfield of dsc register only the function name can be changed to intel_dsc_uncompressed_joiner_enable(). > >> >> > +{ >> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> > + u32 dss_ctl1_val = 0; >> > + >> > + if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { >> > + if (crtc_state->bigjoiner_slave) >> > + dss_ctl1_val |= UNCOMPRESSED_JOINER_SLAVE; >> > + else >> > + dss_ctl1_val |= UNCOMPRESSED_JOINER_MASTER; >> > + >> > + intel_de_write(dev_priv, dss_ctl1_reg(crtc_state), dss_ctl1_val); >> > + } >> > +} >> > + >> > void intel_dsc_enable(struct intel_encoder *encoder, >> > const struct intel_crtc_state *crtc_state) { @@ -1060,13 >> > +1076,35 @@ void intel_dsc_disable(const struct intel_crtc_state >> *old_crtc_state) >> > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); >> > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> > >> > - if (!old_crtc_state->dsc.compression_enable) >> > + if (!(old_crtc_state->dsc.compression_enable && >> > + old_crtc_state->bigjoiner)) >> >> This fails to disable compression if we only have compression but no bigjoiner, >> which is the more common case! >> >> See also: >> >> https://gitlab.freedesktop.org/drm/intel/-/issues/3537 >> https://patchwork.freedesktop.org/patch/msgid/20210603065356.15435-1- >> vandita.kulkarni@intel.com >> > > We may need to remove both the condition check. > In uncompressed bigjoiner, compression_enable flag will be 0 and may not clear the bit of dss_ctl1_reg. > So can we remove both the checks, hoping it will not harm reseting the register even if it is not set. Now it only disables if *both* were enabled. It needs to be disabled if *either* was enabled. But if *neither* was enabled, we don't need to do this. > >> > return; >> > >> > intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0); >> > intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0); } >> > >> > +void intel_uncompressed_joiner_get_config(struct intel_crtc_state >> > +*crtc_state) { >> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> > + u32 dss_ctl1; >> > + >> > + dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg(crtc_state)); >> > + if (dss_ctl1 & UNCOMPRESSED_JOINER_MASTER) { >> > + crtc_state->bigjoiner = true; >> > + if (!WARN_ON(INTEL_NUM_PIPES(dev_priv) == crtc->pipe + 1)) >> > + crtc_state->bigjoiner_linked_crtc = >> > + intel_get_crtc_for_pipe(dev_priv, crtc->pipe + >> 1); >> > + } else if (dss_ctl1 & UNCOMPRESSED_JOINER_SLAVE) { >> > + crtc_state->bigjoiner = true; >> > + crtc_state->bigjoiner_slave = true; >> > + if (!WARN_ON(crtc->pipe == PIPE_A)) >> > + crtc_state->bigjoiner_linked_crtc = >> > + intel_get_crtc_for_pipe(dev_priv, crtc->pipe - >> 1); >> > + } >> >> Nitpick: This duplicates a bunch of logic for figuring out master/slave. >> >> The static checker warning was about crtc->pipe + 1 usage. Since >> INTEL_NUM_PIPES() looks at the hamming weight of i915->pipe_mask, the >> checker has a hard time figuring out it does not overflow >> i915->pipe_to_crtc_mapping[] in intel_get_crtc_for_pipe(). >> >> So here in intel_vdsc.c the checks are for overflowing/underflowing the pipe >> range. In intel_get_crtc_for_pipe() there's a check for the pipe actually existing - >> the pipe numbering might not be contiguous. >> >> Superficially the static checker warning is bogus, as in we won't overflow >> anything. However, deep down there are issues in the consistency of the checks >> and how to handle non-contigouous pipe numbering. >> >> Indeed, this does not *need* the number. We should figure out the next *crtc*, >> not the next pipe *number*, which may or may not be pipe + 1. > > dss_ctl1 value is used to identify master or slave crtc. If needed WARN_ON check can be removed... but pipe enum values like PIPE_A/B/C/D is used to get the master/slave crtc and always bigjoiner is possible with two adjacent pipes. I sent the patch for this and you reviewed it... but are adjacent pipes a requirement for bigjoiner? Would e.g. A+C work if B was fused off? If not, my patch 2/2 is incorrect. BR, Jani. > > Regards, > Animesh > >> >> >> BR, >> Jani. >> >> > +} >> > + >> > void intel_dsc_get_config(struct intel_crtc_state *crtc_state) { >> > struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; diff >> > --git a/drivers/gpu/drm/i915/display/intel_vdsc.h >> > b/drivers/gpu/drm/i915/display/intel_vdsc.h >> > index 65d301c23580..fe4d45561253 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.h >> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h >> > @@ -12,11 +12,13 @@ struct intel_encoder; struct intel_crtc_state; >> > >> > bool intel_dsc_source_support(const struct intel_crtc_state >> > *crtc_state); >> > +void intel_uncompressed_joiner_enable(const struct intel_crtc_state >> > +*crtc_state); >> > void intel_dsc_enable(struct intel_encoder *encoder, >> > const struct intel_crtc_state *crtc_state); void >> > intel_dsc_disable(const struct intel_crtc_state *crtc_state); int >> > intel_dsc_compute_params(struct intel_encoder *encoder, >> > struct intel_crtc_state *pipe_config); >> > +void intel_uncompressed_joiner_get_config(struct intel_crtc_state >> > +*crtc_state); >> > void intel_dsc_get_config(struct intel_crtc_state *crtc_state); enum >> > intel_display_power_domain intel_dsc_power_domain(const struct >> > intel_crtc_state *crtc_state); diff --git >> > a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> > index 31bc413dbba1..dd6e0bae9573 100644 >> > --- a/drivers/gpu/drm/i915/i915_reg.h >> > +++ b/drivers/gpu/drm/i915/i915_reg.h >> > @@ -11493,6 +11493,8 @@ enum skl_power_gate { >> > #define SPLITTER_CONFIGURATION_MASK REG_GENMASK(26, 25) >> > #define SPLITTER_CONFIGURATION_2_SEGMENT >> REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 0) >> > #define SPLITTER_CONFIGURATION_4_SEGMENT >> REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 1) >> > +#define UNCOMPRESSED_JOINER_MASTER (1 << 21) >> > +#define UNCOMPRESSED_JOINER_SLAVE (1 << 20) >> > >> > #define _ICL_PIPE_DSS_CTL2_PB 0x78204 >> > #define _ICL_PIPE_DSS_CTL2_PC 0x78404 >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
On Thu, Jun 03, 2021 at 06:49:23AM -0700, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Jani Nikula <jani.nikula@linux.intel.com> > > Sent: Thursday, June 3, 2021 6:03 PM > > To: Roper, Matthew D <matthew.d.roper@intel.com>; intel- > > gfx@lists.freedesktop.org > > Cc: Manna, Animesh <animesh.manna@intel.com>; Navare, Manasi D > > <manasi.d.navare@intel.com>; Kulkarni, Vandita <vandita.kulkarni@intel.com> > > Subject: Re: [Intel-gfx] [CI 15/19] drm/i915/bigjoiner: atomic commit changes > > for uncompressed joiner > > > > On Thu, 03 Jun 2021, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > On Fri, 14 May 2021, Matt Roper <matthew.d.roper@intel.com> wrote: > > >> From: Animesh Manna <animesh.manna@intel.com> > > >> > > >> Respective bit for master or slave to be set for uncompressed > > >> bigjoiner in dss_ctl1 register. > > > > > > I was looking at the changes here due to a static checker complaint, > > > and I think there are a number of issues here. Some more serious than > > > others, and some predate the patch. > > > > > > Comments inline. > > > > > >> Cc: Manasi Navare <manasi.d.navare@intel.com> > > >> Signed-off-by: Animesh Manna <animesh.manna@intel.com> > > >> Signed-off-by: Clinton Taylor <Clinton.A.Taylor@intel.com> > > >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > >> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > > >> --- > > >> drivers/gpu/drm/i915/display/intel_display.c | 6 +++ > > >> drivers/gpu/drm/i915/display/intel_vdsc.c | 40 +++++++++++++++++++- > > >> drivers/gpu/drm/i915/display/intel_vdsc.h | 2 + > > >> drivers/gpu/drm/i915/i915_reg.h | 2 + > > >> 4 files changed, 49 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > >> b/drivers/gpu/drm/i915/display/intel_display.c > > >> index b5fd721137d3..422b59ebf6dc 100644 > > >> --- a/drivers/gpu/drm/i915/display/intel_display.c > > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c > > >> @@ -3411,6 +3411,7 @@ static void icl_ddi_bigjoiner_pre_enable(struct > > intel_atomic_state *state, > > >> const struct intel_crtc_state > > *crtc_state) { > > >> struct intel_crtc *master = to_intel_crtc(crtc_state->uapi.crtc); > > >> + struct drm_i915_private *dev_priv = to_i915(master->base.dev); > > >> struct intel_crtc_state *master_crtc_state; > > >> struct drm_connector_state *conn_state; > > >> struct drm_connector *conn; > > >> @@ -3444,6 +3445,9 @@ static void icl_ddi_bigjoiner_pre_enable(struct > > intel_atomic_state *state, > > >> /* and DSC on slave */ > > >> intel_dsc_enable(NULL, crtc_state); > > >> } > > >> + > > >> + if (DISPLAY_VER(dev_priv) >= 13) > > > > I don't think we should add these checks here. Make sure the crtc_state only has > > the relevant stuff enabled if the platform supports it. Don't duplicate the checks. > > Agree. > > > > > >> + intel_uncompressed_joiner_enable(crtc_state); > > > > As this is always called after intel_dsc_enable(), I think it would make sense to > > move this within intel_dsc_enable(). > > Agree. @Jani I had asked to create a separate joiner enable since having an uncompressed enable inside dsc_enable just from the names makes it contradicting. But sure may be with a comment before calling uncompressed_joiner_enable() it can be just called from inside dsc enable() And instead of checking the display ver there we just use this condition: if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { intel_uncompressed_joiner_enable(crtc_state); } Since the atomic check ensures that only for ver >=13 is when we allow big joiner and no DSC combination. Animesh, Jani does that sound good? Manasi > > > > >> } > > >> > > >> static void hsw_crtc_enable(struct intel_atomic_state *state, @@ > > >> -6250,6 +6254,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc, > > >> } > > >> > > >> intel_dsc_get_config(pipe_config); > > >> + if (DISPLAY_VER(dev_priv) >= 13 && !pipe_config- > > >dsc.compression_enable) > > >> + intel_uncompressed_joiner_get_config(pipe_config); > > > > As this is always called after intel_dsc_get_config(), I think it would make sense > > to move this within intel_dsc_get_config. > > > > >> > > >> if (!active) { > > >> /* bigjoiner slave doesn't enable transcoder */ diff --git > > >> a/drivers/gpu/drm/i915/display/intel_vdsc.c > > >> b/drivers/gpu/drm/i915/display/intel_vdsc.c > > >> index adcd6752f919..efc3184d8315 100644 > > >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c > > >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c > > >> @@ -1021,6 +1021,22 @@ static i915_reg_t dss_ctl2_reg(const struct > > intel_crtc_state *crtc_state) > > >> return is_pipe_dsc(crtc_state) ? ICL_PIPE_DSS_CTL2(pipe) : > > >> DSS_CTL2; } > > >> > > >> +void intel_uncompressed_joiner_enable(const struct intel_crtc_state > > >> +*crtc_state) > > > > > > Naming. Basically for any new function, the function name prefix > > > should match the file name. intel_vdsc.[ch] should have functions > > > prefixed intel_vdsc_*(). This is where we're headed to increase clarity. > > > > > > intel_uncompressed_*() is something completely different. > > > > > > Granted, here we already have intel_dsc_*() in intel_vdsc.c. We should > > > probably stick with intel_dsc_*(). A possible function or file rename > > > is not out of the question, but that's a separate matter. > > > > > >> +{ > > >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > >> + u32 dss_ctl1_val = 0; > > >> + > > >> + if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { > > >> + if (crtc_state->bigjoiner_slave) > > >> + dss_ctl1_val |= UNCOMPRESSED_JOINER_SLAVE; > > >> + else > > >> + dss_ctl1_val |= UNCOMPRESSED_JOINER_MASTER; > > >> + > > >> + intel_de_write(dev_priv, dss_ctl1_reg(crtc_state), dss_ctl1_val); > > >> + } > > >> +} > > >> + > > >> void intel_dsc_enable(struct intel_encoder *encoder, > > >> const struct intel_crtc_state *crtc_state) { @@ -1060,13 > > >> +1076,35 @@ void intel_dsc_disable(const struct intel_crtc_state > > *old_crtc_state) > > >> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); > > >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > >> > > >> - if (!old_crtc_state->dsc.compression_enable) > > >> + if (!(old_crtc_state->dsc.compression_enable && > > >> + old_crtc_state->bigjoiner)) > > > > > > This fails to disable compression if we only have compression but no > > > bigjoiner, which is the more common case! > > > > > > See also: > > > > > > https://gitlab.freedesktop.org/drm/intel/-/issues/3537 > > > https://patchwork.freedesktop.org/patch/msgid/20210603065356.15435-1-v > > > andita.kulkarni@intel.com > > > > > >> return; > > >> > > >> intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0); > > >> intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0); } > > >> > > >> +void intel_uncompressed_joiner_get_config(struct intel_crtc_state > > >> +*crtc_state) { > > >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > >> + u32 dss_ctl1; > > >> + > > >> + dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg(crtc_state)); > > > > You can't read this without holding the power domain. > > > > Since this is always called after intel_dsc_get_config(), I think it would make > > sense to move this reading there. > > Ok. > Earlier the plan is to have separate functions for uncompressed joiner, but as you suggested it can be done in the same functions of compressed bigjoiner code. > Thanks Jani for review. > > Regards, > Animesh > > > > > >> + if (dss_ctl1 & UNCOMPRESSED_JOINER_MASTER) { > > >> + crtc_state->bigjoiner = true; > > >> + if (!WARN_ON(INTEL_NUM_PIPES(dev_priv) == crtc->pipe + 1)) > > >> + crtc_state->bigjoiner_linked_crtc = > > >> + intel_get_crtc_for_pipe(dev_priv, crtc->pipe + > > 1); > > >> + } else if (dss_ctl1 & UNCOMPRESSED_JOINER_SLAVE) { > > >> + crtc_state->bigjoiner = true; > > >> + crtc_state->bigjoiner_slave = true; > > >> + if (!WARN_ON(crtc->pipe == PIPE_A)) > > >> + crtc_state->bigjoiner_linked_crtc = > > >> + intel_get_crtc_for_pipe(dev_priv, crtc->pipe - > > 1); > > >> + } > > > > > > Nitpick: This duplicates a bunch of logic for figuring out master/slave. > > > > > > The static checker warning was about crtc->pipe + 1 usage. Since > > > INTEL_NUM_PIPES() looks at the hamming weight of i915->pipe_mask, the > > > checker has a hard time figuring out it does not overflow > > > i915->pipe_to_crtc_mapping[] in intel_get_crtc_for_pipe(). > > > > > > So here in intel_vdsc.c the checks are for overflowing/underflowing > > > the pipe range. In intel_get_crtc_for_pipe() there's a check for the > > > pipe actually existing - the pipe numbering might not be contiguous. > > > > > > Superficially the static checker warning is bogus, as in we won't > > > overflow anything. However, deep down there are issues in the > > > consistency of the checks and how to handle non-contigouous pipe > > > numbering. > > > > > > Indeed, this does not *need* the number. We should figure out the next > > > *crtc*, not the next pipe *number*, which may or may not be pipe + 1. > > > > > > > > > BR, > > > Jani. > > > > > >> +} > > >> + > > >> void intel_dsc_get_config(struct intel_crtc_state *crtc_state) { > > >> struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; diff > > >> --git a/drivers/gpu/drm/i915/display/intel_vdsc.h > > >> b/drivers/gpu/drm/i915/display/intel_vdsc.h > > >> index 65d301c23580..fe4d45561253 100644 > > >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.h > > >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h > > >> @@ -12,11 +12,13 @@ struct intel_encoder; struct intel_crtc_state; > > >> > > >> bool intel_dsc_source_support(const struct intel_crtc_state > > >> *crtc_state); > > >> +void intel_uncompressed_joiner_enable(const struct intel_crtc_state > > >> +*crtc_state); > > >> void intel_dsc_enable(struct intel_encoder *encoder, > > >> const struct intel_crtc_state *crtc_state); void > > >> intel_dsc_disable(const struct intel_crtc_state *crtc_state); int > > >> intel_dsc_compute_params(struct intel_encoder *encoder, > > >> struct intel_crtc_state *pipe_config); > > >> +void intel_uncompressed_joiner_get_config(struct intel_crtc_state > > >> +*crtc_state); > > >> void intel_dsc_get_config(struct intel_crtc_state *crtc_state); > > >> enum intel_display_power_domain intel_dsc_power_domain(const struct > > >> intel_crtc_state *crtc_state); diff --git > > >> a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > >> index 31bc413dbba1..dd6e0bae9573 100644 > > >> --- a/drivers/gpu/drm/i915/i915_reg.h > > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > > >> @@ -11493,6 +11493,8 @@ enum skl_power_gate { > > >> #define SPLITTER_CONFIGURATION_MASK REG_GENMASK(26, 25) > > >> #define SPLITTER_CONFIGURATION_2_SEGMENT > > REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 0) > > >> #define SPLITTER_CONFIGURATION_4_SEGMENT > > REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 1) > > >> +#define UNCOMPRESSED_JOINER_MASTER (1 << 21) > > >> +#define UNCOMPRESSED_JOINER_SLAVE (1 << 20) > > >> > > >> #define _ICL_PIPE_DSS_CTL2_PB 0x78204 > > >> #define _ICL_PIPE_DSS_CTL2_PC 0x78404 > > > > -- > > Jani Nikula, Intel Open Source Graphics Center
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Thursday, June 3, 2021 9:12 PM > To: Manna, Animesh <animesh.manna@intel.com>; Roper, Matthew D > <matthew.d.roper@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Navare, Manasi D <manasi.d.navare@intel.com>; Kulkarni, Vandita > <vandita.kulkarni@intel.com> > Subject: RE: [Intel-gfx] [CI 15/19] drm/i915/bigjoiner: atomic commit changes > for uncompressed joiner > > On Thu, 03 Jun 2021, "Manna, Animesh" <animesh.manna@intel.com> wrote: > >> -----Original Message----- > >> From: Jani Nikula <jani.nikula@linux.intel.com> > >> Sent: Thursday, June 3, 2021 3:10 PM > >> To: Roper, Matthew D <matthew.d.roper@intel.com>; intel- > >> gfx@lists.freedesktop.org > >> Cc: Manna, Animesh <animesh.manna@intel.com>; Navare, Manasi D > >> <manasi.d.navare@intel.com>; Kulkarni, Vandita > >> <vandita.kulkarni@intel.com> > >> Subject: Re: [Intel-gfx] [CI 15/19] drm/i915/bigjoiner: atomic commit > >> changes for uncompressed joiner > >> > >> On Fri, 14 May 2021, Matt Roper <matthew.d.roper@intel.com> wrote: > >> > From: Animesh Manna <animesh.manna@intel.com> > >> > > >> > Respective bit for master or slave to be set for uncompressed > >> > bigjoiner in dss_ctl1 register. > >> > >> I was looking at the changes here due to a static checker complaint, > >> and I think there are a number of issues here. Some more serious than > >> others, and some predate the patch. > >> > >> Comments inline. > >> > >> > Cc: Manasi Navare <manasi.d.navare@intel.com> > >> > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > >> > Signed-off-by: Clinton Taylor <Clinton.A.Taylor@intel.com> > >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > >> > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_display.c | 6 +++ > >> > drivers/gpu/drm/i915/display/intel_vdsc.c | 40 +++++++++++++++++++- > >> > drivers/gpu/drm/i915/display/intel_vdsc.h | 2 + > >> > drivers/gpu/drm/i915/i915_reg.h | 2 + > >> > 4 files changed, 49 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > >> > b/drivers/gpu/drm/i915/display/intel_display.c > >> > index b5fd721137d3..422b59ebf6dc 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_display.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c > >> > @@ -3411,6 +3411,7 @@ static void > >> > icl_ddi_bigjoiner_pre_enable(struct > >> intel_atomic_state *state, > >> > const struct intel_crtc_state > >> *crtc_state) { > >> > struct intel_crtc *master = to_intel_crtc(crtc_state->uapi.crtc); > >> > + struct drm_i915_private *dev_priv = to_i915(master->base.dev); > >> > struct intel_crtc_state *master_crtc_state; > >> > struct drm_connector_state *conn_state; > >> > struct drm_connector *conn; > >> > @@ -3444,6 +3445,9 @@ static void > >> > icl_ddi_bigjoiner_pre_enable(struct > >> intel_atomic_state *state, > >> > /* and DSC on slave */ > >> > intel_dsc_enable(NULL, crtc_state); > >> > } > >> > + > >> > + if (DISPLAY_VER(dev_priv) >= 13) > >> > + intel_uncompressed_joiner_enable(crtc_state); > >> > } > >> > > >> > static void hsw_crtc_enable(struct intel_atomic_state *state, @@ > >> > -6250,6 +6254,8 @@ static bool hsw_get_pipe_config(struct intel_crtc > *crtc, > >> > } > >> > > >> > intel_dsc_get_config(pipe_config); > >> > + if (DISPLAY_VER(dev_priv) >= 13 && !pipe_config- > >> >dsc.compression_enable) > >> > + intel_uncompressed_joiner_get_config(pipe_config); > >> > > >> > if (!active) { > >> > /* bigjoiner slave doesn't enable transcoder */ diff --git > >> > a/drivers/gpu/drm/i915/display/intel_vdsc.c > >> > b/drivers/gpu/drm/i915/display/intel_vdsc.c > >> > index adcd6752f919..efc3184d8315 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c > >> > @@ -1021,6 +1021,22 @@ static i915_reg_t dss_ctl2_reg(const struct > >> intel_crtc_state *crtc_state) > >> > return is_pipe_dsc(crtc_state) ? ICL_PIPE_DSS_CTL2(pipe) : > >> > DSS_CTL2; } > >> > > >> > +void intel_uncompressed_joiner_enable(const struct > >> > +intel_crtc_state > >> > +*crtc_state) > >> > >> Naming. Basically for any new function, the function name prefix > >> should match the file name. intel_vdsc.[ch] should have functions > >> prefixed intel_vdsc_*(). This is where we're headed to increase clarity. > >> > >> intel_uncompressed_*() is something completely different. > >> > >> Granted, here we already have intel_dsc_*() in intel_vdsc.c. We > >> should probably stick with intel_dsc_*(). A possible function or file > >> rename is not out of the question, but that's a separate matter. > > > > As there is not separate register for uncompressed joiner, using bitfield of dsc > register only the function name can be changed to > intel_dsc_uncompressed_joiner_enable(). > > > >> > >> > +{ > >> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > >> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >> > + u32 dss_ctl1_val = 0; > >> > + > >> > + if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { > >> > + if (crtc_state->bigjoiner_slave) > >> > + dss_ctl1_val |= UNCOMPRESSED_JOINER_SLAVE; > >> > + else > >> > + dss_ctl1_val |= UNCOMPRESSED_JOINER_MASTER; > >> > + > >> > + intel_de_write(dev_priv, dss_ctl1_reg(crtc_state), dss_ctl1_val); > >> > + } > >> > +} > >> > + > >> > void intel_dsc_enable(struct intel_encoder *encoder, > >> > const struct intel_crtc_state *crtc_state) { @@ -1060,13 > >> > +1076,35 @@ void intel_dsc_disable(const struct intel_crtc_state > >> *old_crtc_state) > >> > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); > >> > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >> > > >> > - if (!old_crtc_state->dsc.compression_enable) > >> > + if (!(old_crtc_state->dsc.compression_enable && > >> > + old_crtc_state->bigjoiner)) > >> > >> This fails to disable compression if we only have compression but no > >> bigjoiner, which is the more common case! > >> > >> See also: > >> > >> https://gitlab.freedesktop.org/drm/intel/-/issues/3537 > >> https://patchwork.freedesktop.org/patch/msgid/20210603065356.15435-1- > >> vandita.kulkarni@intel.com > >> > > > > We may need to remove both the condition check. > > In uncompressed bigjoiner, compression_enable flag will be 0 and may not > clear the bit of dss_ctl1_reg. > > So can we remove both the checks, hoping it will not harm reseting the > register even if it is not set. > > Now it only disables if *both* were enabled. It needs to be disabled if > *either* was enabled. But if *neither* was enabled, we don't need to do this. Ok. > > > > > >> > return; > >> > > >> > intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0); > >> > intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0); } > >> > > >> > +void intel_uncompressed_joiner_get_config(struct intel_crtc_state > >> > +*crtc_state) { > >> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > >> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >> > + u32 dss_ctl1; > >> > + > >> > + dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg(crtc_state)); > >> > + if (dss_ctl1 & UNCOMPRESSED_JOINER_MASTER) { > >> > + crtc_state->bigjoiner = true; > >> > + if (!WARN_ON(INTEL_NUM_PIPES(dev_priv) == crtc->pipe + 1)) > >> > + crtc_state->bigjoiner_linked_crtc = > >> > + intel_get_crtc_for_pipe(dev_priv, crtc->pipe + > >> 1); > >> > + } else if (dss_ctl1 & UNCOMPRESSED_JOINER_SLAVE) { > >> > + crtc_state->bigjoiner = true; > >> > + crtc_state->bigjoiner_slave = true; > >> > + if (!WARN_ON(crtc->pipe == PIPE_A)) > >> > + crtc_state->bigjoiner_linked_crtc = > >> > + intel_get_crtc_for_pipe(dev_priv, crtc->pipe - > >> 1); > >> > + } > >> > >> Nitpick: This duplicates a bunch of logic for figuring out master/slave. > >> > >> The static checker warning was about crtc->pipe + 1 usage. Since > >> INTEL_NUM_PIPES() looks at the hamming weight of i915->pipe_mask, the > >> checker has a hard time figuring out it does not overflow > >> i915->pipe_to_crtc_mapping[] in intel_get_crtc_for_pipe(). > >> > >> So here in intel_vdsc.c the checks are for overflowing/underflowing > >> the pipe range. In intel_get_crtc_for_pipe() there's a check for the > >> pipe actually existing - the pipe numbering might not be contiguous. > >> > >> Superficially the static checker warning is bogus, as in we won't > >> overflow anything. However, deep down there are issues in the > >> consistency of the checks and how to handle non-contigouous pipe > numbering. > >> > >> Indeed, this does not *need* the number. We should figure out the > >> next *crtc*, not the next pipe *number*, which may or may not be pipe + 1. > > > > dss_ctl1 value is used to identify master or slave crtc. If needed WARN_ON > check can be removed... but pipe enum values like PIPE_A/B/C/D is used to get > the master/slave crtc and always bigjoiner is possible with two adjacent pipes. > > I sent the patch for this and you reviewed it... but are adjacent pipes a > requirement for bigjoiner? Would e.g. A+C work if B was fused off? If not, my > patch 2/2 is incorrect. Yes, I am also tried to check again in bspec. Adjacent pipe is needed for bigjoiner though have not mentioned anything in case of pipe fusing. Regards, Animesh > > BR, > Jani. > > > > > > Regards, > > Animesh > > > >> > >> > >> BR, > >> Jani. > >> > >> > +} > >> > + > >> > void intel_dsc_get_config(struct intel_crtc_state *crtc_state) { > >> > struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; diff > >> > --git a/drivers/gpu/drm/i915/display/intel_vdsc.h > >> > b/drivers/gpu/drm/i915/display/intel_vdsc.h > >> > index 65d301c23580..fe4d45561253 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.h > >> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h > >> > @@ -12,11 +12,13 @@ struct intel_encoder; struct intel_crtc_state; > >> > > >> > bool intel_dsc_source_support(const struct intel_crtc_state > >> > *crtc_state); > >> > +void intel_uncompressed_joiner_enable(const struct > >> > +intel_crtc_state *crtc_state); > >> > void intel_dsc_enable(struct intel_encoder *encoder, > >> > const struct intel_crtc_state *crtc_state); void > >> > intel_dsc_disable(const struct intel_crtc_state *crtc_state); int > >> > intel_dsc_compute_params(struct intel_encoder *encoder, > >> > struct intel_crtc_state *pipe_config); > >> > +void intel_uncompressed_joiner_get_config(struct intel_crtc_state > >> > +*crtc_state); > >> > void intel_dsc_get_config(struct intel_crtc_state *crtc_state); > >> > enum intel_display_power_domain intel_dsc_power_domain(const > >> > struct intel_crtc_state *crtc_state); diff --git > >> > a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> > index 31bc413dbba1..dd6e0bae9573 100644 > >> > --- a/drivers/gpu/drm/i915/i915_reg.h > >> > +++ b/drivers/gpu/drm/i915/i915_reg.h > >> > @@ -11493,6 +11493,8 @@ enum skl_power_gate { > >> > #define SPLITTER_CONFIGURATION_MASK REG_GENMASK(26, 25) > >> > #define SPLITTER_CONFIGURATION_2_SEGMENT > >> REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 0) > >> > #define SPLITTER_CONFIGURATION_4_SEGMENT > >> REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 1) > >> > +#define UNCOMPRESSED_JOINER_MASTER (1 << 21) > >> > +#define UNCOMPRESSED_JOINER_SLAVE (1 << 20) > >> > > >> > #define _ICL_PIPE_DSS_CTL2_PB 0x78204 > >> > #define _ICL_PIPE_DSS_CTL2_PC 0x78404 > >> > >> -- > >> Jani Nikula, Intel Open Source Graphics Center > > -- > Jani Nikula, Intel Open Source Graphics Center
On Thu, 03 Jun 2021, "Navare, Manasi" <manasi.d.navare@intel.com> wrote: > On Thu, Jun 03, 2021 at 06:49:23AM -0700, Manna, Animesh wrote: >> >> >> > -----Original Message----- >> > From: Jani Nikula <jani.nikula@linux.intel.com> >> > Sent: Thursday, June 3, 2021 6:03 PM >> > To: Roper, Matthew D <matthew.d.roper@intel.com>; intel- >> > gfx@lists.freedesktop.org >> > Cc: Manna, Animesh <animesh.manna@intel.com>; Navare, Manasi D >> > <manasi.d.navare@intel.com>; Kulkarni, Vandita <vandita.kulkarni@intel.com> >> > Subject: Re: [Intel-gfx] [CI 15/19] drm/i915/bigjoiner: atomic commit changes >> > for uncompressed joiner >> > >> > On Thu, 03 Jun 2021, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> > > On Fri, 14 May 2021, Matt Roper <matthew.d.roper@intel.com> wrote: >> > >> From: Animesh Manna <animesh.manna@intel.com> >> > >> >> > >> Respective bit for master or slave to be set for uncompressed >> > >> bigjoiner in dss_ctl1 register. >> > > >> > > I was looking at the changes here due to a static checker complaint, >> > > and I think there are a number of issues here. Some more serious than >> > > others, and some predate the patch. >> > > >> > > Comments inline. >> > > >> > >> Cc: Manasi Navare <manasi.d.navare@intel.com> >> > >> Signed-off-by: Animesh Manna <animesh.manna@intel.com> >> > >> Signed-off-by: Clinton Taylor <Clinton.A.Taylor@intel.com> >> > >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> > >> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> >> > >> --- >> > >> drivers/gpu/drm/i915/display/intel_display.c | 6 +++ >> > >> drivers/gpu/drm/i915/display/intel_vdsc.c | 40 +++++++++++++++++++- >> > >> drivers/gpu/drm/i915/display/intel_vdsc.h | 2 + >> > >> drivers/gpu/drm/i915/i915_reg.h | 2 + >> > >> 4 files changed, 49 insertions(+), 1 deletion(-) >> > >> >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c >> > >> b/drivers/gpu/drm/i915/display/intel_display.c >> > >> index b5fd721137d3..422b59ebf6dc 100644 >> > >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> > >> @@ -3411,6 +3411,7 @@ static void icl_ddi_bigjoiner_pre_enable(struct >> > intel_atomic_state *state, >> > >> const struct intel_crtc_state >> > *crtc_state) { >> > >> struct intel_crtc *master = to_intel_crtc(crtc_state->uapi.crtc); >> > >> + struct drm_i915_private *dev_priv = to_i915(master->base.dev); >> > >> struct intel_crtc_state *master_crtc_state; >> > >> struct drm_connector_state *conn_state; >> > >> struct drm_connector *conn; >> > >> @@ -3444,6 +3445,9 @@ static void icl_ddi_bigjoiner_pre_enable(struct >> > intel_atomic_state *state, >> > >> /* and DSC on slave */ >> > >> intel_dsc_enable(NULL, crtc_state); >> > >> } >> > >> + >> > >> + if (DISPLAY_VER(dev_priv) >= 13) >> > >> > I don't think we should add these checks here. Make sure the crtc_state only has >> > the relevant stuff enabled if the platform supports it. Don't duplicate the checks. >> >> Agree. > > >> >> > >> > >> + intel_uncompressed_joiner_enable(crtc_state); >> > >> > As this is always called after intel_dsc_enable(), I think it would make sense to >> > move this within intel_dsc_enable(). >> >> Agree. > > @Jani I had asked to create a separate joiner enable since having an uncompressed enable inside dsc_enable > just from the names makes it contradicting. > But sure may be with a comment before calling uncompressed_joiner_enable() it can be just called from inside dsc enable() > And instead of checking the display ver there we just use this condition: > if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { > intel_uncompressed_joiner_enable(crtc_state); > } > Since the atomic check ensures that only for ver >=13 is when we allow big joiner and no DSC combination. > > Animesh, Jani does that sound good? Yes. Regarding naming semantics, I think we just need to consider DSC an engine that does other things than just compression. The actual registers being the same makes, I think, the logic harder to follow with these being separate steps. Also, you can then cover them with a single power domain get/put. Anyway, care must be taken to not change the logic for compressed paths. BR, Jani. > > Manasi > > >> > >> > >> } >> > >> >> > >> static void hsw_crtc_enable(struct intel_atomic_state *state, @@ >> > >> -6250,6 +6254,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc, >> > >> } >> > >> >> > >> intel_dsc_get_config(pipe_config); >> > >> + if (DISPLAY_VER(dev_priv) >= 13 && !pipe_config- >> > >dsc.compression_enable) >> > >> + intel_uncompressed_joiner_get_config(pipe_config); >> > >> > As this is always called after intel_dsc_get_config(), I think it would make sense >> > to move this within intel_dsc_get_config. >> > >> > >> >> > >> if (!active) { >> > >> /* bigjoiner slave doesn't enable transcoder */ diff --git >> > >> a/drivers/gpu/drm/i915/display/intel_vdsc.c >> > >> b/drivers/gpu/drm/i915/display/intel_vdsc.c >> > >> index adcd6752f919..efc3184d8315 100644 >> > >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c >> > >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c >> > >> @@ -1021,6 +1021,22 @@ static i915_reg_t dss_ctl2_reg(const struct >> > intel_crtc_state *crtc_state) >> > >> return is_pipe_dsc(crtc_state) ? ICL_PIPE_DSS_CTL2(pipe) : >> > >> DSS_CTL2; } >> > >> >> > >> +void intel_uncompressed_joiner_enable(const struct intel_crtc_state >> > >> +*crtc_state) >> > > >> > > Naming. Basically for any new function, the function name prefix >> > > should match the file name. intel_vdsc.[ch] should have functions >> > > prefixed intel_vdsc_*(). This is where we're headed to increase clarity. >> > > >> > > intel_uncompressed_*() is something completely different. >> > > >> > > Granted, here we already have intel_dsc_*() in intel_vdsc.c. We should >> > > probably stick with intel_dsc_*(). A possible function or file rename >> > > is not out of the question, but that's a separate matter. >> > > >> > >> +{ >> > >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> > >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> > >> + u32 dss_ctl1_val = 0; >> > >> + >> > >> + if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { >> > >> + if (crtc_state->bigjoiner_slave) >> > >> + dss_ctl1_val |= UNCOMPRESSED_JOINER_SLAVE; >> > >> + else >> > >> + dss_ctl1_val |= UNCOMPRESSED_JOINER_MASTER; >> > >> + >> > >> + intel_de_write(dev_priv, dss_ctl1_reg(crtc_state), dss_ctl1_val); >> > >> + } >> > >> +} >> > >> + >> > >> void intel_dsc_enable(struct intel_encoder *encoder, >> > >> const struct intel_crtc_state *crtc_state) { @@ -1060,13 >> > >> +1076,35 @@ void intel_dsc_disable(const struct intel_crtc_state >> > *old_crtc_state) >> > >> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); >> > >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> > >> >> > >> - if (!old_crtc_state->dsc.compression_enable) >> > >> + if (!(old_crtc_state->dsc.compression_enable && >> > >> + old_crtc_state->bigjoiner)) >> > > >> > > This fails to disable compression if we only have compression but no >> > > bigjoiner, which is the more common case! >> > > >> > > See also: >> > > >> > > https://gitlab.freedesktop.org/drm/intel/-/issues/3537 >> > > https://patchwork.freedesktop.org/patch/msgid/20210603065356.15435-1-v >> > > andita.kulkarni@intel.com >> > > >> > >> return; >> > >> >> > >> intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0); >> > >> intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0); } >> > >> >> > >> +void intel_uncompressed_joiner_get_config(struct intel_crtc_state >> > >> +*crtc_state) { >> > >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> > >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> > >> + u32 dss_ctl1; >> > >> + >> > >> + dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg(crtc_state)); >> > >> > You can't read this without holding the power domain. >> > >> > Since this is always called after intel_dsc_get_config(), I think it would make >> > sense to move this reading there. >> >> Ok. >> Earlier the plan is to have separate functions for uncompressed joiner, but as you suggested it can be done in the same functions of compressed bigjoiner code. >> Thanks Jani for review. >> >> Regards, >> Animesh >> >> > >> > >> + if (dss_ctl1 & UNCOMPRESSED_JOINER_MASTER) { >> > >> + crtc_state->bigjoiner = true; >> > >> + if (!WARN_ON(INTEL_NUM_PIPES(dev_priv) == crtc->pipe + 1)) >> > >> + crtc_state->bigjoiner_linked_crtc = >> > >> + intel_get_crtc_for_pipe(dev_priv, crtc->pipe + >> > 1); >> > >> + } else if (dss_ctl1 & UNCOMPRESSED_JOINER_SLAVE) { >> > >> + crtc_state->bigjoiner = true; >> > >> + crtc_state->bigjoiner_slave = true; >> > >> + if (!WARN_ON(crtc->pipe == PIPE_A)) >> > >> + crtc_state->bigjoiner_linked_crtc = >> > >> + intel_get_crtc_for_pipe(dev_priv, crtc->pipe - >> > 1); >> > >> + } >> > > >> > > Nitpick: This duplicates a bunch of logic for figuring out master/slave. >> > > >> > > The static checker warning was about crtc->pipe + 1 usage. Since >> > > INTEL_NUM_PIPES() looks at the hamming weight of i915->pipe_mask, the >> > > checker has a hard time figuring out it does not overflow >> > > i915->pipe_to_crtc_mapping[] in intel_get_crtc_for_pipe(). >> > > >> > > So here in intel_vdsc.c the checks are for overflowing/underflowing >> > > the pipe range. In intel_get_crtc_for_pipe() there's a check for the >> > > pipe actually existing - the pipe numbering might not be contiguous. >> > > >> > > Superficially the static checker warning is bogus, as in we won't >> > > overflow anything. However, deep down there are issues in the >> > > consistency of the checks and how to handle non-contigouous pipe >> > > numbering. >> > > >> > > Indeed, this does not *need* the number. We should figure out the next >> > > *crtc*, not the next pipe *number*, which may or may not be pipe + 1. >> > > >> > > >> > > BR, >> > > Jani. >> > > >> > >> +} >> > >> + >> > >> void intel_dsc_get_config(struct intel_crtc_state *crtc_state) { >> > >> struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; diff >> > >> --git a/drivers/gpu/drm/i915/display/intel_vdsc.h >> > >> b/drivers/gpu/drm/i915/display/intel_vdsc.h >> > >> index 65d301c23580..fe4d45561253 100644 >> > >> --- a/drivers/gpu/drm/i915/display/intel_vdsc.h >> > >> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h >> > >> @@ -12,11 +12,13 @@ struct intel_encoder; struct intel_crtc_state; >> > >> >> > >> bool intel_dsc_source_support(const struct intel_crtc_state >> > >> *crtc_state); >> > >> +void intel_uncompressed_joiner_enable(const struct intel_crtc_state >> > >> +*crtc_state); >> > >> void intel_dsc_enable(struct intel_encoder *encoder, >> > >> const struct intel_crtc_state *crtc_state); void >> > >> intel_dsc_disable(const struct intel_crtc_state *crtc_state); int >> > >> intel_dsc_compute_params(struct intel_encoder *encoder, >> > >> struct intel_crtc_state *pipe_config); >> > >> +void intel_uncompressed_joiner_get_config(struct intel_crtc_state >> > >> +*crtc_state); >> > >> void intel_dsc_get_config(struct intel_crtc_state *crtc_state); >> > >> enum intel_display_power_domain intel_dsc_power_domain(const struct >> > >> intel_crtc_state *crtc_state); diff --git >> > >> a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> > >> index 31bc413dbba1..dd6e0bae9573 100644 >> > >> --- a/drivers/gpu/drm/i915/i915_reg.h >> > >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> > >> @@ -11493,6 +11493,8 @@ enum skl_power_gate { >> > >> #define SPLITTER_CONFIGURATION_MASK REG_GENMASK(26, 25) >> > >> #define SPLITTER_CONFIGURATION_2_SEGMENT >> > REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 0) >> > >> #define SPLITTER_CONFIGURATION_4_SEGMENT >> > REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 1) >> > >> +#define UNCOMPRESSED_JOINER_MASTER (1 << 21) >> > >> +#define UNCOMPRESSED_JOINER_SLAVE (1 << 20) >> > >> >> > >> #define _ICL_PIPE_DSS_CTL2_PB 0x78204 >> > >> #define _ICL_PIPE_DSS_CTL2_PC 0x78404 >> > >> > -- >> > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index b5fd721137d3..422b59ebf6dc 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -3411,6 +3411,7 @@ static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state, const struct intel_crtc_state *crtc_state) { struct intel_crtc *master = to_intel_crtc(crtc_state->uapi.crtc); + struct drm_i915_private *dev_priv = to_i915(master->base.dev); struct intel_crtc_state *master_crtc_state; struct drm_connector_state *conn_state; struct drm_connector *conn; @@ -3444,6 +3445,9 @@ static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state, /* and DSC on slave */ intel_dsc_enable(NULL, crtc_state); } + + if (DISPLAY_VER(dev_priv) >= 13) + intel_uncompressed_joiner_enable(crtc_state); } static void hsw_crtc_enable(struct intel_atomic_state *state, @@ -6250,6 +6254,8 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc, } intel_dsc_get_config(pipe_config); + if (DISPLAY_VER(dev_priv) >= 13 && !pipe_config->dsc.compression_enable) + intel_uncompressed_joiner_get_config(pipe_config); if (!active) { /* bigjoiner slave doesn't enable transcoder */ diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c index adcd6752f919..efc3184d8315 100644 --- a/drivers/gpu/drm/i915/display/intel_vdsc.c +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c @@ -1021,6 +1021,22 @@ static i915_reg_t dss_ctl2_reg(const struct intel_crtc_state *crtc_state) return is_pipe_dsc(crtc_state) ? ICL_PIPE_DSS_CTL2(pipe) : DSS_CTL2; } +void intel_uncompressed_joiner_enable(const struct intel_crtc_state *crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + u32 dss_ctl1_val = 0; + + if (crtc_state->bigjoiner && !crtc_state->dsc.compression_enable) { + if (crtc_state->bigjoiner_slave) + dss_ctl1_val |= UNCOMPRESSED_JOINER_SLAVE; + else + dss_ctl1_val |= UNCOMPRESSED_JOINER_MASTER; + + intel_de_write(dev_priv, dss_ctl1_reg(crtc_state), dss_ctl1_val); + } +} + void intel_dsc_enable(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state) { @@ -1060,13 +1076,35 @@ void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state) struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - if (!old_crtc_state->dsc.compression_enable) + if (!(old_crtc_state->dsc.compression_enable && + old_crtc_state->bigjoiner)) return; intel_de_write(dev_priv, dss_ctl1_reg(old_crtc_state), 0); intel_de_write(dev_priv, dss_ctl2_reg(old_crtc_state), 0); } +void intel_uncompressed_joiner_get_config(struct intel_crtc_state *crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + u32 dss_ctl1; + + dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg(crtc_state)); + if (dss_ctl1 & UNCOMPRESSED_JOINER_MASTER) { + crtc_state->bigjoiner = true; + if (!WARN_ON(INTEL_NUM_PIPES(dev_priv) == crtc->pipe + 1)) + crtc_state->bigjoiner_linked_crtc = + intel_get_crtc_for_pipe(dev_priv, crtc->pipe + 1); + } else if (dss_ctl1 & UNCOMPRESSED_JOINER_SLAVE) { + crtc_state->bigjoiner = true; + crtc_state->bigjoiner_slave = true; + if (!WARN_ON(crtc->pipe == PIPE_A)) + crtc_state->bigjoiner_linked_crtc = + intel_get_crtc_for_pipe(dev_priv, crtc->pipe - 1); + } +} + void intel_dsc_get_config(struct intel_crtc_state *crtc_state) { struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config; diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.h b/drivers/gpu/drm/i915/display/intel_vdsc.h index 65d301c23580..fe4d45561253 100644 --- a/drivers/gpu/drm/i915/display/intel_vdsc.h +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h @@ -12,11 +12,13 @@ struct intel_encoder; struct intel_crtc_state; bool intel_dsc_source_support(const struct intel_crtc_state *crtc_state); +void intel_uncompressed_joiner_enable(const struct intel_crtc_state *crtc_state); void intel_dsc_enable(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state); void intel_dsc_disable(const struct intel_crtc_state *crtc_state); int intel_dsc_compute_params(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config); +void intel_uncompressed_joiner_get_config(struct intel_crtc_state *crtc_state); void intel_dsc_get_config(struct intel_crtc_state *crtc_state); enum intel_display_power_domain intel_dsc_power_domain(const struct intel_crtc_state *crtc_state); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 31bc413dbba1..dd6e0bae9573 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -11493,6 +11493,8 @@ enum skl_power_gate { #define SPLITTER_CONFIGURATION_MASK REG_GENMASK(26, 25) #define SPLITTER_CONFIGURATION_2_SEGMENT REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 0) #define SPLITTER_CONFIGURATION_4_SEGMENT REG_FIELD_PREP(SPLITTER_CONFIGURATION_MASK, 1) +#define UNCOMPRESSED_JOINER_MASTER (1 << 21) +#define UNCOMPRESSED_JOINER_SLAVE (1 << 20) #define _ICL_PIPE_DSS_CTL2_PB 0x78204 #define _ICL_PIPE_DSS_CTL2_PC 0x78404