Page MenuHomePhabricator

ecore-evas-drm: Add support for per-output ticking
Changes PlannedPublic

Authored by devilhorns on May 9 2018, 4:50 AM.

Details

Summary

This patch modifies the ecore_evas animator ticking code for drm in
order to support ticks on a per-output basis
Depends on D6137

Diff Detail

Repository
rEFL core/efl
Branch
feature/wayland/multi-output
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6248
devilhorns created this revision.May 9 2018, 4:50 AM
ManMower added a subscriber: ManMower.
ManMower added inline comments.
src/modules/ecore_evas/engines/drm/ecore_evas_drm.c
79

I think there's a 1:1 relationship between "tick" and "output" so it seems like there should be a way to get around ever having to do a list search for the tick stuff.

I suppose setting a user data pointer for an Ecore_Drm2_Output is one way (though perhaps not the best way)

267

This fixme seems to indicate that this code will crash on GL engines? Would like to see that fixed before landing.

330

I'm not sure I'm following this correctly - is this function now just setting the "primary" output to match canvas rotation, but leaving all other outputs in their current orientation?

417

I don't think this works for all output ordering?

for example:

O<O
  ^
O>O

the geometry would be one screen wider than it should?

449

Hmm, feels like this is going to bite us at some point. But I guess it's a reasonable stop-gap for the short term.

Is there a long term plan for handling mixed DPI multi-monitor setups?

983

Hmm I think for some graphics drivers this has the potential to fall apart hard, since each blanktime_get could end up waiting for a head to blank.

I *think* it might be safest to just use the first output's blank_time_get for all tick jobs - or just disable the tick job code entirely if there are multiple outputs enabled (but that can potentially hurt performance a little bit at start of tick too)

1195

I think these params will all throw unused variable errors if BUILD_ECORE_EVAS_GL_DRM is unset?

1251

I'd prefer to see this sort of purely cosmetic change that doesn't touch any code related to the functional change in this patch in another patch.

1269

This is correct because edata came from a calloc, but I'd rather see this as a different patch as it's not a related change.

1343

This is a nice refactor, but it would be easier to review in a separate patch

1384

why is this commented out here?

ManMower requested changes to this revision.May 24 2018, 8:08 AM
This revision now requires changes to proceed.May 24 2018, 8:08 AM
devilhorns planned changes to this revision.Jul 17 2018, 1:45 AM
devilhorns added inline comments.
src/modules/ecore_evas/engines/drm/ecore_evas_drm.c
267

Yea, this was just a note I left myself

449

I've no plan Yet .. but yes, the same thought crossed my mind at the time too

1195

Ahh yes, good catch. WIll fix in the next patchset.

1251

Sure, no problem

1343

Ok, duly noted

1384

Well, if you have 2 side-by-side monitors and try to pointer warp to the middle ... theoretically your mouse pointer could end up being drawn half on one and half on another. Really tho, this was just commented out (for the moment) because I was not going to deal with that problem in this patchset ... was more just looking at "getting this darn thing to work" ;)

devilhorns marked 4 inline comments as done.Jan 17 2019, 6:30 AM
devilhorns marked 2 inline comments as done.Jan 17 2019, 7:02 AM
devilhorns added inline comments.
src/modules/ecore_evas/engines/drm/ecore_evas_drm.c
983

question regarding this: Is an output's blank time variable ? or consistent between vblanks ? Asking because if it's consistent than we could store those values into the Output structure (after the first vblank) and inside output_blanktime_get check & return them if already set ... thus avoiding the potential of waiting for a head to blank.,...

I don't think using the first output's blank_time_get for all ticks is valid as the blank_time may be different between 2 different outputs ... and I don't want to disable tick_job code entirely either :(

devilhorns added inline comments.Jan 17 2019, 7:32 AM
src/modules/ecore_evas/engines/drm/ecore_evas_drm.c
983

Ok, crap :( After running some tests, it seems output blank time can vary :(

Not sure how we should proceed here ... Any Thoughts ?

devilhorns added inline comments.Jan 22 2019, 5:52 AM
src/modules/ecore_evas/engines/drm/ecore_evas_drm.c
417

you're diagram is confusing me a bit :( Maybe it's too early in the morning, or lack of coffee ... but can you put this in english please ? :)

ManMower added inline comments.Jan 22 2019, 7:36 AM
src/modules/ecore_evas/engines/drm/ecore_evas_drm.c
417

Was trying to explain a 4 output array like:

43
12