[Intel-gfx] [PATCH v2 07/20] drm/i915: Rework primary plane stuff slightly.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Tue Jul 7 07:02:32 PDT 2015
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 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.
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
More information about the Intel-gfx
mailing list