| Message ID | 20200319102103.28895-6-pankaj.laxminarayan.bharadiya@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce drm scaling filter property | expand |
On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote: > GEN >= 10 hardware supports the programmable scaler filter. > > Attach scaling filter property for CRTC and plane for GEN >= 10 > hardwares and program scaler filter based on the selected filter > type. > > changes since v1: > * None > Changes since RFC: > * Enable properties for GEN >= 10 platforms (Ville) > * Do not round off the crtc co-ordinate (Danial Stone, Ville) > * Add new functions to handle scaling filter setup (Ville) > * Remove coefficient set 0 hardcoding. > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > Signed-off-by: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 32 ++++++++++++++++++-- > drivers/gpu/drm/i915/display/intel_sprite.c | 31 ++++++++++++++++++- > 2 files changed, 60 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 791dd908aa89..4b3387ee332e 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -6309,6 +6309,25 @@ void skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv, > } > } > > +static u32 > +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe, > + int id, int set, enum drm_crtc_scaling_filter filter) > +{ > + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; > + > + if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) { > + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, > + set); > + scaler_filter_ctl = PS_FILTER_PROGRAMMED | > + PS_UV_VERT_FILTER_SELECT(set) | > + PS_UV_HORZ_FILTER_SELECT(set) | > + PS_Y_VERT_FILTER_SELECT(set) | > + PS_Y_HORZ_FILTER_SELECT(set); > + > + } > + return scaler_filter_ctl; This function does too many things. > +} > + > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) > enum pipe pipe = crtc->pipe; > const struct intel_crtc_scaler_state *scaler_state = > &crtc_state->scaler_state; > + const struct drm_crtc_state *state = &crtc_state->uapi; Pls don't add this kind of aliases. We're moving away from using the drm_ types as much as possible. > > if (crtc_state->pch_pfit.enabled) { > u16 uv_rgb_hphase, uv_rgb_vphase; > int pfit_w, pfit_h, hscale, vscale; > unsigned long irqflags; > int id; > + int scaler_filter_ctl; It's a register value so u32. I'd also s/scaler_filter_ctl/filter_select/ or something like that. Alternatively we could just call it ps_ctrl and have it contain the full register value for that particular register. > > if (drm_WARN_ON(&dev_priv->drm, > crtc_state->scaler_state.scaler_id < 0)) > @@ -6340,8 +6361,12 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN | > - PS_FILTER_MEDIUM | scaler_state->scalers[id].mode); > + scaler_filter_ctl = > + skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0, > + state->scaling_filter); > + intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), > + PS_SCALER_EN | scaler_filter_ctl | > + scaler_state->scalers[id].mode); > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id), > PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase)); > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id), > @@ -16777,6 +16802,9 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe) > dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc; > } > > + if (INTEL_GEN(dev_priv) >= 10) > + drm_crtc_enable_scaling_filter(&crtc->base); > + > intel_color_init(crtc); > > intel_crtc_crc_init(crtc); > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c > index deda351719db..ac3fd9843ace 100644 > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane, > return min(8192 * cpp, 32768); > } > > +static u32 > +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe, > + int id, int set, > + enum drm_plane_scaling_filter filter) > +{ > + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; > + > + if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) { > + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, > + set); > + scaler_filter_ctl = PS_FILTER_PROGRAMMED | > + PS_UV_VERT_FILTER_SELECT(set) | > + PS_UV_HORZ_FILTER_SELECT(set) | > + PS_Y_VERT_FILTER_SELECT(set) | > + PS_Y_HORZ_FILTER_SELECT(set); > + > + } > + return scaler_filter_ctl; > +} > + We don't want such copy pasta between planes and crtcs. > static void > skl_program_scaler(struct intel_plane *plane, > const struct intel_crtc_state *crtc_state, > @@ -406,6 +426,7 @@ skl_program_scaler(struct intel_plane *plane, > int scaler_id = plane_state->scaler_id; > const struct intel_scaler *scaler = > &crtc_state->scaler_state.scalers[scaler_id]; > + const struct drm_plane_state *state = &plane_state->uapi; > int crtc_x = plane_state->uapi.dst.x1; > int crtc_y = plane_state->uapi.dst.y1; > u32 crtc_w = drm_rect_width(&plane_state->uapi.dst); > @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane, > u16 y_hphase, uv_rgb_hphase; > u16 y_vphase, uv_rgb_vphase; > int hscale, vscale; > + int scaler_filter_ctl; > > hscale = drm_rect_calc_hscale(&plane_state->uapi.src, > &plane_state->uapi.dst, > @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane, > uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); > } > > + scaler_filter_ctl = > + skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0, > + state->scaling_filter); > intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id), > - PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler->mode); > + PS_SCALER_EN | PS_PLANE_SEL(plane->id) | > + scaler->mode | scaler_filter_ctl); > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, scaler_id), > PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase)); > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, scaler_id), > @@ -3121,6 +3147,9 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv, > > drm_plane_create_zpos_immutable_property(&plane->base, plane_id); > > + if (INTEL_GEN(dev_priv) >= 10) > + drm_plane_enable_scaling_filter(&plane->base); > + > drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs); > > return plane; > -- > 2.23.0
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: 23 March 2020 20:18 > To: Laxminarayan Bharadiya, Pankaj > <pankaj.laxminarayan.bharadiya@intel.com> > Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>; > jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org; > dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen > <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; > David Airlie <airlied@linux.ie>; Chris Wilson <chris@chris-wilson.co.uk>; > Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Souza, Jose > <jose.souza@intel.com>; Deak, Imre <imre.deak@intel.com>; Shankar, Uma > <uma.shankar@intel.com> > Subject: Re: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC > > On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote: > > GEN >= 10 hardware supports the programmable scaler filter. > > > > Attach scaling filter property for CRTC and plane for GEN >= 10 > > hardwares and program scaler filter based on the selected filter type. > > > > changes since v1: > > * None > > Changes since RFC: > > * Enable properties for GEN >= 10 platforms (Ville) > > * Do not round off the crtc co-ordinate (Danial Stone, Ville) > > * Add new functions to handle scaling filter setup (Ville) > > * Remove coefficient set 0 hardcoding. > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > Signed-off-by: Pankaj Bharadiya > > <pankaj.laxminarayan.bharadiya@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 32 > > ++++++++++++++++++-- drivers/gpu/drm/i915/display/intel_sprite.c | > > 31 ++++++++++++++++++- > > 2 files changed, 60 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index 791dd908aa89..4b3387ee332e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -6309,6 +6309,25 @@ void > skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv, > > } > > } > > > > +static u32 > > +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe > pipe, > > + int id, int set, enum drm_crtc_scaling_filter filter) { > > + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; > > + > > + if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) { > > + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, > > + set); > > + scaler_filter_ctl = PS_FILTER_PROGRAMMED | > > + PS_UV_VERT_FILTER_SELECT(set) | > > + PS_UV_HORZ_FILTER_SELECT(set) | > > + PS_Y_VERT_FILTER_SELECT(set) | > > + PS_Y_HORZ_FILTER_SELECT(set); > > + > > + } > > + return scaler_filter_ctl; > > This function does too many things. I was thinking to have a common function which configures the filter and also provides the register bits (ps_ctrl) to select a desired filter so that we need not have extra condition to figure out filter select register bits where this function is being called. How about renaming this function to some better name like skl_scaler_set_and_get_filter_select() or something else? Or shall I breakdown this function into multiple functions? Any suggestions? > > > +} > > + > > static void skl_pfit_enable(const struct intel_crtc_state > > *crtc_state) { > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct > intel_crtc_state *crtc_state) > > enum pipe pipe = crtc->pipe; > > const struct intel_crtc_scaler_state *scaler_state = > > &crtc_state->scaler_state; > > + const struct drm_crtc_state *state = &crtc_state->uapi; > > Pls don't add this kind of aliases. We're moving away from using the drm_ types > as much as possible. OK. > > > > > if (crtc_state->pch_pfit.enabled) { > > u16 uv_rgb_hphase, uv_rgb_vphase; > > int pfit_w, pfit_h, hscale, vscale; > > unsigned long irqflags; > > int id; > > + int scaler_filter_ctl; > > It's a register value so u32. I'd also Yes, I missed it. Thanks for pointing out. > s/scaler_filter_ctl/filter_select/ or something like that. > > Alternatively we could just call it ps_ctrl and have it contain the full register > value for that particular register. ps_ctrl sounds better, will use this name. > > > > > if (drm_WARN_ON(&dev_priv->drm, > > crtc_state->scaler_state.scaler_id < 0)) @@ - > 6340,8 +6361,12 @@ > > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) > > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > - intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), > PS_SCALER_EN | > > - PS_FILTER_MEDIUM | scaler_state- > >scalers[id].mode); > > + scaler_filter_ctl = > > + skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0, > > + state->scaling_filter); > > + intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), > > + PS_SCALER_EN | scaler_filter_ctl | > > + scaler_state->scalers[id].mode); > > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id), > > PS_Y_PHASE(0) | > PS_UV_RGB_PHASE(uv_rgb_vphase)); > > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id), @@ - > 16777,6 > > +16802,9 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, > enum pipe pipe) > > dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc; > > } > > > > + if (INTEL_GEN(dev_priv) >= 10) > > + drm_crtc_enable_scaling_filter(&crtc->base); > > + > > intel_color_init(crtc); > > > > intel_crtc_crc_init(crtc); > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c > > b/drivers/gpu/drm/i915/display/intel_sprite.c > > index deda351719db..ac3fd9843ace 100644 > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane, > > return min(8192 * cpp, 32768); > > } > > > > +static u32 > > +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe > pipe, > > + int id, int set, > > + enum drm_plane_scaling_filter filter) { > > + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; > > + > > + if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) { > > + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, > > + set); > > + scaler_filter_ctl = PS_FILTER_PROGRAMMED | > > + PS_UV_VERT_FILTER_SELECT(set) | > > + PS_UV_HORZ_FILTER_SELECT(set) | > > + PS_Y_VERT_FILTER_SELECT(set) | > > + PS_Y_HORZ_FILTER_SELECT(set); > > + > > + } > > + return scaler_filter_ctl; > > +} > > + > > We don't want such copy pasta between planes and crtcs. Yeah, got it. Will add a common enum drm_scaling_filter and use it. Thanks, Pankaj > > > static void > > skl_program_scaler(struct intel_plane *plane, > > const struct intel_crtc_state *crtc_state, @@ -406,6 +426,7 > @@ > > skl_program_scaler(struct intel_plane *plane, > > int scaler_id = plane_state->scaler_id; > > const struct intel_scaler *scaler = > > &crtc_state->scaler_state.scalers[scaler_id]; > > + const struct drm_plane_state *state = &plane_state->uapi; > > int crtc_x = plane_state->uapi.dst.x1; > > int crtc_y = plane_state->uapi.dst.y1; > > u32 crtc_w = drm_rect_width(&plane_state->uapi.dst); > > @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane, > > u16 y_hphase, uv_rgb_hphase; > > u16 y_vphase, uv_rgb_vphase; > > int hscale, vscale; > > + int scaler_filter_ctl; > > > > hscale = drm_rect_calc_hscale(&plane_state->uapi.src, > > &plane_state->uapi.dst, > > @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane, > > uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); > > } > > > > + scaler_filter_ctl = > > + skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0, > > + state->scaling_filter); > > intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id), > > - PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler- > >mode); > > + PS_SCALER_EN | PS_PLANE_SEL(plane->id) | > > + scaler->mode | scaler_filter_ctl); > > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, scaler_id), > > PS_Y_PHASE(y_vphase) | > PS_UV_RGB_PHASE(uv_rgb_vphase)); > > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, scaler_id), @@ > > -3121,6 +3147,9 @@ skl_universal_plane_create(struct drm_i915_private > > *dev_priv, > > > > drm_plane_create_zpos_immutable_property(&plane->base, plane_id); > > > > + if (INTEL_GEN(dev_priv) >= 10) > > + drm_plane_enable_scaling_filter(&plane->base); > > + > > drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs); > > > > return plane; > > -- > > 2.23.0 > > -- > Ville Syrjälä > Intel
On Tue, Mar 24, 2020 at 03:32:09PM +0000, Laxminarayan Bharadiya, Pankaj wrote: > > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: 23 March 2020 20:18 > > To: Laxminarayan Bharadiya, Pankaj > > <pankaj.laxminarayan.bharadiya@intel.com> > > Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>; > > jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org; > > dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen > > <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; > > David Airlie <airlied@linux.ie>; Chris Wilson <chris@chris-wilson.co.uk>; > > Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Souza, Jose > > <jose.souza@intel.com>; Deak, Imre <imre.deak@intel.com>; Shankar, Uma > > <uma.shankar@intel.com> > > Subject: Re: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC > > > > On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote: > > > GEN >= 10 hardware supports the programmable scaler filter. > > > > > > Attach scaling filter property for CRTC and plane for GEN >= 10 > > > hardwares and program scaler filter based on the selected filter type. > > > > > > changes since v1: > > > * None > > > Changes since RFC: > > > * Enable properties for GEN >= 10 platforms (Ville) > > > * Do not round off the crtc co-ordinate (Danial Stone, Ville) > > > * Add new functions to handle scaling filter setup (Ville) > > > * Remove coefficient set 0 hardcoding. > > > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > > Signed-off-by: Pankaj Bharadiya > > > <pankaj.laxminarayan.bharadiya@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 32 > > > ++++++++++++++++++-- drivers/gpu/drm/i915/display/intel_sprite.c | > > > 31 ++++++++++++++++++- > > > 2 files changed, 60 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index 791dd908aa89..4b3387ee332e 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -6309,6 +6309,25 @@ void > > skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv, > > > } > > > } > > > > > > +static u32 > > > +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe > > pipe, > > > + int id, int set, enum drm_crtc_scaling_filter filter) { > > > + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; > > > + > > > + if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) { > > > + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, > > > + set); > > > + scaler_filter_ctl = PS_FILTER_PROGRAMMED | > > > + PS_UV_VERT_FILTER_SELECT(set) | > > > + PS_UV_HORZ_FILTER_SELECT(set) | > > > + PS_Y_VERT_FILTER_SELECT(set) | > > > + PS_Y_HORZ_FILTER_SELECT(set); > > > + > > > + } > > > + return scaler_filter_ctl; > > > > This function does too many things. > > I was thinking to have a common function which configures the filter and also > provides the register bits (ps_ctrl) to select a desired filter so that we need > not have extra condition to figure out filter select register bits where this > function is being called. > How about renaming this function to some better name like > skl_scaler_set_and_get_filter_select() or something else? > Or shall I breakdown this function into multiple functions? I'd just inline the PS_CTRL stuff into the current function. > > Any suggestions? > > > > > > +} > > > + > > > static void skl_pfit_enable(const struct intel_crtc_state > > > *crtc_state) { > > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct > > intel_crtc_state *crtc_state) > > > enum pipe pipe = crtc->pipe; > > > const struct intel_crtc_scaler_state *scaler_state = > > > &crtc_state->scaler_state; > > > + const struct drm_crtc_state *state = &crtc_state->uapi; > > > > Pls don't add this kind of aliases. We're moving away from using the drm_ types > > as much as possible. > > OK. > > > > > > > > > if (crtc_state->pch_pfit.enabled) { > > > u16 uv_rgb_hphase, uv_rgb_vphase; > > > int pfit_w, pfit_h, hscale, vscale; > > > unsigned long irqflags; > > > int id; > > > + int scaler_filter_ctl; > > > > It's a register value so u32. I'd also > > Yes, I missed it. Thanks for pointing out. > > > s/scaler_filter_ctl/filter_select/ or something like that. > > > > Alternatively we could just call it ps_ctrl and have it contain the full register > > value for that particular register. > > ps_ctrl sounds better, will use this name. > > > > > > > > > if (drm_WARN_ON(&dev_priv->drm, > > > crtc_state->scaler_state.scaler_id < 0)) @@ - > > 6340,8 +6361,12 @@ > > > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) > > > > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > > > - intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), > > PS_SCALER_EN | > > > - PS_FILTER_MEDIUM | scaler_state- > > >scalers[id].mode); > > > + scaler_filter_ctl = > > > + skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0, > > > + state->scaling_filter); > > > + intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), > > > + PS_SCALER_EN | scaler_filter_ctl | > > > + scaler_state->scalers[id].mode); > > > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id), > > > PS_Y_PHASE(0) | > > PS_UV_RGB_PHASE(uv_rgb_vphase)); > > > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id), @@ - > > 16777,6 > > > +16802,9 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, > > enum pipe pipe) > > > dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc; > > > } > > > > > > + if (INTEL_GEN(dev_priv) >= 10) > > > + drm_crtc_enable_scaling_filter(&crtc->base); > > > + > > > intel_color_init(crtc); > > > > > > intel_crtc_crc_init(crtc); > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c > > > b/drivers/gpu/drm/i915/display/intel_sprite.c > > > index deda351719db..ac3fd9843ace 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > > @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane, > > > return min(8192 * cpp, 32768); > > > } > > > > > > +static u32 > > > +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe > > pipe, > > > + int id, int set, > > > + enum drm_plane_scaling_filter filter) { > > > + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; > > > + > > > + if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) { > > > + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, > > > + set); > > > + scaler_filter_ctl = PS_FILTER_PROGRAMMED | > > > + PS_UV_VERT_FILTER_SELECT(set) | > > > + PS_UV_HORZ_FILTER_SELECT(set) | > > > + PS_Y_VERT_FILTER_SELECT(set) | > > > + PS_Y_HORZ_FILTER_SELECT(set); > > > + > > > + } > > > + return scaler_filter_ctl; > > > +} > > > + > > > > We don't want such copy pasta between planes and crtcs. > > Yeah, got it. > Will add a common enum drm_scaling_filter and use it. > > Thanks, > Pankaj > > > > > > static void > > > skl_program_scaler(struct intel_plane *plane, > > > const struct intel_crtc_state *crtc_state, @@ -406,6 +426,7 > > @@ > > > skl_program_scaler(struct intel_plane *plane, > > > int scaler_id = plane_state->scaler_id; > > > const struct intel_scaler *scaler = > > > &crtc_state->scaler_state.scalers[scaler_id]; > > > + const struct drm_plane_state *state = &plane_state->uapi; > > > int crtc_x = plane_state->uapi.dst.x1; > > > int crtc_y = plane_state->uapi.dst.y1; > > > u32 crtc_w = drm_rect_width(&plane_state->uapi.dst); > > > @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane, > > > u16 y_hphase, uv_rgb_hphase; > > > u16 y_vphase, uv_rgb_vphase; > > > int hscale, vscale; > > > + int scaler_filter_ctl; > > > > > > hscale = drm_rect_calc_hscale(&plane_state->uapi.src, > > > &plane_state->uapi.dst, > > > @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane, > > > uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); > > > } > > > > > > + scaler_filter_ctl = > > > + skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0, > > > + state->scaling_filter); > > > intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id), > > > - PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler- > > >mode); > > > + PS_SCALER_EN | PS_PLANE_SEL(plane->id) | > > > + scaler->mode | scaler_filter_ctl); > > > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, scaler_id), > > > PS_Y_PHASE(y_vphase) | > > PS_UV_RGB_PHASE(uv_rgb_vphase)); > > > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, scaler_id), @@ > > > -3121,6 +3147,9 @@ skl_universal_plane_create(struct drm_i915_private > > > *dev_priv, > > > > > > drm_plane_create_zpos_immutable_property(&plane->base, plane_id); > > > > > > + if (INTEL_GEN(dev_priv) >= 10) > > > + drm_plane_enable_scaling_filter(&plane->base); > > > + > > > drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs); > > > > > > return plane; > > > -- > > > 2.23.0 > > > > -- > > Ville Syrjälä > > Intel
On Tue, Mar 24, 2020 at 06:46:10PM +0200, Ville Syrjälä wrote: > On Tue, Mar 24, 2020 at 03:32:09PM +0000, Laxminarayan Bharadiya, Pankaj wrote: > > > > > > > -----Original Message----- > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Sent: 23 March 2020 20:18 > > > To: Laxminarayan Bharadiya, Pankaj > > > <pankaj.laxminarayan.bharadiya@intel.com> > > > Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>; > > > jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org; > > > dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen > > > <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; > > > David Airlie <airlied@linux.ie>; Chris Wilson <chris@chris-wilson.co.uk>; > > > Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Souza, Jose > > > <jose.souza@intel.com>; Deak, Imre <imre.deak@intel.com>; Shankar, Uma > > > <uma.shankar@intel.com> > > > Subject: Re: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC > > > > > > On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote: > > > > GEN >= 10 hardware supports the programmable scaler filter. > > > > > > > > Attach scaling filter property for CRTC and plane for GEN >= 10 > > > > hardwares and program scaler filter based on the selected filter type. > > > > > > > > changes since v1: > > > > * None > > > > Changes since RFC: > > > > * Enable properties for GEN >= 10 platforms (Ville) > > > > * Do not round off the crtc co-ordinate (Danial Stone, Ville) > > > > * Add new functions to handle scaling filter setup (Ville) > > > > * Remove coefficient set 0 hardcoding. > > > > > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > > > Signed-off-by: Pankaj Bharadiya > > > > <pankaj.laxminarayan.bharadiya@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display.c | 32 > > > > ++++++++++++++++++-- drivers/gpu/drm/i915/display/intel_sprite.c | > > > > 31 ++++++++++++++++++- > > > > 2 files changed, 60 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > > index 791dd908aa89..4b3387ee332e 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > @@ -6309,6 +6309,25 @@ void > > > skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv, > > > > } > > > > } > > > > > > > > +static u32 > > > > +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe > > > pipe, > > > > + int id, int set, enum drm_crtc_scaling_filter filter) { > > > > + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; > > > > + > > > > + if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) { > > > > + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, > > > > + set); > > > > + scaler_filter_ctl = PS_FILTER_PROGRAMMED | > > > > + PS_UV_VERT_FILTER_SELECT(set) | > > > > + PS_UV_HORZ_FILTER_SELECT(set) | > > > > + PS_Y_VERT_FILTER_SELECT(set) | > > > > + PS_Y_HORZ_FILTER_SELECT(set); > > > > + > > > > + } > > > > + return scaler_filter_ctl; > > > > > > This function does too many things. > > > > I was thinking to have a common function which configures the filter and also > > provides the register bits (ps_ctrl) to select a desired filter so that we need > > not have extra condition to figure out filter select register bits where this > > function is being called. > > How about renaming this function to some better name like > > skl_scaler_set_and_get_filter_select() or something else? > > Or shall I breakdown this function into multiple functions? > > I'd just inline the PS_CTRL stuff into the current function. I am yet to verify this, but would like to get your early comments. How about something like this? - +inline u32 skl_scaler_get_filter_select(enum drm_scaling_filter filter, int set) +{ + u32 filter_select = PS_FILTER_MEDIUM; + + if (filter == DRM_SCALING_FILTER_NEAREST_NEIGHBOR) { + filter_select = PS_FILTER_PROGRAMMED | + PS_UV_VERT_FILTER_SELECT(set) | + PS_UV_HORZ_FILTER_SELECT(set) | + PS_Y_VERT_FILTER_SELECT(set) | + PS_Y_HORZ_FILTER_SELECT(set); + } + + return filter_select; +} + +void skl_scaler_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe, + int id, int set, enum drm_scaling_filter filter) +{ + switch(filter) { + case DRM_SCALING_FILTER_DEFAULT: + break; + case DRM_SCALING_FILTER_NEAREST_NEIGHBOR: + cnl_program_nearest_filter_coefs(dev_priv, pipe, id, set); + break; + default: + default: + MISSING_CASE(filter); + } +} + static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) { struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); @@ -6250,6 +6351,7 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) int pfit_w, pfit_h, hscale, vscale; unsigned long irqflags; int id; + u32 ps_ctrl; if (drm_WARN_ON(&dev_priv->drm, crtc_state->scaler_state.scaler_id < 0)) @@ -6268,8 +6370,13 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); - intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN | - PS_FILTER_MEDIUM | scaler_state->scalers[id].mode); + skl_scaler_setup_filter(dev_priv, pipe, id, 0, + crtc_state->uapi.scaling_filter); + + ps_ctrl = skl_scaler_get_filter_select(crtc_state->uapi.scaling_filter, 0); + ps_ctrl |= PS_SCALER_EN | scaler_state->scalers[id].mode; + + intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), ps_ctrl); intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id), PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase)); intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id), @@ -16703,6 +16810,11 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe) dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc; } Thanks, Pankaj > > > > > Any suggestions? > > > > > > > > > +} > > > > + > > > > static void skl_pfit_enable(const struct intel_crtc_state > > > > *crtc_state) { > > > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > > @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct > > > intel_crtc_state *crtc_state) > > > > enum pipe pipe = crtc->pipe; > > > > const struct intel_crtc_scaler_state *scaler_state = > > > > &crtc_state->scaler_state; > > > > + const struct drm_crtc_state *state = &crtc_state->uapi; > > > > > > Pls don't add this kind of aliases. We're moving away from using the drm_ types > > > as much as possible. > > > > OK. > > > > > > > > > > > > > if (crtc_state->pch_pfit.enabled) { > > > > u16 uv_rgb_hphase, uv_rgb_vphase; > > > > int pfit_w, pfit_h, hscale, vscale; > > > > unsigned long irqflags; > > > > int id; > > > > + int scaler_filter_ctl; > > > > > > It's a register value so u32. I'd also > > > > Yes, I missed it. Thanks for pointing out. > > > > > s/scaler_filter_ctl/filter_select/ or something like that. > > > > > > Alternatively we could just call it ps_ctrl and have it contain the full register > > > value for that particular register. > > > > ps_ctrl sounds better, will use this name. > > > > > > > > > > > > > if (drm_WARN_ON(&dev_priv->drm, > > > > crtc_state->scaler_state.scaler_id < 0)) @@ - > > > 6340,8 +6361,12 @@ > > > > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) > > > > > > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > > > > > - intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), > > > PS_SCALER_EN | > > > > - PS_FILTER_MEDIUM | scaler_state- > > > >scalers[id].mode); > > > > + scaler_filter_ctl = > > > > + skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0, > > > > + state->scaling_filter); > > > > + intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), > > > > + PS_SCALER_EN | scaler_filter_ctl | > > > > + scaler_state->scalers[id].mode); > > > > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id), > > > > PS_Y_PHASE(0) | > > > PS_UV_RGB_PHASE(uv_rgb_vphase)); > > > > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id), @@ - > > > 16777,6 > > > > +16802,9 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, > > > enum pipe pipe) > > > > dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc; > > > > } > > > > > > > > + if (INTEL_GEN(dev_priv) >= 10) > > > > + drm_crtc_enable_scaling_filter(&crtc->base); > > > > + > > > > intel_color_init(crtc); > > > > > > > > intel_crtc_crc_init(crtc); > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c > > > > b/drivers/gpu/drm/i915/display/intel_sprite.c > > > > index deda351719db..ac3fd9843ace 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > > > @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane, > > > > return min(8192 * cpp, 32768); > > > > } > > > > > > > > +static u32 > > > > +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe > > > pipe, > > > > + int id, int set, > > > > + enum drm_plane_scaling_filter filter) { > > > > + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; > > > > + > > > > + if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) { > > > > + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, > > > > + set); > > > > + scaler_filter_ctl = PS_FILTER_PROGRAMMED | > > > > + PS_UV_VERT_FILTER_SELECT(set) | > > > > + PS_UV_HORZ_FILTER_SELECT(set) | > > > > + PS_Y_VERT_FILTER_SELECT(set) | > > > > + PS_Y_HORZ_FILTER_SELECT(set); > > > > + > > > > + } > > > > + return scaler_filter_ctl; > > > > +} > > > > + > > > > > > We don't want such copy pasta between planes and crtcs. > > > > Yeah, got it. > > Will add a common enum drm_scaling_filter and use it. > > > > Thanks, > > Pankaj > > > > > > > > > static void > > > > skl_program_scaler(struct intel_plane *plane, > > > > const struct intel_crtc_state *crtc_state, @@ -406,6 +426,7 > > > @@ > > > > skl_program_scaler(struct intel_plane *plane, > > > > int scaler_id = plane_state->scaler_id; > > > > const struct intel_scaler *scaler = > > > > &crtc_state->scaler_state.scalers[scaler_id]; > > > > + const struct drm_plane_state *state = &plane_state->uapi; > > > > int crtc_x = plane_state->uapi.dst.x1; > > > > int crtc_y = plane_state->uapi.dst.y1; > > > > u32 crtc_w = drm_rect_width(&plane_state->uapi.dst); > > > > @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane, > > > > u16 y_hphase, uv_rgb_hphase; > > > > u16 y_vphase, uv_rgb_vphase; > > > > int hscale, vscale; > > > > + int scaler_filter_ctl; > > > > > > > > hscale = drm_rect_calc_hscale(&plane_state->uapi.src, > > > > &plane_state->uapi.dst, > > > > @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane, > > > > uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); > > > > } > > > > > > > > + scaler_filter_ctl = > > > > + skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0, > > > > + state->scaling_filter); > > > > intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id), > > > > - PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler- > > > >mode); > > > > + PS_SCALER_EN | PS_PLANE_SEL(plane->id) | > > > > + scaler->mode | scaler_filter_ctl); > > > > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, scaler_id), > > > > PS_Y_PHASE(y_vphase) | > > > PS_UV_RGB_PHASE(uv_rgb_vphase)); > > > > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, scaler_id), @@ > > > > -3121,6 +3147,9 @@ skl_universal_plane_create(struct drm_i915_private > > > > *dev_priv, > > > > > > > > drm_plane_create_zpos_immutable_property(&plane->base, plane_id); > > > > > > > > + if (INTEL_GEN(dev_priv) >= 10) > > > > + drm_plane_enable_scaling_filter(&plane->base); > > > > + > > > > drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs); > > > > > > > > return plane; > > > > -- > > > > 2.23.0 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Thu, Mar 26, 2020 at 08:45:59PM +0530, Bharadiya,Pankaj wrote: > On Tue, Mar 24, 2020 at 06:46:10PM +0200, Ville Syrjälä wrote: > > On Tue, Mar 24, 2020 at 03:32:09PM +0000, Laxminarayan Bharadiya, Pankaj wrote: > > > > > > > > > > -----Original Message----- > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Sent: 23 March 2020 20:18 > > > > To: Laxminarayan Bharadiya, Pankaj > > > > <pankaj.laxminarayan.bharadiya@intel.com> > > > > Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>; > > > > jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org; > > > > dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen > > > > <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; > > > > David Airlie <airlied@linux.ie>; Chris Wilson <chris@chris-wilson.co.uk>; > > > > Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Souza, Jose > > > > <jose.souza@intel.com>; Deak, Imre <imre.deak@intel.com>; Shankar, Uma > > > > <uma.shankar@intel.com> > > > > Subject: Re: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC > > > > > > > > On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote: > > > > > GEN >= 10 hardware supports the programmable scaler filter. > > > > > > > > > > Attach scaling filter property for CRTC and plane for GEN >= 10 > > > > > hardwares and program scaler filter based on the selected filter type. > > > > > > > > > > changes since v1: > > > > > * None > > > > > Changes since RFC: > > > > > * Enable properties for GEN >= 10 platforms (Ville) > > > > > * Do not round off the crtc co-ordinate (Danial Stone, Ville) > > > > > * Add new functions to handle scaling filter setup (Ville) > > > > > * Remove coefficient set 0 hardcoding. > > > > > > > > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > > > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > > > > Signed-off-by: Pankaj Bharadiya > > > > > <pankaj.laxminarayan.bharadiya@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_display.c | 32 > > > > > ++++++++++++++++++-- drivers/gpu/drm/i915/display/intel_sprite.c | > > > > > 31 ++++++++++++++++++- > > > > > 2 files changed, 60 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > > > index 791dd908aa89..4b3387ee332e 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > @@ -6309,6 +6309,25 @@ void > > > > skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv, > > > > > } > > > > > } > > > > > > > > > > +static u32 > > > > > +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe > > > > pipe, > > > > > + int id, int set, enum drm_crtc_scaling_filter filter) { > > > > > + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; > > > > > + > > > > > + if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) { > > > > > + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, > > > > > + set); > > > > > + scaler_filter_ctl = PS_FILTER_PROGRAMMED | > > > > > + PS_UV_VERT_FILTER_SELECT(set) | > > > > > + PS_UV_HORZ_FILTER_SELECT(set) | > > > > > + PS_Y_VERT_FILTER_SELECT(set) | > > > > > + PS_Y_HORZ_FILTER_SELECT(set); > > > > > + > > > > > + } > > > > > + return scaler_filter_ctl; > > > > > > > > This function does too many things. > > > > > > I was thinking to have a common function which configures the filter and also > > > provides the register bits (ps_ctrl) to select a desired filter so that we need > > > not have extra condition to figure out filter select register bits where this > > > function is being called. > > > How about renaming this function to some better name like > > > skl_scaler_set_and_get_filter_select() or something else? > > > Or shall I breakdown this function into multiple functions? > > > > I'd just inline the PS_CTRL stuff into the current function. > > I am yet to verify this, but would like to get your early comments. > How about something like this? - > > +inline u32 skl_scaler_get_filter_select(enum drm_scaling_filter filter, int set) > +{ > + u32 filter_select = PS_FILTER_MEDIUM; Pointless variable. if (whatever) return A; else return B; > + > + if (filter == DRM_SCALING_FILTER_NEAREST_NEIGHBOR) { > + filter_select = PS_FILTER_PROGRAMMED | > + PS_UV_VERT_FILTER_SELECT(set) | > + PS_UV_HORZ_FILTER_SELECT(set) | > + PS_Y_VERT_FILTER_SELECT(set) | > + PS_Y_HORZ_FILTER_SELECT(set); > + } > + > + return filter_select; > +} > + > +void skl_scaler_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe, > + int id, int set, enum drm_scaling_filter filter) > +{ > + switch(filter) { > + case DRM_SCALING_FILTER_DEFAULT: > + break; > + case DRM_SCALING_FILTER_NEAREST_NEIGHBOR: > + cnl_program_nearest_filter_coefs(dev_priv, pipe, id, set); > + break; > + default: > + default: > + MISSING_CASE(filter); > + } > +} > + > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > @@ -6250,6 +6351,7 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) > int pfit_w, pfit_h, hscale, vscale; > unsigned long irqflags; > int id; > + u32 ps_ctrl; > > if (drm_WARN_ON(&dev_priv->drm, > crtc_state->scaler_state.scaler_id < 0)) > @@ -6268,8 +6370,13 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN | > - PS_FILTER_MEDIUM | scaler_state->scalers[id].mode); > + skl_scaler_setup_filter(dev_priv, pipe, id, 0, > + crtc_state->uapi.scaling_filter); btw we need to duplicate the scaling filter in the hw state. > + > + ps_ctrl = skl_scaler_get_filter_select(crtc_state->uapi.scaling_filter, 0); > + ps_ctrl |= PS_SCALER_EN | scaler_state->scalers[id].mode; This can all be done outside the spinlock. Otherwise lgtm > + > + intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), ps_ctrl); > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id), > PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase)); > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id), > @@ -16703,6 +16810,11 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe) > dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc; > } > > Thanks, > Pankaj > > > > > > > > > Any suggestions? > > > > > > > > > > > > +} > > > > > + > > > > > static void skl_pfit_enable(const struct intel_crtc_state > > > > > *crtc_state) { > > > > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > > > @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct > > > > intel_crtc_state *crtc_state) > > > > > enum pipe pipe = crtc->pipe; > > > > > const struct intel_crtc_scaler_state *scaler_state = > > > > > &crtc_state->scaler_state; > > > > > + const struct drm_crtc_state *state = &crtc_state->uapi; > > > > > > > > Pls don't add this kind of aliases. We're moving away from using the drm_ types > > > > as much as possible. > > > > > > OK. > > > > > > > > > > > > > > > > > if (crtc_state->pch_pfit.enabled) { > > > > > u16 uv_rgb_hphase, uv_rgb_vphase; > > > > > int pfit_w, pfit_h, hscale, vscale; > > > > > unsigned long irqflags; > > > > > int id; > > > > > + int scaler_filter_ctl; > > > > > > > > It's a register value so u32. I'd also > > > > > > Yes, I missed it. Thanks for pointing out. > > > > > > > s/scaler_filter_ctl/filter_select/ or something like that. > > > > > > > > Alternatively we could just call it ps_ctrl and have it contain the full register > > > > value for that particular register. > > > > > > ps_ctrl sounds better, will use this name. > > > > > > > > > > > > > > > > > if (drm_WARN_ON(&dev_priv->drm, > > > > > crtc_state->scaler_state.scaler_id < 0)) @@ - > > > > 6340,8 +6361,12 @@ > > > > > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) > > > > > > > > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > > > > > > > - intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), > > > > PS_SCALER_EN | > > > > > - PS_FILTER_MEDIUM | scaler_state- > > > > >scalers[id].mode); > > > > > + scaler_filter_ctl = > > > > > + skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0, > > > > > + state->scaling_filter); > > > > > + intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), > > > > > + PS_SCALER_EN | scaler_filter_ctl | > > > > > + scaler_state->scalers[id].mode); > > > > > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id), > > > > > PS_Y_PHASE(0) | > > > > PS_UV_RGB_PHASE(uv_rgb_vphase)); > > > > > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id), @@ - > > > > 16777,6 > > > > > +16802,9 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, > > > > enum pipe pipe) > > > > > dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc; > > > > > } > > > > > > > > > > + if (INTEL_GEN(dev_priv) >= 10) > > > > > + drm_crtc_enable_scaling_filter(&crtc->base); > > > > > + > > > > > intel_color_init(crtc); > > > > > > > > > > intel_crtc_crc_init(crtc); > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c > > > > > b/drivers/gpu/drm/i915/display/intel_sprite.c > > > > > index deda351719db..ac3fd9843ace 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > > > > @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane, > > > > > return min(8192 * cpp, 32768); > > > > > } > > > > > > > > > > +static u32 > > > > > +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe > > > > pipe, > > > > > + int id, int set, > > > > > + enum drm_plane_scaling_filter filter) { > > > > > + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; > > > > > + > > > > > + if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) { > > > > > + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, > > > > > + set); > > > > > + scaler_filter_ctl = PS_FILTER_PROGRAMMED | > > > > > + PS_UV_VERT_FILTER_SELECT(set) | > > > > > + PS_UV_HORZ_FILTER_SELECT(set) | > > > > > + PS_Y_VERT_FILTER_SELECT(set) | > > > > > + PS_Y_HORZ_FILTER_SELECT(set); > > > > > + > > > > > + } > > > > > + return scaler_filter_ctl; > > > > > +} > > > > > + > > > > > > > > We don't want such copy pasta between planes and crtcs. > > > > > > Yeah, got it. > > > Will add a common enum drm_scaling_filter and use it. > > > > > > Thanks, > > > Pankaj > > > > > > > > > > > > static void > > > > > skl_program_scaler(struct intel_plane *plane, > > > > > const struct intel_crtc_state *crtc_state, @@ -406,6 +426,7 > > > > @@ > > > > > skl_program_scaler(struct intel_plane *plane, > > > > > int scaler_id = plane_state->scaler_id; > > > > > const struct intel_scaler *scaler = > > > > > &crtc_state->scaler_state.scalers[scaler_id]; > > > > > + const struct drm_plane_state *state = &plane_state->uapi; > > > > > int crtc_x = plane_state->uapi.dst.x1; > > > > > int crtc_y = plane_state->uapi.dst.y1; > > > > > u32 crtc_w = drm_rect_width(&plane_state->uapi.dst); > > > > > @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane, > > > > > u16 y_hphase, uv_rgb_hphase; > > > > > u16 y_vphase, uv_rgb_vphase; > > > > > int hscale, vscale; > > > > > + int scaler_filter_ctl; > > > > > > > > > > hscale = drm_rect_calc_hscale(&plane_state->uapi.src, > > > > > &plane_state->uapi.dst, > > > > > @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane, > > > > > uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); > > > > > } > > > > > > > > > > + scaler_filter_ctl = > > > > > + skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0, > > > > > + state->scaling_filter); > > > > > intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id), > > > > > - PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler- > > > > >mode); > > > > > + PS_SCALER_EN | PS_PLANE_SEL(plane->id) | > > > > > + scaler->mode | scaler_filter_ctl); > > > > > intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, scaler_id), > > > > > PS_Y_PHASE(y_vphase) | > > > > PS_UV_RGB_PHASE(uv_rgb_vphase)); > > > > > intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, scaler_id), @@ > > > > > -3121,6 +3147,9 @@ skl_universal_plane_create(struct drm_i915_private > > > > > *dev_priv, > > > > > > > > > > drm_plane_create_zpos_immutable_property(&plane->base, plane_id); > > > > > > > > > > + if (INTEL_GEN(dev_priv) >= 10) > > > > > + drm_plane_enable_scaling_filter(&plane->base); > > > > > + > > > > > drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs); > > > > > > > > > > return plane; > > > > > -- > > > > > 2.23.0 > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 791dd908aa89..4b3387ee332e 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -6309,6 +6309,25 @@ void skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv, } } +static u32 +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe, + int id, int set, enum drm_crtc_scaling_filter filter) +{ + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; + + if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) { + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, + set); + scaler_filter_ctl = PS_FILTER_PROGRAMMED | + PS_UV_VERT_FILTER_SELECT(set) | + PS_UV_HORZ_FILTER_SELECT(set) | + PS_Y_VERT_FILTER_SELECT(set) | + PS_Y_HORZ_FILTER_SELECT(set); + + } + return scaler_filter_ctl; +} + static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) { struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) enum pipe pipe = crtc->pipe; const struct intel_crtc_scaler_state *scaler_state = &crtc_state->scaler_state; + const struct drm_crtc_state *state = &crtc_state->uapi; if (crtc_state->pch_pfit.enabled) { u16 uv_rgb_hphase, uv_rgb_vphase; int pfit_w, pfit_h, hscale, vscale; unsigned long irqflags; int id; + int scaler_filter_ctl; if (drm_WARN_ON(&dev_priv->drm, crtc_state->scaler_state.scaler_id < 0)) @@ -6340,8 +6361,12 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state) spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); - intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), PS_SCALER_EN | - PS_FILTER_MEDIUM | scaler_state->scalers[id].mode); + scaler_filter_ctl = + skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0, + state->scaling_filter); + intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id), + PS_SCALER_EN | scaler_filter_ctl | + scaler_state->scalers[id].mode); intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id), PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase)); intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id), @@ -16777,6 +16802,9 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe) dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc; } + if (INTEL_GEN(dev_priv) >= 10) + drm_crtc_enable_scaling_filter(&crtc->base); + intel_color_init(crtc); intel_crtc_crc_init(crtc); diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c index deda351719db..ac3fd9843ace 100644 --- a/drivers/gpu/drm/i915/display/intel_sprite.c +++ b/drivers/gpu/drm/i915/display/intel_sprite.c @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane, return min(8192 * cpp, 32768); } +static u32 +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe pipe, + int id, int set, + enum drm_plane_scaling_filter filter) +{ + u32 scaler_filter_ctl = PS_FILTER_MEDIUM; + + if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) { + skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id, + set); + scaler_filter_ctl = PS_FILTER_PROGRAMMED | + PS_UV_VERT_FILTER_SELECT(set) | + PS_UV_HORZ_FILTER_SELECT(set) | + PS_Y_VERT_FILTER_SELECT(set) | + PS_Y_HORZ_FILTER_SELECT(set); + + } + return scaler_filter_ctl; +} + static void skl_program_scaler(struct intel_plane *plane, const struct intel_crtc_state *crtc_state, @@ -406,6 +426,7 @@ skl_program_scaler(struct intel_plane *plane, int scaler_id = plane_state->scaler_id; const struct intel_scaler *scaler = &crtc_state->scaler_state.scalers[scaler_id]; + const struct drm_plane_state *state = &plane_state->uapi; int crtc_x = plane_state->uapi.dst.x1; int crtc_y = plane_state->uapi.dst.y1; u32 crtc_w = drm_rect_width(&plane_state->uapi.dst); @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane, u16 y_hphase, uv_rgb_hphase; u16 y_vphase, uv_rgb_vphase; int hscale, vscale; + int scaler_filter_ctl; hscale = drm_rect_calc_hscale(&plane_state->uapi.src, &plane_state->uapi.dst, @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane, uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); } + scaler_filter_ctl = + skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0, + state->scaling_filter); intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id), - PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler->mode); + PS_SCALER_EN | PS_PLANE_SEL(plane->id) | + scaler->mode | scaler_filter_ctl); intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, scaler_id), PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase)); intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, scaler_id), @@ -3121,6 +3147,9 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv, drm_plane_create_zpos_immutable_property(&plane->base, plane_id); + if (INTEL_GEN(dev_priv) >= 10) + drm_plane_enable_scaling_filter(&plane->base); + drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs); return plane;