dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: "Laxminarayan Bharadiya, Pankaj" <pankaj.laxminarayan.bharadiya@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: "Lattannavar, Sameer" <sameer.lattannavar@intel.com>,
	"daniels@collabora.com" <daniels@collabora.com>,
	David Airlie <airlied@linux.ie>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Shankar, Uma" <uma.shankar@intel.com>,
	"Vivi, Rodrigo" <rodrigo.vivi@intel.com>,
	"Souza, Jose" <jose.souza@intel.com>
Subject: RE: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC
Date: Tue, 24 Mar 2020 15:32:09 +0000	[thread overview]
Message-ID: <E92BA18FDE0A5B43B7B3DA7FCA031286057CDD2B@BGSMSX107.gar.corp.intel.com> (raw)
In-Reply-To: <20200323144749.GV13686@intel.com>



> -----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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2020-03-24 15:32 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-19 10:20 [PATCH v2 0/5] Introduce drm scaling filter property Pankaj Bharadiya
2020-03-19 10:20 ` [PATCH v2 1/5] drm: Introduce plane and CRTC scaling filter properties Pankaj Bharadiya
2020-03-23 14:21   ` Ville Syrjälä
2020-03-24 14:22     ` Laxminarayan Bharadiya, Pankaj
2020-03-19 10:21 ` [PATCH v2 2/5] drm/drm-kms.rst: Add plane and CRTC scaling filter property documentation Pankaj Bharadiya
2020-03-19 10:21 ` [PATCH v2 3/5] drm/i915: Introduce scaling filter related registers and bit fields Pankaj Bharadiya
2020-03-23 14:39   ` Ville Syrjälä
2020-03-24 14:36     ` Laxminarayan Bharadiya, Pankaj
2020-03-24 16:43       ` Ville Syrjälä
2020-03-19 10:21 ` [PATCH v2 4/5] drm/i915/display: Add Nearest-neighbor based integer scaling support Pankaj Bharadiya
2020-03-23 14:41   ` Ville Syrjälä
2020-03-19 10:21 ` [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC Pankaj Bharadiya
2020-03-23 14:47   ` Ville Syrjälä
2020-03-24 15:32     ` Laxminarayan Bharadiya, Pankaj [this message]
2020-03-24 16:46       ` Ville Syrjälä
2020-03-26 15:15         ` Bharadiya,Pankaj
2020-03-26 15:36           ` Ville Syrjälä

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=E92BA18FDE0A5B43B7B3DA7FCA031286057CDD2B@BGSMSX107.gar.corp.intel.com \
    --to=pankaj.laxminarayan.bharadiya@intel.com \
    --cc=airlied@linux.ie \
    --cc=daniels@collabora.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=jose.souza@intel.com \
    --cc=rodrigo.vivi@intel.com \
    --cc=sameer.lattannavar@intel.com \
    --cc=uma.shankar@intel.com \
    --cc=ville.syrjala@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).