Re: [Intel-gfx] [PATCH 09/13] drm/i915: Make DSB lower level
> -Original Message-
> From: Intel-gfx On Behalf Of
> Nautiyal,
> Ankit K
> Sent: Thursday, December 1, 2022 11:52 AM
> To: Ville Syrjala ;
> intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 09/13] drm/i915: Make DSB lower level
>
> Patch looks good to me.
>
> There are couple of minor nitpicks mentioned inline.
>
> In any case this is:
>
> Reviewed-by: Ankit Nautiyal
Looks good to me as well.
Reviewed-by: Uma Shankar
>
> On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > We could have many different uses for the DSB(s) during a single
> > commit, so the current approach of passing the whole crtc_state to the
> > DSB functions is far too high level. Lower the abstraction a little
> > bit so each DSB user can decide where to stick the command buffer/etc.
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/display/intel_color.c | 17 +++--
> > drivers/gpu/drm/i915/display/intel_dsb.c | 79 ++
> > drivers/gpu/drm/i915/display/intel_dsb.h | 13 ++--
> > 3 files changed, 55 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > b/drivers/gpu/drm/i915/display/intel_color.c
> > index 5a8652407f30..2715f1b617e1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -842,7 +842,7 @@ static void ilk_lut_write(const struct intel_crtc_state
> *crtc_state,
> > struct drm_i915_private *i915 =
> > to_i915(crtc_state->uapi.crtc->dev);
> >
> > if (crtc_state->dsb)
> > - intel_dsb_reg_write(crtc_state, reg, val);
> > + intel_dsb_reg_write(crtc_state->dsb, reg, val);
> > else
> > intel_de_write_fw(i915, reg, val);
> > }
> > @@ -853,7 +853,7 @@ static void ilk_lut_write_indexed(const struct
> intel_crtc_state *crtc_state,
> > struct drm_i915_private *i915 =
> > to_i915(crtc_state->uapi.crtc->dev);
> >
> > if (crtc_state->dsb)
> > - intel_dsb_indexed_reg_write(crtc_state, reg, val);
> > + intel_dsb_indexed_reg_write(crtc_state->dsb, reg, val);
> > else
> > intel_de_write_fw(i915, reg, val);
> > }
> > @@ -1273,7 +1273,8 @@ static void icl_load_luts(const struct
> > intel_crtc_state
> *crtc_state)
> > break;
> > }
> >
> > - intel_dsb_commit(crtc_state);
> > + if (crtc_state->dsb)
> > + intel_dsb_commit(crtc_state->dsb);
> > }
> >
> > static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color) @@
> > -1391,12 +1392,18 @@ void intel_color_commit_arm(const struct
> > intel_crtc_state *crtc_state)
> >
> > void intel_color_prepare_commit(struct intel_crtc_state *crtc_state)
> > {
> > - intel_dsb_prepare(crtc_state);
> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +
> > + crtc_state->dsb = intel_dsb_prepare(crtc);
> > }
> >
> > void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state)
> > {
> > - intel_dsb_cleanup(crtc_state);
> > + if (!crtc_state->dsb)
> > + return;
> > +
> > + intel_dsb_cleanup(crtc_state->dsb);
> > + crtc_state->dsb = NULL;
> > }
> >
> > static bool intel_can_preload_luts(const struct intel_crtc_state
> > *new_crtc_state) diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > index b4f0356c2463..ab74bfc89465 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > @@ -24,8 +24,10 @@ enum dsb_id {
> >
> > struct intel_dsb {
> > enum dsb_id id;
> > +
>
> Is this extra line required?
>
>
> > u32 *cmd_buf;
> > struct i915_vma *vma;
> > + struct intel_crtc *crtc;
> >
> > /*
> > * free_pos will point the first free entry position @@ -113,7
> > +115,7 @@ static bool intel_dsb_disable_engine(struct drm_i915_private
> > *i915,
> > /**
> >* intel_dsb_indexed_reg_write() -Write to the DSB context for auto
> >* increment register.
> > - * @crtc_state: intel_crtc_state structure
> > + * @dsb: DSB context
> >* @reg: register address.
> >* @val: value.
> >*
> > @@ -123,11 +125,10 @@ static bool intel_dsb_disable_engine(struct
> drm_i915_priv
Re: [Intel-gfx] [PATCH 09/13] drm/i915: Make DSB lower level
Patch looks good to me.
There are couple of minor nitpicks mentioned inline.
In any case this is:
Reviewed-by: Ankit Nautiyal
On 11/23/2022 8:56 PM, Ville Syrjala wrote:
From: Ville Syrjälä
We could have many different uses for the DSB(s) during a
single commit, so the current approach of passing the whole
crtc_state to the DSB functions is far too high level. Lower
the abstraction a little bit so each DSB user can decide where
to stick the command buffer/etc.
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/display/intel_color.c | 17 +++--
drivers/gpu/drm/i915/display/intel_dsb.c | 79 ++
drivers/gpu/drm/i915/display/intel_dsb.h | 13 ++--
3 files changed, 55 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_color.c
b/drivers/gpu/drm/i915/display/intel_color.c
index 5a8652407f30..2715f1b617e1 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -842,7 +842,7 @@ static void ilk_lut_write(const struct intel_crtc_state
*crtc_state,
struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
if (crtc_state->dsb)
- intel_dsb_reg_write(crtc_state, reg, val);
+ intel_dsb_reg_write(crtc_state->dsb, reg, val);
else
intel_de_write_fw(i915, reg, val);
}
@@ -853,7 +853,7 @@ static void ilk_lut_write_indexed(const struct
intel_crtc_state *crtc_state,
struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
if (crtc_state->dsb)
- intel_dsb_indexed_reg_write(crtc_state, reg, val);
+ intel_dsb_indexed_reg_write(crtc_state->dsb, reg, val);
else
intel_de_write_fw(i915, reg, val);
}
@@ -1273,7 +1273,8 @@ static void icl_load_luts(const struct intel_crtc_state
*crtc_state)
break;
}
- intel_dsb_commit(crtc_state);
+ if (crtc_state->dsb)
+ intel_dsb_commit(crtc_state->dsb);
}
static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color)
@@ -1391,12 +1392,18 @@ void intel_color_commit_arm(const struct
intel_crtc_state *crtc_state)
void intel_color_prepare_commit(struct intel_crtc_state *crtc_state)
{
- intel_dsb_prepare(crtc_state);
+ struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+
+ crtc_state->dsb = intel_dsb_prepare(crtc);
}
void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state)
{
- intel_dsb_cleanup(crtc_state);
+ if (!crtc_state->dsb)
+ return;
+
+ intel_dsb_cleanup(crtc_state->dsb);
+ crtc_state->dsb = NULL;
}
static bool intel_can_preload_luts(const struct intel_crtc_state *new_crtc_state)
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
b/drivers/gpu/drm/i915/display/intel_dsb.c
index b4f0356c2463..ab74bfc89465 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -24,8 +24,10 @@ enum dsb_id {
struct intel_dsb {
enum dsb_id id;
+
Is this extra line required?
u32 *cmd_buf;
struct i915_vma *vma;
+ struct intel_crtc *crtc;
/*
* free_pos will point the first free entry position
@@ -113,7 +115,7 @@ static bool intel_dsb_disable_engine(struct
drm_i915_private *i915,
/**
* intel_dsb_indexed_reg_write() -Write to the DSB context for auto
* increment register.
- * @crtc_state: intel_crtc_state structure
+ * @dsb: DSB context
* @reg: register address.
* @val: value.
*
@@ -123,11 +125,10 @@ static bool intel_dsb_disable_engine(struct
drm_i915_private *i915,
* is done through mmio write.
*/
-void intel_dsb_indexed_reg_write(const struct intel_crtc_state *crtc_state,
+void intel_dsb_indexed_reg_write(struct intel_dsb *dsb,
i915_reg_t reg, u32 val)
{
- struct intel_dsb *dsb = crtc_state->dsb;
- struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+ struct intel_crtc *crtc = dsb->crtc;
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
u32 *buf = dsb->cmd_buf;
u32 reg_val;
@@ -195,12 +196,11 @@ void intel_dsb_indexed_reg_write(const struct
intel_crtc_state *crtc_state,
* and rest all erroneous condition register programming is done
* through mmio write.
*/
-void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
+void intel_dsb_reg_write(struct intel_dsb *dsb,
i915_reg_t reg, u32 val)
{
- struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+ struct intel_crtc *crtc = dsb->crtc;
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
- struct intel_dsb *dsb = crtc_state->dsb;
u32 *buf = dsb->cmd_buf;
if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
@@ -217,17 +217,14 @@ void intel_dsb_reg_write(const struct intel_crtc_
[Intel-gfx] [PATCH 09/13] drm/i915: Make DSB lower level
From: Ville Syrjälä
We could have many different uses for the DSB(s) during a
single commit, so the current approach of passing the whole
crtc_state to the DSB functions is far too high level. Lower
the abstraction a little bit so each DSB user can decide where
to stick the command buffer/etc.
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/display/intel_color.c | 17 +++--
drivers/gpu/drm/i915/display/intel_dsb.c | 79 ++
drivers/gpu/drm/i915/display/intel_dsb.h | 13 ++--
3 files changed, 55 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_color.c
b/drivers/gpu/drm/i915/display/intel_color.c
index 5a8652407f30..2715f1b617e1 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -842,7 +842,7 @@ static void ilk_lut_write(const struct intel_crtc_state
*crtc_state,
struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
if (crtc_state->dsb)
- intel_dsb_reg_write(crtc_state, reg, val);
+ intel_dsb_reg_write(crtc_state->dsb, reg, val);
else
intel_de_write_fw(i915, reg, val);
}
@@ -853,7 +853,7 @@ static void ilk_lut_write_indexed(const struct
intel_crtc_state *crtc_state,
struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
if (crtc_state->dsb)
- intel_dsb_indexed_reg_write(crtc_state, reg, val);
+ intel_dsb_indexed_reg_write(crtc_state->dsb, reg, val);
else
intel_de_write_fw(i915, reg, val);
}
@@ -1273,7 +1273,8 @@ static void icl_load_luts(const struct intel_crtc_state
*crtc_state)
break;
}
- intel_dsb_commit(crtc_state);
+ if (crtc_state->dsb)
+ intel_dsb_commit(crtc_state->dsb);
}
static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color)
@@ -1391,12 +1392,18 @@ void intel_color_commit_arm(const struct
intel_crtc_state *crtc_state)
void intel_color_prepare_commit(struct intel_crtc_state *crtc_state)
{
- intel_dsb_prepare(crtc_state);
+ struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+
+ crtc_state->dsb = intel_dsb_prepare(crtc);
}
void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state)
{
- intel_dsb_cleanup(crtc_state);
+ if (!crtc_state->dsb)
+ return;
+
+ intel_dsb_cleanup(crtc_state->dsb);
+ crtc_state->dsb = NULL;
}
static bool intel_can_preload_luts(const struct intel_crtc_state
*new_crtc_state)
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
b/drivers/gpu/drm/i915/display/intel_dsb.c
index b4f0356c2463..ab74bfc89465 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -24,8 +24,10 @@ enum dsb_id {
struct intel_dsb {
enum dsb_id id;
+
u32 *cmd_buf;
struct i915_vma *vma;
+ struct intel_crtc *crtc;
/*
* free_pos will point the first free entry position
@@ -113,7 +115,7 @@ static bool intel_dsb_disable_engine(struct
drm_i915_private *i915,
/**
* intel_dsb_indexed_reg_write() -Write to the DSB context for auto
* increment register.
- * @crtc_state: intel_crtc_state structure
+ * @dsb: DSB context
* @reg: register address.
* @val: value.
*
@@ -123,11 +125,10 @@ static bool intel_dsb_disable_engine(struct
drm_i915_private *i915,
* is done through mmio write.
*/
-void intel_dsb_indexed_reg_write(const struct intel_crtc_state *crtc_state,
+void intel_dsb_indexed_reg_write(struct intel_dsb *dsb,
i915_reg_t reg, u32 val)
{
- struct intel_dsb *dsb = crtc_state->dsb;
- struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+ struct intel_crtc *crtc = dsb->crtc;
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
u32 *buf = dsb->cmd_buf;
u32 reg_val;
@@ -195,12 +196,11 @@ void intel_dsb_indexed_reg_write(const struct
intel_crtc_state *crtc_state,
* and rest all erroneous condition register programming is done
* through mmio write.
*/
-void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
+void intel_dsb_reg_write(struct intel_dsb *dsb,
i915_reg_t reg, u32 val)
{
- struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+ struct intel_crtc *crtc = dsb->crtc;
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
- struct intel_dsb *dsb = crtc_state->dsb;
u32 *buf = dsb->cmd_buf;
if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
@@ -217,17 +217,14 @@ void intel_dsb_reg_write(const struct intel_crtc_state
*crtc_state,
/**
* intel_dsb_commit() - Trigger workload execution of DSB.
- * @crtc_state: intel_crtc_state structure
+ * @dsb: DSB context
*
* This function is used to do actual write to hardware using DSB.
- * On err
