[PATCH v14 09/41] compositor-drm: Track unused connectors and CRTCs

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:26 +0000
Daniel Stone <daniels at collabora.com> wrote:

> Rather than a more piecemeal approach at backend creation, explicitly
> track connectors and CRTCs we do not intend to use, so we can ensure
> they are disabled where appropriate.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 04d52f2c6..9c7564383 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -188,6 +188,9 @@ struct drm_backend {
>  
>  	void *repaint_data;
>  
> +	struct wl_array unused_connectors;
> +	struct wl_array unused_crtcs;
> +
>  	int cursors_are_broken;
>  
>  	bool universal_planes;
> @@ -386,6 +389,26 @@ static struct gl_renderer_interface *gl_renderer;
>  
>  static const char default_seat[] = "seat0";
>  
> +static void
> +wl_array_remove_uint32(struct wl_array *array, uint32_t elm)
> +{
> +	uint32_t *pos, *end;
> +
> +	end = (uint32_t *) ((char *) array->data + array->size);
> +
> +	wl_array_for_each(pos, array) {
> +		if (*pos != elm)
> +			continue;
> +
> +		array->size -= sizeof(*pos);
> +		if (pos + 1 == end)
> +			break;
> +
> +		memmove(pos, pos + 1, (char *) end -  (char *) (pos + 1));
> +		break;
> +	}
> +}
> +
>  static inline struct drm_output *
>  to_drm_output(struct weston_output *base)
>  {
> @@ -1327,6 +1350,7 @@ drm_output_assign_state(struct drm_output_state *state,
>  			enum drm_state_apply_mode mode)
>  {
>  	struct drm_output *output = state->output;
> +	struct drm_backend *b = to_drm_backend(output->base.compositor);
>  	struct drm_plane_state *plane_state;
>  
>  	assert(!output->state_last);
> @@ -1363,6 +1387,12 @@ drm_output_assign_state(struct drm_output_state *state,
>  		else if (plane->type == WDRM_PLANE_TYPE_PRIMARY)
>  			output->page_flip_pending = 1;
>  	}
> +
> +	if (output->dpms == WESTON_DPMS_ON) {
> +		wl_array_remove_uint32(&b->unused_connectors,
> +				       output->connector_id);
> +		wl_array_remove_uint32(&b->unused_crtcs, output->crtc_id);
> +	}

This gets called from drm_repaint_flush(). In the following patch,
drm_repaint_flush() starts to explicitly disable unused CRTCs. That
happens before calling drm_output_assign_state(). It would seem that it
is possible to disable a CRTC that is being taken into use.

The CRTC and connector should probably be removed from the unused lists
already under drm_output_repaint() if it's to be done by state
application.

On another thought, "unused" at this patch means "no owning drm_output"
if we look at drm_backend_update_unused_outputs(), but OTOH it means
"not being driven" if we look at drm_output_deinit() called from
drm_output_disable() which still leaves the drm_output existing.

- Any hotplug event will rewrite all "unused" arrays according to "no
  owning drm_output".

- Disable, then enable an output without a hotplug even in between
  would probably cause the CRTC to be erroneuously turned off by the
  following patch.

Could you explain the intention here, what does "unused" mean?

I suppose, as disabled outputs do not go through repaint, disabled but
connected (drm_output exists for the CRTC and connector) should be part
of the unused arrays, which implies that the SetCrtc vs. array_remove
ordering will be wrong and drm_backend_update_unused_outputs() should
check the drm_output is enabled as well.

One of these patches should change the behaviour from "do not touch an
output unless explicitly enabled or disabled" to "disable all outputs
not explicitly enabled", as keeping the don't-touch mode seems awkward.


Thanks,
pq


>  }
>  
>  
> @@ -3983,6 +4013,7 @@ drm_output_deinit(struct weston_output *base)
>  {
>  	struct drm_output *output = to_drm_output(base);
>  	struct drm_backend *b = to_drm_backend(base->compositor);
> +	uint32_t *unused;
>  
>  	if (b->use_pixman)
>  		drm_output_fini_pixman(output);
> @@ -4004,6 +4035,11 @@ drm_output_deinit(struct weston_output *base)
>  			drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
>  		}
>  	}
> +
> +	unused = wl_array_add(&b->unused_connectors, sizeof(*unused));
> +	*unused = output->connector_id;
> +	unused = wl_array_add(&b->unused_crtcs, sizeof(*unused));
> +	*unused = output->crtc_id;
>  }
>  
>  static void
> @@ -4099,6 +4135,47 @@ drm_output_disable(struct weston_output *base)
>  	return 0;
>  }
>  
> +/**
> + * Update the list of unused connectors and CRTCs
> + *
> + * This keeps the unused_connectors and unused_crtcs arrays up to date.
> + *
> + * @param b Weston backend structure
> + * @param resources DRM resources for this device
> + */
> +static void
> +drm_backend_update_unused_outputs(struct drm_backend *b, drmModeRes *resources)
> +{
> +	int i;
> +
> +	wl_array_release(&b->unused_connectors);
> +	wl_array_init(&b->unused_connectors);
> +
> +	for (i = 0; i < resources->count_connectors; i++) {
> +		uint32_t *connector_id;
> +
> +		if (drm_output_find_by_connector(b, resources->connectors[i]))
> +			continue;
> +
> +		connector_id = wl_array_add(&b->unused_connectors,
> +					    sizeof(*connector_id));
> +		*connector_id = resources->connectors[i];
> +	}
> +
> +	wl_array_release(&b->unused_crtcs);
> +	wl_array_init(&b->unused_crtcs);
> +
> +	for (i = 0; i < resources->count_crtcs; i++) {
> +		uint32_t *crtc_id;
> +
> +		if (drm_output_find_by_crtc(b, resources->crtcs[i]))
> +			continue;
> +
> +		crtc_id = wl_array_add(&b->unused_crtcs, sizeof(*crtc_id));
> +		*crtc_id = resources->crtcs[i];
> +	}
> +}
> +
>  /**
>   * Create a Weston output structure
>   *
> @@ -4259,6 +4336,8 @@ create_outputs(struct drm_backend *b, struct udev_device *drm_device)
>  		}
>  	}
>  
> +	drm_backend_update_unused_outputs(b, resources);
> +
>  	if (wl_list_empty(&b->compositor->output_list) &&
>  	    wl_list_empty(&b->compositor->pending_output_list))
>  		weston_log("No currently active connector found.\n");
> @@ -4350,6 +4429,8 @@ update_outputs(struct drm_backend *b, struct udev_device *drm_device)
>  		drm_output_destroy(&output->base);
>  	}
>  
> +	drm_backend_update_unused_outputs(b, resources);
> +
>  	free(connected);
>  	drmModeFreeResources(resources);
>  }
> @@ -4416,6 +4497,9 @@ drm_destroy(struct weston_compositor *ec)
>  
>  	weston_launcher_destroy(ec->launcher);
>  
> +	wl_array_release(&b->unused_crtcs);
> +	wl_array_release(&b->unused_connectors);
> +
>  	close(b->drm.fd);
>  	free(b);
>  }
> @@ -4848,6 +4932,8 @@ drm_backend_create(struct weston_compositor *compositor,
>  		return NULL;
>  
>  	b->drm.fd = -1;
> +	wl_array_init(&b->unused_crtcs);
> +	wl_array_init(&b->unused_connectors);
>  
>  	/*
>  	 * KMS support for hardware planes cannot properly synchronize

-------------- 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/20180116/c8db80c4/attachment.sig>