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@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
