RE: [PATCH v10 00/61] Atomic modesetting rides again

[Fabien DESSENNE]

Hi all,

It looks like the great work proposed by Daniel to have Atomic Modesetting in 
Weston did not find the success it deserves.
We tested it successfully on STMicroelectronics boards and it works great.

The kernel DRM framework has been reworked in the past months (years) in order 
to support Atomic, but one of the reference compositor, Weston, does not 
support this feature.
And as long as this is not supported, there is no way to make use of HW planes 
('sprites are broken'): all of the composition has to be done by the GPU.

So, for the time being, we have a nice "HW plane enable" kernel, but there is 
no userland compositor in front of it! (except Android HWC...)

Can some of you review those patches and help to get it merged?
Without review from the community, shall we conclude that only us are 
interested in using HW planes in Weston?

BR

Fabien

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

RE: weston-simple-egl fullscreen broken?

[Fabien DESSENNE]

Hi,

Thank you for your feedback.
I am using ARM mali for GL rendering, and I found that there is something wrong 
with its EGL backend (stuck in a glxxx() call, probably wrong buffer init).
I did not investigate deeper for the time being, but I hope Vincent will do ;)

BR

Fabien


>-Original Message-
>From: Daniel Stone [mailto:dan...@fooishbar.org]
>Sent: mardi 28 février 2017 00:18
>To: Arnaud Vrac <raw...@gmail.com>
>Cc: Fabien DESSENNE <fabien.desse...@st.com>; wayland-
>de...@lists.freedesktop.org
>Subject: Re: weston-simple-egl fullscreen broken?
>
>Hi Arnaud,
>
>On 27 February 2017 at 23:12, Arnaud Vrac <raw...@gmail.com> wrote:
>> On Mon, Feb 27, 2017 at 11:18 PM Daniel Stone <dan...@fooishbar.org>
>wrote:
>>> On 6 February 2017 at 16:56, Fabien DESSENNE <fabien.desse...@st.com>
>wrote:
>>> > I remember I used to get « weston-simple-egl -f » working fine.
>>> > But it does not work anymore : nothing is displayed. From the logs
>>> > I can see (among others) zxdg_toplevel_v6.configure and
>>> > wl_surface.commit Testing with another client works fine:
>>> > weston-terminal -f -> OK There may be something wrong with my
>>> > environment since I am testing with the Daniel’s atomic version
>>> > (forked from weston-1.12.0+), but I guess this is broken also with the 
>>> > official
>version.
>>> > If anyone can make a quick test and let me know.
>>>
>>> This also works fine for me, both upstream and on the atomic branch;
>>> it's one of the main tests I do.
>>
>> I remember that weston-simple-egl -f was broken when using software
>> rendering in mesa. From my irc log:
>>
>> 21:56:01< rawoul> 'weston-simple-egl -f' is also broken with
>> LIBGL_ALWAYS_SOFTWARE=1 on libweston-desktop... That's a mesa bug
>> though, the first commited buffer does not use the size sent in the
>> configure event 22:02:04< SardemFF7> not sure it’s a mesa bug
>> 22:04:32< SardemFF7> mesa doesn’t know about the shell protocol, and
>> that’s a requirement from xdg_shell_v6 22:05:16< rawoul> SardemFF7:
>> the client does properly calls wl_egl_window_resize with the correct
>> size before the first call to eglSwapBuffers. It actually works with
>> dri2, but it seems swrast defers the resize
>
>I'm pretty sure that Fabien is using a real GL driver; software GL would never
>make it to scanout as it's a SHM buffer. Either way, I'm pretty sure this bug 
>is
>fixed in Mesa 17.0. :)
>
>Cheers,
>Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

weston-simple-egl fullscreen broken?

[Fabien DESSENNE]

Hi

I remember I used to get < weston-simple-egl -f > working fine.
But it does not work anymore : nothing is displayed. From the logs I can see 
(among others) zxdg_toplevel_v6.configure and wl_surface.commit
Testing with another client works fine: weston-terminal -f -> OK
There may be something wrong with my environment since I am testing with the 
Daniel's atomic version (forked from weston-1.12.0+), but I guess this is 
broken also with the official version.
If anyone can make a quick test and let me know.

Fabien


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

Re: [PATCH weston 66/68] compositor-drm: Test plane states before application

[Fabien DESSENNE]

Hi Daniel


On 09/12/16 20:58, Daniel Stone wrote:
> Generate an output state in two stages. Firstly, attempt to run through
> and generate a configuration with all views in planes. If this succeeds,
> we can bypass the renderer completely.
>
> If this fails, we know we need to have the renderer output used as a
> base on the scanout plane, and can incrementally attempt to construct a
> working state, by trying the combination of our old renderer state with
> planes added one-by-one. If they pass the test commit stage, we can use
> it, so we stash that state and continue on.
>
> Signed-off-by: Daniel Stone 
>
> Differential Revision: https://phabricator.freedesktop.org/D1535
> ---
>   libweston/compositor-drm.c | 135 
> +
>   1 file changed, 101 insertions(+), 34 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index e51a5b2..7c8b5d9 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1669,12 +1669,15 @@ enum drm_output_propose_state_mode {
>   
>   static struct drm_plane_state *
>   drm_output_prepare_scanout_view(struct drm_output_state *output_state,
> - struct weston_view *ev)
> + struct weston_view *ev,
> + enum drm_output_propose_state_mode mode)
>   {
>   struct drm_output *output = output_state->output;
>   struct drm_plane *scanout_plane = output->scanout_plane;
>   struct drm_plane_state *state;
> + struct drm_plane_state *state_old = NULL;
>   struct drm_fb *fb;
> + int ret;
>   
>   fb = drm_fb_get_from_view(output_state, ev);
>   if (!fb)
> @@ -1687,7 +1690,16 @@ drm_output_prepare_scanout_view(struct 
> drm_output_state *output_state,
>   }
>   
>   state = drm_output_state_get_plane(output_state, scanout_plane);
> - if (state->fb) {
> +
> + /* Check if we've already placed a buffer on this plane; if there's a
> +  * buffer there but it comes from GBM, then it's the result of
> +  * drm_output_propose_state placing it here for testing purposes. */
> + if (state->fb &&
> + (state->fb->type == BUFFER_GBM_SURFACE ||
> +  state->fb->type == BUFFER_PIXMAN_DUMB)) {
> + state_old = calloc(1, sizeof(*state_old));
> + memcpy(state_old, state, sizeof(*state_old));
> + } else if (state->fb) {
>   drm_fb_unref(fb);
>   return NULL;
>   }
> @@ -1706,10 +1718,21 @@ drm_output_prepare_scanout_view(struct 
> drm_output_state *output_state,
>   state->dest_h != (unsigned) output->base.current_mode->height)
>   goto err;
>   
> + if (mode == DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY)
> + return state;
> +
> + ret = drm_output_apply_state(output_state, mode);

"mode" is not the correct parameter (it is an 'enum 
drm_output_propose_state_mode' and drm_output_apply_state expects an 
'enum drm_output_apply_state_mode mode').
Use DRM_OUTPUT_APPLY_STATE_TEST instead of mode

> + if (ret != 0)
> + goto err;
> +
>   return state;
>   
>   err:
> - drm_plane_state_put_back(state);
> + drm_plane_state_free(state, false);
> + if (state_old) {
> + wl_list_insert(_state->plane_list, _old->link);
> + state_old->output_state = output_state;
> + }
>   return NULL;
>   }
>   
> @@ -1780,7 +1803,9 @@ drm_output_render(struct drm_output *output, 
> pixman_region32_t *damage)
>* want to render. */
>   scanout_state = drm_output_state_get_plane(output->state_pending,
>  output->scanout_plane);
> - if (scanout_state->fb)
> + if (scanout_state->fb &&
> + scanout_state->fb->type != BUFFER_GBM_SURFACE &&
> + scanout_state->fb->type != BUFFER_PIXMAN_DUMB)
>   return;
>   
>   if (!pixman_region32_not_empty(damage) &&
> @@ -1803,6 +1828,7 @@ drm_output_render(struct drm_output *output, 
> pixman_region32_t *damage)
>   return;
>   }
>   
> + drm_fb_unref(scanout_state->fb);
>   scanout_state->fb = fb;
>   scanout_state->output = output;
>   
> @@ -2401,24 +2427,24 @@ atomic_flip_handler(int fd, unsigned int frame,
>   
>   static struct drm_plane_state *
>   drm_output_prepare_overlay_view(struct drm_output_state *output_state,
> - struct weston_view *ev)
> + struct weston_view *ev,
> + enum drm_output_propose_state_mode mode)
>   {
>   struct drm_output *output = output_state->output;
>   struct weston_compositor *ec = output->base.compositor;
>   struct drm_backend *b = to_drm_backend(ec);
>   struct drm_plane *p;
> - struct drm_plane_state *state = NULL;
>   struct drm_fb *fb;
>   unsigned int i;
> -
> - if (b->sprites_are_broken)
> - 

[PATCH v2] compositor-drm: allow mode frequency selection

[Fabien Dessenne]

As an option, allow to specify a mode (from the configuration file) by
its refresh rate.
Example of valid syntax:
- "mode=1920x1080"Select a 1920x1080 mode, refresh rate undefined.
- "mode=1920x1080@60" Select the (or one of the) 1920x1080 60 Hz mode.

Signed-off-by: Fabien Dessenne <fabien.desse...@st.com>
Reviewed-by: Daniel Stone <dani...@collabora.com>
---
Changes in v2: rework after Daniel Stone review:
 - use "@" as frequency delimiter
 - add missing space

 libweston/compositor-drm.c | 8 ++--
 weston.ini.in  | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 70514ea..5ebd9ce 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -2210,9 +2210,12 @@ drm_output_choose_initial_mode(struct drm_backend 
*backend,
drmModeModeInfo drm_modeline;
int32_t width = 0;
int32_t height = 0;
+   uint32_t refresh = 0;
+   int n;
 
if (mode == WESTON_DRM_BACKEND_OUTPUT_PREFERRED && modeline) {
-   if (sscanf(modeline, "%dx%d", , ) != 2) {
+   n = sscanf(modeline, "%dx%d@%d", , , );
+   if (n != 2 && n !=3 ) {
width = -1;
 
if (parse_modeline(modeline, _modeline) == 0) {
@@ -2228,7 +2231,8 @@ drm_output_choose_initial_mode(struct drm_backend 
*backend,
 
wl_list_for_each_reverse(drm_mode, >base.mode_list, base.link) {
if (width == drm_mode->base.width &&
-   height == drm_mode->base.height)
+   height == drm_mode->base.height &&
+   (refresh == 0 || (refresh == drm_mode->mode_info.vrefresh)))
configured = drm_mode;
 
if (memcmp(current_mode, _mode->mode_info,
diff --git a/weston.ini.in b/weston.ini.in
index d837fb5..71e879a 100644
--- a/weston.ini.in
+++ b/weston.ini.in
@@ -55,7 +55,7 @@ path=@libexecdir@/weston-keyboard
 
 #[output]
 #name=X1
-#mode=1024x768
+#mode=1024x768@60
 #transform=flipped-90
 
 #[libinput]
-- 
2.7.4

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

[PATCH] compositor-drm: allow mode frequency selection

[Fabien Dessenne]

As an option, allow to specify a mode (from the configuration file) by
its refresh rate.
Example of valid syntax:
- "mode=1920x1080"Select a 1920x1080 mode, refresh rate undefined.
- "mode=1920x1080-60" Select the (or one of the) 1920x1080 60 Hz mode.

Signed-off-by: Fabien Dessenne <fabien.desse...@st.com>
---
 libweston/compositor-drm.c | 8 ++--
 weston.ini.in  | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 70514ea..0add22b 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -2210,9 +2210,12 @@ drm_output_choose_initial_mode(struct drm_backend 
*backend,
drmModeModeInfo drm_modeline;
int32_t width = 0;
int32_t height = 0;
+   uint32_t refresh = 0;
+   int n;
 
if (mode == WESTON_DRM_BACKEND_OUTPUT_PREFERRED && modeline) {
-   if (sscanf(modeline, "%dx%d", , ) != 2) {
+   n = sscanf(modeline, "%dx%d-%d", , , );
+   if (n != 2 && n !=3) {
width = -1;
 
if (parse_modeline(modeline, _modeline) == 0) {
@@ -2228,7 +2231,8 @@ drm_output_choose_initial_mode(struct drm_backend 
*backend,
 
wl_list_for_each_reverse(drm_mode, >base.mode_list, base.link) {
if (width == drm_mode->base.width &&
-   height == drm_mode->base.height)
+   height == drm_mode->base.height &&
+   (refresh == 0 || (refresh == drm_mode->mode_info.vrefresh)))
configured = drm_mode;
 
if (memcmp(current_mode, _mode->mode_info,
diff --git a/weston.ini.in b/weston.ini.in
index d837fb5..b2ccde3 100644
--- a/weston.ini.in
+++ b/weston.ini.in
@@ -55,7 +55,7 @@ path=@libexecdir@/weston-keyboard
 
 #[output]
 #name=X1
-#mode=1024x768
+#mode=1024x768-60
 #transform=flipped-90
 
 #[libinput]
-- 
2.7.4

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

Re: [PATCH weston v5 30/42] compositor-drm: Introduce drm_output_state structure

[Fabien DESSENNE]

On 08/12/16 13:58, Daniel Stone wrote:
> Hi Fabien,
>
> On 21 November 2016 at 14:58, Fabien DESSENNE <fabien.desse...@st.com> wrote:
>> On 11/16/2016 03:25 PM, Daniel Stone wrote:
>>> +/**
>>> + * Mark a drm_output_state (the output's last state) as complete. This 
>>> handles
>>> + * any post-completion actions such as updating the repaint timer, 
>>> disabling the
>>> + * output, and finally freeing the state.
>>> + */
>>> +static void
>>> +drm_output_update_complete(struct drm_output *output, uint32_t flags,
>>> +  unsigned int sec, unsigned int usec)
>>> +{
>>> +   struct timespec ts;
>>> +
>>> +   drm_output_state_free(output->state_last);
>>> +   output->state_last = NULL;
>>> +
>>> +   if (output->destroy_pending) {
>>> +   drm_output_destroy(>base);
>>> +   goto out;
>>> +   }
>>> +   else if (output->disable_pending) {
>> Remove 'else'
> You're right that it isn't strictly necessary, as the 'goto' will jump
> away from it and make these branches mutually exclusive anyway. But I
> do like that it makes things clearer at a first read ('right, only one
> of these branches will ever be executed at a time'), and also it does
> match what the previous code this moved was doing.

OK, no problem Daniel. You may also consider keeping "} else if (...) {" 
on the same line.
BR
Fabien

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

Re: [PATCH weston v5 31/42] compositor-drm: Introduce drm_plane_state structure

[Fabien DESSENNE]

Hi Daniel


Below are two remarks regarding (m/z/c)alloc.

By the way, as a general remark (and maybe a personal opinion) zalloc(x)
is a bit more readable than calloc(1,x)



On 11/16/2016 03:25 PM, Daniel Stone wrote:
> Track dynamic plane state (CRTC, FB, position) in separate structures,
> rather than as part of the plane. This will make it easier to handle
> state management later, and much more closely tracks what the kernel
> does with atomic modesets.
>
> The fb_last pointer previously used in drm_plane now becomes part of
> output->state_last, and is not directly visible from the plane itself.
>
> Signed-off-by: Daniel Stone 
>
> Differential Revision: https://phabricator.freedesktop.org/D1412
> ---
>   libweston/compositor-drm.c | 319 
> -
>   1 file changed, 258 insertions(+), 61 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 1089d77..e80bd71 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -226,6 +226,29 @@ struct drm_edid {
>*/
>   struct drm_output_state {
>  struct drm_output *output;
> +
> +   struct wl_list plane_list;
> +};
> +
> +/**
> + * Plane state holds the dynamic state for a plane: where it is positioned,
> + * and which buffer it is currently displaying.
> + */
> +struct drm_plane_state {
> +   struct drm_plane *plane;
> +   struct drm_output *output;
> +   struct drm_output_state *output_state;
> +
> +   struct drm_fb *fb;
> +
> +   int32_t src_x, src_y;
> +   uint32_t src_w, src_h;
> +   uint32_t dest_x, dest_y;
> +   uint32_t dest_w, dest_h;
> +
> +   bool complete;
> +
> +   struct wl_list link; /* drm_output_state::plane_list */
>   };
>
>   /**
> @@ -244,14 +267,12 @@ struct drm_output_state {
>* are referred to as 'sprites'.
>*/
>   struct drm_plane {
> -   struct wl_list link;
> -
>  struct weston_plane base;
>
> -   struct drm_fb *fb_current, *fb_pending, *fb_last;
> -   struct drm_output *output;
>  struct drm_backend *backend;
>
> +   struct drm_plane_state *state_cur;
> +
>  enum wdrm_plane_type type;
>  struct plane_properties props;
>
> @@ -259,10 +280,7 @@ struct drm_plane {
>  uint32_t plane_id;
>  uint32_t count_formats;
>
> -   int32_t src_x, src_y;
> -   uint32_t src_w, src_h;
> -   uint32_t dest_x, dest_y;
> -   uint32_t dest_w, dest_h;
> +   struct wl_list link;
>
>  uint32_t formats[];
>   };
> @@ -921,6 +939,128 @@ drm_fb_unref(struct drm_fb *fb)
>   }
>
>   /**
> + * Allocate a new, empty, plane state.
> + */
> +static struct drm_plane_state *
> +drm_plane_state_alloc(struct drm_output_state *state_output,
> + struct drm_plane *plane)
> +{
> +   struct drm_plane_state *state = calloc(1, sizeof(*state));
> +
> +   assert(state);
> +   memset(state, 0, sizeof(*state));

Remove this memset (0-allocated memory)


> +   state->output_state = state_output;
> +   state->plane = plane;
> +
> +   /* Here we only add the plane state to the desired link, and not
> +* set the member. Having an output pointer set means that the
> +* plane will be displayed on the output; this won't be the case
> +* when we go to disable a plane. In this case, it must be part of
> +* the commit (and thus the output state), but the member must be
> +* NULL, as it will not be on any output when the state takes
> +* effect.
> +*/
> +   if (state_output)
> +   wl_list_insert(_output->plane_list, >link);
> +   else
> +   wl_list_init(>link);
> +
> +   return state;
> +}
> +
> +/**
> + * Duplicate an existing plane state into a new output state.
> + */
> +static struct drm_plane_state *
> +drm_plane_state_duplicate(struct drm_output_state *state_output,
> + struct drm_plane_state *src)
> +{
> +   struct drm_plane_state *dst = calloc(1, sizeof(*dst));

No need for 0-memory here (memcpy follows), so use malloc() instead of
calloc()

> +
> +   assert(src);
> +   assert(dst);
> +   memcpy(dst, src, sizeof(*dst));
> +   wl_list_insert(_output->plane_list, >link);
> +   if (src->fb)
> +   dst->fb = drm_fb_ref(src->fb);
> +   dst->output_state = state_output;
> +   dst->complete = false;
> +
> +   return dst;
> +}
> +
> +/**
> + * Free an existing plane state. As a special case, the state will not
> + * normally be freed if it is the current state; see drm_plane_set_state.
> + */
> +static void
> +drm_plane_state_free(struct drm_plane_state *state, bool force)
> +{
> +   if (!state)
> +   return;
> +
> +   wl_list_remove(>link);
> +   wl_list_init(>link);
> +   state->output_state = NULL;
> +
> +   if (force || state != state->plane->state_cur) {
> +   

Re: [PATCH weston v5 30/42] compositor-drm: Introduce drm_output_state structure

[Fabien DESSENNE]

Hi Daniel



On 11/16/2016 03:25 PM, Daniel Stone wrote:
> Currently this doesn't actually really do anything, but will be used in
> the future to track the state for both modeset and repaint requests.
> Completion of the request gives us a single request-completion path for
> both pageflip and vblank events.
>
> Signed-off-by: Daniel Stone 
>
> Differential Revision: https://phabricator.freedesktop.org/D1497
> ---
>   libweston/compositor-drm.c | 230 
> ++---
>   1 file changed, 197 insertions(+), 33 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index c24129b..1089d77 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -120,6 +120,19 @@ struct plane_properties {
>  uint64_t value;
>   };
>
> +/**
> + * Mode for drm_output_state_duplicate.
> + */
> +enum drm_output_state_duplicate_mode {
> +   DRM_OUTPUT_STATE_CLEAR_PLANES, /**< reset all planes to off */
> +   DRM_OUTPUT_STATE_PRESERVE_PLANES, /**< preserve plane state */
> +};
> +
> +enum drm_output_state_update_mode {
> +   DRM_OUTPUT_STATE_UPDATE_SYNCHRONOUS, /**< state already applied */
> +   DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS, /**< pending event delivery */
> +};
> +
>   struct drm_backend {
>  struct weston_backend base;
>  struct weston_compositor *compositor;
> @@ -206,6 +219,16 @@ struct drm_edid {
>   };
>
>   /**
> + * Output state holds the dynamic state for one Weston output, i.e. a KMS 
> CRTC,
> + * plus >= 1 each of encoder/connector/plane. Since everything but the planes
> + * is currently statically assigned per-output, we mainly use this to track
> + * plane state.
> + */
> +struct drm_output_state {
> +   struct drm_output *output;
> +};
> +
> +/**
>* A plane represents one buffer, positioned within a CRTC, and stacked
>* relative to other planes on the same CRTC.
>*
> @@ -274,6 +297,10 @@ struct drm_output {
>  struct drm_fb *fb_current, *fb_pending, *fb_last;
>  struct backlight *backlight;
>
> +   struct drm_output_state *state_last;
> +   struct drm_output_state *state_cur;
> +   struct drm_output_state *state_pending;
> +
>  struct drm_fb *dumb[2];
>  pixman_image_t *image[2];
>  int current_image;
> @@ -641,6 +668,9 @@ drm_output_set_cursor(struct drm_output *output);
>   static void
>   drm_output_update_msc(struct drm_output *output, unsigned int seq);
>
> +static void
> +drm_output_destroy(struct weston_output *output_base);
> +
>   static int
>   drm_plane_crtc_supported(struct drm_output *output, struct drm_plane *plane)
>   {
> @@ -890,6 +920,113 @@ drm_fb_unref(struct drm_fb *fb)
>  }
>   }
>
> +/**
> + * Allocate a new, empty drm_output_state. This should not generally be used
> + * in the repaint cycle; see drm_output_state_duplicate.
> + */
> +static struct drm_output_state *
> +drm_output_state_alloc(struct drm_output *output)
> +{
> +   struct drm_output_state *state = calloc(1, sizeof(*state));
> +
> +   state->output = output;
> +
> +   return state;
> +}
> +
> +/**
> + * Duplicate an existing drm_output_state into a new one. This is generally
> + * used during the repaint cycle, to capture the existing state of an output
> + * and modify it to create a new state to be used.
> + *
> + * The mode determines whether the output will be reset to an a blank state,
> + * or an exact mirror of the current state.
> + */
> +static struct drm_output_state *
> +drm_output_state_duplicate(struct drm_output_state *src,
> +  enum drm_output_state_duplicate_mode plane_mode)
> +{
> +   struct drm_output_state *dst = malloc(sizeof(*dst));
> +
> +   assert(dst);
> +   memcpy(dst, src, sizeof(*dst));
> +
> +   return dst;
> +}
> +
> +/**
> + * Free an unused drm_output_state.
> + */
> +static void
> +drm_output_state_free(struct drm_output_state *state)
> +{
> +   if (!state)
> +   return;
> +
> +   free(state);
> +}
> +
> +/**
> + * Mark a drm_output_state (the output's last state) as complete. This 
> handles
> + * any post-completion actions such as updating the repaint timer, disabling 
> the
> + * output, and finally freeing the state.
> + */
> +static void
> +drm_output_update_complete(struct drm_output *output, uint32_t flags,
> +  unsigned int sec, unsigned int usec)
> +{
> +   struct timespec ts;
> +
> +   drm_output_state_free(output->state_last);
> +   output->state_last = NULL;
> +
> +   if (output->destroy_pending) {
> +   drm_output_destroy(>base);
> +   goto out;
> +   }
> +   else if (output->disable_pending) {

Remove 'else'

> +   weston_output_disable(>base);
> +   goto out;
> +   }
> +
> +   ts.tv_sec = sec;
> +   ts.tv_nsec = usec * 1000;
> +   weston_output_finish_frame(>base, , 

Re: [PATCH weston v5 21/42] compositor-drm: Return FB directly from render

[Fabien DESSENNE]

Hi Daniel,


On 11/16/2016 03:25 PM, Daniel Stone wrote:
> Instead of setting state members directly in the drm_output_render
> functions (to paint using Pixman or GL), just return a drm_fb, and let
> the core function place it in state.
>
> Signed-off-by: Daniel Stone 
>
> Differential Revision: https://phabricator.freedesktop.org/D1419
> ---
>   libweston/compositor-drm.c | 30 +++---
>   1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 88a890e..ee66dbe 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -601,11 +601,12 @@ drm_output_prepare_scanout_view(struct drm_output 
> *output,
>   return >fb_plane;
>   }
>   
> -static void
> +static struct drm_fb *
>   drm_output_render_gl(struct drm_output *output, pixman_region32_t *damage)
>   {
>   struct drm_backend *b = to_drm_backend(output->base.compositor);
>   struct gbm_bo *bo;
> + struct drm_fb *ret;
>   
>   output->base.compositor->renderer->repaint_output(>base,
> damage);
> @@ -613,20 +614,21 @@ drm_output_render_gl(struct drm_output *output, 
> pixman_region32_t *damage)
>   bo = gbm_surface_lock_front_buffer(output->gbm_surface);
>   if (!bo) {
>   weston_log("failed to lock front buffer: %m\n");
> - return;
> + return NULL;
>   }
>   
> - output->fb_pending = drm_fb_get_from_bo(bo, b, output->gbm_format);
> - if (!output->fb_pending) {
> + ret = drm_fb_get_from_bo(bo, b, output->gbm_format);
> + if (!ret) {
>   weston_log("failed to get drm_fb for bo\n");
>   gbm_surface_release_buffer(output->gbm_surface, bo);
> - return;
return NULL missing
>   }
> - output->fb_pending->type = BUFFER_GBM_SURFACE;
> - output->fb_pending->gbm_surface = output->gbm_surface;
> + ret->type = BUFFER_GBM_SURFACE;
> + ret->gbm_surface = output->gbm_surface;
> +
> + return ret;
>   }
>   
> -static void
> +static struct drm_fb *
>   drm_output_render_pixman(struct drm_output *output, pixman_region32_t 
> *damage)
>   {
>   struct weston_compositor *ec = output->base.compositor;
> @@ -642,7 +644,6 @@ drm_output_render_pixman(struct drm_output *output, 
> pixman_region32_t *damage)
>   
>   output->current_image ^= 1;
>   
> - output->fb_pending = drm_fb_ref(output->dumb[output->current_image]);
>   pixman_renderer_output_set_buffer(>base,
> output->image[output->current_image]);
>   
> @@ -650,6 +651,8 @@ drm_output_render_pixman(struct drm_output *output, 
> pixman_region32_t *damage)
>   
>   pixman_region32_fini(_damage);
>   pixman_region32_fini(_damage);
> +
> + return drm_fb_ref(output->dumb[output->current_image]);
>   }
>   
>   static void
> @@ -657,6 +660,7 @@ drm_output_render(struct drm_output *output, 
> pixman_region32_t *damage)
>   {
>   struct weston_compositor *c = output->base.compositor;
>   struct drm_backend *b = to_drm_backend(c);
> + struct drm_fb *fb;
>   
>   /* If we already have a client buffer promoted to scanout, then we don't
>* want to render. */
> @@ -664,9 +668,13 @@ drm_output_render(struct drm_output *output, 
> pixman_region32_t *damage)
>   return;
>   
>   if (b->use_pixman)
> - drm_output_render_pixman(output, damage);
> + fb = drm_output_render_pixman(output, damage);
>   else
> - drm_output_render_gl(output, damage);
> + fb = drm_output_render_gl(output, damage);
> +
> + if (!fb)
> + return;
> + output->fb_pending = fb;
>   
>   pixman_region32_subtract(>primary_plane.damage,
>>primary_plane.damage, damage);
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v2 3/3] compositor-drm: Support linux_dmabuf output for sprite planes without gbm

[Fabien DESSENNE]

Hi,


Please find below a comment.


Fabien


On 09/30/2016 11:28 AM, Tomohito Esaki wrote:
> Multiplanar formats are supported by using drmModeAddFB2 and bypassing
> gbm. If drmModeAddFB2 isn't available, the existing gbm bo import path
> is used and multiplanar formats are unsupported.
>
> Signed-off-by: Tomohito Esaki 
> ---
>   libweston/compositor-drm.c | 53 
> +++---
>   1 file changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index b15fa01..f0e6f7c 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1008,11 +1008,9 @@ page_flip_handler(int fd, unsigned int frame,
>   
>   static uint32_t
>   drm_output_check_sprite_format(struct drm_sprite *s,
> -struct weston_view *ev, struct gbm_bo *bo)
> +struct weston_view *ev, uint32_t format)
>   {
> - uint32_t i, format;
> -
> - format = gbm_bo_get_format(bo);
> + uint32_t i;
>   
>   if (format == GBM_FORMAT_ARGB) {
>   pixman_region32_t r;
> @@ -1053,15 +1051,12 @@ drm_output_prepare_overlay_view(struct drm_output 
> *output,
>   struct drm_sprite *s;
>   struct linux_dmabuf_buffer *dmabuf;
>   int found = 0;
> - struct gbm_bo *bo;
> + struct gbm_bo *bo = NULL;
>   pixman_region32_t dest_rect, src_rect;
>   pixman_box32_t *box, tbox;
>   uint32_t format;
>   wl_fixed_t sx1, sy1, sx2, sy2;
>   
> - if (b->gbm == NULL)
> - return NULL;
> -
>   if (viewport->buffer.transform != output->base.transform)
>   return NULL;
>   
> @@ -1101,15 +1096,9 @@ drm_output_prepare_overlay_view(struct drm_output 
> *output,
>   if (!found)
>   return NULL;
>   
> - if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource))) {
> + if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource)) &&
> + b->no_addfb2 && b->gbm) {
>   #ifdef HAVE_GBM_FD_IMPORT
> - /* XXX: TODO:
> -  *
> -  * Use AddFB2 directly, do not go via GBM.
> -  * Add support for multiplanar formats.
> -  * Both require refactoring in the DRM-backend to
> -  * support a mix of gbm_bos and drmfbs.
> -  */
>   struct gbm_import_fd_data gbm_dmabuf = {
>   .fd = dmabuf->attributes.fd[0],
>   .width  = dmabuf->attributes.width,
> @@ -1126,22 +1115,32 @@ drm_output_prepare_overlay_view(struct drm_output 
> *output,
>   #else
>   return NULL;
>   #endif
> - } else {
> + } else if (b->gbm) {
>   bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
>  buffer_resource, GBM_BO_USE_SCANOUT);
>   }
> - if (!bo)
> - return NULL;
>   
> - format = drm_output_check_sprite_format(s, ev, bo);
> - if (format == 0) {
> - gbm_bo_destroy(bo);
> - return NULL;
> - }
> + if (bo) {
> + format = drm_output_check_sprite_format(
> + s, ev, gbm_bo_get_format(bo));
> + if (format == 0)
Unless I missed something, you shall call gbm_bo_destroy(bo) before 
returning.

> + return NULL;
> +
> + s->next = drm_fb_get_from_bo(bo, b, format);
> + if (!s->next) {
> + gbm_bo_destroy(bo);
> + return NULL;
> + }
> + } else if (dmabuf) {
> + format = drm_output_check_sprite_format(
> + s, ev, dmabuf->attributes.format);
> + if (format == 0)
> + return NULL;
>   
> - s->next = drm_fb_get_from_bo(bo, b, format);
> - if (!s->next) {
> - gbm_bo_destroy(bo);
> + s->next = drm_fb_create_dmabuf(dmabuf, b, format);
> + if (!s->next)
> + return NULL;
> + } else {
>   return NULL;
>   }
>   
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v2 2/3] compositor-drm: Add scanout support for linux_dmabuf buffers

[Fabien DESSENNE]

Hi


Thank you for the patch. Please, find some minor comments below.


On 09/30/2016 11:28 AM, Tomohito Esaki wrote:
> This implementations bypasses gbm and passes the dmabuf handles directly
> to libdrm for composition.

Maybe you can split the patch in two parts:
- one part dealing with drm_fb_create_dmabuf (since this part is used by 
both scanout and sprites)
- a second part dealing with scanout

>
> Signed-off-by: Tomohito Esaki 
> ---
>   libweston/compositor-drm.c | 125 
> ++---
>   1 file changed, 107 insertions(+), 18 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index a707fc4..b15fa01 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -151,6 +151,9 @@ struct drm_fb {
>   
>   /* Used by dumb fbs */
>   void *map;
> +
> + /* Used by dmabuf */
> + bool is_dmabuf;
>   };
>   
>   struct drm_edid {
> @@ -389,6 +392,76 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
>   drmIoctl(fd, DRM_IOCTL_MODE_DESTROY_DUMB, _arg);
>   }
>   
> +static inline void
> +close_drm_handle(int fd, uint32_t handle)
> +{
> + struct drm_gem_close gem_close = { .handle = handle };
> + int ret;
> +
> + ret = drmIoctl(fd, DRM_IOCTL_GEM_CLOSE, _close);
> + if (ret)
> + weston_log("DRM_IOCTL_GEM_CLOSE failed.(%s)\n",
> +strerror(errno));
> +}
> +
> +static struct drm_fb *
> +drm_fb_create_dmabuf(struct linux_dmabuf_buffer *dmabuf,
> +  struct drm_backend *backend, uint32_t format)
> +{
> + struct drm_fb *fb = NULL;
> + uint32_t width, height, fb_id, handles[4] = {0};
> + int i, ret;
> +
> + if (!format)
> + return NULL;
I do not think that this check is needed (you already checked format in 
the caller function + you can delegate this error handling to drmModeAddFB2)

> +
> + width = dmabuf->attributes.width;
> + height = dmabuf->attributes.height;
> + if (backend->min_width > width ||
> + width > backend->max_width ||
> + backend->min_height > height ||
> + height > backend->max_height)
> + return NULL;
Just like in drm_fb_get_from_bo(), add an error log "bo geometry out of 
bounds"

> +
> + for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> + ret = drmPrimeFDToHandle(backend->drm.fd,
> +  dmabuf->attributes.fd[i],
> +  [i]);
> + if (ret)
Add an error log here too (ex "drmPrimeFDToHandle failed")

> + goto done;
> + }
> +
> + ret = drmModeAddFB2(backend->drm.fd, width, height,
> + format, handles, dmabuf->attributes.stride,
> + dmabuf->attributes.offset, _id, 0);
Flags are ignore here (using "0"). Suggesting to use something like this:
 if (dmabuf->attributes.flags & 
ZWP_LINUX_BUFFER_PARAMS_V1_FLAGS_INTERLACED)
 flags |= DRM_MODE_FB_INTERLACED;

> + if (ret)
> + goto done;
Just like in drm_fb_get_from_bo(), add an error log  "addfb2 failed"

> +
> + fb = zalloc(sizeof *fb);
> + if (!fb)
> + goto done;
> +
> + fb->fb_id = fb_id;
> + fb->stride = dmabuf->attributes.stride[0];
> + fb->fd = backend->drm.fd;
> + fb->is_dmabuf = true;
> +
> +done:
> + for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> + if (!handles[i])
> + continue;
> + close_drm_handle(backend->drm.fd, handles[i]);
> + }
> +
> + return fb;
> +}
> +
> +static void
> +drm_fb_destroy_dmabuf(struct drm_fb *fb)
> +{
> + drm_fb_destroy(fb, fb->fd);
> +}
> +
>   static struct drm_fb *
>   drm_fb_get_from_bo(struct gbm_bo *bo,
>  struct drm_backend *backend, uint32_t format)
> @@ -475,6 +548,8 @@ drm_output_release_fb(struct drm_output *output, struct 
> drm_fb *fb)
>   if (fb->map &&
>   (fb != output->dumb[0] && fb != output->dumb[1])) {
>   drm_fb_destroy_dumb(fb);
> + } else if (fb->is_dmabuf) {
> + drm_fb_destroy_dmabuf(fb);
>   } else if (fb->bo) {
>   if (fb->is_client_buffer)
>   gbm_bo_destroy(fb->bo);
> @@ -486,12 +561,12 @@ drm_output_release_fb(struct drm_output *output, struct 
> drm_fb *fb)
>   
>   static uint32_t
>   drm_output_check_scanout_format(struct drm_output *output,
> - struct weston_surface *es, struct gbm_bo *bo)
> + struct weston_surface *es, uint32_t format)
>   {
> - uint32_t format;
>   pixman_region32_t r;
>   
> - format = gbm_bo_get_format(bo);
> + /* We relay on the GBM format enum and DRM format enum to be
> +identical */
>   
>   if (format == GBM_FORMAT_ARGB) {
>   /* We can scanout an ARGB buffer if the surface's
> @@ -521,12 +596,13 @@ 

Re: [PATCH weston v2 3/3] compositor-drm: Support linux_dmabuf output for sprite planes without gbm

[Fabien DESSENNE]

On 09/30/2016 08:49 PM, Derek Foreman wrote:
> On 30/09/16 04:28 AM, Tomohito Esaki wrote:
>> Multiplanar formats are supported by using drmModeAddFB2 and bypassing
>> gbm. If drmModeAddFB2 isn't available, the existing gbm bo import path
>> is used and multiplanar formats are unsupported.
> I'm not sure we should be doing anything with the existing sprite code
> at all (except perhaps removing it, though I'm sure that's an unpopular
> point of view :) since it's not really viable without atomic mode
> setting, and is currently disabled until someone uses a debug key to
> enable it.
>
> Pekka - I can't recall, is the atomic mode setting series going to build
> on the current sprite stuff or blow it away and start over?
>
> I'm wondering if we should just tear out the existing sprite code in the
> meantime...
>
> Thanks,
> Derek
Hi Derek,

I do not know all the history of Weston, and I am not aware of all the 
reasons that made the sprites "broken". I have always been told that it 
was not a good idea to enable sprites because it was not working as 
expected unless we have the atomic mode.

Anyway, with my limited background, i'd like to share my point of view:

Firstly, sprite works quite fine (with a bunch of fixes that I will be 
glad to share once sprites are accepted by all) : I can have up to 3 
sprites (inluding gstreamer video playback with linux-dmabuf) + cursor 
working smoothly on two parallel outputs (extended desktop) So, sprites 
are probably a bit broken, but not totally out of order. We just still 
need to improve it.

Secondly, it is a (very) long time now, that I am waiting for atomic. I 
discussed a bit about it recently with Pekka and my understanding is 
that because people are quite busy, atomic is not about to land in main 
in a short time.

So, for theses two reasons, I think that is a good idea to improve 
sprites now : let's keep them marked as broken for the time being.
The improvements proposed by Tomohito will serve as a basis for sprites 
in atomic (I can confirm that the atomic WIP branch does not blow away 
the sprite part, the prepare_overlay_view is still there)

For what it worth, I have recently played with the atomic WIP branch, 
ported it to 1.11, and enabled linux-dmabuf in sprites. Not 100% perfect 
but it works. I know that I am not the only one who did this job 
recently, so I guess that we shall not have so many problem to find 
people to contribute to the atomic revival (with unbroken sprites inside)

BR

Fabien, a sprite fan ;)

>> Signed-off-by: Tomohito Esaki 
>> ---
>>   libweston/compositor-drm.c | 53 
>> +++---
>>   1 file changed, 26 insertions(+), 27 deletions(-)
>>
>> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
>> index b15fa01..f0e6f7c 100644
>> --- a/libweston/compositor-drm.c
>> +++ b/libweston/compositor-drm.c
>> @@ -1008,11 +1008,9 @@ page_flip_handler(int fd, unsigned int frame,
>>
>>   static uint32_t
>>   drm_output_check_sprite_format(struct drm_sprite *s,
>> -   struct weston_view *ev, struct gbm_bo *bo)
>> +   struct weston_view *ev, uint32_t format)
>>   {
>> -uint32_t i, format;
>> -
>> -format = gbm_bo_get_format(bo);
>> +uint32_t i;
>>
>>  if (format == GBM_FORMAT_ARGB) {
>>  pixman_region32_t r;
>> @@ -1053,15 +1051,12 @@ drm_output_prepare_overlay_view(struct drm_output 
>> *output,
>>  struct drm_sprite *s;
>>  struct linux_dmabuf_buffer *dmabuf;
>>  int found = 0;
>> -struct gbm_bo *bo;
>> +struct gbm_bo *bo = NULL;
>>  pixman_region32_t dest_rect, src_rect;
>>  pixman_box32_t *box, tbox;
>>  uint32_t format;
>>  wl_fixed_t sx1, sy1, sx2, sy2;
>>
>> -if (b->gbm == NULL)
>> -return NULL;
>> -
>>  if (viewport->buffer.transform != output->base.transform)
>>  return NULL;
>>
>> @@ -1101,15 +1096,9 @@ drm_output_prepare_overlay_view(struct drm_output 
>> *output,
>>  if (!found)
>>  return NULL;
>>
>> -if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource))) {
>> +if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource)) &&
>> +b->no_addfb2 && b->gbm) {
>>   #ifdef HAVE_GBM_FD_IMPORT
>> -/* XXX: TODO:
>> - *
>> - * Use AddFB2 directly, do not go via GBM.
>> - * Add support for multiplanar formats.
>> - * Both require refactoring in the DRM-backend to
>> - * support a mix of gbm_bos and drmfbs.
>> - */
>>  struct gbm_import_fd_data gbm_dmabuf = {
>>  .fd = dmabuf->attributes.fd[0],
>>  .width  = dmabuf->attributes.width,
>> @@ -1126,22 +1115,32 @@ drm_output_prepare_overlay_view(struct drm_output 
>> *output,
>>   #else
>>  return NULL;
>>   #endif
>> -} else {
>> +} else if (b->gbm) {
>>  bo = gbm_bo_import(b->gbm, 

Re: [PATCH] gl-renderer: emit frame_signal after eglSwapBuffers

[Fabien DESSENNE]

On 08/17/2016 09:42 AM, Pekka Paalanen wrote:
> On Tue, 16 Aug 2016 14:46:23 +0200
> Fabien DESSENNE <fabien.desse...@st.com> wrote:
>
>> On 08/16/2016 10:18 AM, Pekka Paalanen wrote:
>>> On Fri, 12 Aug 2016 14:11:40 +0200
>>> Fabien Dessenne <fabien.desse...@st.com> wrote:
>>>   
>>>> Emit frame_signal at the end of the GL renderer processing, so the
>>>> frame_signal clients are informed after the buffer is actually updated.
>>>>
>>>> Signed-off-by: Fabien Dessenne <fabien.desse...@st.com>
>>>> ---
>>>>libweston/gl-renderer.c | 2 +-
>>>>1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c
>>>> index d624453..f4d4a5e 100644
>>>> --- a/libweston/gl-renderer.c
>>>> +++ b/libweston/gl-renderer.c
>>>> @@ -1138,7 +1138,6 @@ gl_renderer_repaint_output(struct weston_output 
>>>> *output,
>>>>draw_output_borders(output, border_damage);
>>>>
>>>>pixman_region32_copy(>previous_damage, output_damage);
>>>> -  wl_signal_emit(>frame_signal, output);
>>>>
>>>>if (gr->swap_buffers_with_damage) {
>>>>pixman_region32_init(_damage);
>>>> @@ -1185,6 +1184,7 @@ gl_renderer_repaint_output(struct weston_output 
>>>> *output,
>>>>}
>>>>
>>>>go->border_status = BORDER_STATUS_CLEAN;
>>>> +  wl_signal_emit(>frame_signal, output);
>>>>}
>>>>
>>>>static int
>>> Hi,
>>>
>>> I think this will break screenshooting.
>>>
>>> Screenshooter relies on glReadPixels, but if you issue eglSwapBuffers
>>> first, you no longer know what glReadPixels will return. Therefore the
>>> signal must be emitted before the swapbuffers.
>> Oh, yes, you are right, I missed this part.
>>> You forgot to explain what problem this change solves, too.
>> I am trying to improve the way to record the display since
>> glReadPixels() is quite slow: although it works fine for a one frame
>> capture, the performances are not acceptable for a continuous capture.
>>
>> So I am prototyping something between screenshooter and VAAPI recorder
>> but my problem is that my recorder is always one frame late.
>>
>> This issue was solved with my proposed patch, but I agree that it is not
>> acceptable.
>>
>> Now I come to the conclusion that VAAPI recorder has probably the same
>> 'one frame late' issue, which I unfortunately cannot verify, but reading
>> the code let me think that there is something wrong there:
>> drm_output_render_gl() calls:
>> - output->base.compositor->renderer->repaint_output()
>> - bo = gbm_surface_lock_front_buffer(output->gbm_surface)
>> - output->next = drm_fb_get_from_bo(bo, b, output->gbm_format)
>>
>> Then, "output->current" is updated with the fresh "output->next" at the
>> next page_flip_handler() call.
>>
>> And before that, VAAPI recorder captures "output->current" (see
>> recorder_frame_notify) just after repaint_output() is called (triggered
>> by frame_signal emit).
>> But at that time, "output->current" is not updated yet, it still refers
>> to the previous frame (it is updated at the next page_flip)
>>
>> Can you let me know your opinion about it?
> Hi,
>
> I didn't verify your reasoning, but it does sound plausible, yes. You
> might add a new 'post_frame_signal' perhaps for your purposes, or maybe
> something DRM-backend specific because of reliance on GBM.
Hi,

This is what I have in mind, I'll give it a try.
> I can also imagine a reason why you would actually want to be one frame
> late: might calling into VAAPI cause a compositor stall?
>
> If VAAPI was blocking somehow, being a frame late allows you avoid
> blocking for the client and composite rendering. It's sub-optimal, but
> I am not familiar with VAAPI. If it doesn't have that issue, then
> adding a new hook sounds fine.
I am not familiar with VAAPI, and have no idea whether it is blocking or 
not. But you are right, the priority must be given to the rendering, so 
capturing an outdated frame would be a good workaround if there is 
something blocking.

Thank you for your valuable comments.

Fabien
>
> Thanks,
> pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] gl-renderer: emit frame_signal after eglSwapBuffers

[Fabien DESSENNE]

On 08/16/2016 10:18 AM, Pekka Paalanen wrote:
> On Fri, 12 Aug 2016 14:11:40 +0200
> Fabien Dessenne <fabien.desse...@st.com> wrote:
>
>> Emit frame_signal at the end of the GL renderer processing, so the
>> frame_signal clients are informed after the buffer is actually updated.
>>
>> Signed-off-by: Fabien Dessenne <fabien.desse...@st.com>
>> ---
>>   libweston/gl-renderer.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c
>> index d624453..f4d4a5e 100644
>> --- a/libweston/gl-renderer.c
>> +++ b/libweston/gl-renderer.c
>> @@ -1138,7 +1138,6 @@ gl_renderer_repaint_output(struct weston_output 
>> *output,
>>  draw_output_borders(output, border_damage);
>>   
>>  pixman_region32_copy(>previous_damage, output_damage);
>> -wl_signal_emit(>frame_signal, output);
>>   
>>  if (gr->swap_buffers_with_damage) {
>>  pixman_region32_init(_damage);
>> @@ -1185,6 +1184,7 @@ gl_renderer_repaint_output(struct weston_output 
>> *output,
>>  }
>>   
>>  go->border_status = BORDER_STATUS_CLEAN;
>> +wl_signal_emit(>frame_signal, output);
>>   }
>>   
>>   static int
> Hi,
>
> I think this will break screenshooting.
>
> Screenshooter relies on glReadPixels, but if you issue eglSwapBuffers
> first, you no longer know what glReadPixels will return. Therefore the
> signal must be emitted before the swapbuffers.
Oh, yes, you are right, I missed this part.
> You forgot to explain what problem this change solves, too.
I am trying to improve the way to record the display since 
glReadPixels() is quite slow: although it works fine for a one frame 
capture, the performances are not acceptable for a continuous capture.

So I am prototyping something between screenshooter and VAAPI recorder 
but my problem is that my recorder is always one frame late.

This issue was solved with my proposed patch, but I agree that it is not 
acceptable.

Now I come to the conclusion that VAAPI recorder has probably the same 
'one frame late' issue, which I unfortunately cannot verify, but reading 
the code let me think that there is something wrong there:
drm_output_render_gl() calls:
- output->base.compositor->renderer->repaint_output()
- bo = gbm_surface_lock_front_buffer(output->gbm_surface)
- output->next = drm_fb_get_from_bo(bo, b, output->gbm_format)

Then, "output->current" is updated with the fresh "output->next" at the 
next page_flip_handler() call.

And before that, VAAPI recorder captures "output->current" (see 
recorder_frame_notify) just after repaint_output() is called (triggered 
by frame_signal emit).
But at that time, "output->current" is not updated yet, it still refers 
to the previous frame (it is updated at the next page_flip)

Can you let me know your opinion about it?
Fabien
> I only see it breaking things.
>
>
> Thanks,
> pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] gl-renderer: emit frame_signal after eglSwapBuffers

[Fabien Dessenne]

Emit frame_signal at the end of the GL renderer processing, so the
frame_signal clients are informed after the buffer is actually updated.

Signed-off-by: Fabien Dessenne <fabien.desse...@st.com>
---
 libweston/gl-renderer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c
index d624453..f4d4a5e 100644
--- a/libweston/gl-renderer.c
+++ b/libweston/gl-renderer.c
@@ -1138,7 +1138,6 @@ gl_renderer_repaint_output(struct weston_output *output,
draw_output_borders(output, border_damage);
 
pixman_region32_copy(>previous_damage, output_damage);
-   wl_signal_emit(>frame_signal, output);
 
if (gr->swap_buffers_with_damage) {
pixman_region32_init(_damage);
@@ -1185,6 +1184,7 @@ gl_renderer_repaint_output(struct weston_output *output,
}
 
go->border_status = BORDER_STATUS_CLEAN;
+   wl_signal_emit(>frame_signal, output);
 }
 
 static int
-- 
1.9.1

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

Re: Process for implementing a double buffer on Wayland

[Fabien DESSENNE]

>  > Date: Thu, 26 Nov 2015 10:40:20 +0200
>  > From: ppaala...@gmail.com
>  > To: rdp.eff...@gmail.com; mikeyj...@hotmail.com
>  > CC: wayland-devel@lists.freedesktop.org; dan...@fooishbar.org
>  > Subject: Re: Process for implementing a double buffer on Wayland
>  >
>  > On Wed, 25 Nov 2015 21:43:49 +0100
>  > Hardening  wrote:
>  >
>  > > Le 25/11/2015 17:18, Daniel Stone a écrit :
>  > > > Hi Mike,
>  > > >
>  > > > On 25 November 2015 at 16:06, Mike Johnson
>  wrote:
>  > > >> I've created 2 buffers of the same size (800x600 pixels). So I
> want the
>  > > >> input buffer to get filled off-screen, while the output buffer
> will show the
>  > > >> content on-screen.
>  > > >>
>  > > >> First of all what sort of content could be used to illustrate this
>  > > >> technique, and secondly, what mechanisms are available to:
>  > > >>
>  > > >> a) Notify that the input buffer is full
>  > > >> b) Copy the content to the output buffer so that it shows on-screen
>  > > >
>  > > > It's quite simple. wl_surface_attach(surf, buf) +
>  > > > wl_surface_commit(surf) will display 'buf' for that surface. At that
>  > > > point, the compositor owns that buffer, so you should stop drawing on
>  > > > it. When the compositor has finished with a buffer, it will send
> you a
>  > > > wl_buffer.release event. You can sync your paint clock to the
>  > > > compositor's repaint loop with wl_surface_frame.
>  > > >
>  > > > So, the normal workflow is:
>  > > > - create surface S, buffer A, buffer B
>  > > > - draw first frame into buffer A
>  > > > - call wl_surface_frame(S) + wl_surface_attach(S, A) +
>  > > > wl_surface_commit(S) + wl_display_flush()
>  > > > - go to sleep
>  > > > - receive completion for wl_surface_frame callback
>  > > > - draw second frame into buffer B
>  > > > - call wl_surface_frame(S) + wl_surface_attach(S, B) +
>  > > > wl_surface_commit(S) + wl_display_flush()
>  > > > - compositor now owns both buffers, so don't touch any
>  > > > - receive wl_buffer.release event for buffer A - now unused
>  > > > - receive completion for wl_surface_frame callback
>  > > > - draw third frame into buffer A
>  > > > - ...
>  > > >
>  > >
>  > > I may be wrong, but there's no guaranty that the compositor sends
>  > > wl_buffer.release event on buffer A. I think I have experimented this
>  > > when the renderer in weston is the pixman renderer.
>  > > IIRC I have been told on IRC that the compositor decides when the
> buffer
>  > > is not used, so you may not receive the release message immediately. I
>  > > have hit that kind of bug when coding libUWAC.
>  >
>  > You are definitely right if you don't commit buffer B.
>  >
>  > When you have committed buffer B, the compositor eventually will
>  > release buffer A if it had not already, but there are some exceptions
>  > too. One is on certain circumstances when using a sub-surface.
>  >
>  >
>  > Mike,
>  >
>  > essentially one should decouple the buffer management from the repaint
>  > cycle, like simple-shm.c does.
>  >
>  > The reply to wl_surface.frame tells you when it is appropriate to draw
>  > a new frame after the previous one. Whenever you decide to draw a new
>  > frame, check your buffer pool for available buffers (I mean a collection
>  > of wl_buffers, not wl_shm_pool). If none are available, create a new
>  > buffer to draw into, or in some cases you can wait for a buffer become
>  > available again.
>  >
>  > When a buffer is created, it is naturally available to be drawn into.
>  > Using wl_surface.attach and wl_surface.commit with the buffer makes it
>  > reserved by the server, unavailable, until you receive a
>  > wl_buffer.release event making it available again.
>  >
>  > When you follow these rules, you get adaptive buffering. You'll only
>  > ever use just one buffer if possible, and use more if necessary to
>  > achieve glitch-free output. Double-buffering in the client is not
>  > always mandatory to achieve the effect of double-buffering, because the
>  > server might be buffering too.
>  >
>  >
>  > Thanks,
>  > pq
>
> Hi Pekka,
>
> Thanks for your input.  If I understand you correctly, you recommend
> creating buffers dynamically in the event there aren't any available to
> use in the shm pool.  I can see how that would help the frame rate.
>
> Thanks again for your comments guys.  I have the info I need.
> Best regards
> Mike

Hi all,

I have also experienced this : the replaced buffer (A) is not 
immediately released after the new buffer (B) is on display.
I guess that the behavior depends on both the backend (eg DRM) and the 
renderer (GL, pixman)
In compositor-drm.c you can read this comment
  * Also, keep a reference when using the pixman renderer.
  * That makes it possible to do a seamless switch to the GL
  * renderer and since the pixman renderer keeps a reference
  * to the buffer anyway, there is no side effects.
I am not sure, but this may be be a reason why a buffer is kept longer 

RE: [PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Fabien DESSENNE]

Hi,
> -Original Message-
> From: Daniel Stone [mailto:dan...@fooishbar.org]
> Sent: lundi 2 novembre 2015 12:44
> To: Fabien DESSENNE
> Cc: Bryce Harrington; Giulio Camuffo; Vincent ABRIOU; Benjamin Gaignard;
> wayland-devel@lists.freedesktop.org
> Subject: Re: [PATCH] dmabuf: get supported dmabuf formats from
> compositor backend
> 
> Hi,
> 
> On 2 November 2015 at 09:39, Fabien DESSENNE <fabien.desse...@st.com>
> wrote:
> >> On 30 October 2015 at 00:27, Bryce Harrington
> <br...@osg.samsung.com>
> >> wrote:
> >> > I'd like to better understand what this is going to be used for,
> >> > before landing it.  Another R-b on this would be nice as well;
> >> > Giulio perhaps you could give this patch a review?
> >>
> >> Hm, to be honest I'd prefer this not land just now, or like this.
> >>
> >> dmabuf usage in compositor-drm is just an optimisation, where the
> >> fallback path is through gl-renderer. Anything gl-renderer doesn't
> >> support, we essentially can't display.
> >
> > Maybe this is not always true : I use a forked version of
> > compositor-drm (I plan to upstream some patches later) where the
> > dmabuf buffers are displayed by the DRM planes: these buffers are not
> used by gl-renderer.
> >
> > This is a typical optimization use case: video playback frames being
> > sent as dmabuf buffer to the compositor, and rendered without any copy
> > through a DRM plane.
> 
> Yeah, of course. In your confined usecase this will always be true, but in
> general we cannot rely on it being true: we will always need a fallback to gl-
> renderer. Since gl-renderer is the fallback, we need to limit the exposed
> formats to what it can display, for the general case.
> 
> > Now, back to the proposed patch: in my proposal building the list of
> > dmabuf formats is delegated to the backend, not built by the
> > compositor itself.
> > Not a big change and it does not solve the "EGL dmabuf formats" issue
> > but there are two improvements:
> > 1/ IMHO the supported formats are backend-dependent, so this patch
> > makes some sense here.
> > 2/ Depending on the backend implementation, the list of formats may be
> > successfully built: this is the case with the compositor I use as it
> > does not use gl-renderer for dmabuf buffers.
> 
> I'd say more renderer-dependent than backend-dependent, as below.
> (Again, talking about the _general_ upstream case, not the specific usecase
> you have in a closed - in the engineering rather than freedom sense -
> system.)
> 
> >> Those formats are what we need to send to the compositor (possibly
> >> with the intersection between gl-renderer and compositor-drm marked
> >> as preferred), but right now, as the comment notes, we lack a way to
> >> query this through EGL. This is being worked on though.
> >
> > Good news! Do you know if there is any schedule for this?
> 
> Unfortunately Khronos work is necessarily opaque, but it is being actively
> worked on and is a relatively straightforward change, so.
> 
> > By the way, I think that my proposed patch and this EGL future patch
> > are not incompatible: as explained, EGL is not the only dmabuf consumer:
> > DRM plane is another consumer. At the end the format list shall be
> > built by the backend, not by the compositor.
> 
> Yes, but the problem is that while there are a million different ways we can
> require EGL but not KMS (recording/screenshot, view transformation,
> partially-occluded buffer, etc etc), and if you end up in a situation where 
> you
> both require EGL, but also have a buffer you cannot display through EGL, the
> result is not pretty.
> 
> I don't think your patch is in any way wrong for your usecase, but
> unfortunately it is very much unsuitable for upstream, as it does break
> everything listed above.
> 
> My preference, as stated, would be to generate a list of formats/modifiers
> from the renderer (in practical terms, always EGL), then pass this list to the
> backend for filtering, which would essentially just be adding 'preferred' 
> flags
> to formats which could be pulled out to hardware planes.
> 
> Cheers,
> Daniel

OK, so let's wait for the EGL update and see how we can implement all of this.
My proposed patch can be abandoned.
BR,
Fabien
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

RE: [PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Fabien DESSENNE]

Hi,

> -Original Message-
> From: Daniel Stone [mailto:dan...@fooishbar.org]
> Sent: vendredi 30 octobre 2015 09:31
> To: Bryce Harrington
> Cc: Fabien DESSENNE; Giulio Camuffo; Vincent ABRIOU; Benjamin Gaignard;
> wayland-devel@lists.freedesktop.org
> Subject: Re: [PATCH] dmabuf: get supported dmabuf formats from
> compositor backend
> 
> Hi,
> 
> On 30 October 2015 at 00:27, Bryce Harrington <br...@osg.samsung.com>
> wrote:
> > On Tue, Oct 20, 2015 at 08:45:50AM -0700, Bryce Harrington wrote:
> >> On Tue, Oct 20, 2015 at 04:54:30PM +0200, Fabien Dessenne wrote:
> >> > Add the possibility for the compositor backend to provide with the
> >> > list of supported pixel formats for dmabuf-based buffers.
> >> > This information is used by linux_dmabuf to inform clients when
> binding.
> >> >
> >> > Signed-off-by: Fabien Dessenne <fabien.desse...@st.com>
> >>
> >> Reviewed-by <br...@osg.samsung.com>
> >>
> >> There's not a way to verify the list is getting sent across to the
> >> client is there?
> >
> > Also, patch no longer applies since e3c0d8af.
> >
> > I'd like to better understand what this is going to be used for,
> > before landing it.  Another R-b on this would be nice as well; Giulio
> > perhaps you could give this patch a review?
> 
> Hm, to be honest I'd prefer this not land just now, or like this.
> 
> dmabuf usage in compositor-drm is just an optimisation, where the fallback
> path is through gl-renderer. Anything gl-renderer doesn't support, we
> essentially can't display.

Maybe this is not always true : I use a forked version of compositor-drm
(I plan to upstream some patches later) where the dmabuf buffers are
displayed by the DRM planes: these buffers are not used by gl-renderer.

This is a typical optimization use case: video playback frames being sent
as dmabuf buffer to the compositor, and rendered without any copy
through a DRM plane.

Now, back to the proposed patch: in my proposal building the list of
dmabuf formats is delegated to the backend, not built by the compositor
itself.
Not a big change and it does not solve the "EGL dmabuf formats" issue
but there are two improvements:
1/ IMHO the supported formats are backend-dependent, so this patch
makes some sense here.
2/ Depending on the backend implementation, the list of formats may
be successfully built: this is the case with the compositor I use as it does
not use gl-renderer for dmabuf buffers.

> Those formats are what we need to send to the
> compositor (possibly with the intersection between gl-renderer and
> compositor-drm marked as preferred), but right now, as the comment notes,
> we lack a way to query this through EGL. This is being worked on though.

Good news! Do you know if there is any schedule for this?

By the way, I think that my proposed patch and this EGL future patch
are not incompatible: as explained, EGL is not the only dmabuf consumer:
DRM plane is another consumer. At the end the format list shall be built
by the backend, not by the compositor.

Please, let me know your feedback.

Fabien

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

[PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Fabien Dessenne]

Add the possibility for the compositor backend to provide with the list
of supported pixel formats for dmabuf-based buffers.
This information is used by linux_dmabuf to inform clients when binding.

Signed-off-by: Fabien Dessenne <fabien.desse...@st.com>
---
 src/compositor.h   |  2 ++
 src/linux-dmabuf.c | 16 +---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/compositor.h b/src/compositor.h
index 2e2a185..f58e0cc 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -613,6 +613,8 @@ enum weston_capability {
 struct weston_backend {
void (*destroy)(struct weston_compositor *ec);
void (*restore)(struct weston_compositor *ec);
+   void (*get_dmabuf_formats)(struct weston_compositor *ec,
+  struct wl_array *formats);
 };
 
 struct weston_compositor {
diff --git a/src/linux-dmabuf.c b/src/linux-dmabuf.c
index 90c9757..ee02ea4 100644
--- a/src/linux-dmabuf.c
+++ b/src/linux-dmabuf.c
@@ -433,9 +433,19 @@ bind_linux_dmabuf(struct wl_client *client,
wl_resource_set_implementation(resource, _dmabuf_implementation,
   compositor, NULL);
 
-   /* EGL_EXT_image_dma_buf_import does not provide a way to query the
-* supported pixel formats. */
-   /* XXX: send formats */
+   if (compositor->backend->get_dmabuf_formats) {
+   struct wl_array dmabuf_formats;
+   uint32_t *p;
+
+   wl_array_init(_formats);
+
+   compositor->backend->get_dmabuf_formats(compositor,
+   _formats);
+   wl_array_for_each(p, _formats)
+   zlinux_dmabuf_send_format(resource, *p);
+
+   wl_array_release(_formats);
+   }
 }
 
 /** Advertise linux_dmabuf support
-- 
1.9.1

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

RE: [PATCH] compositor-drm: consider the best mode of the mode_list as an option

[Fabien DESSENNE]

What is the next step to get this patch merged ?
I am asking because I am a new contributor and I am not really aware of the 
acceptance / merge process.
Fabien

 -Original Message-
 From: Fabien DESSENNE
 Sent: mardi 17 décembre 2013 10:01
 To: 'Bryce W. Harrington'
 Cc: wayland-devel@lists.freedesktop.org; Benjamin Gaignard
 Subject: RE: [PATCH] compositor-drm: consider the best mode of the
 mode_list as an option
 
  From: Bryce W. Harrington [mailto:b.harring...@samsung.com]
  Sent: mardi 17 décembre 2013 03:26
  To: Fabien DESSENNE
  Cc: wayland-devel@lists.freedesktop.org; Benjamin Gaignard
  Subject: Re: [PATCH] compositor-drm: consider the best mode of the
  mode_list as an option
 
  On Thu, Dec 12, 2013 at 05:13:56PM +0100, Fabien DESSENNE wrote:
   This patch fixes an issue where Weston using the DRM backend, cannot
   start the display. This happens in the following context:
   - no video mode is set before weston starts (eg no /dev/fb set up)
   - weston is not configured with any default video mode (nothing from
 weston.ini nor command line)
   - the DRM driver provides with a list of supported modes, but none
   of
  them
 is marked as PREFERRED (which is not a usual case, but it happens)
 
  Good point, I've seen such hardware myself.
 
   In that case, according to the current implementation, the DRM
   compositor fails to set a video mode.
   This fix lets the DRM compositor selects a video mode (the best one
   of the list, which is the first) from the ones provided by the driver.
 
  Is it always guaranteed to be the best, or is it just returning the
  list in the order stored in EDID?  Either way, picking the first in
  the list is probably sensible, however in the latter case I don't know
  that we can assume it's the 'best'.  Maybe the variable should be called
 'first'
  rather than 'best'?  But I'm just bikeshedding...
 
 Well, initially the variable was 'first', but I decided to change it to 
 'best'...
 Using 'first' may be confusing as the mode list is being parsed in the reverse
 order.
 The list provided by the DRM driver as implemented by the generic core part
 (drm_modes.c) is ordered according to the following rule:
 - first the preferred mode(s) as defined by the EDID
 - then from the highest to the lowest resolution So the first one is the best
 one.
 
 
   Signed-off-by: Fabien Dessenne fabien.desse...@st.com
 
  Reviewed-by: Bryce Harrington b.harring...@samsung.com
 
   ---
src/compositor-drm.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
  
   diff --git a/src/compositor-drm.c b/src/compositor-drm.c index
   fbf6e49..54caaa9 100644
   --- a/src/compositor-drm.c
   +++ b/src/compositor-drm.c
   @@ -1858,7 +1858,7 @@ create_output_for_connector(struct
  drm_compositor *ec,
 int x, int y, struct udev_device *drm_device)  {
 struct drm_output *output;
   - struct drm_mode *drm_mode, *next, *preferred, *current,
  *configured;
   + struct drm_mode *drm_mode, *next, *preferred, *current,
  *configured,
   +*best;
 struct weston_mode *m;
 struct weston_config_section *section;
 drmModeEncoder *encoder;
   @@ -1961,6 +1961,7 @@ create_output_for_connector(struct
  drm_compositor *ec,
 preferred = NULL;
 current = NULL;
 configured = NULL;
   + best = NULL;
  
 wl_list_for_each_reverse(drm_mode, output-base.mode_list,
  base.link) {
 if (config == OUTPUT_CONFIG_MODE  @@ -1971,6
  +1972,7 @@
   create_output_for_connector(struct drm_compositor *ec,
 current = drm_mode;
 if (drm_mode-base.flags 
  WL_OUTPUT_MODE_PREFERRED)
 preferred = drm_mode;
   + best = drm_mode;
 }
  
 if (config == OUTPUT_CONFIG_MODELINE) { @@ -1996,6 +1998,8
  @@
   create_output_for_connector(struct drm_compositor *ec,
 output-base.current_mode = preferred-base;
 else if (current)
 output-base.current_mode = current-base;
   + else if (best)
   + output-base.current_mode = best-base;
  
 if (output-base.current_mode == NULL) {
 weston_log(no available modes for %s\n, output-
 base.name);
   --
   1.7.9.5
  
   ___
   wayland-devel mailing list
   wayland-devel@lists.freedesktop.org
   http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

RE: [PATCH] compositor-drm: consider the best mode of the mode_list as an option

[Fabien DESSENNE]

 From: Bryce W. Harrington [mailto:b.harring...@samsung.com]
 Sent: mardi 17 décembre 2013 03:26
 To: Fabien DESSENNE
 Cc: wayland-devel@lists.freedesktop.org; Benjamin Gaignard
 Subject: Re: [PATCH] compositor-drm: consider the best mode of the
 mode_list as an option
 
 On Thu, Dec 12, 2013 at 05:13:56PM +0100, Fabien DESSENNE wrote:
  This patch fixes an issue where Weston using the DRM backend, cannot
  start the display. This happens in the following context:
  - no video mode is set before weston starts (eg no /dev/fb set up)
  - weston is not configured with any default video mode (nothing from
weston.ini nor command line)
  - the DRM driver provides with a list of supported modes, but none of
 them
is marked as PREFERRED (which is not a usual case, but it happens)
 
 Good point, I've seen such hardware myself.
 
  In that case, according to the current implementation, the DRM
  compositor fails to set a video mode.
  This fix lets the DRM compositor selects a video mode (the best one of
  the list, which is the first) from the ones provided by the driver.
 
 Is it always guaranteed to be the best, or is it just returning the list in 
 the
 order stored in EDID?  Either way, picking the first in the list is probably
 sensible, however in the latter case I don't know that we can assume it's the
 'best'.  Maybe the variable should be called 'first'
 rather than 'best'?  But I'm just bikeshedding...

Well, initially the variable was 'first', but I decided to change it to 
'best'...
Using 'first' may be confusing as the mode list is being parsed in the reverse 
order.
The list provided by the DRM driver as implemented by the generic core part 
(drm_modes.c) is ordered according to the following rule:
- first the preferred mode(s) as defined by the EDID
- then from the highest to the lowest resolution
So the first one is the best one.

 
  Signed-off-by: Fabien Dessenne fabien.desse...@st.com
 
 Reviewed-by: Bryce Harrington b.harring...@samsung.com
 
  ---
   src/compositor-drm.c |6 +-
   1 file changed, 5 insertions(+), 1 deletion(-)
 
  diff --git a/src/compositor-drm.c b/src/compositor-drm.c index
  fbf6e49..54caaa9 100644
  --- a/src/compositor-drm.c
  +++ b/src/compositor-drm.c
  @@ -1858,7 +1858,7 @@ create_output_for_connector(struct
 drm_compositor *ec,
  int x, int y, struct udev_device *drm_device)  {
  struct drm_output *output;
  -   struct drm_mode *drm_mode, *next, *preferred, *current,
 *configured;
  +   struct drm_mode *drm_mode, *next, *preferred, *current,
 *configured,
  +*best;
  struct weston_mode *m;
  struct weston_config_section *section;
  drmModeEncoder *encoder;
  @@ -1961,6 +1961,7 @@ create_output_for_connector(struct
 drm_compositor *ec,
  preferred = NULL;
  current = NULL;
  configured = NULL;
  +   best = NULL;
 
  wl_list_for_each_reverse(drm_mode, output-base.mode_list,
 base.link) {
  if (config == OUTPUT_CONFIG_MODE  @@ -1971,6
 +1972,7 @@
  create_output_for_connector(struct drm_compositor *ec,
  current = drm_mode;
  if (drm_mode-base.flags 
 WL_OUTPUT_MODE_PREFERRED)
  preferred = drm_mode;
  +   best = drm_mode;
  }
 
  if (config == OUTPUT_CONFIG_MODELINE) { @@ -1996,6 +1998,8
 @@
  create_output_for_connector(struct drm_compositor *ec,
  output-base.current_mode = preferred-base;
  else if (current)
  output-base.current_mode = current-base;
  +   else if (best)
  +   output-base.current_mode = best-base;
 
  if (output-base.current_mode == NULL) {
  weston_log(no available modes for %s\n, output-
 base.name);
  --
  1.7.9.5
 
  ___
  wayland-devel mailing list
  wayland-devel@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] compositor-drm: consider the best mode of the mode_list as an option

[Fabien DESSENNE]

This patch fixes an issue where Weston using the DRM backend, cannot start
the display. This happens in the following context:
- no video mode is set before weston starts (eg no /dev/fb set up)
- weston is not configured with any default video mode (nothing from
  weston.ini nor command line)
- the DRM driver provides with a list of supported modes, but none of them
  is marked as PREFERRED (which is not a usual case, but it happens)
In that case, according to the current implementation, the DRM compositor
fails to set a video mode.
This fix lets the DRM compositor selects a video mode (the best one of the
list, which is the first) from the ones provided by the driver.

Signed-off-by: Fabien Dessenne fabien.desse...@st.com
---
 src/compositor-drm.c |6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/compositor-drm.c b/src/compositor-drm.c
index fbf6e49..54caaa9 100644
--- a/src/compositor-drm.c
+++ b/src/compositor-drm.c
@@ -1858,7 +1858,7 @@ create_output_for_connector(struct drm_compositor *ec,
int x, int y, struct udev_device *drm_device)
 {
struct drm_output *output;
-   struct drm_mode *drm_mode, *next, *preferred, *current, *configured;
+   struct drm_mode *drm_mode, *next, *preferred, *current, *configured, 
*best;
struct weston_mode *m;
struct weston_config_section *section;
drmModeEncoder *encoder;
@@ -1961,6 +1961,7 @@ create_output_for_connector(struct drm_compositor *ec,
preferred = NULL;
current = NULL;
configured = NULL;
+   best = NULL;
 
wl_list_for_each_reverse(drm_mode, output-base.mode_list, base.link) {
if (config == OUTPUT_CONFIG_MODE 
@@ -1971,6 +1972,7 @@ create_output_for_connector(struct drm_compositor *ec,
current = drm_mode;
if (drm_mode-base.flags  WL_OUTPUT_MODE_PREFERRED)
preferred = drm_mode;
+   best = drm_mode;
}
 
if (config == OUTPUT_CONFIG_MODELINE) {
@@ -1996,6 +1998,8 @@ create_output_for_connector(struct drm_compositor *ec,
output-base.current_mode = preferred-base;
else if (current)
output-base.current_mode = current-base;
+   else if (best)
+   output-base.current_mode = best-base;
 
if (output-base.current_mode == NULL) {
weston_log(no available modes for %s\n, output-base.name);
-- 
1.7.9.5

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