| Message ID | 20161219172840.31842-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2016-12-19 at 19:28 +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Rather than recomptuing the pipe pixel rate on demand everwhere, let's > just stick the precomputed value into the crtc state. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 31 ++++++++++++++++++++++++++----- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_fbc.c | 3 +-- > drivers/gpu/drm/i915/intel_pm.c | 14 +++++++------- > 4 files changed, 36 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0b0d7e8be630..1d979041c52c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > [...] > @@ -16919,7 +16938,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) __drm_atomic_helper_crtc_destroy_state(&crtc_state->base); memset(crtc_state, 0, sizeof(*crtc_state)); crtc_state->base.crtc = &crtc->base; crtc_state->base.active = crtc_state->base.enable = dev_priv->display.get_pipe_config(crtc, crtc_state); crtc->base.enabled = crtc_state->base.enable; crtc->active = crtc_state->base.active; if (crtc_state->base.active) { > dev_priv->active_crtcs |= 1 << crtc->pipe; > > if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > - pixclk = ilk_pipe_pixel_rate(crtc_state); > + pixclk = crtc_state->pixel_rate; Aren't you reading 0 here, because of the memset above? As far as I can tell, haswell_get_pipe_config() doesn't set crtc_state->pixel_rate. > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > pixclk = crtc_state->base.adjusted_mode.crtc_clock; > else > @@ -17031,6 +17050,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > */ > crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED; > > + intel_crtc_compute_pixel_rate(crtc->config); > + > drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode); > update_scanline_offset(crtc); > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index f61ea43c7532..3969e786d566 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -541,6 +541,8 @@ struct intel_crtc_state { > * and get clipped at the edges. */ > int pipe_src_w, pipe_src_h; > > + unsigned int pixel_rate; > + Maybe add some comment about this parameter. This is not in kernel doc, but having that already would probably make it easier for whoever does it in the end. Ander
On Tue, Dec 20, 2016 at 03:10:53PM +0200, Ander Conselvan De Oliveira wrote: > On Mon, 2016-12-19 at 19:28 +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Rather than recomptuing the pipe pixel rate on demand everwhere, let's > > just stick the precomputed value into the crtc state. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 31 ++++++++++++++++++++++++++----- > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_fbc.c | 3 +-- > > drivers/gpu/drm/i915/intel_pm.c | 14 +++++++------- > > 4 files changed, 36 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 0b0d7e8be630..1d979041c52c 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > [...] > > > @@ -16919,7 +16938,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > __drm_atomic_helper_crtc_destroy_state(&crtc_state->base); > memset(crtc_state, 0, sizeof(*crtc_state)); > crtc_state->base.crtc = &crtc->base; > > crtc_state->base.active = crtc_state->base.enable = > dev_priv->display.get_pipe_config(crtc, crtc_state); > > crtc->base.enabled = crtc_state->base.enable; > crtc->active = crtc_state->base.active; > > if (crtc_state->base.active) { > > dev_priv->active_crtcs |= 1 << crtc->pipe; > > > > if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > > - pixclk = ilk_pipe_pixel_rate(crtc_state); > > + pixclk = crtc_state->pixel_rate; > > Aren't you reading 0 here, because of the memset above? As far as I can tell, > haswell_get_pipe_config() doesn't set crtc_state->pixel_rate. Hmm, yeah. Which means this whole piece of min_pixclk[] code is in the wrong place. You can't know the pixel rate until you know the clock, and you don't know that until you've done the full readout (meaning the encoder .get_config() hooks have been called as well). > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > pixclk = crtc_state->base.adjusted_mode.crtc_clock; > > else > > @@ -17031,6 +17050,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > */ > > crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED; > > > > + intel_crtc_compute_pixel_rate(crtc->config); > > + > > drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode); > > update_scanline_offset(crtc); > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index f61ea43c7532..3969e786d566 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -541,6 +541,8 @@ struct intel_crtc_state { > > * and get clipped at the edges. */ > > int pipe_src_w, pipe_src_h; > > > > + unsigned int pixel_rate; > > + > > Maybe add some comment about this parameter. This is not in kernel doc, but > having that already would probably make it easier for whoever does it in the > end. > > Ander
On Tue, Dec 20, 2016 at 03:29:54PM +0200, Ville Syrjälä wrote: > On Tue, Dec 20, 2016 at 03:10:53PM +0200, Ander Conselvan De Oliveira wrote: > > On Mon, 2016-12-19 at 19:28 +0200, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Rather than recomptuing the pipe pixel rate on demand everwhere, let's > > > just stick the precomputed value into the crtc state. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 31 ++++++++++++++++++++++++++----- > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > drivers/gpu/drm/i915/intel_fbc.c | 3 +-- > > > drivers/gpu/drm/i915/intel_pm.c | 14 +++++++------- > > > 4 files changed, 36 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 0b0d7e8be630..1d979041c52c 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > > > [...] > > > > > @@ -16919,7 +16938,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > __drm_atomic_helper_crtc_destroy_state(&crtc_state->base); > > memset(crtc_state, 0, sizeof(*crtc_state)); > > crtc_state->base.crtc = &crtc->base; > > > > crtc_state->base.active = crtc_state->base.enable = > > dev_priv->display.get_pipe_config(crtc, crtc_state); > > > > crtc->base.enabled = crtc_state->base.enable; > > crtc->active = crtc_state->base.active; > > > > if (crtc_state->base.active) { > > > dev_priv->active_crtcs |= 1 << crtc->pipe; > > > > > > if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > > > - pixclk = ilk_pipe_pixel_rate(crtc_state); > > > + pixclk = crtc_state->pixel_rate; > > > > Aren't you reading 0 here, because of the memset above? As far as I can tell, > > haswell_get_pipe_config() doesn't set crtc_state->pixel_rate. > > Hmm, yeah. Which means this whole piece of min_pixclk[] code is in > the wrong place. You can't know the pixel rate until you know the > clock, and you don't know that until you've done the full readout > (meaning the encoder .get_config() hooks have been called as well). Apparently this is the only piece blocking this already reviewed series, right? So, what are the plans? drop this patch and move with the other patches? > > > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > pixclk = crtc_state->base.adjusted_mode.crtc_clock; > > > else > > > @@ -17031,6 +17050,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > > */ > > > crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED; > > > > > > + intel_crtc_compute_pixel_rate(crtc->config); > > > + > > > drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode); > > > update_scanline_offset(crtc); > > > } > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > index f61ea43c7532..3969e786d566 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -541,6 +541,8 @@ struct intel_crtc_state { > > > * and get clipped at the edges. */ > > > int pipe_src_w, pipe_src_h; > > > > > > + unsigned int pixel_rate; > > > + > > > > Maybe add some comment about this parameter. This is not in kernel doc, but > > having that already would probably make it easier for whoever does it in the > > end. > > > > Ander > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jan 12, 2017 at 12:37:46PM -0800, Rodrigo Vivi wrote: > On Tue, Dec 20, 2016 at 03:29:54PM +0200, Ville Syrjälä wrote: > > On Tue, Dec 20, 2016 at 03:10:53PM +0200, Ander Conselvan De Oliveira wrote: > > > On Mon, 2016-12-19 at 19:28 +0200, ville.syrjala@linux.intel.com wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Rather than recomptuing the pipe pixel rate on demand everwhere, let's > > > > just stick the precomputed value into the crtc state. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 31 ++++++++++++++++++++++++++----- > > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > > drivers/gpu/drm/i915/intel_fbc.c | 3 +-- > > > > drivers/gpu/drm/i915/intel_pm.c | 14 +++++++------- > > > > 4 files changed, 36 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index 0b0d7e8be630..1d979041c52c 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > > > > > > [...] > > > > > > > @@ -16919,7 +16938,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > > __drm_atomic_helper_crtc_destroy_state(&crtc_state->base); > > > memset(crtc_state, 0, sizeof(*crtc_state)); > > > crtc_state->base.crtc = &crtc->base; > > > > > > crtc_state->base.active = crtc_state->base.enable = > > > dev_priv->display.get_pipe_config(crtc, crtc_state); > > > > > > crtc->base.enabled = crtc_state->base.enable; > > > crtc->active = crtc_state->base.active; > > > > > > if (crtc_state->base.active) { > > > > dev_priv->active_crtcs |= 1 << crtc->pipe; > > > > > > > > if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) > > > > - pixclk = ilk_pipe_pixel_rate(crtc_state); > > > > + pixclk = crtc_state->pixel_rate; > > > > > > Aren't you reading 0 here, because of the memset above? As far as I can tell, > > > haswell_get_pipe_config() doesn't set crtc_state->pixel_rate. > > > > Hmm, yeah. Which means this whole piece of min_pixclk[] code is in > > the wrong place. You can't know the pixel rate until you know the > > clock, and you don't know that until you've done the full readout > > (meaning the encoder .get_config() hooks have been called as well). > > Apparently this is the only piece blocking this already reviewed series, right? Not sure. I'll have to go over it again to see what was said. At least adding some docs for intel_cdclk.c was requested. > > So, what are the plans? drop this patch and move with the other patches? I think I pushed the fix to move the min_pixclk[] handling to happen a little later, so all I should have to do is rebase this patch and it should all be good. But as usual I have a few too many irons in the fire, so I haven't managed to get back to this series yet.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0b0d7e8be630..1d979041c52c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7178,7 +7178,7 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv, * * Should measure whether using a lower cdclk w/o IPS */ - return ilk_pipe_pixel_rate(pipe_config) <= + return pipe_config->pixel_rate <= dev_priv->max_cdclk_freq * 95 / 100; } @@ -7202,6 +7202,19 @@ static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc) (crtc->pipe == PIPE_A || IS_I915G(dev_priv)); } +static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); + + if (HAS_GMCH_DISPLAY(dev_priv)) + /* FIXME calculate proper pipe pixel rate GMCH pfit */ + crtc_state->pixel_rate = + crtc_state->base.adjusted_mode.crtc_clock; + else + crtc_state->pixel_rate = + ilk_pipe_pixel_rate(crtc_state); +} + static int intel_crtc_compute_config(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { @@ -7248,6 +7261,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, adjusted_mode->crtc_hsync_start == adjusted_mode->crtc_hdisplay) return -EINVAL; + intel_crtc_compute_pixel_rate(pipe_config); + if (HAS_IPS(dev_priv)) hsw_compute_ips_config(crtc, pipe_config); @@ -10297,7 +10312,7 @@ static int ilk_max_pixel_rate(struct drm_atomic_state *state) continue; } - pixel_rate = ilk_pipe_pixel_rate(crtc_state); + pixel_rate = crtc_state->pixel_rate; if (IS_BROADWELL(dev_priv) || IS_GEN9(dev_priv)) pixel_rate = bdw_adjust_min_pipe_pixel_rate(crtc_state, @@ -12802,9 +12817,10 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, DRM_DEBUG_KMS("adjusted mode:\n"); drm_mode_debug_printmodeline(&pipe_config->base.adjusted_mode); intel_dump_crtc_timings(&pipe_config->base.adjusted_mode); - DRM_DEBUG_KMS("port clock: %d, pipe src size: %dx%d\n", + DRM_DEBUG_KMS("port clock: %d, pipe src size: %dx%d, pixel rate %d\n", pipe_config->port_clock, - pipe_config->pipe_src_w, pipe_config->pipe_src_h); + pipe_config->pipe_src_w, pipe_config->pipe_src_h, + pipe_config->pixel_rate); if (INTEL_GEN(dev_priv) >= 9) DRM_DEBUG_KMS("num_scalers: %d, scaler_users: 0x%x, scaler_id: %d\n", @@ -13409,6 +13425,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, } PIPE_CONF_CHECK_I(scaler_state.scaler_id); + PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate); } /* BDW+ don't expose a synchronous way to read the state */ @@ -13700,6 +13717,8 @@ verify_crtc_state(struct drm_crtc *crtc, } } + intel_crtc_compute_pixel_rate(pipe_config); + if (!new_crtc_state->active) return; @@ -16919,7 +16938,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) dev_priv->active_crtcs |= 1 << crtc->pipe; if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) - pixclk = ilk_pipe_pixel_rate(crtc_state); + pixclk = crtc_state->pixel_rate; else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) pixclk = crtc_state->base.adjusted_mode.crtc_clock; else @@ -17031,6 +17050,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) */ crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED; + intel_crtc_compute_pixel_rate(crtc->config); + drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode); update_scanline_offset(crtc); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index f61ea43c7532..3969e786d566 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -541,6 +541,8 @@ struct intel_crtc_state { * and get clipped at the edges. */ int pipe_src_w, pipe_src_h; + unsigned int pixel_rate; + /* Whether to set up the PCH/FDI. Note that we never allow sharing * between pch encoders and cpu encoders. */ bool has_pch_encoder; diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 659cebc3bfd2..463056d80f9b 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -747,8 +747,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags; if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) - cache->crtc.hsw_bdw_pixel_rate = - ilk_pipe_pixel_rate(crtc_state); + cache->crtc.hsw_bdw_pixel_rate = crtc_state->pixel_rate; cache->plane.rotation = plane_state->base.rotation; cache->plane.src_w = drm_rect_width(&plane_state->base.src) >> 16; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 7cbd6181a612..79e2be4216e4 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -1807,12 +1807,12 @@ static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate, cpp = pstate->base.fb->format->cpp[0]; - method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value); + method1 = ilk_wm_method1(cstate->pixel_rate, cpp, mem_value); if (!is_lp) return method1; - method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate), + method2 = ilk_wm_method2(cstate->pixel_rate, cstate->base.adjusted_mode.crtc_htotal, drm_rect_width(&pstate->base.dst), cpp, mem_value); @@ -1836,8 +1836,8 @@ static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate, cpp = pstate->base.fb->format->cpp[0]; - method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value); - method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate), + method1 = ilk_wm_method1(cstate->pixel_rate, cpp, mem_value); + method2 = ilk_wm_method2(cstate->pixel_rate, cstate->base.adjusted_mode.crtc_htotal, drm_rect_width(&pstate->base.dst), cpp, mem_value); @@ -1863,7 +1863,7 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate, if (!cstate->base.active) return 0; - return ilk_wm_method2(ilk_pipe_pixel_rate(cstate), + return ilk_wm_method2(cstate->pixel_rate, cstate->base.adjusted_mode.crtc_htotal, width, cpp, mem_value); } @@ -3547,7 +3547,7 @@ static uint32_t skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cst * Adjusted plane pixel rate is just the pipe's adjusted pixel rate * with additional adjustments for plane-specific scaling. */ - adjusted_pixel_rate = ilk_pipe_pixel_rate(cstate); + adjusted_pixel_rate = cstate->pixel_rate; downscale_amount = skl_plane_downscale_amount(pstate); pixel_rate = adjusted_pixel_rate * downscale_amount >> 16; @@ -3775,7 +3775,7 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate) if (!cstate->base.active) return 0; - pixel_rate = ilk_pipe_pixel_rate(cstate); + pixel_rate = cstate->pixel_rate; if (WARN_ON(pixel_rate == 0)) return 0;