| Message ID | 1436252911-5703-8-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 07-07-15 om 13:16 schreef Daniel Vetter: > 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@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. I thought this was about multiple planes sharing the same fb with same offset. And as such checking for the crtc is unnecessary, for the current crtc it will be be NULL here. This only reads out the current fb, not different ones. And sharing the same fb with other crtc's is done in intel_fbdev_init_bios. >> 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? Just the page flip handler. >> - 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. On resume I force a modeset for this reason, and set the plane_mask. This makes sure any plane gets disabled. The modeset will also add all planes, which sets planes_changed if needed. A commit on its own doesn't do this, when a plane doesn't have a fb it won't disable it on its own because the plane's assumed to be disabled when old_plane_state->fb == NULL and new_plane_state->fb == NULL. I don't know if I can call disable_plane in this loop either, because that will update the watermarks on skylake with the call to intel_update_sprite_watermarks calling skl_update_sprite_wm calling skl_update_wm. We don't even have any watermarks calculated yet, so that will break. > 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. I do this trick for atomic resume, we can't allocate the original fb in that case but I still want to sanitize everything. This either happens because a new primary fb gets committed or the primary fb gets disabled. >> >> @@ -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). Why bother handling the return value at all then? ~Maarten
On Tue, Jul 07, 2015 at 04:02:32PM +0200, Maarten Lankhorst wrote: > Op 07-07-15 om 13:16 schreef Daniel Vetter: > > 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@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. > I thought this was about multiple planes sharing the same fb with same offset. > And as such checking for the crtc is unnecessary, for the current crtc it will be be NULL here. > > This only reads out the current fb, not different ones. > > And sharing the same fb with other crtc's is done in intel_fbdev_init_bios. This is about sharing the same fb but across different crtcs - bios never enables more than the primary plane anyway. And you can't rely upon fbdev_init_bios since that's not run at all when I915_FBDEV=n. So yes with current code this loop here reconstruct the shared between primary planes on different crtcs (if the stolen allocator tells us that our range is occupied already). fbdev_init_bios just tries to create a config matching the one the bios has set up (and then pick a suitable fb for fbcon from the ones already allocated). Maybe we should extract this as try_to_find_shared_fb or similar to make the code self-explanatory? > > >> 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? > Just the page flip handler. > >> - 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. > On resume I force a modeset for this reason, and set the plane_mask. > This makes sure any plane gets disabled. > > The modeset will also add all planes, which sets planes_changed if needed. Yeah that's what I had in mind for resume: We need to grab all plane states anyway (to be able to restore the old config), so at most we need to set a planes_changed to make sure the update happens. > A commit on its own doesn't do this, when a plane doesn't have a fb > it won't disable it on its own because the plane's assumed to be disabled > when old_plane_state->fb == NULL and new_plane_state->fb == NULL. Not a problem since too many planes doesn't seem to happen in reality. At least we didn't have code to force-disable planes on resume/lid_notify before, which means we don't suddenly need it now. sanitize_* should only fix up stuff that's broken for i915 on real-world machines, not what all could be possible. > I don't know if I can call disable_plane in this loop either, because that will > update the watermarks on skylake with the call to intel_update_sprite_watermarks > calling skl_update_sprite_wm calling skl_update_wm. Hm that sounds like a bug still. Maybe just forget about disabling non-primary planes for now on driver load? We don't seem to have any need currently either. And for primary planes maybe we can hard-code something which just clears the PLANE_ENABLE bit and does nothing else? Kind of a super-low-level plane force disable. Similar to how we have a special function to force-disable the vga plane. > We don't even have any watermarks calculated yet, so that will break. > > 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. > I do this trick for atomic resume, we can't allocate the original fb in that case > but I still want to sanitize everything. This either happens because > a new primary fb gets committed or the primary fb gets disabled. Well on resume we don't care at all about the original fb for two reasons: - Anything remotely modern resumes with everything off. - We only recover the initial plane from the bios to ensure boot-up is completely flicker-free. If we don't reserve that we'll allocate rings and other gpu objects in there which isn't pretty. I don't think we need to sanitize planes either as long as we force a full plane update on resume (by forcing planes_changed or similar). And for the lid_restore case the problem hasn't been that the bios changed the plane state really, but that it just went ahead and disabled everything on its own. That will result in a crtc_enable which will restore everything correctly. Imo we don't need to have a perfect sanitize for everything, this code really just fixes up what's actually been broken in real-world machines out there. That kind of real-world testing is also why I want to change as little as possible in the sanitize_* functions. > >> @@ -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). > Why bother handling the return value at all then? To print the warning. I think even the cleanup is pointless since at worst we'll underrun the pin count, and we have protection for that already (again with a loud warning). If you want you can remove that code. -Daniel
Op 08-07-15 om 11:27 schreef Daniel Vetter: > On Tue, Jul 07, 2015 at 04:02:32PM +0200, Maarten Lankhorst wrote: >> Op 07-07-15 om 13:16 schreef Daniel Vetter: >>> 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@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. >> I thought this was about multiple planes sharing the same fb with same offset. >> And as such checking for the crtc is unnecessary, for the current crtc it will be be NULL here. >> >> This only reads out the current fb, not different ones. >> >> And sharing the same fb with other crtc's is done in intel_fbdev_init_bios. > This is about sharing the same fb but across different crtcs - bios never > enables more than the primary plane anyway. And you can't rely upon > fbdev_init_bios since that's not run at all when I915_FBDEV=n. > > So yes with current code this loop here reconstruct the shared between > primary planes on different crtcs (if the stolen allocator tells us that > our range is occupied already). fbdev_init_bios just tries to create a > config matching the one the bios has set up (and then pick a suitable fb > for fbcon from the ones already allocated). > > Maybe we should extract this as try_to_find_shared_fb or similar to make > the code self-explanatory? > >>>> 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? >> Just the page flip handler. >>>> - 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. >> On resume I force a modeset for this reason, and set the plane_mask. >> This makes sure any plane gets disabled. >> >> The modeset will also add all planes, which sets planes_changed if needed. > Yeah that's what I had in mind for resume: We need to grab all plane > states anyway (to be able to restore the old config), so at most we need > to set a planes_changed to make sure the update happens. > >> A commit on its own doesn't do this, when a plane doesn't have a fb >> it won't disable it on its own because the plane's assumed to be disabled >> when old_plane_state->fb == NULL and new_plane_state->fb == NULL. > Not a problem since too many planes doesn't seem to happen in reality. At > least we didn't have code to force-disable planes on resume/lid_notify > before, which means we don't suddenly need it now. sanitize_* should only > fix up stuff that's broken for i915 on real-world machines, not what all > could be possible. > >> I don't know if I can call disable_plane in this loop either, because that will >> update the watermarks on skylake with the call to intel_update_sprite_watermarks >> calling skl_update_sprite_wm calling skl_update_wm. > Hm that sounds like a bug still. Maybe just forget about disabling > non-primary planes for now on driver load? We don't seem to have any need > currently either. And for primary planes maybe we can hard-code something > which just clears the PLANE_ENABLE bit and does nothing else? Kind of a > super-low-level plane force disable. Similar to how we have a special > function to force-disable the vga plane. > >> We don't even have any watermarks calculated yet, so that will break. >>> 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. >> I do this trick for atomic resume, we can't allocate the original fb in that case >> but I still want to sanitize everything. This either happens because >> a new primary fb gets committed or the primary fb gets disabled. > Well on resume we don't care at all about the original fb for two reasons: > - Anything remotely modern resumes with everything off. > - We only recover the initial plane from the bios to ensure boot-up is > completely flicker-free. If we don't reserve that we'll allocate rings > and other gpu objects in there which isn't pretty. > > I don't think we need to sanitize planes either as long as we force a full > plane update on resume (by forcing planes_changed or similar). This doesn't work. If a plane has no fb in software you won't update it with commit_planes_on_crtc, because old_state->crtc and plane_state->crtc are both NULL. > And for the lid_restore case the problem hasn't been that the bios changed > the plane state really, but that it just went ahead and disabled > everything on its own. That will result in a crtc_enable which will > restore everything correctly. > > Imo we don't need to have a perfect sanitize for everything, this code > really just fixes up what's actually been broken in real-world machines > out there. That kind of real-world testing is also why I want to change as > little as possible in the sanitize_* functions. > >>>> @@ -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). >> Why bother handling the return value at all then? > To print the warning. I think even the cleanup is pointless since at worst > we'll underrun the pin count, and we have protection for that already > (again with a loud warning). If you want you can remove that code. Ok. ~Maarten
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; 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); - 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); } } @@ -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; } 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;
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. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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(-)