Re: [Intel-gfx] [PATCH v3 15/20] drm/i915: Finish the LUT state checker
> -Original Message-
> From: Intel-gfx On Behalf Of Ville
> Syrjala
> Sent: Monday, November 14, 2022 9:07 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v3 15/20] drm/i915: Finish the LUT state checker
>
> From: Ville Syrjälä
>
> We have full readout now for all platforms (sans the icl+ multi-segment
> readout hw
> fail), so hook up the LUT state checker for everyone.
>
> We add a new vfunc for this since different platforms need to handle the
> details a bit
> differently.
>
> The implementation is rather repetitive in places. Probably we want to think
> of a
> more declarative approach for the LUT precision/etc. stuff in the future...
Yeah some places do look as if can be optimized as you already mentioned.
But no major concerns on this one.
Looks Good to me.
Reviewed-by: Uma Shankar
> Note that we're currently missing readout for c8_planes, so we'll have to
> skip the
> state check in that case.
>
> v2: Fix readout for C8 use cases
> v3: Skip C8 entirely due to lack of c8_planes readout
> Add ilk_has_pre_csc_lut() helper and use other such helpers
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_color.c | 275 ++-
> drivers/gpu/drm/i915/display/intel_color.h | 8 +-
> drivers/gpu/drm/i915/display/intel_display.c | 29 +-
> 3 files changed, 225 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index f0bb4227338c..e2bcfbffb298 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -53,7 +53,18 @@ struct intel_color_funcs {
>* involved with the same commit.
>*/
> void (*load_luts)(const struct intel_crtc_state *crtc_state);
> + /*
> + * Read out the LUTs from the hardware into the software state.
> + * Used by eg. the hardware state checker.
> + */
> void (*read_luts)(struct intel_crtc_state *crtc_state);
> + /*
> + * Compare the LUTs
> + */
> + bool (*lut_equal)(const struct intel_crtc_state *crtc_state,
> + const struct drm_property_blob *blob1,
> + const struct drm_property_blob *blob2,
> + bool is_pre_csc_lut);
> };
>
> #define CTM_COEFF_SIGN (1ULL << 63)
> @@ -1234,6 +1245,24 @@ void intel_color_get_config(struct intel_crtc_state
> *crtc_state)
> i915->display.funcs.color->read_luts(crtc_state);
> }
>
> +bool intel_color_lut_equal(const struct intel_crtc_state *crtc_state,
> +const struct drm_property_blob *blob1,
> +const struct drm_property_blob *blob2,
> +bool is_pre_csc_lut)
> +{
> + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> + /*
> + * FIXME c8_planes readout missing thus
> + * .read_luts() doesn't read out post_csc_lut.
> + */
> + if (!is_pre_csc_lut && crtc_state->c8_planes)
> + return true;
> +
> + return i915->display.funcs.color->lut_equal(crtc_state, blob1, blob2,
> + is_pre_csc_lut);
> +}
> +
> static bool need_plane_update(struct intel_plane *plane,
> const struct intel_crtc_state *crtc_state) { @@
> -1814,6
> +1843,24 @@ static int i9xx_post_csc_lut_precision(const struct
> intel_crtc_state
> *crtc_state
> }
> }
>
> +static int i9xx_pre_csc_lut_precision(const struct intel_crtc_state
> +*crtc_state) {
> + return 0;
> +}
> +
> +static int ilk_gamma_mode_precision(u32 gamma_mode) {
> + switch (gamma_mode) {
> + case GAMMA_MODE_MODE_8BIT:
> + return 8;
> + case GAMMA_MODE_MODE_10BIT:
> + return 10;
> + default:
> + MISSING_CASE(gamma_mode);
> + return 0;
> + }
> +}
> +
> static bool ilk_has_post_csc_lut(const struct intel_crtc_state *crtc_state)
> {
> if (crtc_state->c8_planes)
> @@ -1823,28 +1870,60 @@ static bool ilk_has_post_csc_lut(const struct
> intel_crtc_state *crtc_state)
> (crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) != 0; }
>
> +static bool ilk_has_pre_csc_lut(const struct intel_crtc_state
> +*crtc_state) {
> + return crtc_state->gamma_enable &&
> + (crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) == 0; }
> +
> static int ilk_post_csc_lut_precision(const struct intel_crtc_state
> *crtc_state) {
>
[Intel-gfx] [PATCH v3 15/20] drm/i915: Finish the LUT state checker
From: Ville Syrjälä
We have full readout now for all platforms (sans the icl+
multi-segment readout hw fail), so hook up the LUT state
checker for everyone.
We add a new vfunc for this since different platforms need
to handle the details a bit differently.
The implementation is rather repetitive in places. Probably
we want to think of a more declarative approach for the
LUT precision/etc. stuff in the future...
Note that we're currently missing readout for c8_planes, so
we'll have to skip the state check in that case.
v2: Fix readout for C8 use cases
v3: Skip C8 entirely due to lack of c8_planes readout
Add ilk_has_pre_csc_lut() helper and use other such helpers
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/display/intel_color.c | 275 ++-
drivers/gpu/drm/i915/display/intel_color.h | 8 +-
drivers/gpu/drm/i915/display/intel_display.c | 29 +-
3 files changed, 225 insertions(+), 87 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_color.c
b/drivers/gpu/drm/i915/display/intel_color.c
index f0bb4227338c..e2bcfbffb298 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -53,7 +53,18 @@ struct intel_color_funcs {
* involved with the same commit.
*/
void (*load_luts)(const struct intel_crtc_state *crtc_state);
+ /*
+* Read out the LUTs from the hardware into the software state.
+* Used by eg. the hardware state checker.
+*/
void (*read_luts)(struct intel_crtc_state *crtc_state);
+ /*
+* Compare the LUTs
+*/
+ bool (*lut_equal)(const struct intel_crtc_state *crtc_state,
+ const struct drm_property_blob *blob1,
+ const struct drm_property_blob *blob2,
+ bool is_pre_csc_lut);
};
#define CTM_COEFF_SIGN (1ULL << 63)
@@ -1234,6 +1245,24 @@ void intel_color_get_config(struct intel_crtc_state
*crtc_state)
i915->display.funcs.color->read_luts(crtc_state);
}
+bool intel_color_lut_equal(const struct intel_crtc_state *crtc_state,
+ const struct drm_property_blob *blob1,
+ const struct drm_property_blob *blob2,
+ bool is_pre_csc_lut)
+{
+ struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+
+ /*
+* FIXME c8_planes readout missing thus
+* .read_luts() doesn't read out post_csc_lut.
+*/
+ if (!is_pre_csc_lut && crtc_state->c8_planes)
+ return true;
+
+ return i915->display.funcs.color->lut_equal(crtc_state, blob1, blob2,
+ is_pre_csc_lut);
+}
+
static bool need_plane_update(struct intel_plane *plane,
const struct intel_crtc_state *crtc_state)
{
@@ -1814,6 +1843,24 @@ static int i9xx_post_csc_lut_precision(const struct
intel_crtc_state *crtc_state
}
}
+static int i9xx_pre_csc_lut_precision(const struct intel_crtc_state
*crtc_state)
+{
+ return 0;
+}
+
+static int ilk_gamma_mode_precision(u32 gamma_mode)
+{
+ switch (gamma_mode) {
+ case GAMMA_MODE_MODE_8BIT:
+ return 8;
+ case GAMMA_MODE_MODE_10BIT:
+ return 10;
+ default:
+ MISSING_CASE(gamma_mode);
+ return 0;
+ }
+}
+
static bool ilk_has_post_csc_lut(const struct intel_crtc_state *crtc_state)
{
if (crtc_state->c8_planes)
@@ -1823,28 +1870,60 @@ static bool ilk_has_post_csc_lut(const struct
intel_crtc_state *crtc_state)
(crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) != 0;
}
+static bool ilk_has_pre_csc_lut(const struct intel_crtc_state *crtc_state)
+{
+ return crtc_state->gamma_enable &&
+ (crtc_state->csc_mode & CSC_POSITION_BEFORE_GAMMA) == 0;
+}
+
static int ilk_post_csc_lut_precision(const struct intel_crtc_state
*crtc_state)
{
if (!ilk_has_post_csc_lut(crtc_state))
return 0;
- switch (crtc_state->gamma_mode) {
- case GAMMA_MODE_MODE_8BIT:
- return 8;
- case GAMMA_MODE_MODE_10BIT:
- return 10;
- default:
- MISSING_CASE(crtc_state->gamma_mode);
+ return ilk_gamma_mode_precision(crtc_state->gamma_mode);
+}
+
+static int ilk_pre_csc_lut_precision(const struct intel_crtc_state *crtc_state)
+{
+ if (!ilk_has_pre_csc_lut(crtc_state))
return 0;
- }
+
+ return ilk_gamma_mode_precision(crtc_state->gamma_mode);
+}
+
+static int ivb_post_csc_lut_precision(const struct intel_crtc_state
*crtc_state)
+{
+ if (crtc_state->gamma_enable &&
+ crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
+ return 10;
+
+ return ilk_post_csc_lut_precision(crtc_state);
+}
+
+static int ivb_pre_csc_lut_precision(const struct intel_crtc_state *crtc_state)
+{
+
