On 28 June 2017 at 14:10, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > Hi Emil, > > On Wednesday 28 Jun 2017 12:27:06 Emil Velikov wrote: >> On 27 June 2017 at 21:38, Laurent Pinchart wrote: >> > Hello, >> > >> > The atomic helpers favour the .enable() and .atomic_disable() CRTC helper >> > operations, but still support the legacy .prepare(), .commit(), .disable() >> > and .dpms() operations. Some drivers have been updated, but most still >> > use various combination of new and legacy operations, leading to >> > confusion among new developers when they read the code. >> >> Just had a crazy idea: >> >> Would it make sense for the function(s) that takes the >> driver_foo_funcs as an argument to cross-check for such mistakes? >> Since many of the hooks are optional, one could check for the >> 'forbidden' combinations: >> Atomic drivers should not have any legacy hooks set, while legacy ones >> should not have any of the atomic ones. > > I think it's a good idea, although transition to atomic modesetting (following > the procedure outlined in Daniel's blog) might mix legacy and atomic > operations. > Right, there's that and drivers which use atomic internally, but don't advertise it. IIRC only nouveau is in the boat, but could be addressed. To minimise noise one could use WARN_ONCE and apply only for legacy hooks on atomic drivers. The inverse - atomic hooks on legacy drivers, should be fine but will cause the occasional distraction. > At the moment the only atomic driver that still uses legacy CRTC helper > operations is vmwgfx. The driver implements both the .prepare() and > .atomic_disable() helper operations, with .prepare() being empty. The CRTC > helpers call those operations from disable_outputs(): > > /* Right function depends upon target state. */ > if (new_crtc_state->enable && funcs->prepare) > funcs->prepare(crtc); > else if (funcs->atomic_disable) > funcs->atomic_disable(crtc, old_crtc_state); > else if (funcs->disable) > funcs->disable(crtc); > else > funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > > .prepare() and .atomic_disable() are thus not completely identical. > > I believe this could be addressed by adding > > if (crtc->state->enable) > return; > > to the .atomic_disable() handler in the vmwgfx driver. I'll submit a patch for > that. > With the guards mentioned, the VMWare team would have spotted/fixed this as part of their atomic series. Either way - it's just an idea, plus I won't be able to pursue it anytime soon. Regards, Emil _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
