[PATCH weston v11 08/13] compositor-drm: Introduce drm_plane_state structure

[Daniel Stone]

Hi Pekka,

On 20 July 2017 at 18:01, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Tue, 18 Jul 2017 14:14:30 +0100
> Daniel Stone <daniels at collabora.com> wrote:
>> @@ -986,6 +1130,20 @@ drm_output_state_duplicate(struct drm_output_state *src,
>>       else
>>               wl_list_init(&dst->link);
>>
>> +     wl_list_init(&dst->plane_list);
>> +
>> +     wl_list_for_each(ps, &src->plane_list, link) {
>> +             /* Don't carry planes which are now disabled; these should be
>> +              * free for other outputs to reuse. */
>> +             if (!ps->output)
>> +                     continue;
>
> Ooh, very clever. Elegant even. This code implies that a plane cannot
> be switched from one output to another in a single commit, there needs
> to be a commit disabling it in between. Or even more important with
> pre-atomic, since programming a plane to a new output might cause a
> glitch on the old output. Right?

Something bad would happen. Glitches (in either or both old/new
outputs), arbitrary long blocking when using the legacy API, etc.
Atomic enforces that a plane cannot hop between outputs without a
disable having completed in between. If old/new CRTCs were genlocked,
then it may be possible to cut out the cycle, but not in the general
case.

>> @@ -1122,6 +1291,24 @@ drm_output_assign_state(struct drm_output_state *state,
>>       wl_list_remove(&state->link);
>>       wl_list_init(&state->link);
>>       output->state_cur = state;
>> +
>> +     /* Replace state_cur on each affected plane with the new state, being
>> +      * careful to dispose of orphaned (but only orphaned) previous state.
>> +      * If the previous state is not orphaned (still has an output_state
>> +      * attached), it will be disposed of by freeing the output_state. */
>> +     wl_list_for_each(plane_state, &state->plane_list, link) {
>> +             struct drm_plane *plane = plane_state->plane;
>> +
>> +             if (plane->state_cur && !plane->state_cur->output_state)
>> +                     drm_plane_state_free(plane->state_cur, true);
>> +             plane->state_cur = plane_state;
>> +
>> +             if (mode != DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS)
>> +                     continue;
>> +
>> +             if (plane->type == WDRM_PLANE_TYPE_OVERLAY)
>> +                     output->vblank_pending++;
>> +     }
>
> Yes. This is the reason you add the per-plane vblank event
> subscriptions to drm_output_start_repaint_loop().

Correct! Generally speaking, moving to the model of tracking state
everywhere, rather than just programming in the desired state and
hoping it's followed the expected sequence, requires us to cut out
special cases like start_repaint_loop. By the point we start using the
atomic API, we only have one codepath to program in the new state to
KMS, which does that. So this is just getting consistency of behaviour
in early.

Cheers,
Daniel