On Tue, Dec 27, 2016 at 03:56:54PM +0100, Daniel Vetter wrote:
> On Thu, Dec 22, 2016 at 04:04:41PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > crtc->config is on its way out. Let's reduce our dependence on it a
> > little bit by removing it from intel_modeset_readout_hw_state().
> >
> > Also replace crtc->acttive checks with crtc_state->base.active checks.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Suggested-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
Pushed both patches to dinq. Thanks for the reviews.
>
> Idling musing: I wonder whether we should convert the readout code to use
> an entirely free-standing atomic static tracked in drm_atomic_commit. And
> then apply it with a directly call to swap_state. That way we could reuse
> all the neat iterators.
>
> Not sure that's a good idea though.
Hmm. Maybe.
At some point I was toying with the idea of making the old LVDS/DVO
intel_crtc_mode_get() thing use the normal readout paths, but I was
slightly worried that the lack of the top level atomic state could have
been a problem. I don't think it actually was for the platforms where
that codepath can be taken, but I did have to think about it a bit at
least.
> -Daniel
>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++++++++-------------
> > 1 file changed, 24 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index d8effd4da034..091c192c6c5c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -17032,7 +17032,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > dev_priv->active_crtcs = 0;
> >
> > for_each_intel_crtc(dev, crtc) {
> > - struct intel_crtc_state *crtc_state = crtc->config;
> > + struct intel_crtc_state *crtc_state =
> > + to_intel_crtc_state(crtc->base.state);
> >
> > __drm_atomic_helper_crtc_destroy_state(&crtc_state->base);
> > memset(crtc_state, 0, sizeof(*crtc_state));
> > @@ -17051,7 +17052,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >
> > DRM_DEBUG_KMS("[CRTC:%d:%s] hw state readout: %s\n",
> > crtc->base.base.id, crtc->base.name,
> > - enableddisabled(crtc->active));
> > + enableddisabled(crtc_state->base.active));
> > }
> >
> > for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> > @@ -17061,7 +17062,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > &pll->config.hw_state);
> > pll->config.crtc_mask = 0;
> > for_each_intel_crtc(dev, crtc) {
> > - if (crtc->active && crtc->config->shared_dpll == pll)
> > + struct intel_crtc_state *crtc_state =
> > + to_intel_crtc_state(crtc->base.state);
> > +
> > + if (crtc_state->base.active &&
> > + crtc_state->shared_dpll == pll)
> > pll->config.crtc_mask |= 1 << crtc->pipe;
> > }
> > pll->active_mask = pll->config.crtc_mask;
> > @@ -17074,11 +17079,14 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > pipe = 0;
> >
> > if (encoder->get_hw_state(encoder, &pipe)) {
> > + struct intel_crtc_state *crtc_state;
> > +
> > crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > + crtc_state = to_intel_crtc_state(crtc->base.state);
> >
> > encoder->base.crtc = &crtc->base;
> > - crtc->config->output_types |= 1 << encoder->type;
> > - encoder->get_config(encoder, crtc->config);
> > + crtc_state->output_types |= 1 << encoder->type;
> > + encoder->get_config(encoder, crtc_state);
> > } else {
> > encoder->base.crtc = NULL;
> > }
> > @@ -17119,14 +17127,16 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > }
> >
> > for_each_intel_crtc(dev, crtc) {
> > + struct intel_crtc_state *crtc_state =
> > + to_intel_crtc_state(crtc->base.state);
> > int pixclk = 0;
> >
> > - crtc->base.hwmode = crtc->config->base.adjusted_mode;
> > + crtc->base.hwmode = crtc_state->base.adjusted_mode;
> >
> > memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
> > - if (crtc->base.state->active) {
> > - intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
> > - intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
> > + if (crtc_state->base.active) {
> > + intel_mode_from_pipe_config(&crtc->base.mode, crtc_state);
> > + intel_mode_from_pipe_config(&crtc_state->base.adjusted_mode, crtc_state);
> > WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
> >
> > /*
> > @@ -17146,17 +17156,17 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > * mode change, which means it's safe to do a full
> > * recalculation.
> > */
> > - crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
> > + crtc_state->base.mode.private_flags = I915_MODE_FLAG_INHERITED;
> >
> > if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > - pixclk = ilk_pipe_pixel_rate(crtc->config);
> > + pixclk = ilk_pipe_pixel_rate(crtc_state);
> > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > - pixclk = crtc->config->base.adjusted_mode.crtc_clock;
> > + pixclk = crtc_state->base.adjusted_mode.crtc_clock;
> > else
> > WARN_ON(dev_priv->display.modeset_calc_cdclk);
> >
> > /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> > - if (IS_BROADWELL(dev_priv) && crtc->config->ips_enabled)
> > + if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> > pixclk = DIV_ROUND_UP(pixclk * 100, 95);
> >
> > drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
> > @@ -17165,7 +17175,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >
> > dev_priv->min_pixclk[crtc->pipe] = pixclk;
> >
> > - intel_pipe_config_sanity_check(dev_priv, crtc->config);
> > + intel_pipe_config_sanity_check(dev_priv, crtc_state);
> > }
> > }
> >
> > --
> > 2.10.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
