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 > _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
