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

[Alex Goins]

Anyone have some feedback on this? It seems Dave is the original author of
rrCrtcSetScanoutPixmap(), so maybe he could provide some input? It should be a
pretty straightforward fix, but I need to know the preferred usage of
rrCrtcSetScanoutPixmap(), or if I should bypass it altogether.

Also interested in feedback on the rest of the patch set; hoping to get this in
ASAP so we can target it with our driver.

Thanks,
Alex

On Tue, 8 Mar 2016, Alex Goins wrote:

> > 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
>