[PATCH v14 03/41] compositor-drm: Introduce drm_plane_state structure

[Daniel Stone]

Hi Philipp,

On 29 January 2018 at 10:19, Philipp Zabel <p.zabel at pengutronix.de> wrote:
> On Wed, 2017-12-20 at 12:26 +0000, Daniel Stone wrote:
>> +     wl_list_for_each(ps, &src->plane_list, link) {
>> +             /* Don't carry planes which are now disabled; these should be
>> +              * free for other outputs to reuse. */
>> +             if (!ps->output)
>> +                     continue;
>> +
>> +             if (plane_mode == DRM_OUTPUT_STATE_CLEAR_PLANES)
>> +                     (void) drm_plane_state_alloc(dst, ps->plane);
>
> The drm_output_state_duplicate(..., DRM_OUTPUT_STATE_CLEAR_PLANES)
> call in drm_output_get_disable_state() causes the following issue on
> i.MX6 with two outputs (HDMI-A-1 and LVDS-1) if weston.conf disables one
> of them:
>
>   [output]
>   name=LVDS-1
>   mode=off
>
> The mode=off causes drm_output_disable() to be called before any state
> is set up for the HDMI output. The resulting initial atomic commit will
> disable the LVDS output, but also clear the planes for the HDMI output
> without disabling it:
>
>   [02:07:19.265] Disabling output LVDS-1
>   [02:07:19.265] plane_add_prop(plane-41, CRTC_ID, 0)
>   [02:07:19.265] plane_add_prop(plane-41, FB_ID, 0)
>   [... same for every plane on the device ...]
>   [02:07:19.266] crtc_add_prop(crtc-30, MODE_ID, 0)
>   [02:07:19.266] crtc_add_prop(crtc-30, ACTIVE, 0)
>   [02:07:19.266] connector_add_prop(connector-45, CRTC_ID, 0)
>   [02:07:19.267] atomic: couldn't commit new state: Invalid argument
>
> The commit fails with -EINVAL on imx-drm, because we don't have support
> for activating a crtc (here: keeping 38 active) without its base plane
> (36). Before weston was started, both outputs and their base planes were
> active due to kernel fbdev emulation:
>
>  [...]
>
> After the first atomic commit failed, any further commit will fail as
> well, now because the LVDS output crtc (30) was not actually disabled,
> and we keep committing state to clear its base plane (28):
>
>   [...]
>   [02:07:19.407] atomic: couldn't commit new state: Invalid argument
>
> Should the initial disabling of outputs be deferred until state for all
> outputs is complete?
> Should the planes in the state returned by drm_output_get_disable_state
> be limited to those that actually have the given crtc in their
> possible_crtcs?
> And if the initial disabling of an output fails, should further commits
> deactivate a crtc if there are no planes set on it?

Thanks a lot for tracking this down and the analysis! Now I've
digested it, it makes sense:
  * to ensure something coherent, we clear the state of every plane at
the first atomic commit (i.e. b->state_invalid == true) to make sure
we have a completely known state and nothing shown on screen we didn't
want
  * when starting up, all output enables lead to the output entering
the repaint cycle, with repaint itself deferred until idle, meaning
they will all get grouped together between repaint_begin() /
repaint_flush()
  * when starting up, all output enables take effect immediately
  * if any outputs are explicitly disabled, our first atomic commit
will be to disable them, which thanks to #1 will include disabling all
planes including primary planes on other outputs

I think the best way to solve this, would be to ignore output disables
whilst state_invalid is false. For the common case, this would mean
that your enables and disables all got committed together when
state_invalid was true. This also ties in nicely with Pekka's
suggestion on the patches to track unused outputs. There is a
pathological case: when we enter with state_invalid and every output
enumerated was disabled, we would not actually disable those outputs
but leave them waiting until an output which actually should be
enabled was connected. That could be worked around as well with more
tracking inside the outputs, but I think it's marginal enough that
there's not much point. Given that we want to rework the output
configuration flow in the long term, we could probably just wait until
then.

Any thoughts?

Cheers,
Daniel