| Message ID | 20181029181820.21956-1-ville.syrjala@linux.intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
"drm/i915: Account for scale factor when calculating initial phase"
( rev:
2
)
in
Intel GFX |
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fe045abb6472..33dd2e9751e6 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4786,8 +4786,31 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe) * chroma samples for both of the luma samples, and thus we don't * actually get the expected MPEG2 chroma siting convention :( * The same behaviour is observed on pre-SKL platforms as well. + * + * Theory behind the formula (note that we ignore sub-pixel + * source coordinates): + * s = source sample position + * d = destination sample position + * + * Downscaling 4:1: + * -0.5 + * | 0.0 + * | | 1.5 (initial phase) + * | | | + * v v v + * | s | s | s | s | + * | d | + * + * Upscaling 1:4: + * -0.5 + * | -0.375 (initial phase) + * | | 0.0 + * | | | + * v v v + * | s | + * | d | d | d | d | */ -u16 skl_scaler_calc_phase(int sub, bool chroma_cosited) +u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_cosited) { int phase = -0x8000; u16 trip = 0; @@ -4795,6 +4818,15 @@ u16 skl_scaler_calc_phase(int sub, bool chroma_cosited) if (chroma_cosited) phase += (sub - 1) * 0x8000 / sub; + phase += scale / (2 * sub); + + /* + * Hardware initial phase limited to [-0.5:1.5]. + * Since the max hardware scale factor is 3.0, we + * should never actually excdeed 1.0 here. + */ + WARN_ON(phase < -0x8000 || phase > 0x18000); + if (phase < 0) phase = 0x10000 + phase; else @@ -5003,13 +5035,20 @@ static void skylake_pfit_enable(const struct intel_crtc_state *crtc_state) if (crtc_state->pch_pfit.enabled) { u16 uv_rgb_hphase, uv_rgb_vphase; + int pfit_w, pfit_h, hscale, vscale; int id; if (WARN_ON(crtc_state->scaler_state.scaler_id < 0)) return; - uv_rgb_hphase = skl_scaler_calc_phase(1, false); - uv_rgb_vphase = skl_scaler_calc_phase(1, false); + pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF; + pfit_h = crtc_state->pch_pfit.size & 0xFFFF; + + hscale = (crtc_state->pipe_src_w << 16) / pfit_w; + vscale = (crtc_state->pipe_src_h << 16) / pfit_h; + + uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false); + uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); id = scaler_state->scaler_id; I915_WRITE(SKL_PS_CTRL(pipe, id), PS_SCALER_EN | diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index db24308729b4..86d551a331b1 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1709,7 +1709,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); -u16 skl_scaler_calc_phase(int sub, bool chroma_center); +u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_center); int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); int skl_max_scale(const struct intel_crtc_state *crtc_state, u32 pixel_format); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index cfaddc05fea6..fbb916506c77 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -326,27 +326,35 @@ skl_program_scaler(struct drm_i915_private *dev_priv, uint32_t crtc_h = drm_rect_height(&plane_state->base.dst); u16 y_hphase, uv_rgb_hphase; u16 y_vphase, uv_rgb_vphase; + int hscale, vscale; /* Sizes are 0 based */ crtc_w--; crtc_h--; + hscale = drm_rect_calc_hscale(&plane_state->base.src, + &plane_state->base.dst, + 0, INT_MAX); + vscale = drm_rect_calc_vscale(&plane_state->base.src, + &plane_state->base.dst, + 0, INT_MAX); + /* TODO: handle sub-pixel coordinates */ if (plane_state->base.fb->format->format == DRM_FORMAT_NV12 && !icl_is_hdr_plane(plane)) { - y_hphase = skl_scaler_calc_phase(1, false); - y_vphase = skl_scaler_calc_phase(1, false); + y_hphase = skl_scaler_calc_phase(1, hscale, false); + y_vphase = skl_scaler_calc_phase(1, vscale, false); /* MPEG2 chroma siting convention */ - uv_rgb_hphase = skl_scaler_calc_phase(2, true); - uv_rgb_vphase = skl_scaler_calc_phase(2, false); + uv_rgb_hphase = skl_scaler_calc_phase(2, hscale, true); + uv_rgb_vphase = skl_scaler_calc_phase(2, vscale, false); } else { /* not used */ y_hphase = 0; y_vphase = 0; - uv_rgb_hphase = skl_scaler_calc_phase(1, false); - uv_rgb_vphase = skl_scaler_calc_phase(1, false); + uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false); + uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); } I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
This seems to fix some DRM_FORMAT_RGB565 (up-)scaling IGT tests on on my KBL. Tested-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> On 29.10.2018 20:18, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > To get the initial phase correct we need to account for the scale > factor as well. I forgot this initially and was mostly looking at > heavily upscaled content where the minor difference between -0.5 > and the proper initial phase was not readily apparent. > > And let's toss in a comment that tries to explain the formula > a little bit. > > v2: The initial phase upper limit is 1.5, not 24.0! > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Fixes: 0a59952b24e2 ("drm/i915: Configure SKL+ scaler initial phase correctly") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 45 ++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > drivers/gpu/drm/i915/intel_sprite.c | 20 +++++++++---- > 3 files changed, 57 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index fe045abb6472..33dd2e9751e6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4786,8 +4786,31 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe) > * chroma samples for both of the luma samples, and thus we don't > * actually get the expected MPEG2 chroma siting convention :( > * The same behaviour is observed on pre-SKL platforms as well. > + * > + * Theory behind the formula (note that we ignore sub-pixel > + * source coordinates): > + * s = source sample position > + * d = destination sample position > + * > + * Downscaling 4:1: > + * -0.5 > + * | 0.0 > + * | | 1.5 (initial phase) > + * | | | > + * v v v > + * | s | s | s | s | > + * | d | > + * > + * Upscaling 1:4: > + * -0.5 > + * | -0.375 (initial phase) > + * | | 0.0 > + * | | | > + * v v v > + * | s | > + * | d | d | d | d | > */ > -u16 skl_scaler_calc_phase(int sub, bool chroma_cosited) > +u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_cosited) > { > int phase = -0x8000; > u16 trip = 0; > @@ -4795,6 +4818,15 @@ u16 skl_scaler_calc_phase(int sub, bool chroma_cosited) > if (chroma_cosited) > phase += (sub - 1) * 0x8000 / sub; > > + phase += scale / (2 * sub); > + > + /* > + * Hardware initial phase limited to [-0.5:1.5]. > + * Since the max hardware scale factor is 3.0, we > + * should never actually excdeed 1.0 here. > + */ > + WARN_ON(phase < -0x8000 || phase > 0x18000); > + > if (phase < 0) > phase = 0x10000 + phase; > else > @@ -5003,13 +5035,20 @@ static void skylake_pfit_enable(const struct intel_crtc_state *crtc_state) > > if (crtc_state->pch_pfit.enabled) { > u16 uv_rgb_hphase, uv_rgb_vphase; > + int pfit_w, pfit_h, hscale, vscale; > int id; > > if (WARN_ON(crtc_state->scaler_state.scaler_id < 0)) > return; > > - uv_rgb_hphase = skl_scaler_calc_phase(1, false); > - uv_rgb_vphase = skl_scaler_calc_phase(1, false); > + pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF; > + pfit_h = crtc_state->pch_pfit.size & 0xFFFF; > + > + hscale = (crtc_state->pipe_src_w << 16) / pfit_w; > + vscale = (crtc_state->pipe_src_h << 16) / pfit_h; > + > + uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false); > + uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); > > id = scaler_state->scaler_id; > I915_WRITE(SKL_PS_CTRL(pipe, id), PS_SCALER_EN | > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index db24308729b4..86d551a331b1 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1709,7 +1709,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, > void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, > struct intel_crtc_state *crtc_state); > > -u16 skl_scaler_calc_phase(int sub, bool chroma_center); > +u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_center); > int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); > int skl_max_scale(const struct intel_crtc_state *crtc_state, > u32 pixel_format); > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index cfaddc05fea6..fbb916506c77 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -326,27 +326,35 @@ skl_program_scaler(struct drm_i915_private *dev_priv, > uint32_t crtc_h = drm_rect_height(&plane_state->base.dst); > u16 y_hphase, uv_rgb_hphase; > u16 y_vphase, uv_rgb_vphase; > + int hscale, vscale; > > /* Sizes are 0 based */ > crtc_w--; > crtc_h--; > > + hscale = drm_rect_calc_hscale(&plane_state->base.src, > + &plane_state->base.dst, > + 0, INT_MAX); > + vscale = drm_rect_calc_vscale(&plane_state->base.src, > + &plane_state->base.dst, > + 0, INT_MAX); > + > /* TODO: handle sub-pixel coordinates */ > if (plane_state->base.fb->format->format == DRM_FORMAT_NV12 && > !icl_is_hdr_plane(plane)) { > - y_hphase = skl_scaler_calc_phase(1, false); > - y_vphase = skl_scaler_calc_phase(1, false); > + y_hphase = skl_scaler_calc_phase(1, hscale, false); > + y_vphase = skl_scaler_calc_phase(1, vscale, false); > > /* MPEG2 chroma siting convention */ > - uv_rgb_hphase = skl_scaler_calc_phase(2, true); > - uv_rgb_vphase = skl_scaler_calc_phase(2, false); > + uv_rgb_hphase = skl_scaler_calc_phase(2, hscale, true); > + uv_rgb_vphase = skl_scaler_calc_phase(2, vscale, false); > } else { > /* not used */ > y_hphase = 0; > y_vphase = 0; > > - uv_rgb_hphase = skl_scaler_calc_phase(1, false); > - uv_rgb_vphase = skl_scaler_calc_phase(1, false); > + uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false); > + uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); > } > > I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id), >
On Fri, Nov 02, 2018 at 11:47:13AM +0200, Juha-Pekka Heikkila wrote: > This seems to fix some DRM_FORMAT_RGB565 (up-)scaling IGT tests on on my > KBL. > > Tested-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> Pushed with Maarten's irc r-b and t-b. Thanks for the review and testing. > > On 29.10.2018 20:18, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > To get the initial phase correct we need to account for the scale > > factor as well. I forgot this initially and was mostly looking at > > heavily upscaled content where the minor difference between -0.5 > > and the proper initial phase was not readily apparent. > > > > And let's toss in a comment that tries to explain the formula > > a little bit. > > > > v2: The initial phase upper limit is 1.5, not 24.0! > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Fixes: 0a59952b24e2 ("drm/i915: Configure SKL+ scaler initial phase correctly") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 45 ++++++++++++++++++++++++++-- > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_sprite.c | 20 +++++++++---- > > 3 files changed, 57 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index fe045abb6472..33dd2e9751e6 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4786,8 +4786,31 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe) > > * chroma samples for both of the luma samples, and thus we don't > > * actually get the expected MPEG2 chroma siting convention :( > > * The same behaviour is observed on pre-SKL platforms as well. > > + * > > + * Theory behind the formula (note that we ignore sub-pixel > > + * source coordinates): > > + * s = source sample position > > + * d = destination sample position > > + * > > + * Downscaling 4:1: > > + * -0.5 > > + * | 0.0 > > + * | | 1.5 (initial phase) > > + * | | | > > + * v v v > > + * | s | s | s | s | > > + * | d | > > + * > > + * Upscaling 1:4: > > + * -0.5 > > + * | -0.375 (initial phase) > > + * | | 0.0 > > + * | | | > > + * v v v > > + * | s | > > + * | d | d | d | d | > > */ > > -u16 skl_scaler_calc_phase(int sub, bool chroma_cosited) > > +u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_cosited) > > { > > int phase = -0x8000; > > u16 trip = 0; > > @@ -4795,6 +4818,15 @@ u16 skl_scaler_calc_phase(int sub, bool chroma_cosited) > > if (chroma_cosited) > > phase += (sub - 1) * 0x8000 / sub; > > > > + phase += scale / (2 * sub); > > + > > + /* > > + * Hardware initial phase limited to [-0.5:1.5]. > > + * Since the max hardware scale factor is 3.0, we > > + * should never actually excdeed 1.0 here. > > + */ > > + WARN_ON(phase < -0x8000 || phase > 0x18000); > > + > > if (phase < 0) > > phase = 0x10000 + phase; > > else > > @@ -5003,13 +5035,20 @@ static void skylake_pfit_enable(const struct intel_crtc_state *crtc_state) > > > > if (crtc_state->pch_pfit.enabled) { > > u16 uv_rgb_hphase, uv_rgb_vphase; > > + int pfit_w, pfit_h, hscale, vscale; > > int id; > > > > if (WARN_ON(crtc_state->scaler_state.scaler_id < 0)) > > return; > > > > - uv_rgb_hphase = skl_scaler_calc_phase(1, false); > > - uv_rgb_vphase = skl_scaler_calc_phase(1, false); > > + pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF; > > + pfit_h = crtc_state->pch_pfit.size & 0xFFFF; > > + > > + hscale = (crtc_state->pipe_src_w << 16) / pfit_w; > > + vscale = (crtc_state->pipe_src_h << 16) / pfit_h; > > + > > + uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false); > > + uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); > > > > id = scaler_state->scaler_id; > > I915_WRITE(SKL_PS_CTRL(pipe, id), PS_SCALER_EN | > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index db24308729b4..86d551a331b1 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1709,7 +1709,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, > > void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, > > struct intel_crtc_state *crtc_state); > > > > -u16 skl_scaler_calc_phase(int sub, bool chroma_center); > > +u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_center); > > int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); > > int skl_max_scale(const struct intel_crtc_state *crtc_state, > > u32 pixel_format); > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index cfaddc05fea6..fbb916506c77 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -326,27 +326,35 @@ skl_program_scaler(struct drm_i915_private *dev_priv, > > uint32_t crtc_h = drm_rect_height(&plane_state->base.dst); > > u16 y_hphase, uv_rgb_hphase; > > u16 y_vphase, uv_rgb_vphase; > > + int hscale, vscale; > > > > /* Sizes are 0 based */ > > crtc_w--; > > crtc_h--; > > > > + hscale = drm_rect_calc_hscale(&plane_state->base.src, > > + &plane_state->base.dst, > > + 0, INT_MAX); > > + vscale = drm_rect_calc_vscale(&plane_state->base.src, > > + &plane_state->base.dst, > > + 0, INT_MAX); > > + > > /* TODO: handle sub-pixel coordinates */ > > if (plane_state->base.fb->format->format == DRM_FORMAT_NV12 && > > !icl_is_hdr_plane(plane)) { > > - y_hphase = skl_scaler_calc_phase(1, false); > > - y_vphase = skl_scaler_calc_phase(1, false); > > + y_hphase = skl_scaler_calc_phase(1, hscale, false); > > + y_vphase = skl_scaler_calc_phase(1, vscale, false); > > > > /* MPEG2 chroma siting convention */ > > - uv_rgb_hphase = skl_scaler_calc_phase(2, true); > > - uv_rgb_vphase = skl_scaler_calc_phase(2, false); > > + uv_rgb_hphase = skl_scaler_calc_phase(2, hscale, true); > > + uv_rgb_vphase = skl_scaler_calc_phase(2, vscale, false); > > } else { > > /* not used */ > > y_hphase = 0; > > y_vphase = 0; > > > > - uv_rgb_hphase = skl_scaler_calc_phase(1, false); > > - uv_rgb_vphase = skl_scaler_calc_phase(1, false); > > + uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false); > > + uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false); > > } > > > > I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id), > >