[Intel-gfx] [PATCH 00/42] drm/i915: Convert to atomic, part 2.

Wed May 13 00:04:05 PDT 2015

On Mon, May 11, 2015 at 4:24 PM, Maarten Lankhorst
<maarten.lankhorst at linux.intel.com> wrote:
> This patch makes this happen by consolidating all modeset paths
> and getting rid of most transitional state.
>
> This happens first by unifying all paths so all code that
> disables a crtc goes through either intel_crtc_toggle or
> __intel_set_mode. After that's done crtc_state->active is
> updated in intel_crtc_toggle, and used to check whether
> the crtc is active or not.
>
> At this point crtc->state is equal to intel_crtc->config and
> crtc->state->active is equal to crtc->config.
>
> This gives us enough information to convert all planes to atomic,
> this had to be done in a single commit because the transitional
> helpers don't call crtc_check and some things have to be moved
> there.
>
> This makes the planes fully atomic, next step is getting rid
> of the transitional intel_crtc->active and intel_crtc->config,
> and replacing it with crtc->state or old_crtc_state.
>
> The last part allows setting multiple crtc's in intel_set_mode,
> and restore the sw state after suspend by calculating the hw
> state in a drm_atomic_state, then swapping it with sw state
> and finally call intel_set_mode with the sw state.

Ok done an inital pass over the entire series. I think there's quite a
bit of polish and detail work to be done still, but overall the patch
series makes senes. For merging I think we should concentrate to get
the first few patches into shape first to get them in sooner (and
avoid too much rebasing). But doesn't have to be, whatever you prefer
wrt branch handling.

A few high-level comments:
- I prefer less terse commit messages. A few months down the road when
git blame or git bisect turns up such a commit it's good to understand
it again without digging out all the others. Which means repetition
(especially in all the patches to remove intel_crtc->config) is
totally ok and often needed. I've also made comments when I think that
some tricky issue should be specifically highlighted.

- Wrt the intel_crtc->config removal I think we should strive to make
it as least invasive as possible. Helps a lot with possible reverts,
conflicts and also makes reviewing a lot easier. I've made comments
where I think changes are unecessary or imo even confusing.

- It's scary and there's lots of little special-cases. Most of my
comments (especially about patch splitting) are to make these special
cases stick out more and so hopefully are easier to review. And also
easier to understand when the inevitable regression points at a patch
in here.

Cheers, Daniel

> Ander Conselvan de Oliveira (6):
>   drm/i915: Set mode_changed for audio in intel_modeset_pipe_config()
>   drm/i915: Make __intel_set_mode() take only atomic state as argument
>   drm/i915: Use global atomic state for staged pll config
>   drm/i915: Support modeset across multiple pipes
>   drm/i915: Move cdclk and pll setup to intel_modeset_compute_config()
>   drm/i915: Read hw state into an atomic state struct
>
> Maarten Lankhorst (36):
>   drm/atomic: Allow drivers to subclass drm_atomic_state
>   drm/i915: get rid of intel_crtc_disable and related code, v2
>   drm/i915: Only update required power domains.
>   drm/i915: use intel_crtc_control everywhere
>   drm/i915: Get rid of new_encoder.
>   drm/i915: get rid of new_crtc
>   drm/i915: Get rid of crtc->new_enabled, v2.
>   drm/i915: Implement intel_crtc_toggle using atomic state
>   drm/i915: Make intel_modeset_fixup_state similar to the atomic helper.
>   drm/i915: make plane helpers fully atomic
>   drm/i915: Update less state during modeset.
>   drm/i915: move swap_state to the right place
>   drm/i915: Use hwmode for vblanks.
>   drm/i915: Remove usage of crtc->config from i915_debugfs.c
>   drm/i915: Remove use of crtc->config from intel_pm.c
>   drm/i915: Remove use of crtc->config from intel_audio.c
>   drm/i915: remove use of crtc->config from intel_fbc.c
>   drm/i915: remove use of crtc->config from intel_atomic.c and
>     intel_sprite.c
>   drm/i915: Remove use of crtc->config from intel_overlay.c
>   drm/i915: Pass old state to crtc_disable and use it.
>   drm/i915: Pass old state to encoder->(post_)disable.
>   drm/i915: Remove use of crtc->config from intel_fbdev.c
>   drm/i915: Remove use of crtc->config from intel_psr.c
>   drm/i915: Remove use of crtc->config from intel_ddi.c
>   drm/i915: Remove use of crtc->config from intel_dp.c
>   drm/i915: Remove use of crtc->config from intel_dp_mst.c
>   drm/i915: Remove use of crtc->config from intel_dsi.c
>   drm/i915: Remove use of crtc->config in intel_hdmi.c
>   drm/i915: Remove use of crtc->config in intel_sdvo.c
>   drm/i915: Calculate haswell plane workaround.
>   drm/i915: remove crtc->active tracking completely
>   drm/i915: get rid of crtc->config in intel_display.c, part 1
>   drm/i915: get rid of crtc->config in intel_display.c, part 2
>   drm/i915: get rid of crtc->config
>   drm/i915: swap state correctly in intel_atomic_commit
>   drm/i915: return early in __intel_set_mode_setup_plls without modeset
>
>  drivers/gpu/drm/drm_atomic.c              |   91 +-
>  drivers/gpu/drm/i915/i915_debugfs.c       |   50 +-
>  drivers/gpu/drm/i915/i915_drv.h           |    5 +-
>  drivers/gpu/drm/i915/i915_irq.c           |   13 +-
>  drivers/gpu/drm/i915/intel_atomic.c       |   93 +-
>  drivers/gpu/drm/i915/intel_atomic_plane.c |   59 +-
>  drivers/gpu/drm/i915/intel_audio.c        |    2 +-
>  drivers/gpu/drm/i915/intel_crt.c          |   21 +-
>  drivers/gpu/drm/i915/intel_ddi.c          |   93 +-
>  drivers/gpu/drm/i915/intel_display.c      | 2639 +++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_dp.c           |   86 +-
>  drivers/gpu/drm/i915/intel_dp_mst.c       |   16 +-
>  drivers/gpu/drm/i915/intel_drv.h          |   72 +-
>  drivers/gpu/drm/i915/intel_dsi.c          |   25 +-
>  drivers/gpu/drm/i915/intel_dvo.c          |   15 +-
>  drivers/gpu/drm/i915/intel_fbc.c          |    8 +-
>  drivers/gpu/drm/i915/intel_fbdev.c        |   19 +-
>  drivers/gpu/drm/i915/intel_hdmi.c         |   78 +-
>  drivers/gpu/drm/i915/intel_lvds.c         |   13 +-
>  drivers/gpu/drm/i915/intel_overlay.c      |    8 +-
>  drivers/gpu/drm/i915/intel_panel.c        |    3 +-
>  drivers/gpu/drm/i915/intel_pm.c           |   96 +-
>  drivers/gpu/drm/i915/intel_psr.c          |   25 +-
>  drivers/gpu/drm/i915/intel_sdvo.c         |   22 +-
>  drivers/gpu/drm/i915/intel_sprite.c       |   84 +-
>  drivers/gpu/drm/i915/intel_tv.c           |    5 +-
>  include/drm/drm_atomic.h                  |    4 +
>  include/drm/drm_crtc.h                    |    4 +
>  28 files changed, 1908 insertions(+), 1741 deletions(-)
>
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch