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@xxxxxxxxxxxxxxx wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
