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 |
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
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.
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 >
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 --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