[PATCH weston v12 15/40] compositor-drm: Atomic modesetting support

[Pekka Paalanen]

On Tue, 26 Sep 2017 18:15:48 +0100
Daniel Stone <daniels at collabora.com> wrote:

> Add support for using the atomic-modesetting API to apply output state.
> Unlike previous series, this commit does not unflip sprites_are_broken,
> until further work has been done with assign_planes to make it reliable.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Co-authored-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> Co-authored-by: Louis-Francis Ratté-Boulianne <louis-francis.ratte-boulianne at collabora.com>
> Co-authored-by: Derek Foreman <derek.foreman at collabora.co.uk>
> ---
>  configure.ac               |   2 +-
>  libweston/compositor-drm.c | 507 ++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 434 insertions(+), 75 deletions(-)

Hi Daniel!


> +#ifdef HAVE_DRM_ATOMIC
> +static int
> +crtc_add_prop(drmModeAtomicReq *req, struct drm_output *output,
> +	      enum wdrm_crtc_property prop, uint64_t val)
> +{
> +	struct drm_property_info *info = &output->props_crtc[prop];
> +	int ret;
> +
> +	if (!info)
> +		return -1;

Should not this and the two functions below check that the prop_id was
actually found?

What happens if one uses property ID 0?

> +
> +	ret = drmModeAtomicAddProperty(req, output->crtc_id, info->prop_id,
> +				       val);
> +	return (ret <= 0) ? -1 : 0;
> +}
> +
> +static int
> +connector_add_prop(drmModeAtomicReq *req, struct drm_output *output,
> +		   enum wdrm_connector_property prop, uint64_t val)
> +{
> +	struct drm_property_info *info = &output->props_conn[prop];
> +	int ret;
> +
> +	if (!info)
> +		return -1;
> +
> +	ret = drmModeAtomicAddProperty(req, output->connector_id,
> +				       info->prop_id, val);
> +	return (ret <= 0) ? -1 : 0;
> +}
> +
> +static int
> +plane_add_prop(drmModeAtomicReq *req, struct drm_plane *plane,
> +	       enum wdrm_plane_property prop, uint64_t val)
> +{
> +	struct drm_property_info *info = &plane->props[prop];
> +	int ret;
> +
> +	if (!info)
> +		return -1;
> +
> +	ret = drmModeAtomicAddProperty(req, plane->plane_id, info->prop_id,
> +				       val);
> +	return (ret <= 0) ? -1 : 0;
> +}


> +static int
> +drm_pending_state_apply_atomic(struct drm_pending_state *pending_state)
> +{
> +	struct drm_backend *b = pending_state->backend;
> +	struct drm_output_state *output_state, *tmp;
> +	drmModeAtomicReq *req = drmModeAtomicAlloc();
> +	uint32_t flags = 0;
> +	int ret = 0;
> +
> +	if (!req)
> +		return -1;
> +
> +	if (b->state_invalid) {
> +		uint32_t *unused;
> +		int err;
> +
> +		/* If we need to reset all our state (e.g. because we've
> +		 * just started, or just been VT-switched in), explicitly
> +		 * disable all the CRTCs and connectors we aren't using. */
> +		wl_array_for_each(unused, &b->unused_connectors) {
> +			struct drm_property_info infos[WDRM_CONNECTOR__COUNT];
> +			struct drm_property_info *info;
> +			drmModeObjectProperties *props;
> +
> +			memset(infos, 0, sizeof(infos));

memset would be easier done with
	struct drm_property_info infos[WDRM_CONNECTOR__COUNT] = {};

> +
> +			props = drmModeObjectGetProperties(b->drm.fd,
> +							   *unused,
> +							   DRM_MODE_OBJECT_CONNECTOR);
> +			if (!props) {
> +				ret = -1;
> +				continue;
> +			}
> +
> +			drm_property_info_populate(b, connector_props, infos,
> +						   WDRM_CONNECTOR__COUNT,
> +						   props);
> +			drmModeFreeObjectProperties(props);
> +
> +			info = &infos[WDRM_CONNECTOR_CRTC_ID];
> +			err = drmModeAtomicAddProperty(req, *unused,
> +						       info->prop_id, 0);
> +			if (err <= 0)
> +				ret = -1;
> +
> +			info = &infos[WDRM_CONNECTOR_DPMS];
> +			if (info->prop_id > 0)
> +				err = drmModeAtomicAddProperty(req, *unused,
> +							       info->prop_id,
> +							       DRM_MODE_DPMS_OFF);
> +			if (err <= 0)
> +				ret = -1;
> +
> +			drm_property_info_free(infos, WDRM_CONNECTOR__COUNT);
> +		}
> +
> +		wl_array_for_each(unused, &b->unused_crtcs) {
> +			struct drm_property_info infos[WDRM_CRTC__COUNT];
> +			struct drm_property_info *info;
> +			drmModeObjectProperties *props;
> +			uint64_t active;
> +
> +			memset(infos, 0, sizeof(infos));
> +
> +			/* We can't emit a disable on a CRTC that's already
> +			 * off, as the kernel will refuse to generate an event
> +			 * for an off->off state and fail the commit.
> +			 */
> +			props = drmModeObjectGetProperties(b->drm.fd,
> +							   *unused,
> +							   DRM_MODE_OBJECT_CRTC);
> +			if (!props) {
> +				ret = -1;
> +				continue;
> +			}
> +
> +			drm_property_info_populate(b, crtc_props, infos,
> +						   WDRM_CRTC__COUNT,
> +						   props);
> +
> +			info = &infos[WDRM_CRTC_ACTIVE];
> +			active = drm_property_get_value(info, props, 0);
> +			drmModeFreeObjectProperties(props);
> +			if (active == 0) {
> +				drm_property_info_free(infos, WDRM_CRTC__COUNT);
> +				continue;
> +			}
> +
> +			err = drmModeAtomicAddProperty(req, *unused,
> +						       info->prop_id, 0);
> +			if (err <= 0)
> +				ret = -1;
> +
> +			info = &infos[WDRM_CRTC_MODE_ID];
> +			err = drmModeAtomicAddProperty(req, *unused,
> +						       info->prop_id, 0);
> +			if (err <= 0)
> +				ret = -1;
> +
> +			drm_property_info_free(infos, WDRM_CRTC__COUNT);
> +		}
> +
> +#if 0
> +		wl_array_for_each(unused, &b->unused_planes) {

I don't think we would ever need a unused_planes array since we create
a drm_plane for them all, right?

Should just go through the list of drm_planes.

> +			struct drm_property_info infos[WDRM_PLANE__COUNT];
> +			struct drm_property_info *info;
> +			drmModeObjectProperties *props;
> +
> +			memset(infos, 0, sizeof(infos));
> +
> +			drm_property_info_populate(b, props_plane, infos,
> +						   WDRM_PLANE__COUNT, props);
> +			drmModeFreeObjectProperties(props);
> +
> +			info = &infos[WDRM_PLANE_CRTC_ID];
> +			err = drmModeAtomicAddProperty(req, *unused,
> +						       info->prop_id, 0);
> +			if (err <= 0)
> +				ret = -1;
> +
> +			info = &infos[WDRM_PLANE_FB_ID];
> +			err = drmModeAtomicAddProperty(req, *unused,
> +						       info->prop_id, 0);
> +			if (err <= 0)
> +				ret = -1;
> +
> +			drm_property_info_free(infos, WDRM_PLANE__COUNT);
> +		}
> +#endif
> +
> +		flags |= DRM_MODE_ATOMIC_ALLOW_MODESET;
> +	}
> +
> +	wl_list_for_each(output_state, &pending_state->output_list, link)
> +		ret |= drm_output_apply_state_atomic(output_state, req, &flags);
> +
> +	if (ret != 0) {
> +		weston_log("atomic: couldn't compile atomic state\n");
> +		goto out;
> +	}
> +
> +	flags |= DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_ATOMIC_NONBLOCK;
> +	ret = drmModeAtomicCommit(b->drm.fd, req, flags, b);
> +	if (ret != 0) {
> +		weston_log("atomic: couldn't commit new state: %m\n");
> +		goto out;
> +	}
> +
> +	wl_list_for_each_safe(output_state, tmp, &pending_state->output_list,
> +			      link) {
> +		drm_output_assign_state(output_state,
> +					DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS);
> +	}
> +
> +	b->state_invalid = false;
> +
> +	assert(wl_list_empty(&pending_state->output_list));
> +
> +out:
> +	drmModeAtomicFree(req);

Missing drm_pending_state_free()?


> +	return ret;
> +}
> +#endif
> +
>  static int
>  drm_pending_state_apply(struct drm_pending_state *pending_state)
>  {

Otherwise looks good 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/20171004/980f40a5/attachment.sig>