Re: [Intel-gfx] [PATCH v3 3/6] drm/i915: Use drm_rect to store the pfit window pos/size

[Manasi Navare <manasi.d.navare@intel.com>]

On Thu, Apr 23, 2020 at 06:38:46PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 22, 2020 at 12:20:06PM -0700, Manasi Navare wrote:
> > On Wed, Apr 22, 2020 at 07:19:14PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Make things a bit more abstract by replacing the pch_pfit.pos/size
> > > raw register values with a drm_rect. Makes it slighly more convenient
> > > to eg. compute the scaling factors.
> > > 
> > > v2: Use drm_rect_init()
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c  | 101 +++++++++++-------
> > >  .../drm/i915/display/intel_display_types.h    |   3 +-
> > >  drivers/gpu/drm/i915/display/intel_panel.c    |  13 ++-
> > >  3 files changed, 67 insertions(+), 50 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 96d0768ecf5d..6bb87965801e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -6096,10 +6096,8 @@ static int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state)
> > >  	int width, height;
> > >  
> > >  	if (crtc_state->pch_pfit.enabled) {
> > > -		u32 pfit_size = crtc_state->pch_pfit.size;
> > > -
> > > -		width = pfit_size >> 16;
> > > -		height = pfit_size & 0xffff;
> > > +		width = drm_rect_width(&crtc_state->pch_pfit.dst);
> > > +		height = drm_rect_height(&crtc_state->pch_pfit.dst);
> > >  	} else {
> > >  		width = adjusted_mode->crtc_hdisplay;
> > >  		height = adjusted_mode->crtc_vdisplay;
> > > @@ -6219,11 +6217,20 @@ static void skl_pfit_enable(const 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);
> > > -	enum pipe pipe = crtc->pipe;
> > >  	const struct intel_crtc_scaler_state *scaler_state =
> > >  		&crtc_state->scaler_state;
> > > +	struct drm_rect src = {
> > > +		.x2 = crtc_state->pipe_src_w << 16,
> > > +		.y2 = crtc_state->pipe_src_h << 16,
> > 
> > Its not clear to me why we left shift by 16 for both src_w and src_h? Where can I find the format of
> > how this is stored?
> > 
> > Other than that everything else looks good in terms of replacing with drm_rect()
> > 
> > Manasi
> > 
> <snip>
> > > -	hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
> > > -	vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
> 
> Same <<16 was here already. skl_scaler_calc_phase() wants
> the scaling factor in .16 binary fixed point. Also 
> drm_rect_calc_{h,v}scale() assumes the src coordinates
> to be .16 and returns the result in .16 as well.

Ah okay, thanks for the clarification, is it perhaps a good idea to add this
in the comment somewhere?

In either case though,

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> 
> > > +	hscale = drm_rect_calc_hscale(&src, dst, 0, INT_MAX);
> > > +	vscale = drm_rect_calc_vscale(&src, dst, 0, INT_MAX);
> > >  
> > >  	uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
> > >  	uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx