[PATCH 0/8] Cleanup CRTC .enable()/.disable() cargo-cult

[Emil Velikov]

On 28 June 2017 at 14:10, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> 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