> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> Sent: Friday, August 30, 2024 10:14 AM
> To: Garg, Nemesa <nemesa.garg@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: Garg, Nemesa <nemesa.garg@xxxxxxxxx>
> Subject: RE: [3/5] drm/i915/display: Enable the second scaler for sharpness
>
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> > Nemesa Garg
> > Sent: Monday, July 8, 2024 1:39 PM
> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Garg, Nemesa <nemesa.garg@xxxxxxxxx>
> > Subject: [3/5] drm/i915/display: Enable the second scaler for
> > sharpness
> >
> > As only second scaler can be used for sharpness check if it is
> > available and if panel fitting is also not enabled, the set the
> > sharpness. Panel fitting will have the preference over sharpness property.
> Can you reframe the commit message, it's a bit difficult to understand.
> Sure. Will do.
> >
> > v2: Added the panel fitting check before enabling sharpness
> >
> > Signed-off-by: Nemesa Garg <nemesa.garg@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 10 ++-
> > .../drm/i915/display/intel_display_types.h | 1 +
> > .../drm/i915/display/intel_modeset_verify.c | 1 +
> > drivers/gpu/drm/i915/display/intel_panel.c | 4 +-
> > .../drm/i915/display/intel_sharpen_filter.c | 10 +++
> > .../drm/i915/display/intel_sharpen_filter.h | 1 +
> > drivers/gpu/drm/i915/display/skl_scaler.c | 84 +++++++++++++++++--
> > drivers/gpu/drm/i915/display/skl_scaler.h | 1 +
> > 8 files changed, 99 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index a62560a0c1a9..385a254528f9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2028,7 +2028,7 @@ static void get_crtc_power_domains(struct
> > intel_crtc_state *crtc_state,
> > set_bit(POWER_DOMAIN_PIPE(pipe), mask->bits);
> > set_bit(POWER_DOMAIN_TRANSCODER(cpu_transcoder), mask->bits);
> > if (crtc_state->pch_pfit.enabled ||
> > - crtc_state->pch_pfit.force_thru)
> > + crtc_state->pch_pfit.force_thru ||
> > +crtc_state->hw.casf_params.need_scaler)
> > set_bit(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe), mask-
> > >bits);
> >
> > drm_for_each_encoder_mask(encoder, &dev_priv->drm, @@ -2284,7
> > +2284,7 @@ static u32 ilk_pipe_pixel_rate(const struct
> > +intel_crtc_state
> > *crtc_state)
> > * PF-ID we'll need to adjust the pixel_rate here.
> > */
> >
> > - if (!crtc_state->pch_pfit.enabled)
> > + if (!crtc_state->pch_pfit.enabled ||
> > +crtc_state->hw.casf_params.need_scaler)
> > return pixel_rate;
> >
> > drm_rect_init(&src, 0, 0,
> > @@ -4295,7 +4295,8 @@ static int intel_crtc_atomic_check(struct
> > intel_atomic_state *state,
> >
> > if (DISPLAY_VER(dev_priv) >= 9) {
> > if (intel_crtc_needs_modeset(crtc_state) ||
> > - intel_crtc_needs_fastset(crtc_state)) {
> > + intel_crtc_needs_fastset(crtc_state) ||
> > + crtc_state->hw.casf_params.need_scaler) {
> > ret = skl_update_scaler_crtc(crtc_state);
> > if (ret)
> > return ret;
> > @@ -5481,6 +5482,9 @@ intel_pipe_config_compare(const struct
> > intel_crtc_state *current_config,
> > PIPE_CONF_CHECK_BOOL(cmrr.enable);
> > }
> >
> > + if (pipe_config->uapi.sharpness_strength > 0)
> > + PIPE_CONF_CHECK_BOOL(hw.casf_params.need_scaler);
> > +
> > #undef PIPE_CONF_CHECK_X
> > #undef PIPE_CONF_CHECK_I
> > #undef PIPE_CONF_CHECK_LLI
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 1c3e031ab369..130740aaaa21 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -834,6 +834,7 @@ struct intel_sharpness_filter {
> > u32 scaler_coefficient[119];
> > bool strength_changed;
> > u8 win_size;
> > + bool need_scaler;
> Always for sharpness filter scaler is required, so does this need_scaler make
> sense?
> Rather should we not check for sharpness_filter enabled?
> need_scaler is getting set during compute config when sharpness strength is not 0 and pch_panel_fitting is not enabled.
If any of the above condition is not met then sharpness cannot be enabled.
So I need to rename this flag?
> > };
> >
> > struct intel_crtc_scaler_state {
> > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_verify.c
> > b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
> > index 3491db5cad31..ed75934bed6b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_modeset_verify.c
> > +++ b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
> > @@ -177,6 +177,7 @@ verify_crtc_state(struct intel_atomic_state *state,
> > crtc->base.name);
> >
> > hw_crtc_state->hw.enable = sw_crtc_state->hw.enable;
> > + hw_crtc_state->hw.casf_params.need_scaler =
> > +sw_crtc_state->hw.casf_params.need_scaler;
> >
> > intel_crtc_get_pipe_config(hw_crtc_state);
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
> > b/drivers/gpu/drm/i915/display/intel_panel.c
> > index 71454ddef20f..bfc725d2e178 100644
> > --- a/drivers/gpu/drm/i915/display/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> > @@ -451,7 +451,9 @@ static int pch_panel_fitting(struct
> > intel_crtc_state *crtc_state,
> >
> > drm_rect_init(&crtc_state->pch_pfit.dst,
> > x, y, width, height);
> > - crtc_state->pch_pfit.enabled = true;
> > +
> > + if (!crtc_state->hw.casf_params.need_scaler)
> > + crtc_state->pch_pfit.enabled = true;
> >
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/i915/display/intel_sharpen_filter.c
> > b/drivers/gpu/drm/i915/display/intel_sharpen_filter.c
> > index b3ebd72b4116..627a0dbd3dfd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sharpen_filter.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sharpen_filter.c
> > @@ -36,6 +36,16 @@ void intel_sharpness_filter_enable(struct
> > intel_crtc_state
> > *crtc_state)
> > intel_de_write_fw(dev_priv, GLK_PS_COEF_DATA_SET(crtc-
> > >pipe, id, 1),
> > crtc_state->hw.casf_params.
> > scaler_coefficient[index]);
> > }
> > +
> > + casf_scaler_enable(crtc_state);
> > +}
> > +
> > +int intel_filter_compute_config(struct intel_crtc_state *crtc_state) {
> > + if (!crtc_state->pch_pfit.enabled)
> > + crtc_state->hw.casf_params.need_scaler = true;
> > +
> > + return 0;
> > }
> >
> > static void convert_sharpness_coef_binary(struct scaler_filter_coeff
> > *coeff, diff --git
> > a/drivers/gpu/drm/i915/display/intel_sharpen_filter.h
> > b/drivers/gpu/drm/i915/display/intel_sharpen_filter.h
> > index 6ab70a635e2f..d20e65971a55 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sharpen_filter.h
> > +++ b/drivers/gpu/drm/i915/display/intel_sharpen_filter.h
> > @@ -23,5 +23,6 @@ struct scaler_filter_coeff {
> >
> > void intel_sharpness_filter_enable(struct intel_crtc_state
> > *crtc_state); void intel_sharpness_scaler_compute_config(struct
> > intel_crtc_state *crtc_state);
> > +int intel_filter_compute_config(struct intel_crtc_state *crtc_state);
> >
> > #endif /* __INTEL_SHARPEN_FILTER_H__ */ diff --git
> > a/drivers/gpu/drm/i915/display/skl_scaler.c
> > b/drivers/gpu/drm/i915/display/skl_scaler.c
> > index baa601d27815..9d8bc6c0ab2c 100644
> > --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> > +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> > @@ -253,7 +253,8 @@ int skl_update_scaler_crtc(struct intel_crtc_state
> > *crtc_state)
> > drm_rect_width(&crtc_state->pipe_src),
> > drm_rect_height(&crtc_state->pipe_src),
> > width, height, NULL, 0,
> > - crtc_state->pch_pfit.enabled);
> > + crtc_state->pch_pfit.enabled ||
> > + crtc_state->hw.casf_params.need_scaler);
> > }
> >
> > /**
> > @@ -353,9 +354,10 @@ static int intel_atomic_setup_scaler(struct
> > intel_crtc_scaler_state *scaler_stat
> > int num_scalers_need, struct intel_crtc
> *intel_crtc,
> > const char *name, int idx,
> > struct intel_plane_state *plane_state,
> > - int *scaler_id)
> > + int *scaler_id, bool casf_scaler)
> > {
> > struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> > + struct intel_crtc_state *crtc_state =
> > +to_intel_crtc_state(intel_crtc->base.state);
> > int j;
> > u32 mode;
> >
> > @@ -365,6 +367,11 @@ static int intel_atomic_setup_scaler(struct
> > intel_crtc_scaler_state *scaler_stat
> > if (scaler_state->scalers[j].in_use)
> > continue;
> >
> > + if (!strcmp(name, "CRTC")) {
> > + if (casf_scaler && j != 1)
> Should the scaler id used for sharpness filter be stored and the same be used here
> to check if its in use?
> Instead of j do I need to use some variable representing scaler id for sharpness.
Here need to check if scaler is assigned for crtc or plane and if it is for crtc and if sharpness is enabled then, whether second scaler is
available or not. If second scaler is free then set scaler id as 1.
> > + continue;
> > + }
> > +
> > *scaler_id = j;
> > scaler_state->scalers[*scaler_id].in_use = 1;
> > break;
> > @@ -375,6 +382,10 @@ static int intel_atomic_setup_scaler(struct
> > intel_crtc_scaler_state *scaler_stat
> > "Cannot find scaler for %s:%d\n", name, idx))
> > return -EINVAL;
> >
> > + if (crtc_state->hw.casf_params.need_scaler) {
> > + mode = SKL_PS_SCALER_MODE_HQ;
> > + }
> > +
> > /* set scaler mode */
> > if (plane_state && plane_state->hw.fb &&
> > plane_state->hw.fb->format->is_yuv && @@ -598,7 +609,8 @@ int
> > intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> >
> > ret = intel_atomic_setup_scaler(scaler_state,
> > num_scalers_need,
> > intel_crtc, name, idx,
> > - plane_state, scaler_id);
> > + plane_state, scaler_id,
> > + crtc_state-
> > >hw.casf_params.need_scaler);
> > if (ret < 0)
> > return ret;
> > }
> > @@ -678,6 +690,15 @@ static void
> > glk_program_nearest_filter_coefs(struct
> > drm_i915_private *dev_priv,
> > intel_de_write_fw(dev_priv, GLK_PS_COEF_INDEX_SET(pipe, id, set),
> > 0); }
> >
> > +static u32 scaler_filter_select(void) {
> > + return (PS_FILTER_PROGRAMMED |
> > + PS_Y_VERT_FILTER_SELECT(1) |
> > + PS_Y_HORZ_FILTER_SELECT(0) |
> > + PS_UV_VERT_FILTER_SELECT(1) |
> > + PS_UV_HORZ_FILTER_SELECT(0));
> > +}
> This looks to be a constant value, can it be a macro?
> Noted.
> > +
> > static u32 skl_scaler_get_filter_select(enum drm_scaling_filter filter, int set) {
> > if (filter == DRM_SCALING_FILTER_NEAREST_NEIGHBOR) { @@ -705,6
> > +726,48 @@ static void skl_scaler_setup_filter(struct drm_i915_private
> > *dev_priv, enum pipe
> > }
> > }
> >
> > +void casf_scaler_enable(struct intel_crtc_state *crtc_state) {
> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>
> dev_priv => i915
>
> > + struct drm_display_mode *adjusted_mode =
> > + &crtc_state->hw.adjusted_mode;
> > + struct intel_crtc_scaler_state *scaler_state =
> > + &crtc_state->scaler_state;
> > + struct drm_rect src, dest;
> > + int id, width, height;
> > + int x, y;
> > + enum pipe pipe = crtc->pipe;
> > + u32 ps_ctrl;
> > +
> > + width = adjusted_mode->crtc_hdisplay;
> > + height = adjusted_mode->crtc_vdisplay;
> > +
> > + x = y = 0;
> > + drm_rect_init(&dest, x, y, width, height);
> > +
> > + struct drm_rect *dst = &dest;
> Declaration to be in the beginning of the function.
> Also I don't see the value of dst being changed and dest being used elsewhere in
> this function. In that case why is a copy of dest made?
> Will remove extra declaration.
Thanks and Regards,
Nemesa
> > +
> > + x = dst->x1;
> > + y = dst->y1;
> > + width = drm_rect_width(dst);
> > + height = drm_rect_height(dst);
> > + id = scaler_state->scaler_id;
> > +
> > + drm_rect_init(&src, 0, 0,
> > + drm_rect_width(&crtc_state->pipe_src) << 16,
> > + drm_rect_height(&crtc_state->pipe_src) << 16);
> > +
> > + ps_ctrl = PS_SCALER_EN | PS_BINDING_PIPE | scaler_state-
> > >scalers[id].mode |
> > + PS_BYPASS_ARMING | PS_DB_STALL | scaler_filter_select();
> > +
> > + intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), ps_ctrl);
> > + intel_de_write_fw(dev_priv, SKL_PS_WIN_POS(pipe, id),
> > + PS_WIN_XPOS(x) | PS_WIN_YPOS(y));
> > + intel_de_write_fw(dev_priv, SKL_PS_WIN_SZ(pipe, id),
> > + PS_WIN_XSIZE(width) | PS_WIN_YSIZE(height)); }
> > +
> > void skl_pfit_enable(const struct intel_crtc_state *crtc_state) {
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -875,16 +938,19 @@ void skl_scaler_get_config(struct
> > intel_crtc_state
> > *crtc_state)
> > continue;
> >
> > id = i;
> > - crtc_state->pch_pfit.enabled = true;
> > +
> > + if (!crtc_state->hw.casf_params.need_scaler)
> > + crtc_state->pch_pfit.enabled = true;
> >
> > pos = intel_de_read(dev_priv, SKL_PS_WIN_POS(crtc->pipe, i));
> > size = intel_de_read(dev_priv, SKL_PS_WIN_SZ(crtc->pipe, i));
> >
> > - drm_rect_init(&crtc_state->pch_pfit.dst,
> > - REG_FIELD_GET(PS_WIN_XPOS_MASK, pos),
> > - REG_FIELD_GET(PS_WIN_YPOS_MASK, pos),
> > - REG_FIELD_GET(PS_WIN_XSIZE_MASK, size),
> > - REG_FIELD_GET(PS_WIN_YSIZE_MASK, size));
> > + if (!crtc_state->hw.casf_params.need_scaler)
> > + drm_rect_init(&crtc_state->pch_pfit.dst,
> > + REG_FIELD_GET(PS_WIN_XPOS_MASK,
> > pos),
> > + REG_FIELD_GET(PS_WIN_YPOS_MASK,
> > pos),
> > + REG_FIELD_GET(PS_WIN_XSIZE_MASK,
> > size),
> > + REG_FIELD_GET(PS_WIN_YSIZE_MASK,
> > size));
> >
> > scaler_state->scalers[i].in_use = true;
> > break;
> > diff --git a/drivers/gpu/drm/i915/display/skl_scaler.h
> > b/drivers/gpu/drm/i915/display/skl_scaler.h
> > index 63f93ca03c89..444527e6a15b 100644
> > --- a/drivers/gpu/drm/i915/display/skl_scaler.h
> > +++ b/drivers/gpu/drm/i915/display/skl_scaler.h
> > @@ -33,5 +33,6 @@ void skl_detach_scalers(const struct
> > intel_crtc_state *crtc_state); void skl_scaler_disable(const struct
> > intel_crtc_state *old_crtc_state);
> >
> > void skl_scaler_get_config(struct intel_crtc_state *crtc_state);
> > +void casf_scaler_enable(struct intel_crtc_state *crtc_state);
> >
> > #endif
> > --
> > 2.25.1
>
> Thanks and Regards,
> Arun R Murthy
> --------------------
