[PATCH weston 21/68] compositor-drm: Use drm_fb for cursor buffers

[Daniel Stone]

Hi,

On 22 February 2017 at 14:08, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Fri,  9 Dec 2016 19:57:36 +0000 Daniel Stone <daniels at collabora.com> wrote:
> looks like this patch causes an FB to be created from cursor bo-s, and
> this was not done before and the FBs are not used yet. I think this
> requires an explanation in the commit message: what happens and why it
> will be useful later.
>
> I assume the purpose is to use them with universal planes, right?

Exactly right; added.

>> @@ -183,11 +184,12 @@ struct drm_output {
>>       int disable_pending;
>>
>>       struct gbm_surface *gbm_surface;
>> -     struct gbm_bo *gbm_cursor_bo[2];
>> +     struct drm_fb *gbm_cursor_fb[2];
>>       struct weston_plane cursor_plane;
>> -     struct weston_plane fb_plane;
>>       struct weston_view *cursor_view;
>>       int current_cursor;
>> +
>> +     struct weston_plane fb_plane;
>
> What is fb_plane doing here?

Oops.

>> @@ -1867,6 +1871,48 @@ find_crtc_for_connector(struct drm_backend *b,
>>       return -1;
>>  }
>>
>> +static void drm_output_fini_cursor_egl(struct drm_output *output)
>> +{
>> +     unsigned int i;
>> +
>> +     for (i = 0; i < ARRAY_LENGTH(output->gbm_cursor_fb); i++) {
>> +             drm_fb_unref(output->gbm_cursor_fb[i]);
>
> Oh this is why drm_fb_unref() silently accepts NULL. I thought we had a
> habit of avoiding that, or are destructors an exception?

It's a reason I suppose, but not _the_ reason. The reason is that by
the culmination of the atomic series, there are a number of points
where the argument could legitimately be valid or NULL, and which one
it is makes no difference to logic or consistency. Same rationale as
why free() accepts NULL: because the number of calling points where
adding assert(arg) makes sense would be fewer than the number of
calling points where you'd need to wrap it in if (arg) for entirely
legitimate reasons.

I will grant you that there are fewer of these callsites now that
everything has progressively moved into plane/output/pending (surprise
sneak peek!) states, but there are still a few.

I'm still inclined to treat them like free() though, and am on the
side that adding assert(fb) where necessary would make more sense.

>> @@ -1925,6 +1957,7 @@ drm_output_fini_egl(struct drm_output *output)
>>  {
>>       gl_renderer->output_destroy(&output->base);
>>       gbm_surface_destroy(output->gbm_surface);
>> +     drm_output_fini_cursor_egl(output);
>>  }
>>
>>  static int
>
> Anyway, with fb_plane dropped and commit message improved:
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

Thanks!

Cheers,
Daniel