Op 24-05-16 om 16:00 schreef Daniel Vetter:
> On Tue, May 17, 2016 at 03:08:01PM +0200, Maarten Lankhorst wrote:
>> All of intel_post_plane_update is handled there now, so move it over.
>> This is run after the hw state checker because it can't handle checking
>> crtc's separately yet.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> First this patch is massive, and imo too big and should have been split
> up.
>
>> ---
>> drivers/gpu/drm/i915/intel_atomic.c | 11 ++
>> drivers/gpu/drm/i915/intel_display.c | 344 ++++++++++++++++++++++-------------
>> drivers/gpu/drm/i915/intel_drv.h | 5 +-
>> 3 files changed, 228 insertions(+), 132 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 50ff90aea721..b4927f6bbeac 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -311,6 +311,17 @@ intel_atomic_state_alloc(struct drm_device *dev)
>> void intel_atomic_state_clear(struct drm_atomic_state *s)
>> {
>> struct intel_atomic_state *state = to_intel_atomic_state(s);
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(state->work); i++) {
>> + struct intel_flip_work *work = state->work[i];
>> +
>> + if (work)
>> + intel_free_flip_work(work);
>> +
>> + state->work[i] = NULL;
>> + }
>> +
>> drm_atomic_state_default_clear(&state->base);
>> state->dpll_set = state->modeset = false;
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 69abc808a2c4..16d8e299994d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4537,39 +4537,6 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
>> }
>> }
>>
>> -static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>> -{
>> - struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
>> - struct drm_atomic_state *old_state = old_crtc_state->base.state;
>> - struct intel_crtc_state *pipe_config =
>> - to_intel_crtc_state(crtc->base.state);
>> - struct drm_device *dev = crtc->base.dev;
>> - struct drm_plane *primary = crtc->base.primary;
>> - struct drm_plane_state *old_pri_state =
>> - drm_atomic_get_existing_plane_state(old_state, primary);
>> -
>> - intel_frontbuffer_flip(dev, pipe_config->fb_bits);
>> -
>> - crtc->wm.cxsr_allowed = true;
>> -
>> - if (pipe_config->update_wm_post && pipe_config->base.active)
>> - intel_update_watermarks(&crtc->base);
>> -
>> - if (old_pri_state) {
>> - struct intel_plane_state *primary_state =
>> - to_intel_plane_state(primary->state);
>> - struct intel_plane_state *old_primary_state =
>> - to_intel_plane_state(old_pri_state);
>> -
>> - intel_fbc_post_update(crtc);
>> -
>> - if (primary_state->visible &&
>> - (needs_modeset(&pipe_config->base) ||
>> - !old_primary_state->visible))
>> - intel_post_enable_primary(&crtc->base);
>> - }
>> -}
> The above seems to have moved/disappeared entirely, and is definitely not
> unpin related. Really, this must be split up. I guess another reason to
> revert this for now.
>
> Or at least the commit should have a more accurate summary. But since I
> can't even find the new callsite of these functions in this diff here
> something seems fishy, but I didn't look at the entire series.
>
> Anyway, with that rant out of the way see below for what I really wanted
> to comment on.
>
>
> [snip]
>
>> +static void intel_prepare_work(struct drm_crtc *crtc,
>> + struct intel_flip_work *work,
>> + struct drm_atomic_state *state,
>> + struct drm_crtc_state *old_crtc_state)
>> {
>> - unsigned last_vblank_count[I915_MAX_PIPES];
>> - enum pipe pipe;
>> - int ret;
>> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> + struct drm_plane_state *old_plane_state;
>> + struct drm_plane *plane;
>> + int i, j = 0;
>>
>> - if (!crtc_mask)
>> - return;
>> + INIT_WORK(&work->unpin_work, intel_unpin_work_fn);
>> + INIT_WORK(&work->mmio_work, intel_mmio_flip_work_func);
>> + atomic_inc(&intel_crtc->unpin_work_count);
>>
>> - for_each_pipe(dev_priv, pipe) {
>> - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>> + for_each_plane_in_state(state, plane, old_plane_state, i) {
>> + struct intel_plane_state *old_state = to_intel_plane_state(old_plane_state);
>> + struct intel_plane_state *new_state = to_intel_plane_state(plane->state);
>>
>> - if (!((1 << pipe) & crtc_mask))
>> + if (old_state->base.crtc != crtc &&
>> + new_state->base.crtc != crtc)
>> continue;
>>
>> - ret = drm_crtc_vblank_get(crtc);
>> - if (WARN_ON(ret != 0)) {
>> - crtc_mask &= ~(1 << pipe);
>> - continue;
>> + if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
>> + plane->fb = new_state->base.fb;
>> + crtc->x = new_state->base.src_x >> 16;
>> + crtc->y = new_state->base.src_y >> 16;
>> }
>>
>> - last_vblank_count[pipe] = drm_crtc_vblank_count(crtc);
>> + old_state->wait_req = new_state->wait_req;
>> + new_state->wait_req = NULL;
>> +
>> + old_state->base.fence = new_state->base.fence;
>> + new_state->base.fence = NULL;
>> +
>> + /* remove plane state from the atomic state and move it to work */
>> + old_plane_state->state = NULL;
>> + state->planes[i] = NULL;
>> + state->plane_states[i] = NULL;
>> +
>> + work->old_plane_state[j] = old_state;
>> + work->new_plane_state[j++] = new_state;
>> }
>>
>> - for_each_pipe(dev_priv, pipe) {
>> - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>> - long lret;
>> + old_crtc_state->state = NULL;
>> + state->crtcs[drm_crtc_index(crtc)] = NULL;
>> + state->crtc_states[drm_crtc_index(crtc)] = NULL;
>>
>> - if (!((1 << pipe) & crtc_mask))
>> - continue;
>> + work->old_crtc_state = to_intel_crtc_state(old_crtc_state);
>> + work->new_crtc_state = to_intel_crtc_state(crtc->state);
>> + work->num_planes = j;
>>
>> - lret = wait_event_timeout(dev->vblank[pipe].queue,
>> - last_vblank_count[pipe] !=
>> - drm_crtc_vblank_count(crtc),
>> - msecs_to_jiffies(50));
>> + work->event = crtc->state->event;
>> + crtc->state->event = NULL;
>>
>> - WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe));
>> + if (needs_modeset(crtc->state) || work->new_crtc_state->update_pipe) {
>> + struct drm_connector *conn;
>> + struct drm_connector_state *old_conn_state;
>> + int k = 0;
>>
>> - drm_crtc_vblank_put(crtc);
>> - }
>> + j = 0;
>> +
>> + /*
>> + * intel_unpin_work_fn cannot depend on the connector list
>> + * because it may be freed from underneath it, so add
>> + * them all to the work struct while we're holding locks.
>> + */
> Thanks to connector refcounting no longer true, and as long as you hold
> onto drm_atomic_state the connectors will no longer disappear. Please
> revise/update.
This comment only became true thanks to connector refcounting. unpin_work
cannot take connection_mutex or mode_config->mutex because that would deadlock
if flushed from intel_atomic_commit with those locks held, but needs it for the
hw state verifier.
Before connector refcounting I could perform a flush in intel_dp_destroy_mst_connector
so even though unpin_work wouldn't have the locks, it would never see a modified list.
With connector refcounting I move the connector_states to the work, and free them there.
But it looks like the locking for unreferencing a connector seems to be messed up.
drm_atomic_state_free can be called without locking anything,
so drm_connector_free needs to handle its own locking when removing
the connector from the list. This affects all atomic drivers, and probably means
requiring a work item to free all connectors with the right locks..
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
