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
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
