[PATCH v14 17/41] compositor-drm: Atomic modesetting support

[Philipp Zabel]

Hi Daniel,

I think drm_pending_state_apply_atomic (or its callers) leak
pending_state:

On Wed, 2017-12-20 at 12:26 +0000, Daniel Stone wrote:
> +static int
> +drm_pending_state_apply_atomic(struct drm_pending_state *pending_state,
> +			       enum drm_state_apply_mode mode)
> +{
> +	struct drm_backend *b = pending_state->backend;
> +	struct drm_output_state *output_state, *tmp;
> +	struct drm_plane *plane;
> +	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));
> +
> +			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);
> +		}
> +
> +		/* Disable all the planes; planes which are being used will
> +		 * override this state in the output-state application. */
> +		wl_list_for_each(plane, &b->plane_list, link) {
> +			plane_add_prop(req, plane, WDRM_PLANE_CRTC_ID, 0);
> +			plane_add_prop(req, plane, WDRM_PLANE_FB_ID, 0);
> +		}
> +
> +		flags |= DRM_MODE_ATOMIC_ALLOW_MODESET;
> +	}
> +
> +	wl_list_for_each(output_state, &pending_state->output_list, link) {
> +		if (mode == DRM_STATE_APPLY_SYNC)
> +			assert(output_state->dpms == WESTON_DPMS_OFF);
> +		ret |= drm_output_apply_state_atomic(output_state, req, &flags);
> +	}
> +
> +	if (ret != 0) {
> +		weston_log("atomic: couldn't compile atomic state\n");
> +		goto out;
> +	}
> +
> +	switch (mode) {
> +	case DRM_STATE_APPLY_SYNC:
> +		break;
> +	case DRM_STATE_APPLY_ASYNC:
> +		flags |= DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_ATOMIC_NONBLOCK;
> +		break;
> +	}
> +
> +	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, mode);
> +
> +	b->state_invalid = false;
> +
> +	assert(wl_list_empty(&pending_state->output_list));
> +
> +out:

This looks like it is missing

	drm_pending_state_free(pending_state);

either here or at the callsites (will be made conditional here by the
introduction of DRM_STATE_TEST_ONLY later).

> +	drmModeAtomicFree(req);
> +	return ret;
> +}
> +#endif
> +
>  static int
>  drm_pending_state_apply(struct drm_pending_state *pending_state)
>  {
> @@ -1940,6 +2278,12 @@ drm_pending_state_apply(struct drm_pending_state *pending_state)
>  	struct drm_output_state *output_state, *tmp;
>  	uint32_t *unused;
>  
> +#ifdef HAVE_DRM_ATOMIC
> +	if (b->atomic_modeset)
> +		return drm_pending_state_apply_atomic(pending_state,
> +						      DRM_STATE_APPLY_ASYNC);

It can't be seen from the context, but drm_pending_state_apply calls
	drm_pending_state_free(pending_state);
before returning. In the atomic modeset path we don't free pending_state
anymore. Apart from drm_repaint_cancel, I don't see any other place
where
drm_pending_state_free is called.
Is there any reason to keep pending_state around beyond the end of this
function?

[...]
> @@ -1979,6 +2323,12 @@ drm_pending_state_apply_sync(struct drm_pending_state *pending_state)
>  	struct drm_output_state *output_state, *tmp;
>  	uint32_t *unused;
>  
> +#ifdef HAVE_DRM_ATOMIC
> +	if (b->atomic_modeset)
> +		return drm_pending_state_apply_atomic(pending_state,
> +						      DRM_STATE_APPLY_SYNC);

Same as drm_pending_state_apply, the way drm_pending_state_apply_sync is
called makes it look like it is supposed to take ownership of
pending_state and free it before returning. This does not happen in the
atomic modeset path.

regards
Philipp