[Intel-gfx] [PATCH v2 07/20] drm/i915: Rework primary plane stuff slightly.
Daniel Vetter
daniel at ffwll.ch
Tue Jul 7 04:16:55 PDT 2015
On Tue, Jul 07, 2015 at 09:08:18AM +0200, Maarten Lankhorst wrote:
> Make sure the primary plane is set up correctly. This is done by
> setting plane_state->src and plane_state->crtc.
>
> All non-primary planes get disabled.
Too terse commit message, fails to mention that this is about hw
readout completely. Also should mention that this removes the
initial_planes hack.
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_atomic.c | 7 --
> drivers/gpu/drm/i915/intel_display.c | 167 +++++++++++++----------------------
> drivers/gpu/drm/i915/intel_drv.h | 2 -
> 3 files changed, 60 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 429677a111d5..b593612a917d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev,
> return -EINVAL;
> }
>
> - if (crtc_state &&
> - crtc_state->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> - ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
> - if (ret)
> - return ret;
> - }
> -
> ret = drm_atomic_helper_check_planes(dev, state);
> if (ret)
> return ret;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index eb7c2e2819b7..fa1102392eca 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -109,6 +109,7 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
> struct intel_crtc_state *crtc_state);
> static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
> int num_connectors);
> +static void intel_pre_disable_primary(struct drm_crtc *crtc);
>
> static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
> {
> @@ -2582,11 +2583,12 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> {
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_crtc *c;
> - struct intel_crtc *i;
> struct drm_i915_gem_object *obj;
> - struct drm_plane *primary = intel_crtc->base.primary;
> struct drm_framebuffer *fb;
> + struct drm_plane *primary = intel_crtc->base.primary;
> + struct intel_plane *p;
> + struct intel_plane_state *plane_state =
> + to_intel_plane_state(primary->state);
>
> if (!plane_config->fb)
> return;
> @@ -2602,16 +2604,11 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> * Failed to alloc the obj, check to see if we should share
> * an fb with another CRTC instead
> */
> - for_each_crtc(dev, c) {
> - i = to_intel_crtc(c);
> -
> - if (c == &intel_crtc->base)
> - continue;
> -
> - if (!i->active)
> + for_each_intel_plane(dev, p) {
> + if (p->base.type != DRM_PLANE_TYPE_PRIMARY)
> continue;
>
> - fb = c->primary->fb;
> + fb = p->base.state->fb;
This seems to break the sharing logic completely: We want to check primary
planes of all other crtcs to see whether we could merged the fb together.
We don't even read out plane state for non-primary planes, so the below fb
check will never be non-NULL.
> if (!fb)
> continue;
>
> @@ -2622,18 +2619,28 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> }
> }
>
> + intel_pre_disable_primary(&intel_crtc->base);
> + to_intel_plane(primary)->disable_plane(primary, &intel_crtc->base);
> +
> return;
>
> valid_fb:
> + drm_framebuffer_reference(fb);
> + primary->fb = plane_state->base.fb = fb;
> + plane_state->base.crtc = primary->crtc = &intel_crtc->base;
> +
> + plane_state->base.src_x = plane_state->base.src_y = 0;
> + plane_state->base.src_w = fb->width << 16;
> + plane_state->base.src_h = fb->height << 16;
> +
> + plane_state->base.crtc_x = plane_state->base.src_y = 0;
> + plane_state->base.crtc_w = fb->width;
> + plane_state->base.crtc_h = fb->height;
> +
> + plane_state->visible = true;
> obj = intel_fb_obj(fb);
> if (obj->tiling_mode != I915_TILING_NONE)
> dev_priv->preserve_bios_swizzle = true;
> -
> - primary->fb = fb;
> - primary->crtc = primary->state->crtc = &intel_crtc->base;
> - update_state_fb(primary);
Do we still have other users of update_state_fb left?
> - intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
> - obj->frontbuffer_bits |= to_intel_plane(primary)->frontbuffer_bit;
> }
>
> static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> @@ -11761,34 +11768,6 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
> return true;
> }
>
> -static void intel_crtc_check_initial_planes(struct drm_crtc *crtc,
> - struct drm_crtc_state *crtc_state)
> -{
> - struct intel_crtc_state *pipe_config =
> - to_intel_crtc_state(crtc_state);
> - struct drm_plane *p;
> - unsigned visible_mask = 0;
> -
> - drm_for_each_plane_mask(p, crtc->dev, crtc_state->plane_mask) {
> - struct drm_plane_state *plane_state =
> - drm_atomic_get_existing_plane_state(crtc_state->state, p);
> -
> - if (WARN_ON(!plane_state))
> - continue;
> -
> - if (!plane_state->fb)
> - crtc_state->plane_mask &=
> - ~(1 << drm_plane_index(p));
> - else if (to_intel_plane_state(plane_state)->visible)
> - visible_mask |= 1 << drm_plane_index(p);
> - }
> -
> - if (!visible_mask)
> - return;
> -
> - pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
> -}
> -
> static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> struct drm_crtc_state *crtc_state)
> {
> @@ -11810,10 +11789,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> "[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
> idx, crtc->state->active, intel_crtc->active);
>
> - /* plane mask is fixed up after all initial planes are calculated */
> - if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
> - intel_crtc_check_initial_planes(crtc, crtc_state);
> -
> if (mode_changed && !crtc_state->active)
> intel_crtc->atomic.update_wm_post = true;
>
> @@ -13155,19 +13130,6 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
> continue;
> }
>
> - if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> - ret = drm_atomic_add_affected_planes(state, crtc);
> - if (ret)
> - return ret;
> -
> - /*
> - * We ought to handle i915.fastboot here.
> - * If no modeset is required and the primary plane has
> - * a fb, update the members of crtc_state as needed,
> - * and run the necessary updates during vblank evasion.
> - */
> - }
> -
> if (!needs_modeset(crtc_state)) {
> if (!(pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE))
> continue;
> @@ -15149,25 +15111,30 @@ void intel_modeset_init(struct drm_device *dev)
> drm_modeset_unlock_all(dev);
>
> for_each_intel_crtc(dev, crtc) {
> - if (!crtc->active)
> + struct intel_initial_plane_config plane_config;
> + struct drm_plane *plane;
> +
> + if (!crtc->base.state->active)
> continue;
>
> + /* disable all non-primary planes */
> + drm_for_each_plane_mask(plane, dev,
> + crtc->base.state->plane_mask)
> + if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> + to_intel_plane(plane)->disable_plane(plane, &crtc->base);
> +
> + crtc->base.state->plane_mask &=
> + 1 << drm_plane_index(crtc->base.primary);
> +
> + plane_config.fb = NULL;
> + dev_priv->display.get_initial_plane_config(crtc,
> + &plane_config);
> +
> /*
> - * Note that reserving the BIOS fb up front prevents us
> - * from stuffing other stolen allocations like the ring
> - * on top. This prevents some ugliness at boot time, and
> - * can even allow for smooth boot transitions if the BIOS
> - * fb is large enough for the active pipe configuration.
> + * If the fb is shared between multiple heads, we'll
> + * just get the first one.
> */
> - if (dev_priv->display.get_initial_plane_config) {
> - dev_priv->display.get_initial_plane_config(crtc,
> - &crtc->plane_config);
> - /*
> - * If the fb is shared between multiple heads, we'll
> - * just get the first one.
> - */
> - intel_find_initial_plane_obj(crtc, &crtc->plane_config);
> - }
> + intel_find_initial_plane_obj(crtc, &plane_config);
> }
> }
Ok I looked at this and the readout_plane_state function and I think we
have a bit a confusion about responsibilities here. The big thing is that
only driver load cares about reconstructing plane state accurately, for
resume and lid notifier we just want to make sure that we update the
planes. And that could be achieved by unconditionally setting
crtc_state->planes_changed. We already have all the plane states when
restoring state anyway.
That means we should move readout_plane_state into the above loop. Which
then gets a bit too big, so better extract this into a
intel_reconstruct_plane_state or similar. This function should then do all
the plane state reconstruction.
That also means we don't have to play tricks with plane_mask like you do.
We simply reconstruct the primary plane (if possible) and force-disable
all the others. Since this only happens at driver load there's no need to
clear out any state for sprite/cursors since it's already fully cleared.
I think this way we should be able to have everything in one place, and
that should allow us to simplify things a lot.
>
> @@ -15404,47 +15371,30 @@ static void readout_plane_state(struct intel_crtc *crtc,
> struct intel_plane *p;
> struct drm_plane_state *drm_plane_state;
> bool active = crtc_state->base.active;
> + bool wrong_plane;
>
> - if (active) {
> - crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
> -
> - /* apply to previous sw state too */
> - to_intel_crtc_state(crtc->base.state)->quirks |=
> - PIPE_CONFIG_QUIRK_INITIAL_PLANES;
> - }
> + wrong_plane = active && !intel_check_plane_mapping(crtc);
> + if (wrong_plane)
> + crtc->pipe = !crtc->pipe;
>
> for_each_intel_plane(crtc->base.dev, p) {
> - bool visible = active;
> -
> if (crtc->pipe != p->pipe)
> continue;
>
> drm_plane_state = p->base.state;
>
> - /* Plane scaler state is not touched here. The first atomic
> - * commit will restore all plane scalers to its old state.
> - */
> -
> - if (active && p->base.type == DRM_PLANE_TYPE_PRIMARY) {
> - visible = primary_get_hw_state(crtc);
> - to_intel_plane_state(drm_plane_state)->visible = visible;
> - } else {
> - /*
> - * unknown state, assume it's off to force a transition
> - * to on when calculating state changes.
> - */
> + if (p->base.type == DRM_PLANE_TYPE_PRIMARY)
> + to_intel_plane_state(drm_plane_state)->visible =
> + primary_get_hw_state(crtc);
> + else
> to_intel_plane_state(drm_plane_state)->visible = false;
> - }
>
> - if (visible) {
> - crtc_state->base.plane_mask |=
> - 1 << drm_plane_index(&p->base);
> - } else if (crtc_state->base.state) {
> - /* Make this unconditional for atomic hw readout. */
> - crtc_state->base.plane_mask &=
> - ~(1 << drm_plane_index(&p->base));
> - }
> + crtc_state->base.plane_mask |=
> + 1 << drm_plane_index(&p->base);
> }
> +
> + if (wrong_plane)
> + crtc->pipe = !crtc->pipe;
> }
>
> static void intel_modeset_readout_hw_state(struct drm_device *dev)
> @@ -15664,6 +15614,9 @@ void intel_modeset_gem_init(struct drm_device *dev)
> update_state_fb(c->primary);
> c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
> }
> + else
> + obj->frontbuffer_bits |=
> + to_intel_plane(c->primary)->frontbuffer_bit;
This is part of plane state reconstruction, no need to move it. The only
reason we have to pin late is that gem isn't initialized yet fully when we
want to reconstruct the modeset state. And that pinning shouldn't ever
fail, it just increments pin_count and writes the ptes (again).
-Daniel
> }
>
> intel_backlight_register(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2ed618f78fe6..c3cea178c809 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -341,7 +341,6 @@ struct intel_crtc_state {
> */
> #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS (1<<0) /* unreliable sync mode.flags */
> #define PIPE_CONFIG_QUIRK_INHERITED_MODE (1<<1) /* mode inherited from firmware */
> -#define PIPE_CONFIG_QUIRK_INITIAL_PLANES (1<<2) /* planes are in unknown state */
> unsigned long quirks;
>
> /* Pipe source size (ie. panel fitter input size)
> @@ -550,7 +549,6 @@ struct intel_crtc {
> uint32_t cursor_size;
> uint32_t cursor_base;
>
> - struct intel_initial_plane_config plane_config;
> struct intel_crtc_state *config;
> bool new_enabled;
>
> --
> 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
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list