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);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);
PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler-mode);
PS_SCALER_EN | PS_PLANE_SEL(plane->id) |scaler->mode | scaler_filter_ctl);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