[PATCH v4 02/13] modesetting: scanout_pixmap_gpu one call only

[Alex Goins]

> AFAICT RRReplaceScanoutPixmap may call set_scanout_pixmap while
> randr_crtc->scanout_pixmap is already non-NULL (and pointing to a
> different pixmap).

That seems to be true now that you mention it. I haven't paid that much
attention to RRReplaceScanoutPixmap(), since it doesn't seem to get exercised in
any of the paths in the drivers I've tested. It probably doesn't work with my
patches. Looking at RRReplaceScanoutPixmap(), it seems that there might be a bug
as it stands today, when running in reverse PRIME mode.

If calling rrCrtcSetScanoutPixmap(ppix) without a prior call to
rrCrtcSetScanoutPixmap(NULL) is an acceptable way to replace the scanout pixmap,
drmmode_set_scanout_pixmap_gpu() should probably be reworked to handle that. As
of now, it will dirty track both pixmaps instead of stopping on the first and
starting on the second. It only cleans up anything if ppix == NULL, and even
then it only cleans up crtc->scanout_pixmap. RRReplaceScanoutPixmap() would
leave the old crtc->scanout_pixmap dangling and still tracked by
PixmapStopDirtyTracking(), likely causing a segfault if you ever try to destroy
it.

>Are you saying that only happens in the reverse PRIME case?

Well yes, drmmode_set_scanout_pixmap_gpu() is only called in the reverse PRIME
case, so disallowing multiple calls doesn't break my patches. Maybe it does
break RRReplaceScanoutPixmap(), in which case it might be better to have this
patch handle multiple calls more gracefully instead of disallowing it
altogether.

The pattern in rrSetupPixmapSharing() that requires this patch is when falling
back after a failure in rrStartFlippingPixmapTracking() when reverse PRIME mode
is enabled. It will create two shared pixmaps and call rrCrtcSetScanoutPixmap()
on both of them, then fail and need to destroy the second shared pixmap. That
works okay if the implementation of rrCrtcSetScanoutPixmap() can handle it
properly. In the case of drmmode_set_scanout_pixmap_cpu(), it does cleanup on
both crtc->scanout_pixmap and crtc->scanout_pixmap_back, so it's fine. Of
course, that still relies on RandR setting crtc->scanout_pixmap and
crtc->scanout_pixmap_back as expected, and it doesn't account for
RRReplaceScanoutPixmap().

It would be possible to work around this in rrSetupPixmapSharing() without a
patch to drmmode_set_scanout_pixmap_gpu() by calling
rrCrtcSetScanoutPixmap(NULL) twice with crtc->scanout_pixmap set to each pixmap,
but that seemed to me like an abuse of the API, and it still doesn't fix the
RRReplaceScanoutPixmap() case.

By inspection, I was under the impression that when operating with a single
scanout pixmap, the pattern is:
    1) Call slave->rrCrtcSetScanoutPixmap(crtc->scanout_pixmap)
    2) Set crtc->scanout_pixmap
    3) Do stuff
    4) Call slave->rrCrtcSetScanoutPixmap(NULL)
    5) Set crtc->scanout_pixmap = NULL;

Then I extended it for two scanout pixmaps:
    1) Call slave->rrCrtcSetScanoutPixmap(crtc->scanout_pixmap)
    2) Set crtc->scanout_pixmap
    3) Call slave->rrCrtcSetScanoutPixmap(crtc->scanout_pixmap_back)
    4) Set crtc->scanout_pixmap_back
    5) Do Stuff
    6) Call slave->rrCrtcSetScanoutPixmap(NULL)
    7) Set crtc->scanout_pixmap = crtc->scanout_pixmap_back = NULL

After seeing RRReplaceScanoutPixmap(), maybe I was wrong, where the pattern
    1) Call slave->rrCrtcSetScanoutPixmap(crtc->scanout_pixmap)
    2) Set crtc->scanout_pixmap
    3) Set crtc->scanout_pixmap to something else
    4) Call slave->rrCrtcSetScanoutPixmap(crtc->scanout_pixmap)
    5) Do stuff
    6) Goto 3 an arbitrary number of times
    7) Call slave->rrCrtcSetScanoutPixmap(NULL)
    8) Set crtc->scanout_pixmap = NULL;
seems to be acceptable.

In my opinion it seems like a bad idea to have such a loose linkage between
crtc->scanout_pixmap and rrCrtcSetScanoutPixmap() when
rrCrtcSetScanoutPixmap(NULL) depends on crtc->scanout_pixmap, but I just stuck
to the pattern.

Happy to rework this with a better understanding of the intended semantics of
rrCrtcSetScanoutPixmap(). Maybe I should just not use rrCrtcSetScanoutPixmap()
in the flipping case, and do that work as part of
rr(Enable/Disable)SharedPixmapFlipping() instead.

Thanks,
Alex