diff mbox series

[06/12] drm/i915: Simplify watermark state checker calling convention

Message ID 20231004155607.7719-7-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Display state checker cleanup | expand

Commit Message

Ville Syrjälä Oct. 4, 2023, 3:56 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There is never any reason to pass in both the crtc and its state
as one can always dig out the crtc from its state.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_modeset_verify.c | 2 +-
 drivers/gpu/drm/i915/display/skl_watermark.c        | 4 ++--
 drivers/gpu/drm/i915/display/skl_watermark.h        | 3 +--
 3 files changed, 4 insertions(+), 5 deletions(-)

Comments

Jani Nikula Oct. 4, 2023, 4:57 p.m. UTC | #1 
On Wed, 04 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> There is never any reason to pass in both the crtc and its state
> as one can always dig out the crtc from its state.

I'm wondering whether we shouldn't just always pass 

struct intel_atomic_state *state, struct intel_crtc *crtc

to the checker, everywhere, even if that's excessive in some cases. I
think some of the confusion you're fixing in this series comes from
people cargo culting the conventions, and then adding some new
parameters as needed, deviating from the state, crtc pair.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_modeset_verify.c | 2 +-
>  drivers/gpu/drm/i915/display/skl_watermark.c        | 4 ++--
>  drivers/gpu/drm/i915/display/skl_watermark.h        | 3 +--
>  3 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_verify.c b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
> index 47d45ba1e707..bbee79aad0cd 100644
> --- a/drivers/gpu/drm/i915/display/intel_modeset_verify.c
> +++ b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
> @@ -233,7 +233,7 @@ void intel_modeset_verify_crtc(struct intel_crtc *crtc,
>  	    !intel_crtc_needs_fastset(new_crtc_state))
>  		return;
>  
> -	intel_wm_state_verify(crtc, new_crtc_state);
> +	intel_wm_state_verify(new_crtc_state);
>  	verify_connector_state(state, crtc);
>  	verify_crtc_state(state, crtc);
>  	intel_shared_dpll_state_verify(state, crtc);
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index d51cf92c96ae..e7a9fb4b1f6b 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -3134,9 +3134,9 @@ static void skl_wm_get_hw_state_and_sanitize(struct drm_i915_private *i915)
>  	skl_wm_sanitize(i915);
>  }
>  
> -void intel_wm_state_verify(struct intel_crtc *crtc,
> -			   const struct intel_crtc_state *new_crtc_state)
> +void intel_wm_state_verify(const struct intel_crtc_state *new_crtc_state)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
>  	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>  	struct skl_hw_state {
>  		struct skl_ddb_entry ddb[I915_MAX_PLANES];
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h
> index 18e4b0661cbb..ca4312bf7012 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.h
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.h
> @@ -38,8 +38,7 @@ bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb,
>  				 const struct skl_ddb_entry *entries,
>  				 int num_entries, int ignore_idx);
>  
> -void intel_wm_state_verify(struct intel_crtc *crtc,
> -			   const struct intel_crtc_state *new_crtc_state);
> +void intel_wm_state_verify(const struct intel_crtc_state *new_crtc_state);
>  
>  void skl_watermark_ipc_init(struct drm_i915_private *i915);
>  void skl_watermark_ipc_update(struct drm_i915_private *i915);
Ville Syrjälä Oct. 4, 2023, 5:18 p.m. UTC | #2 
On Wed, Oct 04, 2023 at 07:57:08PM +0300, Jani Nikula wrote:
> On Wed, 04 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > There is never any reason to pass in both the crtc and its state
> > as one can always dig out the crtc from its state.
> 
> I'm wondering whether we shouldn't just always pass 
> 
> struct intel_atomic_state *state, struct intel_crtc *crtc
> 
> to the checker, everywhere, even if that's excessive in some cases. I
> think some of the confusion you're fixing in this series comes from
> people cargo culting the conventions, and then adding some new
> parameters as needed, deviating from the state, crtc pair.

Yeah, I guess that makes sense. I can respin with that.

> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_modeset_verify.c | 2 +-
> >  drivers/gpu/drm/i915/display/skl_watermark.c        | 4 ++--
> >  drivers/gpu/drm/i915/display/skl_watermark.h        | 3 +--
> >  3 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_verify.c b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
> > index 47d45ba1e707..bbee79aad0cd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_modeset_verify.c
> > +++ b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
> > @@ -233,7 +233,7 @@ void intel_modeset_verify_crtc(struct intel_crtc *crtc,
> >  	    !intel_crtc_needs_fastset(new_crtc_state))
> >  		return;
> >  
> > -	intel_wm_state_verify(crtc, new_crtc_state);
> > +	intel_wm_state_verify(new_crtc_state);
> >  	verify_connector_state(state, crtc);
> >  	verify_crtc_state(state, crtc);
> >  	intel_shared_dpll_state_verify(state, crtc);
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index d51cf92c96ae..e7a9fb4b1f6b 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -3134,9 +3134,9 @@ static void skl_wm_get_hw_state_and_sanitize(struct drm_i915_private *i915)
> >  	skl_wm_sanitize(i915);
> >  }
> >  
> > -void intel_wm_state_verify(struct intel_crtc *crtc,
> > -			   const struct intel_crtc_state *new_crtc_state)
> > +void intel_wm_state_verify(const struct intel_crtc_state *new_crtc_state)
> >  {
> > +	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
> >  	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> >  	struct skl_hw_state {
> >  		struct skl_ddb_entry ddb[I915_MAX_PLANES];
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h
> > index 18e4b0661cbb..ca4312bf7012 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.h
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.h
> > @@ -38,8 +38,7 @@ bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb,
> >  				 const struct skl_ddb_entry *entries,
> >  				 int num_entries, int ignore_idx);
> >  
> > -void intel_wm_state_verify(struct intel_crtc *crtc,
> > -			   const struct intel_crtc_state *new_crtc_state);
> > +void intel_wm_state_verify(const struct intel_crtc_state *new_crtc_state);
> >  
> >  void skl_watermark_ipc_init(struct drm_i915_private *i915);
> >  void skl_watermark_ipc_update(struct drm_i915_private *i915);
> 
> -- 
> Jani Nikula, Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_modeset_verify.c b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
index 47d45ba1e707..bbee79aad0cd 100644
--- a/drivers/gpu/drm/i915/display/intel_modeset_verify.c
+++ b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
@@ -233,7 +233,7 @@  void intel_modeset_verify_crtc(struct intel_crtc *crtc,
 	    !intel_crtc_needs_fastset(new_crtc_state))
 		return;
 
-	intel_wm_state_verify(crtc, new_crtc_state);
+	intel_wm_state_verify(new_crtc_state);
 	verify_connector_state(state, crtc);
 	verify_crtc_state(state, crtc);
 	intel_shared_dpll_state_verify(state, crtc);
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index d51cf92c96ae..e7a9fb4b1f6b 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -3134,9 +3134,9 @@  static void skl_wm_get_hw_state_and_sanitize(struct drm_i915_private *i915)
 	skl_wm_sanitize(i915);
 }
 
-void intel_wm_state_verify(struct intel_crtc *crtc,
-			   const struct intel_crtc_state *new_crtc_state)
+void intel_wm_state_verify(const struct intel_crtc_state *new_crtc_state)
 {
+	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
 	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
 	struct skl_hw_state {
 		struct skl_ddb_entry ddb[I915_MAX_PLANES];
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h
index 18e4b0661cbb..ca4312bf7012 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.h
+++ b/drivers/gpu/drm/i915/display/skl_watermark.h
@@ -38,8 +38,7 @@  bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb,
 				 const struct skl_ddb_entry *entries,
 				 int num_entries, int ignore_idx);
 
-void intel_wm_state_verify(struct intel_crtc *crtc,
-			   const struct intel_crtc_state *new_crtc_state);
+void intel_wm_state_verify(const struct intel_crtc_state *new_crtc_state);
 
 void skl_watermark_ipc_init(struct drm_i915_private *i915);
 void skl_watermark_ipc_update(struct drm_i915_private *i915);