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

[Pekka Paalanen]

On Tue, 30 Jan 2018 18:59:35 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> 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

Do you mean disables?

>   * 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?

Your approach sounds good, but I don't understand the detail about
ignoring disables when state_invalid is false. The compositor starts up
with state_invalid=true, so what would it change? And would it not also
prevent disables during a normal steady-state when we have enabled
outputs running, i.e. state_invalid=false?

Did you mean to ignore while state_invalid is true instead? That would
make more sense to me.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180201/085b9578/attachment-0001.sig>