[PATCH weston v10 17/61] compositor-drm: Refactor sprite create/destroy into helpers

[Pekka Paalanen]

On Mon, 17 Apr 2017 20:54:21 +0100
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
> 
> On 7 April 2017 at 15:36, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Tue,  4 Apr 2017 17:54:35 +0100
> > Daniel Stone <daniels at collabora.com> wrote:  
> >> +/**
> >> + * Destroy one DRM plane
> >> + *
> >> + * Destroy a DRM plane, removing it from screen and releasing its retained
> >> + * buffers in the process. The counterpart to drm_plane_create.
> >> + *
> >> + * @param plane Plane to deallocate (will be freed)
> >> + */
> >> +static void
> >> +drm_plane_destroy(struct drm_plane *plane)
> >> +{
> >> +     drmModeSetPlane(plane->backend->drm.fd, plane->plane_id, 0, 0, 0,  
> >
> > Previously the code was doing a nasty hack to get a valid CRTC id to
> > use instead of zero here. Was that just a brainfart, or has something
> > changed in the kernel?  
> 
> It doesn't make a difference. After a plane has an FB ID of 0 set, it
> is no longer associated with any CRTC. I avoided the gymnastics
> because there didn't seem much point in doing so. Should I make a note
> in the commit message, or a comment, or ... ?

Hi Daniel,

yes please, explain this in the commit message.

It's very nice to know the gymnastics are not necessary. Makes me
wonder why it was there in the first place. I've seen versions of
weston that cannot shut down properly because they have the gymnastics
and may fail to have a weston_output at all to get any CRTC id from, at
which state it just abandons all stuff.

> >> -     wl_list_for_each_safe(plane, next, &backend->sprite_list, link) {
> >> -             drmModeSetPlane(backend->drm.fd,
> >> -                             plane->plane_id,
> >> -                             output->crtc_id, 0, 0,
> >> -                             0, 0, 0, 0, 0, 0, 0, 0);
> >> -             assert(!plane->fb_last);
> >> -             assert(!plane->fb_pending);
> >> -             drm_fb_unref(plane->fb_current);
> >> -             weston_plane_release(&plane->base);
> >> -             free(plane);  
> >
> > There was no wl_list_remove()... *facepalm*
> >
> > Nice to fix that, even though it's yet another change that is not
> > strictly refactoring.  
> 
> True. :\

Equally fixed by mentioning this in the commit message, and maybe not
using "refactor" in the title.

With these two, and the other trivial comments addressed:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170509/d70cbf7c/attachment.sig>