On Tue, 30 Jan 2018 18:59:35 +0000 Daniel Stone <dan...@fooishbar.org> wrote:
> Hi Philipp, > > On 29 January 2018 at 10:19, Philipp Zabel <p.za...@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
pgpSi2xkvkGUZ.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel