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

[Philipp Zabel]

Hi Daniel,

On Wed, 2017-12-20 at 12:26 +0000, Daniel Stone wrote:
> Track dynamic plane state (CRTC, FB, position) in separate structures,
> rather than as part of the plane. This will make it easier to handle
> state management later, and much more closely tracks what the kernel
> does with atomic modesets.
> 
> The fb_last pointer previously used in drm_plane now becomes part of
> output->state_last, and is not directly visible from the plane itself.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 348 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 283 insertions(+), 65 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index e123fe503..6ea41ae80 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
[...] 
> @@ -993,6 +1144,20 @@ drm_output_state_duplicate(struct drm_output_state *src,
>  	else
>  		wl_list_init(&dst->link);
>  
> +	wl_list_init(&dst->plane_list);
> +
> +	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)
  [02:07:19.265] plane_add_prop(plane-39, CRTC_ID, 0)
  [02:07:19.266] plane_add_prop(plane-39, FB_ID, 0)
  [02:07:19.266] plane_add_prop(plane-36, CRTC_ID, 0)
  [02:07:19.266] plane_add_prop(plane-36, FB_ID, 0)
  [02:07:19.266] plane_add_prop(plane-33, CRTC_ID, 0)
  [02:07:19.266] plane_add_prop(plane-33, FB_ID, 0)
  [02:07:19.266] plane_add_prop(plane-31, CRTC_ID, 0)
  [02:07:19.266] plane_add_prop(plane-31, FB_ID, 0)
  [02:07:19.266] plane_add_prop(plane-28, CRTC_ID, 0)
  [02:07:19.266] plane_add_prop(plane-28, FB_ID, 0)
  [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:

  $ modetest -pM imx-drm | grep -i "^[a-z0-9]"
  CRTCs:
  id      fb      pos     size
  30      73      (0,0)   (1280x800)
  35      0       (0,0)   (0x0)
  38      73      (0,0)   (1920x1080)
  43      0       (0,0)   (0x0)
  Planes:
  id      crtc    fb      CRTC x,y        x,y     gamma size      possible crtcs
  28      30      73      0,0             0,0     0               0x00000001
  31      0       0       0,0             0,0     0               0x00000001
  33      0       0       0,0             0,0     0               0x00000002
  36      38      73      0,0             0,0     0               0x00000004
  39      0       0       0,0             0,0     0               0x00000004
  41      0       0       0,0             0,0     0               0x00000008

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.268] Output HDMI-A-1, (connector 47, crtc 38)
                 mode 1920x1080 at 60.0, preferred, current
  [...]
  [02:07:19.405] plane_add_prop(plane-41, CRTC_ID, 0)
  [02:07:19.405] plane_add_prop(plane-41, FB_ID, 0)
  [02:07:19.405] plane_add_prop(plane-39, CRTC_ID, 0)
  [02:07:19.405] plane_add_prop(plane-39, FB_ID, 0)
  [02:07:19.405] plane_add_prop(plane-36, CRTC_ID, 0)
  [02:07:19.405] plane_add_prop(plane-36, FB_ID, 0)
  [02:07:19.405] plane_add_prop(plane-33, CRTC_ID, 0)
  [02:07:19.405] plane_add_prop(plane-33, FB_ID, 0)
  [02:07:19.405] plane_add_prop(plane-31, CRTC_ID, 0)
  [02:07:19.405] plane_add_prop(plane-31, FB_ID, 0)
  [02:07:19.405] plane_add_prop(plane-28, CRTC_ID, 0)
  [02:07:19.406] plane_add_prop(plane-28, FB_ID, 0)
  [02:07:19.406] crtc_add_prop(crtc-38, MODE_ID, 76)
  [02:07:19.406] crtc_add_prop(crtc-38, ACTIVE, 1)
  [02:07:19.406] connector_add_prop(connector-47, CRTC_ID, 38)
  [02:07:19.406] plane_add_prop(plane-36, FB_ID, 74)
  [02:07:19.406] plane_add_prop(plane-36, CRTC_ID, 38)
  [02:07:19.406] plane_add_prop(plane-36, SRC_X, 0)
  [02:07:19.406] plane_add_prop(plane-36, SRC_Y, 0)
  [02:07:19.406] plane_add_prop(plane-36, SRC_W, 125829120)
  [02:07:19.406] plane_add_prop(plane-36, SRC_H, 70778880)
  [02:07:19.406] plane_add_prop(plane-36, CRTC_X, 0)
  [02:07:19.406] plane_add_prop(plane-36, CRTC_Y, 0)
  [02:07:19.407] plane_add_prop(plane-36, CRTC_W, 1920)
  [02:07:19.407] plane_add_prop(plane-36, CRTC_H, 1080)
  [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?

regards
Philipp