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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

> > +	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