On Wed, Mar 19, 2014 at 11:15:36AM -0700, Matt Roper wrote:
On Wed, Mar 19, 2014 at 12:28:43PM +0100, Daniel Vetter wrote:
On Tue, Mar 18, 2014 at 05:22:48PM -0700, Matt Roper wrote:
...
- /*
* set_config() adjusts crtc->primary->fb; however the DRM setplane* code that called us expects to handle the framebuffer update and* reference counting; save and restore the current fb before* calling it.*/- tmpfb = plane->fb;
- ret = crtc->funcs->set_config(&set);
I wonder whether we should have an oppportunistic path using the page_flip interface here. Otoh we must have a fallback to ->set_config anyway since the drivers currently only allow pageflips on identical buffers. E.g. i915 rejects tiling changes and stride changes and position changes. So I think having a pageflip fastpath isn't worth the trouble.
Also I think you need to use set_config_internal here to get the refcounting right.
Hmm. I specifically didn't use set_config_internal here because I thought drm_mode_setplane() (and presumably the future atomic ioctl which may also call into this) will already handle the refcounting for us. Am I overlooking something?
Hm, I need to take a closer look again, but set_config has some really funny rules about who updates which pointer and who grabs references for which fb. Mostly since this has all grown rather add-hoc.
But you're right and the update_plane code already has some refcounting and frobbing of its own, so I'm not sure what we really need to do here. I think it at least needs a really big comment explaining what's going on.
Note that iirc the crtc helper code can still disable connectors in _any_ setcrtc call, so you might accidentally end up in a full modeset, with all the fun this entails.
Definitely need to think more about this here. I hope we don't need to rework the semantics of all drivers, since that would be major pain. -Daniel