Re: [PATCH weston 17/68] compositor-drm: Refactor destroy drm_fb function

[Daniel Stone]

Hi,

On 21 February 2017 at 13:58, Pekka Paalanen  wrote:
> On Fri,  9 Dec 2016 19:57:32 + Daniel Stone  wrote:
>> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
>> index af43a15..7dbfc6b 100644
>> --- a/libweston/compositor-drm.c
>> +++ b/libweston/compositor-drm.c
>> @@ -251,16 +251,39 @@ drm_sprite_crtc_supported(struct drm_output *output, 
>> struct drm_sprite *sprite)
>>  }
>>
>>  static void
>> -drm_fb_destroy_callback(struct gbm_bo *bo, void *data)
>> +drm_fb_destroy(struct drm_fb *fb)
>>  {
>> - struct drm_fb *fb = data;
>> + drmModeRmFB(fb->fd, fb->fb_id);
>
> fb_id cannot be zero, even thought earlier there was a check. Ok.
>
>> + weston_buffer_reference(>buffer_ref, NULL);
>> + free(fb);
>> +}
>> +
>> +static void
>> +drm_fb_destroy_dumb(struct drm_fb *fb)
>> +{
>> + struct drm_mode_destroy_dumb destroy_arg;
>>
>> - if (fb->fb_id)
>> - drmModeRmFB(fb->fd, fb->fb_id);
>> + if (!fb)
>> + return;
>
> Silent acceptance of NULL argument, really?

I think this was just misreading the callsites, thinking that some
called it with possibly-NULL. Inspection shows that to be incorrect,
so, fixed.

>> - weston_buffer_reference(>buffer_ref, NULL);
>> + assert(fb->type == BUFFER_PIXMAN_DUMB);
>
> fb->map cannot be NULL, even thought earlier there was a check. Ok.

Both that and fb_id fixed; the error would've been swallowed either
way, but better not to generate errors in the first.

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

Re: [PATCH weston 17/68] compositor-drm: Refactor destroy drm_fb function

[Pekka Paalanen]

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

> From: Tomohito Esaki 
> 
> The drm_fb destroy callback to mostly the same thing regardless of
> whether the buffer is a dumb buffer or gbm buffer. This patch refactors
> the common parts into a new function that can be called for both cases.
> 
> [daniels: Rebased on top of fb->fd changes, cosmetic changes.]
> 
> Differential Revision: https://phabricator.freedesktop.org/D1489
> 
> Signed-off-by: Tomohito Esaki 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 61 
> +++---
>  1 file changed, 30 insertions(+), 31 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index af43a15..7dbfc6b 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -251,16 +251,39 @@ drm_sprite_crtc_supported(struct drm_output *output, 
> struct drm_sprite *sprite)
>  }
>  
>  static void
> -drm_fb_destroy_callback(struct gbm_bo *bo, void *data)
> +drm_fb_destroy(struct drm_fb *fb)
>  {
> - struct drm_fb *fb = data;
> + drmModeRmFB(fb->fd, fb->fb_id);

fb_id cannot be zero, even thought earlier there was a check. Ok.

> + weston_buffer_reference(>buffer_ref, NULL);
> + free(fb);
> +}
> +
> +static void
> +drm_fb_destroy_dumb(struct drm_fb *fb)
> +{
> + struct drm_mode_destroy_dumb destroy_arg;
>  
> - if (fb->fb_id)
> - drmModeRmFB(fb->fd, fb->fb_id);
> + if (!fb)
> + return;

Silent acceptance of NULL argument, really?

>  
> - weston_buffer_reference(>buffer_ref, NULL);
> + assert(fb->type == BUFFER_PIXMAN_DUMB);
> +

fb->map cannot be NULL, even thought earlier there was a check. Ok.

> + munmap(fb->map, fb->size);
> +
> + memset(_arg, 0, sizeof(destroy_arg));
> + destroy_arg.handle = fb->handle;
> + drmIoctl(fb->fd, DRM_IOCTL_MODE_DESTROY_DUMB, _arg);
> +
> + drm_fb_destroy(fb);
> +}

With that one nit of silent acceptance fixed:
Reviewed-by: Pekka Paalanen 


Thanks,
pq


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