Re: [PATCH weston 15/68] compositor-drm: Add explicit type member to drm_fb

[Daniel Stone]

Hi Pekka,
Thanks for the thoughtful review, as ever.

On 22 February 2017 at 13:45, Pekka Paalanen  wrote:
> On Tue, 21 Feb 2017 15:29:09 +0200 Pekka Paalanen  wrote:
>> On Fri,  9 Dec 2016 19:57:30 + Daniel Stone  
>> wrote:
>> > @@ -452,15 +461,19 @@ drm_output_release_fb(struct drm_output *output, 
>> > struct drm_fb *fb)
>> > if (!fb)
>> > return;
>> >
>> > -   if (fb->map &&
>> > -(fb != output->dumb[0] && fb != output->dumb[1])) {
>> > -   drm_fb_destroy_dumb(fb);
>>
>> This piece sent me into a recursive "well, actually..." loop.
>>
>> It looked like dead code at first hand, as this gets hit when
>> output->dumb and fb don't match. When would that be? I would guess when
>> video mode changed.
>>
>> I think changing resolutions would hit this path, when flipping to a
>> new dumb buffer of a different size than one coming out of scanout
>> which cannot be destroyed until pageflip completed.
>>
>> Except I am wrong in a couple of ways: destroying the buffer is fine,
>> the kernel will keep it referenced as long as necessary anyway. And,
>> drm_output_switch_mode() does destroy everything immediately.
>>
>> So this bit is ok. Unless there is a third well-actually.
>
> And there is it:
>
> < MrCooper> pq: IME KMS framebuffers aren't reference-counted in the
> kernel; destroying an fb which is still being scanned out by any CRTCs
> turns off those CRTCs
>
> Is it then so, that drm_output_switch_mode() is wrong to destroy the FB
> immediately before even flipping a new one, but as it does so, there is
> no leak introduced by removing the above drm_fb_destroy_dumb() call?
>
> I.e. the patch does not regress anything because of a bug elsewhere?
>
> If that is so, my R-b stands, as fixing switch_mode is a whole another
> matter. Also the refcounting introduced later in this series offer a
> much better basis for fixing switch_mode.

Correct on all counts, especially on switch_mode. ;) I wrote up my
findings but decided not to conflate that and atomic:
https://phabricator.freedesktop.org/T7621 - if anyone else wants to
pursue that, I'm more than happy to help out and talk you through it.
I think it'd be a really interesting task to attack, and it'd
definitely be really helpful.

Michel is indeed right that calling drmModeRmFB will disable any CRTC
currently scanning out of that buffer. I don't think your analysis is
quite correct though: drm_output_release_fb() gets called from
switch_mode, but _before_ the drm_output_fini_pixman() ->
drm_output_init_pixman() cycle, which is what will set new
output->dumb pointers. So I believe this call was a no-op. Even if it
wasn't a no-op, drm_output_fini_pixman() will destroy those buffers
anyway; if the release_fb() call _wasn't_ a no-op, then
drm_output_fini_pixman() would crash because you'd destroy the buffers
twice. Great.

tl;dr: I think it's fine, you think it's fine, so I'll take the
asserts and your R-b, thankyou ;)

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

Re: [PATCH weston 15/68] compositor-drm: Add explicit type member to drm_fb

[Pekka Paalanen]

On Tue, 21 Feb 2017 15:29:09 +0200
Pekka Paalanen  wrote:

> On Fri,  9 Dec 2016 19:57:30 +
> Daniel Stone  wrote:
> 
> > Rather than magically trying to infer what the buffer is and what we
> > should do with it when we go to destroy it, add an explicit type
> > instead.
> > 
> > Differential Revision: https://phabricator.freedesktop.org/D1488
> > 
> > Signed-off-by: Daniel Stone 
> > ---
> >  libweston/compositor-drm.c | 50 
> > +-
> >  1 file changed, 32 insertions(+), 18 deletions(-)

> > weston_buffer_reference(>buffer_ref, buffer);
> >  }
> >  
> > @@ -452,15 +461,19 @@ drm_output_release_fb(struct drm_output *output, 
> > struct drm_fb *fb)
> > if (!fb)
> > return;
> >  
> > -   if (fb->map &&
> > -(fb != output->dumb[0] && fb != output->dumb[1])) {
> > -   drm_fb_destroy_dumb(fb);  
> 
> This piece sent me into a recursive "well, actually..." loop.
> 
> It looked like dead code at first hand, as this gets hit when
> output->dumb and fb don't match. When would that be? I would guess when
> video mode changed.
> 
> I think changing resolutions would hit this path, when flipping to a
> new dumb buffer of a different size than one coming out of scanout
> which cannot be destroyed until pageflip completed.
> 
> Except I am wrong in a couple of ways: destroying the buffer is fine,
> the kernel will keep it referenced as long as necessary anyway. And,
> drm_output_switch_mode() does destroy everything immediately.
> 
> So this bit is ok. Unless there is a third well-actually.

And there is it:

< MrCooper> pq: IME KMS framebuffers aren't reference-counted in the
kernel; destroying an fb which is still being scanned out by any CRTCs
turns off those CRTCs

Is it then so, that drm_output_switch_mode() is wrong to destroy the FB
immediately before even flipping a new one, but as it does so, there is
no leak introduced by removing the above drm_fb_destroy_dumb() call?

I.e. the patch does not regress anything because of a bug elsewhere?

If that is so, my R-b stands, as fixing switch_mode is a whole another
matter. Also the refcounting introduced later in this series offer a
much better basis for fixing switch_mode.


Thanks,
pq


pgprcatV6EflH.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 15/68] compositor-drm: Add explicit type member to drm_fb

[Pekka Paalanen]

On Fri,  9 Dec 2016 19:57:30 +
Daniel Stone  wrote:

> Rather than magically trying to infer what the buffer is and what we
> should do with it when we go to destroy it, add an explicit type
> instead.
> 
> Differential Revision: https://phabricator.freedesktop.org/D1488
> 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 50 
> +-
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 4ef7343..217db32 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -129,11 +129,19 @@ struct drm_mode {
>   drmModeModeInfo mode_info;
>  };
>  
> +enum drm_fb_type {
> + BUFFER_INVALID = 0, /**< never used */
> + BUFFER_CLIENT, /**< directly sourced from client */
> + BUFFER_PIXMAN_DUMB, /**< internal Pixman rendering */
> + BUFFER_GBM_SURFACE, /**< internal EGL rendering */
> +};

Hi,

cool.

> +
>  struct drm_fb {
> + enum drm_fb_type type;
> +
>   uint32_t fb_id, stride, handle, size;
>   int width, height;
>   int fd;
> - int is_client_buffer;
>   struct weston_buffer_reference buffer_ref;
>  
>   /* Used by gbm fbs */
> @@ -290,6 +298,7 @@ drm_fb_create_dumb(struct drm_backend *b, int width, int 
> height,
>   if (ret)
>   goto err_fb;
>  
> + fb->type = BUFFER_PIXMAN_DUMB;
>   fb->handle = create_arg.handle;
>   fb->stride = create_arg.pitch;
>   fb->size = create_arg.size;
> @@ -352,6 +361,8 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
>  {
>   struct drm_mode_destroy_dumb destroy_arg;
>  
> + assert(fb->type == BUFFER_PIXMAN_DUMB);
> +
>   if (!fb->map)
>   return;
>  
> @@ -370,8 +381,8 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
>  }
>  
>  static struct drm_fb *
> -drm_fb_get_from_bo(struct gbm_bo *bo,
> -struct drm_backend *backend, uint32_t format)
> +drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_backend *backend,
> +uint32_t format, enum drm_fb_type type)
>  {
>   struct drm_fb *fb = gbm_bo_get_user_data(bo);
>   uint32_t handles[4] = { 0 }, pitches[4] = { 0 }, offsets[4] = { 0 };

For the shortcut return:
assert(type == fb->type)?

> @@ -384,6 +395,7 @@ drm_fb_get_from_bo(struct gbm_bo *bo,
>   if (fb == NULL)
>   return NULL;
>  
> + fb->type = type;
>   fb->bo = bo;
>  
>   fb->width = gbm_bo_get_width(bo);
> @@ -440,9 +452,6 @@ static void
>  drm_fb_set_buffer(struct drm_fb *fb, struct weston_buffer *buffer)
>  {
>   assert(fb->buffer_ref.buffer == NULL);
> -
> - fb->is_client_buffer = 1;
> -

assert(fb->type == BUFFER_CLIENT)?

>   weston_buffer_reference(>buffer_ref, buffer);
>  }
>  
> @@ -452,15 +461,19 @@ drm_output_release_fb(struct drm_output *output, struct 
> drm_fb *fb)
>   if (!fb)
>   return;
>  
> - if (fb->map &&
> -(fb != output->dumb[0] && fb != output->dumb[1])) {
> - drm_fb_destroy_dumb(fb);

This piece sent me into a recursive "well, actually..." loop.

It looked like dead code at first hand, as this gets hit when
output->dumb and fb don't match. When would that be? I would guess when
video mode changed.

I think changing resolutions would hit this path, when flipping to a
new dumb buffer of a different size than one coming out of scanout
which cannot be destroyed until pageflip completed.

Except I am wrong in a couple of ways: destroying the buffer is fine,
the kernel will keep it referenced as long as necessary anyway. And,
drm_output_switch_mode() does destroy everything immediately.

So this bit is ok. Unless there is a third well-actually.

> - } else if (fb->bo) {
> - if (fb->is_client_buffer)
> - gbm_bo_destroy(fb->bo);
> - else
> - gbm_surface_release_buffer(output->gbm_surface,
> -fb->bo);
> + switch (fb->type) {
> + case BUFFER_PIXMAN_DUMB:
> + /* nothing: pixman buffers are destroyed manually */
> + break;
> + case BUFFER_CLIENT:
> + gbm_bo_destroy(fb->bo);
> + break;
> + case BUFFER_GBM_SURFACE:
> + gbm_surface_release_buffer(output->gbm_surface, fb->bo);
> + break;
> + default:
> + assert(NULL);
> + break;
>   }
>  }
>  
> @@ -559,7 +572,7 @@ drm_output_prepare_scanout_view(struct drm_output *output,
>   return NULL;
>   }
>  
> - output->next = drm_fb_get_from_bo(bo, b, format);
> + output->next = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT);
>   if (!output->next) {
>   gbm_bo_destroy(bo);
>   return NULL;
> @@ -585,7 +598,8 @@ drm_output_render_gl(struct drm_output *output, 
> pixman_region32_t *damage)
>   return;
>   }
>  

[PATCH weston 15/68] compositor-drm: Add explicit type member to drm_fb

[Daniel Stone]

Rather than magically trying to infer what the buffer is and what we
should do with it when we go to destroy it, add an explicit type
instead.

Differential Revision: https://phabricator.freedesktop.org/D1488

Signed-off-by: Daniel Stone 
---
 libweston/compositor-drm.c | 50 +-
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 4ef7343..217db32 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -129,11 +129,19 @@ struct drm_mode {
drmModeModeInfo mode_info;
 };
 
+enum drm_fb_type {
+   BUFFER_INVALID = 0, /**< never used */
+   BUFFER_CLIENT, /**< directly sourced from client */
+   BUFFER_PIXMAN_DUMB, /**< internal Pixman rendering */
+   BUFFER_GBM_SURFACE, /**< internal EGL rendering */
+};
+
 struct drm_fb {
+   enum drm_fb_type type;
+
uint32_t fb_id, stride, handle, size;
int width, height;
int fd;
-   int is_client_buffer;
struct weston_buffer_reference buffer_ref;
 
/* Used by gbm fbs */
@@ -290,6 +298,7 @@ drm_fb_create_dumb(struct drm_backend *b, int width, int 
height,
if (ret)
goto err_fb;
 
+   fb->type = BUFFER_PIXMAN_DUMB;
fb->handle = create_arg.handle;
fb->stride = create_arg.pitch;
fb->size = create_arg.size;
@@ -352,6 +361,8 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
 {
struct drm_mode_destroy_dumb destroy_arg;
 
+   assert(fb->type == BUFFER_PIXMAN_DUMB);
+
if (!fb->map)
return;
 
@@ -370,8 +381,8 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
 }
 
 static struct drm_fb *
-drm_fb_get_from_bo(struct gbm_bo *bo,
-  struct drm_backend *backend, uint32_t format)
+drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_backend *backend,
+  uint32_t format, enum drm_fb_type type)
 {
struct drm_fb *fb = gbm_bo_get_user_data(bo);
uint32_t handles[4] = { 0 }, pitches[4] = { 0 }, offsets[4] = { 0 };
@@ -384,6 +395,7 @@ drm_fb_get_from_bo(struct gbm_bo *bo,
if (fb == NULL)
return NULL;
 
+   fb->type = type;
fb->bo = bo;
 
fb->width = gbm_bo_get_width(bo);
@@ -440,9 +452,6 @@ static void
 drm_fb_set_buffer(struct drm_fb *fb, struct weston_buffer *buffer)
 {
assert(fb->buffer_ref.buffer == NULL);
-
-   fb->is_client_buffer = 1;
-
weston_buffer_reference(>buffer_ref, buffer);
 }
 
@@ -452,15 +461,19 @@ drm_output_release_fb(struct drm_output *output, struct 
drm_fb *fb)
if (!fb)
return;
 
-   if (fb->map &&
-(fb != output->dumb[0] && fb != output->dumb[1])) {
-   drm_fb_destroy_dumb(fb);
-   } else if (fb->bo) {
-   if (fb->is_client_buffer)
-   gbm_bo_destroy(fb->bo);
-   else
-   gbm_surface_release_buffer(output->gbm_surface,
-  fb->bo);
+   switch (fb->type) {
+   case BUFFER_PIXMAN_DUMB:
+   /* nothing: pixman buffers are destroyed manually */
+   break;
+   case BUFFER_CLIENT:
+   gbm_bo_destroy(fb->bo);
+   break;
+   case BUFFER_GBM_SURFACE:
+   gbm_surface_release_buffer(output->gbm_surface, fb->bo);
+   break;
+   default:
+   assert(NULL);
+   break;
}
 }
 
@@ -559,7 +572,7 @@ drm_output_prepare_scanout_view(struct drm_output *output,
return NULL;
}
 
-   output->next = drm_fb_get_from_bo(bo, b, format);
+   output->next = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT);
if (!output->next) {
gbm_bo_destroy(bo);
return NULL;
@@ -585,7 +598,8 @@ drm_output_render_gl(struct drm_output *output, 
pixman_region32_t *damage)
return;
}
 
-   output->next = drm_fb_get_from_bo(bo, b, output->gbm_format);
+   output->next = drm_fb_get_from_bo(bo, b, output->gbm_format,
+ BUFFER_GBM_SURFACE);
if (!output->next) {
weston_log("failed to get drm_fb for bo\n");
gbm_surface_release_buffer(output->gbm_surface, bo);
@@ -1054,7 +1068,7 @@ drm_output_prepare_overlay_view(struct drm_output *output,
return NULL;
}
 
-   s->next = drm_fb_get_from_bo(bo, b, format);
+   s->next = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT);
if (!s->next) {
gbm_bo_destroy(bo);
return NULL;
-- 
2.9.3

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