Sultan Alsawaf (0dacee6c) at 17 Dec 17:05
@mattst88 Could you take a look at this one too please?
TearFree support has been available in the modesetting driver for a year with no issues reported. The code is mature and robust, with error handling that's been vetted across many hardware configurations.
Notably, TearFree is also the only way to achieve a tear-free desktop with mismatched displays and transformed CRTCs.
Enable TearFree by default for a smooth desktop experience out of the box.
Signed-off-by: Sultan Alsawaf sultan@kerneltoast.com
Sultan Alsawaf (c448ebe5) at 16 Dec 19:09
modesetting: Enable TearFree by default
Sultan Alsawaf (cbfe0262) at 16 Dec 19:06
modesetting: Enable TearFree by default
... and 282 more commits
This change is bogus; please don't merge it.
There's no memory leak because RegionInit()
won't allocate memory for a size of 0:
static inline void
RegionInit(RegionPtr _pReg, BoxPtr _rect, int _size)
{
if ((_rect) != NULL) {
(_pReg)->extents = *(_rect);
(_pReg)->data = (RegDataPtr) NULL;
}
else {
size_t rgnSize;
(_pReg)->extents = RegionEmptyBox;
if (((_size) > 1) && ((rgnSize = RegionSizeof(_size)) > 0) &&
(((_pReg)->data = (RegDataPtr) malloc(rgnSize)) != NULL)) {
(_pReg)->data->size = (_size);
(_pReg)->data->numRects = 0;
}
else
(_pReg)->data = &RegionEmptyData;
}
}
Note the if (((_size) > 1) && ((rgnSize = RegionSizeof(_size)) > 0) &&
above. And the TearFree code calls RegionInit()
with 0 for _size
.
Also, the commit message claims that memory is allocated by RegionIntersect()
, but this isn't true. RegionIntersect()
is just a wrapper for a call into the pixman library, which also doesn't allocate memory. And if it did allocate memory, RegionUninit()
wouldn't free it because RegionUninit()
only frees memory potentially allocated by RegionInit()
.
The code looks good to me; I would just suggest squashing the new commit into the first commit.
Ah OK. For some reason, GitLab won't let me resolve this thread; if you've got the "resolve thread" button, it'd be appreciated if you could resolve it. Thanks, and sorry for the churn :}
Ditto, this MR is a great boon for power consumption on my ADL-P laptop.
Ah, I was thinking that ms->pending_modeset
could be reused instead of adding a new member to drmmode_rec
.
Looks good to me!
I think a better solution would be to do v = drmGetVersion(ms->fd);
on top of this line instead, so the extra call to drmFreeVersion()
is unneeded. Good find!
It looks like this code is also missing a NULL check on v
because drmGetVersion()
may return NULL.
The in_modeset flag forces the check thing to return false.
Oops, I didn't notice that; thanks for pointing it out. You could reuse pending_modeset
for that I think.
The problem we're trying to solve is not that some modifier is entirely unsupported, but that such modifiers may push the display FIFO requirements/etc. too high, and thus prevent new displays from being lit up.
Ahh, thanks for the explanation!
@karlson2k I did a quick grep in Kodi and it looks like Kodi doesn't utilize Present timing events. Kodi would need to listen for PresentComplete events coming from mesa's under-the-hood usage of DRI3/Present when eglSwapBuffers()
is used.
Basically, I suspect this could be fixed in Kodi if Kodi were to just add a listener for Present events from the X-server in order to know when its video frames actually become visible on the display. Kodi already uses eglSwapBuffers()
so it's already receiving Present timing events, but it's just not doing anything with them.
This is how mpv implemented this sort of thing: https://github.com/mpv-player/mpv/commit/3d459832a88a9bd2835b339cf6ca98f84aad0115#diff-ce7f7bf9a3af824d860a7fd9cc5ff45a10cd5b901162f76c91e68f0acd73f190R1283-R1297
As for why the Intel DDX works fine when the modesetting DDX doesn't, the only thing I can think of is that maybe you're using DRI2 with the Intel DDX instead of DRI3?
It'd also be interesting if this still occurs with mpv after it got the timing fixes.
@karlson2k Try the master branch of mpv. I submitted some timing fixes to mpv a little while back:
https://github.com/mpv-player/mpv/commit/4a2aa36674a3989d6567ebe1d3c3f8c6c889d906
https://github.com/mpv-player/mpv/commit/0d44ae319dfd5209eb0733caafdcbcf67d228cf3
Using check_flip_window()
for that will only switch to the regular screen pixmap if some condition changes such that present_check_flip()
returns false. It sounded like from the commit message that the goal is to switch back to the regular screen pixmap when firing up a new CRTC that's incompatible with a DRI3 pixmap. Since you mentioned fancy modifiers, I figured the assumption was that drmmode_is_format_supported()
would return false after the new CRTC incompatible with the modifiers was added, and therefore the check_flip_window()
would trigger an unflip.
But if present_check_flip()
still returns true after the new CRTC is added, then you won't get an unflip from check_flip_window()
. I wanted to point out that drmmode_is_format_supported()
only considers CRTCs with crtc->enabled == TRUE
, so if the new CRTC is incompatible with the DRI3 pixmap's modifiers and crtc->enabled == FALSE
at the time of this check_flip_window()
call, then AFAICT check_flip_window()
won't switch back to the regular screen pixmap.
Sultan Alsawaf (f490622f) at 01 Mar 10:17
@mupuf It's been a week so I've gone ahead and rebased. Let me know if there's anything else needed on my part. Thanks!
Sultan Alsawaf (f490622f) at 01 Mar 06:43
present: Document the TearFree flip reasons in PresentFlipReason
... and 12 more commits
@mupuf updated!