Re: [PATCH weston 10/68] compositor-drm: Make scanout view preparation more stringent

[Derek Foreman]

On 27/01/17 08:30 AM, Daniel Stone wrote:

Hi,

On 27 January 2017 at 14:25, Derek Foreman  wrote:

On 09/12/16 01:57 PM, Daniel Stone wrote:

+static int
+drm_view_transform_supported(struct weston_view *ev)
+{
+   return !ev->transform.enabled ||
+   (ev->transform.matrix.type <
WESTON_MATRIX_TRANSFORM_ROTATE);
+}
+
 static uint32_t
 drm_output_check_scanout_format(struct drm_output *output,


Not sure this function name is really ideal anymore?  Maybe it never was,
lots of stuff here I'm not sure one would all "format".


Yeah, it's probably not the best, but I also didn't touch it here, so ... :)


@@ -502,27 +509,40 @@ drm_output_prepare_scanout_view(struct drm_output
*output,
struct gbm_bo *bo;
uint32_t format;

+   /* Don't import buffers which span multiple outputs. */
+   if (ev->output_mask != (1u << output->base.id))
+   return NULL;
+


I suppose technically this is overly stringent, but I'd rather it be too
careful than broken.


Yeah, it should if anything just serve as an early-out. We can never
hoist multiple-output views to planes, because otherwise it falls out
of the scene graph for the other outputs. So we later build up to
using this unconditionally for plane import; these early patches are
just about making the scanout/overlay/cursor test paths as similar as
possible, to save any surprises further on down the line.


/* 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;
+


Was there a reason to move this block?


Just for symmetry with the others; certainly no functional effect.


I really don't have any concerns that would warrant another revision, this
looks good to me.  (Also looks like it could be trivially landed without
dependency on any other patch in this series, with no functional issues.)


Thanks! I'm going to go ahead and just do that I think, and then if
any of the cleanups apply to the atomic series, we could address them
there perhaps?


Agreed!


Cheers,
Daniel



___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 10/68] compositor-drm: Make scanout view preparation more stringent

[Daniel Stone]

Hi,

On 27 January 2017 at 14:25, Derek Foreman  wrote:
> On 09/12/16 01:57 PM, Daniel Stone wrote:
>> +static int
>> +drm_view_transform_supported(struct weston_view *ev)
>> +{
>> +   return !ev->transform.enabled ||
>> +   (ev->transform.matrix.type <
>> WESTON_MATRIX_TRANSFORM_ROTATE);
>> +}
>> +
>>  static uint32_t
>>  drm_output_check_scanout_format(struct drm_output *output,
>
> Not sure this function name is really ideal anymore?  Maybe it never was,
> lots of stuff here I'm not sure one would all "format".

Yeah, it's probably not the best, but I also didn't touch it here, so ... :)

>> @@ -502,27 +509,40 @@ drm_output_prepare_scanout_view(struct drm_output
>> *output,
>> struct gbm_bo *bo;
>> uint32_t format;
>>
>> +   /* Don't import buffers which span multiple outputs. */
>> +   if (ev->output_mask != (1u << output->base.id))
>> +   return NULL;
>> +
>
> I suppose technically this is overly stringent, but I'd rather it be too
> careful than broken.

Yeah, it should if anything just serve as an early-out. We can never
hoist multiple-output views to planes, because otherwise it falls out
of the scene graph for the other outputs. So we later build up to
using this unconditionally for plane import; these early patches are
just about making the scanout/overlay/cursor test paths as similar as
possible, to save any surprises further on down the line.

>> /* 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;
>> +
>
> Was there a reason to move this block?

Just for symmetry with the others; certainly no functional effect.

> I really don't have any concerns that would warrant another revision, this
> looks good to me.  (Also looks like it could be trivially landed without
> dependency on any other patch in this series, with no functional issues.)

Thanks! I'm going to go ahead and just do that I think, and then if
any of the cleanups apply to the atomic series, we could address them
there perhaps?

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 10/68] compositor-drm: Make scanout view preparation more stringent

[Derek Foreman]

On 09/12/16 01:57 PM, Daniel Stone wrote:

Don't import buffers which span multiple outputs, short-cut any attempt
to import SHM buffers, and ignore buffers with a global alpha set.

I'm not convinced all of these conditions entirely make sense, but this
at least makes them equally nonsensical.

Signed-off-by: Daniel Stone 

Differential Revision: https://phabricator.freedesktop.org/D1414
---
 libweston/compositor-drm.c | 35 ---
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 8cd9d5a..e22d792 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -462,6 +462,13 @@ drm_output_release_fb(struct drm_output *output, struct 
drm_fb *fb)
}
 }

+static int
+drm_view_transform_supported(struct weston_view *ev)
+{
+   return !ev->transform.enabled ||
+   (ev->transform.matrix.type < WESTON_MATRIX_TRANSFORM_ROTATE);
+}
+
 static uint32_t
 drm_output_check_scanout_format(struct drm_output *output,


Not sure this function name is really ideal anymore?  Maybe it never 
was, lots of stuff here I'm not sure one would all "format".



struct weston_surface *es, struct gbm_bo *bo)
@@ -502,27 +509,40 @@ drm_output_prepare_scanout_view(struct drm_output *output,
struct gbm_bo *bo;
uint32_t format;

+   /* Don't import buffers which span multiple outputs. */
+   if (ev->output_mask != (1u << output->base.id))
+   return NULL;
+


I suppose technically this is overly stringent, but I'd rather it be too 
careful than broken.



/* We use GBM to import buffers. */
if (b->gbm == NULL)
return NULL;

if (buffer == NULL)
return NULL;
+   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;
+


Was there a reason to move this block?

I really don't have any concerns that would warrant another revision, 
this looks good to me.  (Also looks like it could be trivially landed 
without dependency on any other patch in this series, with no functional 
issues.)


Reviewed-by: Derek Foreman 


if (ev->transform.enabled)
return NULL;
if (ev->geometry.scissor_enabled)
return NULL;
-
-   if (buffer->width != output->base.current_mode->width ||
-   buffer->height != output->base.current_mode->height)
-   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;

bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
   buffer->resource, GBM_BO_USE_SCANOUT);
@@ -936,13 +956,6 @@ drm_output_check_sprite_format(struct drm_sprite *s,
return 0;
 }

-static int
-drm_view_transform_supported(struct weston_view *ev)
-{
-   return !ev->transform.enabled ||
-   (ev->transform.matrix.type < WESTON_MATRIX_TRANSFORM_ROTATE);
-}
-
 static struct weston_plane *
 drm_output_prepare_overlay_view(struct drm_output *output,
struct weston_view *ev)



___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 10/68] compositor-drm: Make scanout view preparation more stringent

[Daniel Stone]

Don't import buffers which span multiple outputs, short-cut any attempt
to import SHM buffers, and ignore buffers with a global alpha set.

I'm not convinced all of these conditions entirely make sense, but this
at least makes them equally nonsensical.

Signed-off-by: Daniel Stone 

Differential Revision: https://phabricator.freedesktop.org/D1414
---
 libweston/compositor-drm.c | 35 ---
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 8cd9d5a..e22d792 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -462,6 +462,13 @@ drm_output_release_fb(struct drm_output *output, struct 
drm_fb *fb)
}
 }
 
+static int
+drm_view_transform_supported(struct weston_view *ev)
+{
+   return !ev->transform.enabled ||
+   (ev->transform.matrix.type < WESTON_MATRIX_TRANSFORM_ROTATE);
+}
+
 static uint32_t
 drm_output_check_scanout_format(struct drm_output *output,
struct weston_surface *es, struct gbm_bo *bo)
@@ -502,27 +509,40 @@ drm_output_prepare_scanout_view(struct drm_output *output,
struct gbm_bo *bo;
uint32_t format;
 
+   /* Don't import buffers which span multiple outputs. */
+   if (ev->output_mask != (1u << output->base.id))
+   return NULL;
+
/* We use GBM to import buffers. */
if (b->gbm == NULL)
return NULL;
 
if (buffer == NULL)
return NULL;
+   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 (buffer->width != output->base.current_mode->width ||
-   buffer->height != output->base.current_mode->height)
-   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;
 
bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
   buffer->resource, GBM_BO_USE_SCANOUT);
@@ -936,13 +956,6 @@ drm_output_check_sprite_format(struct drm_sprite *s,
return 0;
 }
 
-static int
-drm_view_transform_supported(struct weston_view *ev)
-{
-   return !ev->transform.enabled ||
-   (ev->transform.matrix.type < WESTON_MATRIX_TRANSFORM_ROTATE);
-}
-
 static struct weston_plane *
 drm_output_prepare_overlay_view(struct drm_output *output,
struct weston_view *ev)
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel