[PATCH v14 22/41] compositor-drm: Use plane_state_coords_for_view for scanout

[Daniel Stone]

Hi Pekka,

On 24 January 2018 at 11:30, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Wed, 20 Dec 2017 12:26:39 +0000
> Daniel Stone <daniels at collabora.com> wrote:
>> @@ -1660,44 +1630,50 @@ drm_output_prepare_scanout_view(struct drm_output_state *output_state,
>>               return NULL;
>>       }
>>
>> +     state->output = output;
>> +     drm_plane_state_coords_for_view(state, ev);
>> +
>> +     /* The legacy API does not let us perform cropping or scaling. */
>
> Hi Daniel,
>
> should this not be checking the src coordinates against the buffer
> dimensions as well? If the buffer is bigger than the mode but clipped
> to the mode size, it might pass all the checks here.
>
> Sources of clipping: layer mask, view mask (scissor).

That's fine; the legacy API just crops if a larger-than-CRTC buffer is passed.

> I like the above, it's a good direction. The motivation of this patch
> is to simplify the code, and it is not supposed to introduce any
> behavioral changes like allowing more cases through, right? I'd like to
> see that mentioned in the commit message.

Yep, correct. I'm fine to write that out explicitly.

> The below looks like it's reverting a fix from upstream master, a
> rebasing mistake?

Mis-squashed rather than accidentally reverting. In current master:
commit e2e80136334fe64b12225183d05a0d32c12723de
Author:     Daniel Stone <daniels at collabora.com>

    compositor-drm: Use drm_plane for scanout plane

    Use a real drm_plane to back the scanout plane, displacing
    output->fb_{last,cur,pending} to their plane-tracked equivalents.

[...]

@@ -1456,6 +1451,15 @@ drm_output_prepare_scanout_view(struct
drm_output_state *output_state,
        if (ev->alpha != 1.0f)
                return NULL;

+       state = drm_output_state_get_plane(output_state, scanout_plane);
+       if (state->fb) {
+               /* If there is already a framebuffer on the scanout plane,
+                * a client view has already been placed on the scanout
+                * view. In that case, do not free or put back the state,
+                * but just leave it in place and quietly exit. */
+               return NULL;
+       }


Given that it already landed, and the revert should've gone there,
I'll pull that out into a separate commit for v16.

Cheers,
Daniel