diff mbox series

[v2,05/20] drm/i915: Make skl_compute_dbuf_slices() behave consistently for all platforms

Message ID 20200225171125.28885-6-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Proper dbuf global state | expand

Commit Message

Ville Syrjala Feb. 25, 2020, 5:11 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently skl_compute_dbuf_slices() returns 0 for any inactive pipe on
icl+, but returns BIT(S1) on pre-icl for any pipe (whether it's active or
not). Let's make the behaviour consistent and always return 0 for any
inactive pipe.

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Lisovskiy, Stanislav Feb. 25, 2020, 5:30 p.m. UTC | #1 
On Tue, 2020-02-25 at 19:11 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently skl_compute_dbuf_slices() returns 0 for any inactive pipe
> on
> icl+, but returns BIT(S1) on pre-icl for any pipe (whether it's
> active or
> not). Let's make the behaviour consistent and always return 0 for any
> inactive pipe.
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index a2e78969c0df..640f4c4fd508 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4408,7 +4408,7 @@ static u8 skl_compute_dbuf_slices(const struct
> intel_crtc_state *crtc_state,
>  	 * For anything else just return one slice yet.
>  	 * Should be extended for other platforms.
>  	 */
> -	return BIT(DBUF_S1);
> +	return active_pipes & BIT(pipe) ? BIT(DBUF_S1) : 0;

I think the initial idea was this won't be even called if there 
are no active pipes at all - skl_ddb_get_pipe_allocation_limits would
bail out immediately. If there were some active pipes - then we will
have to use slice S1 anyway - because there were simply no other slices
available. If some pipes were inactive - they are currently skipped by
!crtc_state->hw.active check - so I would just keep it simple and don't
call this function for non-active pipes at all.

Currently we are just ORing slice bitmasks only from active pipes.

Stan

>  }
>  
>  static u64
Ville Syrjala March 2, 2020, 2:50 p.m. UTC | #2 
On Tue, Feb 25, 2020 at 05:30:57PM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2020-02-25 at 19:11 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently skl_compute_dbuf_slices() returns 0 for any inactive pipe
> > on
> > icl+, but returns BIT(S1) on pre-icl for any pipe (whether it's
> > active or
> > not). Let's make the behaviour consistent and always return 0 for any
> > inactive pipe.
> > 
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index a2e78969c0df..640f4c4fd508 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4408,7 +4408,7 @@ static u8 skl_compute_dbuf_slices(const struct
> > intel_crtc_state *crtc_state,
> >  	 * For anything else just return one slice yet.
> >  	 * Should be extended for other platforms.
> >  	 */
> > -	return BIT(DBUF_S1);
> > +	return active_pipes & BIT(pipe) ? BIT(DBUF_S1) : 0;
> 
> I think the initial idea was this won't be even called if there 
> are no active pipes at all - skl_ddb_get_pipe_allocation_limits would
> bail out immediately. If there were some active pipes - then we will
> have to use slice S1 anyway - because there were simply no other slices
> available. If some pipes were inactive - they are currently skipped by
> !crtc_state->hw.active check - so I would just keep it simple and don't
> call this function for non-active pipes at all.

That's just going to make the caller more messy by forcing it to
check for active_pipes 0 vs. not. Ie. we'd be splitting the
responsibility of computing the dbuf slices for this pipe between
skl_compute_dbuf_slices() and its caller. Not a good idea IMO.
Lisovskiy, Stanislav March 2, 2020, 3:50 p.m. UTC | #3 
On Mon, 2020-03-02 at 16:50 +0200, Ville Syrjälä wrote:
> On Tue, Feb 25, 2020 at 05:30:57PM +0000, Lisovskiy, Stanislav wrote:
> > On Tue, 2020-02-25 at 19:11 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Currently skl_compute_dbuf_slices() returns 0 for any inactive
> > > pipe
> > > on
> > > icl+, but returns BIT(S1) on pre-icl for any pipe (whether it's
> > > active or
> > > not). Let's make the behaviour consistent and always return 0 for
> > > any
> > > inactive pipe.
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index a2e78969c0df..640f4c4fd508 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4408,7 +4408,7 @@ static u8 skl_compute_dbuf_slices(const
> > > struct
> > > intel_crtc_state *crtc_state,
> > >  	 * For anything else just return one slice yet.
> > >  	 * Should be extended for other platforms.
> > >  	 */
> > > -	return BIT(DBUF_S1);
> > > +	return active_pipes & BIT(pipe) ? BIT(DBUF_S1) : 0;
> > 
> > I think the initial idea was this won't be even called if there 
> > are no active pipes at all - skl_ddb_get_pipe_allocation_limits
> > would
> > bail out immediately. If there were some active pipes - then we
> > will
> > have to use slice S1 anyway - because there were simply no other
> > slices
> > available. If some pipes were inactive - they are currently skipped
> > by
> > !crtc_state->hw.active check - so I would just keep it simple and
> > don't
> > call this function for non-active pipes at all.
> 
> That's just going to make the caller more messy by forcing it to
> check for active_pipes 0 vs. not. Ie. we'd be splitting the
> responsibility of computing the dbuf slices for this pipe between
> skl_compute_dbuf_slices() and its caller. Not a good idea IMO.

Well, in that sense I agree. Currently it is just that this check is
anyway there when you get ddb allocation limits. 

Could this be actually even nicer to get one more very simple table for
everything "before-gen11"? We would have it then in a quite unified
looking way.

Stan


>
Lisovskiy, Stanislav April 1, 2020, 7:52 a.m. UTC | #4 
On Mon, Mar 02, 2020 at 04:50:37PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 25, 2020 at 05:30:57PM +0000, Lisovskiy, Stanislav wrote:
> > On Tue, 2020-02-25 at 19:11 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Currently skl_compute_dbuf_slices() returns 0 for any inactive pipe
> > > on
> > > icl+, but returns BIT(S1) on pre-icl for any pipe (whether it's
> > > active or
> > > not). Let's make the behaviour consistent and always return 0 for any
> > > inactive pipe.
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index a2e78969c0df..640f4c4fd508 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4408,7 +4408,7 @@ static u8 skl_compute_dbuf_slices(const struct
> > > intel_crtc_state *crtc_state,
> > >  	 * For anything else just return one slice yet.
> > >  	 * Should be extended for other platforms.
> > >  	 */
> > > -	return BIT(DBUF_S1);
> > > +	return active_pipes & BIT(pipe) ? BIT(DBUF_S1) : 0;
> > 
> > I think the initial idea was this won't be even called if there 
> > are no active pipes at all - skl_ddb_get_pipe_allocation_limits would
> > bail out immediately. If there were some active pipes - then we will
> > have to use slice S1 anyway - because there were simply no other slices
> > available. If some pipes were inactive - they are currently skipped by
> > !crtc_state->hw.active check - so I would just keep it simple and don't
> > call this function for non-active pipes at all.
> 
> That's just going to make the caller more messy by forcing it to
> check for active_pipes 0 vs. not. Ie. we'd be splitting the
> responsibility of computing the dbuf slices for this pipe between
> skl_compute_dbuf_slices() and its caller. Not a good idea IMO.

Let's ramp it up. As I understood from your comments we still need dbuf_state.
I would anyway add another table for handling this in some unified manner at least,
however don't want to spend another couple of month discussing that :)

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>


> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a2e78969c0df..640f4c4fd508 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4408,7 +4408,7 @@  static u8 skl_compute_dbuf_slices(const struct intel_crtc_state *crtc_state,
 	 * For anything else just return one slice yet.
 	 * Should be extended for other platforms.
 	 */
-	return BIT(DBUF_S1);
+	return active_pipes & BIT(pipe) ? BIT(DBUF_S1) : 0;
 }
 
 static u64