Re: [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  wrote:
> On Wed, 20 Dec 2017 12:26:39 +
> Daniel Stone  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 

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
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:39 +
Daniel Stone  wrote:

> Use the shiny new helper we have for working through scanout as well.
> 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 81 
> ++
>  1 file changed, 25 insertions(+), 56 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 10bc53ba0..d97efd761 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1575,13 +1575,6 @@ drm_output_assign_state(struct drm_output_state *state,
>   }
>  }
>  
> -static int
> -drm_view_transform_supported(struct weston_view *ev)
> -{
> - return !ev->transform.enabled ||
> - (ev->transform.matrix.type < WESTON_MATRIX_TRANSFORM_ROTATE);
> -}
> -
>  static bool
>  drm_view_is_opaque(struct weston_view *ev)
>  {
> @@ -1613,7 +1606,6 @@ drm_output_prepare_scanout_view(struct drm_output_state 
> *output_state,
>   struct drm_plane *scanout_plane = output->scanout_plane;
>   struct drm_plane_state *state;
>   struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
> - struct weston_buffer_viewport *viewport = >surface->buffer_viewport;
>   struct gbm_bo *bo;
>  
>   /* Don't import buffers which span multiple outputs. */
> @@ -1629,28 +1621,6 @@ drm_output_prepare_scanout_view(struct 
> drm_output_state *output_state,
>   if (wl_shm_buffer_get(buffer->resource))
>   return NULL;
>  
> - /* Make sure our view is exactly compatible with the output. */
> - if (ev->geometry.x != output->base.x ||
> - ev->geometry.y != output->base.y)
> - return NULL;
> - if (buffer->width != output->base.current_mode->width ||
> - buffer->height != output->base.current_mode->height)
> - return NULL;
> -
> - if (ev->transform.enabled)
> - return NULL;
> - if (ev->geometry.scissor_enabled)
> - return NULL;
> - if (viewport->buffer.transform != output->base.transform)
> - return NULL;
> - if (viewport->buffer.scale != output->base.current_scale)
> - return NULL;
> - if (!drm_view_transform_supported(ev))
> - return NULL;
> -
> - 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,
> @@ -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).

> + if (state->src_x != 0 || state->src_y != 0 ||
> + state->src_w != state->dest_w << 16 ||
> + state->src_h != state->dest_h << 16 ||
> + state->dest_x != 0 || state->dest_y != 0 ||
> + state->dest_w != (unsigned) output->base.current_mode->width ||
> + state->dest_h != (unsigned) output->base.current_mode->height)
> + goto err;
> +
> + if (ev->alpha != 1.0f)
> + goto err;
> +
>   bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
>  buffer->resource, GBM_BO_USE_SCANOUT);
>  
>   /* Unable to use the buffer for scanout */
>   if (!bo)
> - return NULL;
> + goto err;
>  
>   state->fb = drm_fb_get_from_bo(bo, b, drm_view_is_opaque(ev),
>  BUFFER_CLIENT);
>   if (!state->fb) {
> - drm_plane_state_put_back(state);
> + /* We need to explicitly destroy the BO. */
>   gbm_bo_destroy(bo);
> - return NULL;
> + goto err;
>   }
>  
>   /* Can't change formats with just a pageflip */
>   if (state->fb->format->format != output->gbm_format) {
>   /* No need to destroy the GBM BO here, as it's now owned
>* by the FB. */
> - drm_plane_state_put_back(state);
> - return NULL;
> + goto err;
>   }
>  
>   drm_fb_set_buffer(state->fb, buffer);
>  
> - state->output = output;
> -
> - state->src_x = 0;
> - state->src_y = 0;
> - state->src_w = state->fb->width << 16;
> - state->src_h = state->fb->height << 16;
> -
> - state->dest_x = 0;
> - state->dest_y = 0;
> - state->dest_w = output->base.current_mode->width;
> - state->dest_h = output->base.current_mode->height;
> -
>   return _plane->base;
> +
> +err:
> +