| Message ID | 20241003090341.3140997-1-sai.teja.pottumuttu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/icl: Update csc and gamma enable checks | expand |
On Thu, 03 Oct 2024, Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> wrote: > With ICL, we have a way to check if gamma and csc are enabled on > a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well > along with the existing BOTTOM_COLOR checks. Why? That's the most important question any commit message must answer. The state checker is supposed to check that the hardware state matches the software state exactly. AFAICT this change makes the write and read asymmetric. .gamma_mode and .csc_mode are already checked verbatim. What benefit does this change bring? BR, Jani. > > BSpec: 7463, 7466 > Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> > --- > drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > index 50f41aeb3c28..1bf36898dc7e 100644 > --- a/drivers/gpu/drm/i915/display/intel_color.c > +++ b/drivers/gpu/drm/i915/display/intel_color.c > @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) > crtc_state->csc_enable = true; > } > > +static void icl_get_config(struct intel_crtc_state *crtc_state) > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > + u32 bottom_color; > + > + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); > + crtc_state->csc_mode = ilk_read_csc_mode(crtc); > + > + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); > + > + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || > + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) > + crtc_state->gamma_enable = true; > + > + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || > + (crtc_state->csc_mode & ICL_CSC_ENABLE)) > + crtc_state->csc_enable = true; > +} > + > static void skl_color_commit_arm(const struct intel_crtc_state *crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > @@ -3811,7 +3831,7 @@ static const struct intel_color_funcs tgl_color_funcs = { > .read_luts = icl_read_luts, > .lut_equal = icl_lut_equal, > .read_csc = icl_read_csc, > - .get_config = skl_get_config, > + .get_config = icl_get_config, > }; > > static const struct intel_color_funcs icl_color_funcs = { > @@ -3823,7 +3843,7 @@ static const struct intel_color_funcs icl_color_funcs = { > .read_luts = icl_read_luts, > .lut_equal = icl_lut_equal, > .read_csc = icl_read_csc, > - .get_config = skl_get_config, > + .get_config = icl_get_config, > }; > > static const struct intel_color_funcs glk_color_funcs = {
On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote: > With ICL, we have a way to check if gamma and csc are enabled on > a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well > along with the existing BOTTOM_COLOR checks. > > BSpec: 7463, 7466 > Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> > --- > drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > index 50f41aeb3c28..1bf36898dc7e 100644 > --- a/drivers/gpu/drm/i915/display/intel_color.c > +++ b/drivers/gpu/drm/i915/display/intel_color.c > @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) > crtc_state->csc_enable = true; > } > > +static void icl_get_config(struct intel_crtc_state *crtc_state) > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > + u32 bottom_color; > + > + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); > + crtc_state->csc_mode = ilk_read_csc_mode(crtc); > + > + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); > + > + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || > + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) > + crtc_state->gamma_enable = true; > + > + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || > + (crtc_state->csc_mode & ICL_CSC_ENABLE)) > + crtc_state->csc_enable = true; We don't use the old per-plane/bottom color way of enabling these. So this is not right. > +} > + > static void skl_color_commit_arm(const struct intel_crtc_state *crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > @@ -3811,7 +3831,7 @@ static const struct intel_color_funcs tgl_color_funcs = { > .read_luts = icl_read_luts, > .lut_equal = icl_lut_equal, > .read_csc = icl_read_csc, > - .get_config = skl_get_config, > + .get_config = icl_get_config, > }; > > static const struct intel_color_funcs icl_color_funcs = { > @@ -3823,7 +3843,7 @@ static const struct intel_color_funcs icl_color_funcs = { > .read_luts = icl_read_luts, > .lut_equal = icl_lut_equal, > .read_csc = icl_read_csc, > - .get_config = skl_get_config, > + .get_config = icl_get_config, > }; > > static const struct intel_color_funcs glk_color_funcs = { > -- > 2.34.1
On Thu, Oct 03, 2024 at 02:10:31PM +0300, Ville Syrjälä wrote: > On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote: > > With ICL, we have a way to check if gamma and csc are enabled on > > a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well > > along with the existing BOTTOM_COLOR checks. > > > > BSpec: 7463, 7466 > > Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > > index 50f41aeb3c28..1bf36898dc7e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_color.c > > +++ b/drivers/gpu/drm/i915/display/intel_color.c > > @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) > > crtc_state->csc_enable = true; > > } > > > > +static void icl_get_config(struct intel_crtc_state *crtc_state) > > +{ > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > + u32 bottom_color; > > + > > + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); > > + crtc_state->csc_mode = ilk_read_csc_mode(crtc); > > + > > + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); > > + > > + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || > > + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) > > + crtc_state->gamma_enable = true; > > + > > + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || > > + (crtc_state->csc_mode & ICL_CSC_ENABLE)) > > + crtc_state->csc_enable = true; > > We don't use the old per-plane/bottom color way of enabling these. > So this is not right. I think that's the reason for the patch --- today we use skl_get_config() which *only* checks the bottom color settings. And that approach is documented as being deprecated (although still supported on current platforms). If we're reading out pre-OS state programmed by the vbios, we probably need to handle whichever approach it took, right? Or are we sanitizing this away to "off" somewhere that makes it okay to miss what was programmed? Matt > > > +} > > + > > static void skl_color_commit_arm(const struct intel_crtc_state *crtc_state) > > { > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > @@ -3811,7 +3831,7 @@ static const struct intel_color_funcs tgl_color_funcs = { > > .read_luts = icl_read_luts, > > .lut_equal = icl_lut_equal, > > .read_csc = icl_read_csc, > > - .get_config = skl_get_config, > > + .get_config = icl_get_config, > > }; > > > > static const struct intel_color_funcs icl_color_funcs = { > > @@ -3823,7 +3843,7 @@ static const struct intel_color_funcs icl_color_funcs = { > > .read_luts = icl_read_luts, > > .lut_equal = icl_lut_equal, > > .read_csc = icl_read_csc, > > - .get_config = skl_get_config, > > + .get_config = icl_get_config, > > }; > > > > static const struct intel_color_funcs glk_color_funcs = { > > -- > > 2.34.1 > > -- > Ville Syrjälä > Intel
On Thu, Oct 03, 2024 at 07:22:37AM -0700, Matt Roper wrote: > On Thu, Oct 03, 2024 at 02:10:31PM +0300, Ville Syrjälä wrote: > > On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote: > > > With ICL, we have a way to check if gamma and csc are enabled on > > > a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well > > > along with the existing BOTTOM_COLOR checks. > > > > > > BSpec: 7463, 7466 > > > Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- > > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > > > index 50f41aeb3c28..1bf36898dc7e 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_color.c > > > +++ b/drivers/gpu/drm/i915/display/intel_color.c > > > @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) > > > crtc_state->csc_enable = true; > > > } > > > > > > +static void icl_get_config(struct intel_crtc_state *crtc_state) > > > +{ > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > > + u32 bottom_color; > > > + > > > + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); > > > + crtc_state->csc_mode = ilk_read_csc_mode(crtc); > > > + > > > + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); > > > + > > > + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || > > > + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) > > > + crtc_state->gamma_enable = true; > > > + > > > + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || > > > + (crtc_state->csc_mode & ICL_CSC_ENABLE)) > > > + crtc_state->csc_enable = true; > > > > We don't use the old per-plane/bottom color way of enabling these. > > So this is not right. > > I think that's the reason for the patch --- today we use > skl_get_config() which *only* checks the bottom color settings. And > that approach is documented as being deprecated (although still > supported on current platforms). > > If we're reading out pre-OS state programmed by the vbios, we probably > need to handle whichever approach it took, right? Or are we sanitizing > this away to "off" somewhere that makes it okay to miss what was > programmed? I think we're not doing anything. I suppose some kind of assert_legacy_color_stuff_is_off() thing somewhere could be a decent addition.
On Thu, Oct 03, 2024 at 05:32:56PM +0300, Ville Syrjälä wrote: > On Thu, Oct 03, 2024 at 07:22:37AM -0700, Matt Roper wrote: > > On Thu, Oct 03, 2024 at 02:10:31PM +0300, Ville Syrjälä wrote: > > > On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote: > > > > With ICL, we have a way to check if gamma and csc are enabled on > > > > a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well > > > > along with the existing BOTTOM_COLOR checks. > > > > > > > > BSpec: 7463, 7466 > > > > Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- > > > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > > > > index 50f41aeb3c28..1bf36898dc7e 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_color.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_color.c > > > > @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) > > > > crtc_state->csc_enable = true; > > > > } > > > > > > > > +static void icl_get_config(struct intel_crtc_state *crtc_state) > > > > +{ > > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > > + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > > > + u32 bottom_color; > > > > + > > > > + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); > > > > + crtc_state->csc_mode = ilk_read_csc_mode(crtc); > > > > + > > > > + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); > > > > + > > > > + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || > > > > + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) > > > > + crtc_state->gamma_enable = true; > > > > + > > > > + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || > > > > + (crtc_state->csc_mode & ICL_CSC_ENABLE)) > > > > + crtc_state->csc_enable = true; > > > > > > We don't use the old per-plane/bottom color way of enabling these. > > > So this is not right. > > > > I think that's the reason for the patch --- today we use > > skl_get_config() which *only* checks the bottom color settings. And > > that approach is documented as being deprecated (although still > > supported on current platforms). > > > > If we're reading out pre-OS state programmed by the vbios, we probably > > need to handle whichever approach it took, right? Or are we sanitizing > > this away to "off" somewhere that makes it okay to miss what was > > programmed? > > I think we're not doing anything. I suppose some kind of > assert_legacy_color_stuff_is_off() thing somewhere could be a > decent addition. Or maybe we don't need to do anything. The state checker will catch this problem with the current code as far as the pipe bottom color is concerned. It won't catch inconsistent plane programming though. But we also don't have such a consistency check for older platforms either.
On 03-10-2024 20:28, Ville Syrjälä wrote: > On Thu, Oct 03, 2024 at 05:32:56PM +0300, Ville Syrjälä wrote: >> On Thu, Oct 03, 2024 at 07:22:37AM -0700, Matt Roper wrote: >>> On Thu, Oct 03, 2024 at 02:10:31PM +0300, Ville Syrjälä wrote: >>>> On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote: >>>>> With ICL, we have a way to check if gamma and csc are enabled on >>>>> a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well >>>>> along with the existing BOTTOM_COLOR checks. >>>>> >>>>> BSpec: 7463, 7466 >>>>> Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- >>>>> 1 file changed, 22 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c >>>>> index 50f41aeb3c28..1bf36898dc7e 100644 >>>>> --- a/drivers/gpu/drm/i915/display/intel_color.c >>>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c >>>>> @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) >>>>> crtc_state->csc_enable = true; >>>>> } >>>>> >>>>> +static void icl_get_config(struct intel_crtc_state *crtc_state) >>>>> +{ >>>>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >>>>> + struct drm_i915_private *i915 = to_i915(crtc->base.dev); >>>>> + u32 bottom_color; >>>>> + >>>>> + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); >>>>> + crtc_state->csc_mode = ilk_read_csc_mode(crtc); >>>>> + >>>>> + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); >>>>> + >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || >>>>> + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) >>>>> + crtc_state->gamma_enable = true; >>>>> + >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || >>>>> + (crtc_state->csc_mode & ICL_CSC_ENABLE)) >>>>> + crtc_state->csc_enable = true; >>>> We don't use the old per-plane/bottom color way of enabling these. >>>> So this is not right. >>> I think that's the reason for the patch --- today we use >>> skl_get_config() which *only* checks the bottom color settings. And >>> that approach is documented as being deprecated (although still >>> supported on current platforms). >>> >>> If we're reading out pre-OS state programmed by the vbios, we probably >>> need to handle whichever approach it took, right? Or are we sanitizing >>> this away to "off" somewhere that makes it okay to miss what was >>> programmed? >> I think we're not doing anything. I suppose some kind of >> assert_legacy_color_stuff_is_off() thing somewhere could be a >> decent addition. > Or maybe we don't need to do anything. The state checker will catch > this problem with the current code as far as the pipe bottom color > is concerned. It won't catch inconsistent plane programming though. > But we also don't have such a consistency check for older platforms > either. So, are we essentially saying that we can completely remove BOTTOM_COLOR from here and just move to using GAMMA_MODE, CSC_MODE for setting gamma_enable and csc_enable?
On Thu, Oct 03, 2024 at 09:08:53PM +0530, Pottumuttu, Sai Teja wrote: > > On 03-10-2024 20:28, Ville Syrjälä wrote: > > On Thu, Oct 03, 2024 at 05:32:56PM +0300, Ville Syrjälä wrote: > >> On Thu, Oct 03, 2024 at 07:22:37AM -0700, Matt Roper wrote: > >>> On Thu, Oct 03, 2024 at 02:10:31PM +0300, Ville Syrjälä wrote: > >>>> On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote: > >>>>> With ICL, we have a way to check if gamma and csc are enabled on > >>>>> a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well > >>>>> along with the existing BOTTOM_COLOR checks. > >>>>> > >>>>> BSpec: 7463, 7466 > >>>>> Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> > >>>>> --- > >>>>> drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- > >>>>> 1 file changed, 22 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > >>>>> index 50f41aeb3c28..1bf36898dc7e 100644 > >>>>> --- a/drivers/gpu/drm/i915/display/intel_color.c > >>>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c > >>>>> @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) > >>>>> crtc_state->csc_enable = true; > >>>>> } > >>>>> > >>>>> +static void icl_get_config(struct intel_crtc_state *crtc_state) > >>>>> +{ > >>>>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > >>>>> + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > >>>>> + u32 bottom_color; > >>>>> + > >>>>> + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); > >>>>> + crtc_state->csc_mode = ilk_read_csc_mode(crtc); > >>>>> + > >>>>> + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); > >>>>> + > >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || > >>>>> + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) > >>>>> + crtc_state->gamma_enable = true; > >>>>> + > >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || > >>>>> + (crtc_state->csc_mode & ICL_CSC_ENABLE)) > >>>>> + crtc_state->csc_enable = true; > >>>> We don't use the old per-plane/bottom color way of enabling these. > >>>> So this is not right. > >>> I think that's the reason for the patch --- today we use > >>> skl_get_config() which *only* checks the bottom color settings. And > >>> that approach is documented as being deprecated (although still > >>> supported on current platforms). > >>> > >>> If we're reading out pre-OS state programmed by the vbios, we probably > >>> need to handle whichever approach it took, right? Or are we sanitizing > >>> this away to "off" somewhere that makes it okay to miss what was > >>> programmed? > >> I think we're not doing anything. I suppose some kind of > >> assert_legacy_color_stuff_is_off() thing somewhere could be a > >> decent addition. > > Or maybe we don't need to do anything. The state checker will catch > > this problem with the current code as far as the pipe bottom color > > is concerned. It won't catch inconsistent plane programming though. > > But we also don't have such a consistency check for older platforms > > either. > > So, are we essentially saying that we can completely remove BOTTOM_COLOR > from here and just move to using GAMMA_MODE, CSC_MODE for setting > gamma_enable and csc_enable? No. The current code already works correctly, and pipe_csc_enable and gamma_enable are not meant to be used on icl+.
On 03-10-2024 21:16, Ville Syrjälä wrote: > No. The current code already works correctly, and pipe_csc_enable and > gamma_enable are not meant to be used on icl+. So, if that is the case, would it be good to still have the icl_get_config function here and completely remove the gamma_enable and csc_enable setting part so that its clear that we don't need these in icl+?
On Thu, 03 Oct 2024, "Pottumuttu, Sai Teja" <sai.teja.pottumuttu@intel.com> wrote: > On 03-10-2024 21:16, Ville Syrjälä wrote: >> No. The current code already works correctly, and pipe_csc_enable and >> gamma_enable are not meant to be used on icl+. > > So, if that is the case, would it be good to still have the > icl_get_config function here and completely remove the gamma_enable and > csc_enable setting part so that its clear that we don't need these in icl+? Then you'll lose the check on those bit actually having been left at 0. BR, Jani.
On Thu, Oct 03, 2024 at 06:46:31PM +0300, Ville Syrjälä wrote: > On Thu, Oct 03, 2024 at 09:08:53PM +0530, Pottumuttu, Sai Teja wrote: > > > > On 03-10-2024 20:28, Ville Syrjälä wrote: > > > On Thu, Oct 03, 2024 at 05:32:56PM +0300, Ville Syrjälä wrote: > > >> On Thu, Oct 03, 2024 at 07:22:37AM -0700, Matt Roper wrote: > > >>> On Thu, Oct 03, 2024 at 02:10:31PM +0300, Ville Syrjälä wrote: > > >>>> On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote: > > >>>>> With ICL, we have a way to check if gamma and csc are enabled on > > >>>>> a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well > > >>>>> along with the existing BOTTOM_COLOR checks. > > >>>>> > > >>>>> BSpec: 7463, 7466 > > >>>>> Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> > > >>>>> --- > > >>>>> drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- > > >>>>> 1 file changed, 22 insertions(+), 2 deletions(-) > > >>>>> > > >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > > >>>>> index 50f41aeb3c28..1bf36898dc7e 100644 > > >>>>> --- a/drivers/gpu/drm/i915/display/intel_color.c > > >>>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c > > >>>>> @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) > > >>>>> crtc_state->csc_enable = true; > > >>>>> } > > >>>>> > > >>>>> +static void icl_get_config(struct intel_crtc_state *crtc_state) > > >>>>> +{ > > >>>>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > >>>>> + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > >>>>> + u32 bottom_color; > > >>>>> + > > >>>>> + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); > > >>>>> + crtc_state->csc_mode = ilk_read_csc_mode(crtc); > > >>>>> + > > >>>>> + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); > > >>>>> + > > >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || > > >>>>> + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) > > >>>>> + crtc_state->gamma_enable = true; > > >>>>> + > > >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || > > >>>>> + (crtc_state->csc_mode & ICL_CSC_ENABLE)) > > >>>>> + crtc_state->csc_enable = true; > > >>>> We don't use the old per-plane/bottom color way of enabling these. > > >>>> So this is not right. > > >>> I think that's the reason for the patch --- today we use > > >>> skl_get_config() which *only* checks the bottom color settings. And > > >>> that approach is documented as being deprecated (although still > > >>> supported on current platforms). > > >>> > > >>> If we're reading out pre-OS state programmed by the vbios, we probably > > >>> need to handle whichever approach it took, right? Or are we sanitizing > > >>> this away to "off" somewhere that makes it okay to miss what was > > >>> programmed? > > >> I think we're not doing anything. I suppose some kind of > > >> assert_legacy_color_stuff_is_off() thing somewhere could be a > > >> decent addition. > > > Or maybe we don't need to do anything. The state checker will catch > > > this problem with the current code as far as the pipe bottom color > > > is concerned. It won't catch inconsistent plane programming though. > > > But we also don't have such a consistency check for older platforms > > > either. > > > > So, are we essentially saying that we can completely remove BOTTOM_COLOR > > from here and just move to using GAMMA_MODE, CSC_MODE for setting > > gamma_enable and csc_enable? > > No. The current code already works correctly, and pipe_csc_enable and > gamma_enable are not meant to be used on icl+. Then does that mean icl_color_commit_arm() is wrong since we're programming those registers there? Matt > > -- > Ville Syrjälä > Intel
On Thu, Oct 03, 2024 at 09:05:40AM -0700, Matt Roper wrote: > On Thu, Oct 03, 2024 at 06:46:31PM +0300, Ville Syrjälä wrote: > > On Thu, Oct 03, 2024 at 09:08:53PM +0530, Pottumuttu, Sai Teja wrote: > > > > > > On 03-10-2024 20:28, Ville Syrjälä wrote: > > > > On Thu, Oct 03, 2024 at 05:32:56PM +0300, Ville Syrjälä wrote: > > > >> On Thu, Oct 03, 2024 at 07:22:37AM -0700, Matt Roper wrote: > > > >>> On Thu, Oct 03, 2024 at 02:10:31PM +0300, Ville Syrjälä wrote: > > > >>>> On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote: > > > >>>>> With ICL, we have a way to check if gamma and csc are enabled on > > > >>>>> a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well > > > >>>>> along with the existing BOTTOM_COLOR checks. > > > >>>>> > > > >>>>> BSpec: 7463, 7466 > > > >>>>> Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> > > > >>>>> --- > > > >>>>> drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- > > > >>>>> 1 file changed, 22 insertions(+), 2 deletions(-) > > > >>>>> > > > >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > > > >>>>> index 50f41aeb3c28..1bf36898dc7e 100644 > > > >>>>> --- a/drivers/gpu/drm/i915/display/intel_color.c > > > >>>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c > > > >>>>> @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) > > > >>>>> crtc_state->csc_enable = true; > > > >>>>> } > > > >>>>> > > > >>>>> +static void icl_get_config(struct intel_crtc_state *crtc_state) > > > >>>>> +{ > > > >>>>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > >>>>> + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > > >>>>> + u32 bottom_color; > > > >>>>> + > > > >>>>> + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); > > > >>>>> + crtc_state->csc_mode = ilk_read_csc_mode(crtc); > > > >>>>> + > > > >>>>> + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); > > > >>>>> + > > > >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || > > > >>>>> + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) > > > >>>>> + crtc_state->gamma_enable = true; > > > >>>>> + > > > >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || > > > >>>>> + (crtc_state->csc_mode & ICL_CSC_ENABLE)) > > > >>>>> + crtc_state->csc_enable = true; > > > >>>> We don't use the old per-plane/bottom color way of enabling these. > > > >>>> So this is not right. > > > >>> I think that's the reason for the patch --- today we use > > > >>> skl_get_config() which *only* checks the bottom color settings. And > > > >>> that approach is documented as being deprecated (although still > > > >>> supported on current platforms). > > > >>> > > > >>> If we're reading out pre-OS state programmed by the vbios, we probably > > > >>> need to handle whichever approach it took, right? Or are we sanitizing > > > >>> this away to "off" somewhere that makes it okay to miss what was > > > >>> programmed? > > > >> I think we're not doing anything. I suppose some kind of > > > >> assert_legacy_color_stuff_is_off() thing somewhere could be a > > > >> decent addition. > > > > Or maybe we don't need to do anything. The state checker will catch > > > > this problem with the current code as far as the pipe bottom color > > > > is concerned. It won't catch inconsistent plane programming though. > > > > But we also don't have such a consistency check for older platforms > > > > either. > > > > > > So, are we essentially saying that we can completely remove BOTTOM_COLOR > > > from here and just move to using GAMMA_MODE, CSC_MODE for setting > > > gamma_enable and csc_enable? > > > > No. The current code already works correctly, and pipe_csc_enable and > > gamma_enable are not meant to be used on icl+. > > Then does that mean icl_color_commit_arm() is wrong since we're > programming those registers there? Not really. It just assumes those bits should never be set, which is true given how icl_color_check() operates.
On Thu, Oct 03, 2024 at 07:12:41PM +0300, Ville Syrjälä wrote: > On Thu, Oct 03, 2024 at 09:05:40AM -0700, Matt Roper wrote: > > On Thu, Oct 03, 2024 at 06:46:31PM +0300, Ville Syrjälä wrote: > > > On Thu, Oct 03, 2024 at 09:08:53PM +0530, Pottumuttu, Sai Teja wrote: > > > > > > > > On 03-10-2024 20:28, Ville Syrjälä wrote: > > > > > On Thu, Oct 03, 2024 at 05:32:56PM +0300, Ville Syrjälä wrote: > > > > >> On Thu, Oct 03, 2024 at 07:22:37AM -0700, Matt Roper wrote: > > > > >>> On Thu, Oct 03, 2024 at 02:10:31PM +0300, Ville Syrjälä wrote: > > > > >>>> On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote: > > > > >>>>> With ICL, we have a way to check if gamma and csc are enabled on > > > > >>>>> a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well > > > > >>>>> along with the existing BOTTOM_COLOR checks. > > > > >>>>> > > > > >>>>> BSpec: 7463, 7466 > > > > >>>>> Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> > > > > >>>>> --- > > > > >>>>> drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- > > > > >>>>> 1 file changed, 22 insertions(+), 2 deletions(-) > > > > >>>>> > > > > >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > > > > >>>>> index 50f41aeb3c28..1bf36898dc7e 100644 > > > > >>>>> --- a/drivers/gpu/drm/i915/display/intel_color.c > > > > >>>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c > > > > >>>>> @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) > > > > >>>>> crtc_state->csc_enable = true; > > > > >>>>> } > > > > >>>>> > > > > >>>>> +static void icl_get_config(struct intel_crtc_state *crtc_state) > > > > >>>>> +{ > > > > >>>>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > > >>>>> + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > > > >>>>> + u32 bottom_color; > > > > >>>>> + > > > > >>>>> + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); > > > > >>>>> + crtc_state->csc_mode = ilk_read_csc_mode(crtc); > > > > >>>>> + > > > > >>>>> + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); > > > > >>>>> + > > > > >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || > > > > >>>>> + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) > > > > >>>>> + crtc_state->gamma_enable = true; > > > > >>>>> + > > > > >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || > > > > >>>>> + (crtc_state->csc_mode & ICL_CSC_ENABLE)) > > > > >>>>> + crtc_state->csc_enable = true; > > > > >>>> We don't use the old per-plane/bottom color way of enabling these. > > > > >>>> So this is not right. > > > > >>> I think that's the reason for the patch --- today we use > > > > >>> skl_get_config() which *only* checks the bottom color settings. And > > > > >>> that approach is documented as being deprecated (although still > > > > >>> supported on current platforms). > > > > >>> > > > > >>> If we're reading out pre-OS state programmed by the vbios, we probably > > > > >>> need to handle whichever approach it took, right? Or are we sanitizing > > > > >>> this away to "off" somewhere that makes it okay to miss what was > > > > >>> programmed? > > > > >> I think we're not doing anything. I suppose some kind of > > > > >> assert_legacy_color_stuff_is_off() thing somewhere could be a > > > > >> decent addition. > > > > > Or maybe we don't need to do anything. The state checker will catch > > > > > this problem with the current code as far as the pipe bottom color > > > > > is concerned. It won't catch inconsistent plane programming though. > > > > > But we also don't have such a consistency check for older platforms > > > > > either. > > > > > > > > So, are we essentially saying that we can completely remove BOTTOM_COLOR > > > > from here and just move to using GAMMA_MODE, CSC_MODE for setting > > > > gamma_enable and csc_enable? > > > > > > No. The current code already works correctly, and pipe_csc_enable and > > > gamma_enable are not meant to be used on icl+. > > > > Then does that mean icl_color_commit_arm() is wrong since we're > > programming those registers there? > > Not really. It just assumes those bits should never be set, > which is true given how icl_color_check() operates. Hmm, this is confusing since I see icl_gamma_mode() setting up pipe-level gamma enable: if (crtc_state->hw.gamma_lut && !crtc_state->c8_planes) gamma_mode |= POST_CSC_GAMMA_ENABLE; and icl_csc_mode() setting up CSC enable: if (crtc_state->hw.ctm) csc_mode |= ICL_CSC_ENABLE; and then those eventually get programmed to hardware in icl_color_commit_arm. But then our get_config function doesn't read these same bits and instead only reads the bottom_color bit instead (which is mutually exclusive with the pipe-level control, and which will likely be removed from the hardware completely at some point since it's already listed as deprecated). What am I overlooking here? Matt > > -- > Ville Syrjälä > Intel
On Thu, Oct 03, 2024 at 09:20:38AM -0700, Matt Roper wrote: > On Thu, Oct 03, 2024 at 07:12:41PM +0300, Ville Syrjälä wrote: > > On Thu, Oct 03, 2024 at 09:05:40AM -0700, Matt Roper wrote: > > > On Thu, Oct 03, 2024 at 06:46:31PM +0300, Ville Syrjälä wrote: > > > > On Thu, Oct 03, 2024 at 09:08:53PM +0530, Pottumuttu, Sai Teja wrote: > > > > > > > > > > On 03-10-2024 20:28, Ville Syrjälä wrote: > > > > > > On Thu, Oct 03, 2024 at 05:32:56PM +0300, Ville Syrjälä wrote: > > > > > >> On Thu, Oct 03, 2024 at 07:22:37AM -0700, Matt Roper wrote: > > > > > >>> On Thu, Oct 03, 2024 at 02:10:31PM +0300, Ville Syrjälä wrote: > > > > > >>>> On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote: > > > > > >>>>> With ICL, we have a way to check if gamma and csc are enabled on > > > > > >>>>> a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well > > > > > >>>>> along with the existing BOTTOM_COLOR checks. > > > > > >>>>> > > > > > >>>>> BSpec: 7463, 7466 > > > > > >>>>> Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> > > > > > >>>>> --- > > > > > >>>>> drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- > > > > > >>>>> 1 file changed, 22 insertions(+), 2 deletions(-) > > > > > >>>>> > > > > > >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > > > > > >>>>> index 50f41aeb3c28..1bf36898dc7e 100644 > > > > > >>>>> --- a/drivers/gpu/drm/i915/display/intel_color.c > > > > > >>>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c > > > > > >>>>> @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) > > > > > >>>>> crtc_state->csc_enable = true; > > > > > >>>>> } > > > > > >>>>> > > > > > >>>>> +static void icl_get_config(struct intel_crtc_state *crtc_state) > > > > > >>>>> +{ > > > > > >>>>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > > > >>>>> + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > > > > >>>>> + u32 bottom_color; > > > > > >>>>> + > > > > > >>>>> + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); > > > > > >>>>> + crtc_state->csc_mode = ilk_read_csc_mode(crtc); > > > > > >>>>> + > > > > > >>>>> + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); > > > > > >>>>> + > > > > > >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || > > > > > >>>>> + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) > > > > > >>>>> + crtc_state->gamma_enable = true; > > > > > >>>>> + > > > > > >>>>> + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || > > > > > >>>>> + (crtc_state->csc_mode & ICL_CSC_ENABLE)) > > > > > >>>>> + crtc_state->csc_enable = true; > > > > > >>>> We don't use the old per-plane/bottom color way of enabling these. > > > > > >>>> So this is not right. > > > > > >>> I think that's the reason for the patch --- today we use > > > > > >>> skl_get_config() which *only* checks the bottom color settings. And > > > > > >>> that approach is documented as being deprecated (although still > > > > > >>> supported on current platforms). > > > > > >>> > > > > > >>> If we're reading out pre-OS state programmed by the vbios, we probably > > > > > >>> need to handle whichever approach it took, right? Or are we sanitizing > > > > > >>> this away to "off" somewhere that makes it okay to miss what was > > > > > >>> programmed? > > > > > >> I think we're not doing anything. I suppose some kind of > > > > > >> assert_legacy_color_stuff_is_off() thing somewhere could be a > > > > > >> decent addition. > > > > > > Or maybe we don't need to do anything. The state checker will catch > > > > > > this problem with the current code as far as the pipe bottom color > > > > > > is concerned. It won't catch inconsistent plane programming though. > > > > > > But we also don't have such a consistency check for older platforms > > > > > > either. > > > > > > > > > > So, are we essentially saying that we can completely remove BOTTOM_COLOR > > > > > from here and just move to using GAMMA_MODE, CSC_MODE for setting > > > > > gamma_enable and csc_enable? > > > > > > > > No. The current code already works correctly, and pipe_csc_enable and > > > > gamma_enable are not meant to be used on icl+. > > > > > > Then does that mean icl_color_commit_arm() is wrong since we're > > > programming those registers there? > > > > Not really. It just assumes those bits should never be set, > > which is true given how icl_color_check() operates. > > Hmm, this is confusing since I see icl_gamma_mode() setting up > pipe-level gamma enable: > > if (crtc_state->hw.gamma_lut && > !crtc_state->c8_planes) > gamma_mode |= POST_CSC_GAMMA_ENABLE; > > and icl_csc_mode() setting up CSC enable: > > if (crtc_state->hw.ctm) > csc_mode |= ICL_CSC_ENABLE; > > and then those eventually get programmed to hardware in > icl_color_commit_arm. > > But then our get_config function doesn't read these same bits and > instead only reads the bottom_color bit instead (which is mutually > exclusive with the pipe-level control, and which will likely be removed > from the hardware completely at some point since it's already listed as > deprecated). What am I overlooking here? csc_mode and gamma_mode are read back fully. If we wanted to be 100% pedantic about how things work in hardware we'd have to duplicate csc_enable/gamma_enable into plane_state, and then copy over the thing we computed for the whole crtc into those and use them when programming the planes. But that's quite a lot of extra tracking, and we haven't even implemented proper plane state readout so not much point at this time. And IIRC there was also some ordering issue in that we compute plane states before color state, hence the whole *_plane_ctl_crtc() stuff.
diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index 50f41aeb3c28..1bf36898dc7e 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state) crtc_state->csc_enable = true; } +static void icl_get_config(struct intel_crtc_state *crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); + struct drm_i915_private *i915 = to_i915(crtc->base.dev); + u32 bottom_color; + + crtc_state->gamma_mode = hsw_read_gamma_mode(crtc); + crtc_state->csc_mode = ilk_read_csc_mode(crtc); + + bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe)); + + if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) || + (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE)) + crtc_state->gamma_enable = true; + + if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) || + (crtc_state->csc_mode & ICL_CSC_ENABLE)) + crtc_state->csc_enable = true; +} + static void skl_color_commit_arm(const struct intel_crtc_state *crtc_state) { struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); @@ -3811,7 +3831,7 @@ static const struct intel_color_funcs tgl_color_funcs = { .read_luts = icl_read_luts, .lut_equal = icl_lut_equal, .read_csc = icl_read_csc, - .get_config = skl_get_config, + .get_config = icl_get_config, }; static const struct intel_color_funcs icl_color_funcs = { @@ -3823,7 +3843,7 @@ static const struct intel_color_funcs icl_color_funcs = { .read_luts = icl_read_luts, .lut_equal = icl_lut_equal, .read_csc = icl_read_csc, - .get_config = skl_get_config, + .get_config = icl_get_config, }; static const struct intel_color_funcs glk_color_funcs = {
With ICL, we have a way to check if gamma and csc are enabled on a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well along with the existing BOTTOM_COLOR checks. BSpec: 7463, 7466 Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com> --- drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)