Re: [Intel-gfx] [PATCH v11] drm/i915/dsb: Pre allocate and late cleanup of cmd buffer
> -Original Message-
> From: Intel-gfx On Behalf Of Animesh
> Manna
> Sent: Wednesday, May 20, 2020 6:38 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Nikula, Jani ; Daniel Vetter
> ;
> Lankhorst, Maarten
> Subject: [Intel-gfx] [PATCH v11] drm/i915/dsb: Pre allocate and late cleanup
> of
> cmd buffer
>
> Pre-allocate command buffer in atomic_commit using intel_dsb_prepare
> function which also includes pinning and map in cpu domain.
>
> No functional change is dsb write/commit functions.
>
> Now dsb get/put function is removed and ref-count mechanism is not needed.
> Below dsb api added to do respective job mentioned below.
>
> intel_dsb_prepare - Allocate, pin and map the buffer.
> intel_dsb_cleanup - Unpin and release the gem object.
>
> RFC: Initial patch for design review.
> v2: included _init() part in _prepare(). [Daniel, Ville]
> v3: dsb_cleanup called after cleanup_planes. [Daniel]
> v4: dsb structure is moved to intel_crtc_state from intel_crtc. [Maarten]
> v5: dsb get/put/ref-count mechanism removed. [Maarten]
> v6: Based on review feedback following changes are added,
> - replaced intel_dsb structure by pointer in intel_crtc_state. [Maarten]
> - passing intel_crtc_state to dsp-api to simplify the code. [Maarten]
> - few dsb functions prototype modified to simplify code.
> v7: added few cosmetic changes suggested by Jani and null check for
> crtc_state in
> dsb_cleanup removed as suggested by Maarten.
> v8: changed the function parameter to intel_crtc_state* of
> ivb_load_lut_ext_max() from intel_crtc. [Maarten]
> v9: error handling improved in _write() and prepare(). [Maarten]
>
> Cc: Maarten Lankhorst
> Cc: Ville Syrjälä
> Cc: Jani Nikula
> Cc: Daniel Vetter
> Acked-by: Daniel Vetter
> Reviewed-by: Maarten Lankhorst
> Signed-off-by: Animesh Manna
Pushed the patch to dinq. Thanks for the patch and reviews.
Regards,
Uma Shankar
> drivers/gpu/drm/i915/display/intel_atomic.c | 3 +
> drivers/gpu/drm/i915/display/intel_color.c| 66 ++---
> drivers/gpu/drm/i915/display/intel_display.c | 58 +++-
> .../drm/i915/display/intel_display_types.h| 6 +-
> drivers/gpu/drm/i915/display/intel_dsb.c | 250 --
> drivers/gpu/drm/i915/display/intel_dsb.h | 17 +-
> 6 files changed, 206 insertions(+), 194 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> b/drivers/gpu/drm/i915/display/intel_atomic.c
> index d043057d2fa0..3cb866f22e74 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -252,6 +252,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> crtc_state->wm.need_postvbl_update = false;
> crtc_state->fb_bits = 0;
> crtc_state->update_planes = 0;
> + crtc_state->dsb = NULL;
>
> return _state->uapi;
> }
> @@ -292,6 +293,8 @@ intel_crtc_destroy_state(struct drm_crtc *crtc, {
> struct intel_crtc_state *crtc_state = to_intel_crtc_state(state);
>
> + drm_WARN_ON(crtc->dev, crtc_state->dsb);
> +
> __drm_atomic_helper_crtc_destroy_state(_state->uapi);
> intel_crtc_free_hw_state(crtc_state);
> kfree(crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 98ece9cd7cdd..945bb03bdd4d 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -714,16 +714,16 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
> intel_de_write(dev_priv, PREC_PAL_INDEX(pipe), 0); }
>
> -static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
> +static void ivb_load_lut_ext_max(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);
> - struct intel_dsb *dsb = intel_dsb_get(crtc);
> enum pipe pipe = crtc->pipe;
>
> /* Program the max register to clamp values > 1.0. */
> - intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
> - intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
> - intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
> + intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 <<
> 16);
> + intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 <<
> 16);
> + intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 <<
> +16);
>
> /*
>* Program the gc max 2 register to clamp values > 1.0.
> @@ -731,15 +731,13 @@ static void
[Intel-gfx] [PATCH v11] drm/i915/dsb: Pre allocate and late cleanup of cmd buffer
Pre-allocate command buffer in atomic_commit using intel_dsb_prepare
function which also includes pinning and map in cpu domain.
No functional change is dsb write/commit functions.
Now dsb get/put function is removed and ref-count mechanism is
not needed. Below dsb api added to do respective job mentioned
below.
intel_dsb_prepare - Allocate, pin and map the buffer.
intel_dsb_cleanup - Unpin and release the gem object.
RFC: Initial patch for design review.
v2: included _init() part in _prepare(). [Daniel, Ville]
v3: dsb_cleanup called after cleanup_planes. [Daniel]
v4: dsb structure is moved to intel_crtc_state from intel_crtc. [Maarten]
v5: dsb get/put/ref-count mechanism removed. [Maarten]
v6: Based on review feedback following changes are added,
- replaced intel_dsb structure by pointer in intel_crtc_state. [Maarten]
- passing intel_crtc_state to dsp-api to simplify the code. [Maarten]
- few dsb functions prototype modified to simplify code.
v7: added few cosmetic changes suggested by Jani and null check for
crtc_state in dsb_cleanup removed as suggested by Maarten.
v8: changed the function parameter to intel_crtc_state* of
ivb_load_lut_ext_max() from intel_crtc. [Maarten]
v9: error handling improved in _write() and prepare(). [Maarten]
Cc: Maarten Lankhorst
Cc: Ville Syrjälä
Cc: Jani Nikula
Cc: Daniel Vetter
Acked-by: Daniel Vetter
Reviewed-by: Maarten Lankhorst
Signed-off-by: Animesh Manna
---
drivers/gpu/drm/i915/display/intel_atomic.c | 3 +
drivers/gpu/drm/i915/display/intel_color.c| 66 ++---
drivers/gpu/drm/i915/display/intel_display.c | 58 +++-
.../drm/i915/display/intel_display_types.h| 6 +-
drivers/gpu/drm/i915/display/intel_dsb.c | 250 --
drivers/gpu/drm/i915/display/intel_dsb.h | 17 +-
6 files changed, 206 insertions(+), 194 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
b/drivers/gpu/drm/i915/display/intel_atomic.c
index d043057d2fa0..3cb866f22e74 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -252,6 +252,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
crtc_state->wm.need_postvbl_update = false;
crtc_state->fb_bits = 0;
crtc_state->update_planes = 0;
+ crtc_state->dsb = NULL;
return _state->uapi;
}
@@ -292,6 +293,8 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
{
struct intel_crtc_state *crtc_state = to_intel_crtc_state(state);
+ drm_WARN_ON(crtc->dev, crtc_state->dsb);
+
__drm_atomic_helper_crtc_destroy_state(_state->uapi);
intel_crtc_free_hw_state(crtc_state);
kfree(crtc_state);
diff --git a/drivers/gpu/drm/i915/display/intel_color.c
b/drivers/gpu/drm/i915/display/intel_color.c
index 98ece9cd7cdd..945bb03bdd4d 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -714,16 +714,16 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
intel_de_write(dev_priv, PREC_PAL_INDEX(pipe), 0);
}
-static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
+static void ivb_load_lut_ext_max(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);
- struct intel_dsb *dsb = intel_dsb_get(crtc);
enum pipe pipe = crtc->pipe;
/* Program the max register to clamp values > 1.0. */
- intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
- intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
- intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
+ intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
+ intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
+ intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
/*
* Program the gc max 2 register to clamp values > 1.0.
@@ -731,15 +731,13 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
* from 3.0 to 7.0
*/
if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
- intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0),
+ intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 0),
1 << 16);
- intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1),
+ intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 1),
1 << 16);
- intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2),
+ intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 2),
1 << 16);
}
-
- intel_dsb_put(dsb);
}
static void ivb_load_luts(const struct intel_crtc_state *crtc_state)
@@ -753,7 +751,7 @@ static void ivb_load_luts(const struct intel_crtc_state
