| Message ID | 20170719225057.20131-2-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thursday 20 July 2017 04:20 AM, Imre Deak wrote: > The crtc state starts out being bzero'd, so no need to clear > scaler_users. Also intel_crtc_init_scalers() knows already which > platforms have scalers, so no need for the platform check here. > Similarly intel_crtc_init_scalers() will init scaler_id as required, > so no need to do it here separately. > > Cc: Chandra Konduru <chandra.konduru@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8a38e64b1931..7210f418e9c0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9132,12 +9132,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > u64 power_domain_mask; > bool active; > > - if (INTEL_GEN(dev_priv) >= 9) { > - intel_crtc_init_scalers(crtc, pipe_config); > - > - pipe_config->scaler_state.scaler_id = -1; > - pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX); > - } > + intel_crtc_init_scalers(crtc, pipe_config); If we are removing gen check, then IMHO we should initialize "scaler_id = -1" even before !crtc->num_scalers check in intel_crtc_init_scalers function, just to make sure scaler state doesn't have wrong value in platforms where we don't have any scalers. -Mahesh > > power_domain = POWER_DOMAIN_PIPE(crtc->pipe); > if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
On Fri, Jul 21, 2017 at 06:44:24PM +0530, Mahesh Kumar wrote: > Hi, > > > On Thursday 20 July 2017 04:20 AM, Imre Deak wrote: > > The crtc state starts out being bzero'd, so no need to clear > > scaler_users. Also intel_crtc_init_scalers() knows already which > > platforms have scalers, so no need for the platform check here. > > Similarly intel_crtc_init_scalers() will init scaler_id as required, > > so no need to do it here separately. > > > > Cc: Chandra Konduru <chandra.konduru@intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 8a38e64b1931..7210f418e9c0 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9132,12 +9132,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > > u64 power_domain_mask; > > bool active; > > - if (INTEL_GEN(dev_priv) >= 9) { > > - intel_crtc_init_scalers(crtc, pipe_config); > > - > > - pipe_config->scaler_state.scaler_id = -1; > > - pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX); > > - } > > + intel_crtc_init_scalers(crtc, pipe_config); > If we are removing gen check, then IMHO we should initialize "scaler_id = > -1" even before !crtc->num_scalers check in intel_crtc_init_scalers > function, just to make sure scaler state doesn't have wrong value in > platforms where we don't have any scalers. I agree that setting scaler_id to -1 on all platforms is a good idea, but it's unrelated to removing here the GEN check. intel_crtc_init_scalers() is already called for all platforms in intel_crtc_init(). So this change won't make a difference for platforms without scalers: we leave scaler_id zeroed on those already now (they have num_scalers set to 0). This isn't either a problem since we allocate scalers only on GEN9+. So we could do what you suggest but imo as a follow-up. --Imre
Hi, On Friday 21 July 2017 06:56 PM, Imre Deak wrote: > On Fri, Jul 21, 2017 at 06:44:24PM +0530, Mahesh Kumar wrote: >> Hi, >> >> >> On Thursday 20 July 2017 04:20 AM, Imre Deak wrote: >>> The crtc state starts out being bzero'd, so no need to clear >>> scaler_users. Also intel_crtc_init_scalers() knows already which >>> platforms have scalers, so no need for the platform check here. >>> Similarly intel_crtc_init_scalers() will init scaler_id as required, >>> so no need to do it here separately. >>> >>> Cc: Chandra Konduru <chandra.konduru@intel.com> >>> Cc: Matt Roper <matthew.d.roper@intel.com> >>> Signed-off-by: Imre Deak <imre.deak@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_display.c | 7 +------ >>> 1 file changed, 1 insertion(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index 8a38e64b1931..7210f418e9c0 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -9132,12 +9132,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, >>> u64 power_domain_mask; >>> bool active; >>> - if (INTEL_GEN(dev_priv) >= 9) { >>> - intel_crtc_init_scalers(crtc, pipe_config); >>> - >>> - pipe_config->scaler_state.scaler_id = -1; >>> - pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX); >>> - } >>> + intel_crtc_init_scalers(crtc, pipe_config); >> If we are removing gen check, then IMHO we should initialize "scaler_id = >> -1" even before !crtc->num_scalers check in intel_crtc_init_scalers >> function, just to make sure scaler state doesn't have wrong value in >> platforms where we don't have any scalers. > I agree that setting scaler_id to -1 on all platforms is a good idea, > but it's unrelated to removing here the GEN check. intel_crtc_init_scalers() > is already called for all platforms in intel_crtc_init(). So this change won't > make a difference for platforms without scalers: we leave scaler_id zeroed > on those already now (they have num_scalers set to 0). This isn't either > a problem since we allocate scalers only on GEN9+. > > So we could do what you suggest but imo as a follow-up. sounds good, But if you are going to write a cleanup patch, will that be possible for you to do following change also. 0 indicates no scaler is used & non-zero +ve value indicates used scaler id. in any case Reviewed-by: Mahesh Kumar <mahesh1.kumar@intel.com> -Mahesh > > --Imre
On Fri, Jul 21, 2017 at 07:36:22PM +0530, Mahesh Kumar wrote: > Hi, > > > On Friday 21 July 2017 06:56 PM, Imre Deak wrote: > > On Fri, Jul 21, 2017 at 06:44:24PM +0530, Mahesh Kumar wrote: > > > Hi, > > > > > > > > > On Thursday 20 July 2017 04:20 AM, Imre Deak wrote: > > > > The crtc state starts out being bzero'd, so no need to clear > > > > scaler_users. Also intel_crtc_init_scalers() knows already which > > > > platforms have scalers, so no need for the platform check here. > > > > Similarly intel_crtc_init_scalers() will init scaler_id as required, > > > > so no need to do it here separately. > > > > > > > > Cc: Chandra Konduru <chandra.konduru@intel.com> > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 7 +------ > > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index 8a38e64b1931..7210f418e9c0 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -9132,12 +9132,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > > > > u64 power_domain_mask; > > > > bool active; > > > > - if (INTEL_GEN(dev_priv) >= 9) { > > > > - intel_crtc_init_scalers(crtc, pipe_config); > > > > - > > > > - pipe_config->scaler_state.scaler_id = -1; > > > > - pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX); > > > > - } > > > > + intel_crtc_init_scalers(crtc, pipe_config); > > > > > > If we are removing gen check, then IMHO we should initialize "scaler_id = > > > -1" even before !crtc->num_scalers check in intel_crtc_init_scalers > > > function, just to make sure scaler state doesn't have wrong value in > > > platforms where we don't have any scalers. > > > > I agree that setting scaler_id to -1 on all platforms is a good idea, > > but it's unrelated to removing here the GEN check. intel_crtc_init_scalers() > > is already called for all platforms in intel_crtc_init(). So this change won't > > make a difference for platforms without scalers: we leave scaler_id zeroed > > on those already now (they have num_scalers set to 0). This isn't either > > a problem since we allocate scalers only on GEN9+. > > > > So we could do what you suggest but imo as a follow-up. > > sounds good, But if you are going to write a cleanup patch, will that > be possible for you to do following change also. 0 indicates no > scaler is used & non-zero +ve value indicates used scaler id. That's one possibility, or we could just use a helper to initialize the intel specific part of the state; there could be other fields there with non-zero default values. This helper could be then used in the few places where this is done now (or done partially) open-coded. Also patches are welcome, feel free to follow-up:) > in any case > > Reviewed-by: Mahesh Kumar <mahesh1.kumar@intel.com> > > -Mahesh > > > > --Imre >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8a38e64b1931..7210f418e9c0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9132,12 +9132,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, u64 power_domain_mask; bool active; - if (INTEL_GEN(dev_priv) >= 9) { - intel_crtc_init_scalers(crtc, pipe_config); - - pipe_config->scaler_state.scaler_id = -1; - pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX); - } + intel_crtc_init_scalers(crtc, pipe_config); power_domain = POWER_DOMAIN_PIPE(crtc->pipe); if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
The crtc state starts out being bzero'd, so no need to clear scaler_users. Also intel_crtc_init_scalers() knows already which platforms have scalers, so no need for the platform check here. Similarly intel_crtc_init_scalers() will init scaler_id as required, so no need to do it here separately. Cc: Chandra Konduru <chandra.konduru@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)