On Wed, Feb 26, 2014 at 4:30 PM, Sean Paul seanpaul@chromium.org wrote:
On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark robdclark@gmail.com wrote:
Break the mutable state of a plane out into a separate structure and use atomic properties mechanism to set plane attributes. This makes it easier to have some helpers for plane->set_property() and for checking for invalid params. The idea is that individual drivers can wrap the state struct in their own struct which adds driver specific parameters, for easy build-up of state across multiple set_property() calls and for easy atomic commit or roll- back.
The same should be done for CRTC, encoder, and connector, but this patch only includes the first part (plane).
Hi Rob, I've been tracking down a crash that came up on my exynos board when I applied this patch. I hope you can hold my hand a little bit and give some guidance.
<snip>
+static int +drm_atomic_helper_commit_plane_state(struct drm_plane *plane,
struct drm_plane_state *pstate)+{
struct drm_framebuffer *old_fb = NULL, *fb = NULL;int ret = 0;fb = pstate->fb;if (pstate->crtc && fb) {ret = plane->funcs->update_plane(plane, pstate->crtc, pstate->fb,pstate->crtc_x, pstate->crtc_y, pstate->crtc_w, pstate->crtc_h,pstate->src_x, pstate->src_y, pstate->src_w, pstate->src_h);if (!ret) {/* on success, update state and fb refcnting: *//* NOTE: if we ensure no driver sets plane->state->fb = NULL* on disable, we can move this up a level and not duplicate* nearly the same thing for both update_plane and disable_plane* cases.. I leave it like this for now to be paranoid due to* the slightly different ordering in the two cases in the* original code.*/old_fb = plane->state->fb;I think this is slightly different than what we had before in setplane. In the previous code, we had:
ret = plane->funcs->update_plane(plane, crtc, fb, plane_req->crtc_x, plane_req->crtc_y, plane_req->crtc_w, plane_req->crtc_h, plane_req->src_x, plane_req->src_y, plane_req->src_w, plane_req->src_h); if (!ret) { old_fb = plane->fb; fb = NULL; plane->crtc = crtc; plane->fb = fb; }
In this case, we'd unreference old_fb which is the previous plane->fb. AFAICT, update_plane did not set plane->fb to fb, so it would, in fact, be the old plane.
to be honest, I need to dig up that branch again and remember (and rebase it while I'm at it)..
I don't think the driver should be changing state->fb (ie. the state object should in theory, once constructed, be a sort of constant thing). So until swap_plane_state() plane->state->fb is *supposed* to be the old fb.
For crtc, as there were so many places in code accessing crtc->fb (and it was pretty intertwined in the crtc helpers), the way I left it as: crtc->state->fb is what is requested by userspace, and crtc->fb is what is currently used (which is updated to the new cstate->fb in the course of modeset).
I don't remember if I left it the same way for plane (which isn't referenced in nearly as many places, and which doesn't have a big chunk of modeset helper to care about).
In the new code, drm_mode_setplane calls drm_mode_plane_set_obj_prop on fb_id, which will update plane->state->fb. Then we come in here and unreference it as old_fb. I _believe_ we're taking one more reference than we need in this case.
well, it should be updating 'pstate', what will become the new plane->state.. But not the current plane->state.
BR, -R
What I'm seeing in exynos is that when we setplane to an fb, we leave it with refcount == 1. If we disable that plane, we'll free the fb. Unfortunately, this leaves the fb in the fbs list and the next time we try to iterate that list Bad Things Happen.
Does this make any sense?
Sean
swap_plane_state(plane, pstate->state);fb = NULL;}} else {old_fb = plane->state->fb;plane->funcs->disable_plane(plane);swap_plane_state(plane, pstate->state);}if (fb)drm_framebuffer_unreference(fb);if (old_fb)drm_framebuffer_unreference(old_fb);return ret;+}
<snip>
+int drm_plane_set_property(struct drm_plane *plane,
struct drm_plane_state *state,struct drm_property *property,uint64_t value, void *blob_data)+{
struct drm_device *dev = plane->dev;struct drm_mode_config *config = &dev->mode_config;drm_object_property_set_value(&plane->base,&state->propvals, property, value, blob_data);if (property == config->prop_fb_id) {state->new_fb = true;state->fb = drm_framebuffer_lookup(dev, value);} else if (property == config->prop_crtc_id) {struct drm_mode_object *obj = drm_property_get_obj(property, value);struct drm_crtc *crtc = obj ? obj_to_crtc(obj) : NULL;/* take the lock of the incoming crtc as well, moving* plane between crtcs is synchronized on both incoming* and outgoing crtc.*/if (crtc) {int ret = drm_modeset_lock(&crtc->mutex, state->state);if (ret)return ret;}state->crtc = crtc;} else if (property == config->prop_crtc_x) {state->crtc_x = U642I64(value);} else if (property == config->prop_crtc_y) {state->crtc_y = U642I64(value);} else if (property == config->prop_crtc_w) {state->crtc_w = value;} else if (property == config->prop_crtc_h) {state->crtc_h = value;} else if (property == config->prop_src_x) {state->src_x = value;} else if (property == config->prop_src_y) {state->src_y = value;} else if (property == config->prop_src_w) {state->src_w = value;} else if (property == config->prop_src_h) {state->src_h = value;} else {return -EINVAL;}return 0;+} +EXPORT_SYMBOL(drm_plane_set_property);
<snip>
@@ -1987,20 +2192,19 @@ int drm_mode_setplane(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_set_plane *plane_req = data;
struct drm_mode_config *config = &dev->mode_config; struct drm_plane *plane;
struct drm_crtc *crtc;struct drm_framebuffer *fb = NULL, *old_fb = NULL;
void *state; int ret = 0;
unsigned int fb_width, fb_height;int i; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;/** First, find the plane, crtc, and fb objects. If not available,* we don't bother to call the driver.*/+retry:
state = dev->driver->atomic_begin(dev, 0);if (IS_ERR(state))return PTR_ERR(state);plane = drm_plane_find(dev, plane_req->plane_id); if (!plane) { DRM_DEBUG_KMS("Unknown plane ID %d\n",@@ -2008,98 +2212,37 @@ int drm_mode_setplane(struct drm_device *dev, void *data, return -ENOENT; }
/* No fb means shut it down */if (!plane_req->fb_id) {drm_modeset_lock_all(dev);old_fb = plane->fb;plane->funcs->disable_plane(plane);plane->crtc = NULL;plane->fb = NULL;drm_modeset_unlock_all(dev);goto out;}crtc = drm_crtc_find(dev, plane_req->crtc_id);if (!crtc) {DRM_DEBUG_KMS("Unknown crtc ID %d\n",plane_req->crtc_id);ret = -ENOENT;goto out;}fb = drm_framebuffer_lookup(dev, plane_req->fb_id);if (!fb) {DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",plane_req->fb_id);ret = -ENOENT;goto out;}/* Check whether this plane supports the fb pixel format. */for (i = 0; i < plane->format_count; i++)if (fb->pixel_format == plane->format_types[i])break;if (i == plane->format_count) {DRM_DEBUG_KMS("Invalid pixel format %s\n",drm_get_format_name(fb->pixel_format));ret = -EINVAL;goto out;}fb_width = fb->width << 16;fb_height = fb->height << 16;/* Make sure source coordinates are inside the fb. */if (plane_req->src_w > fb_width ||plane_req->src_x > fb_width - plane_req->src_w ||plane_req->src_h > fb_height ||plane_req->src_y > fb_height - plane_req->src_h) {DRM_DEBUG_KMS("Invalid source coordinates ""%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",plane_req->src_w >> 16,((plane_req->src_w & 0xffff) * 15625) >> 10,plane_req->src_h >> 16,((plane_req->src_h & 0xffff) * 15625) >> 10,plane_req->src_x >> 16,((plane_req->src_x & 0xffff) * 15625) >> 10,plane_req->src_y >> 16,((plane_req->src_y & 0xffff) * 15625) >> 10);ret = -ENOSPC;goto out;}/* Give drivers some help against integer overflows */if (plane_req->crtc_w > INT_MAX ||plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||plane_req->crtc_h > INT_MAX ||plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",plane_req->crtc_w, plane_req->crtc_h,plane_req->crtc_x, plane_req->crtc_y);ret = -ERANGE;
ret =drm_mode_plane_set_obj_prop(plane, state,config->prop_crtc_id, plane_req->crtc_id, NULL) ||drm_mode_plane_set_obj_prop(plane, state,config->prop_fb_id, plane_req->fb_id, NULL) ||drm_mode_plane_set_obj_prop(plane, state,config->prop_crtc_x, I642U64(plane_req->crtc_x), NULL) ||drm_mode_plane_set_obj_prop(plane, state,config->prop_crtc_y, I642U64(plane_req->crtc_y), NULL) ||drm_mode_plane_set_obj_prop(plane, state,config->prop_crtc_w, plane_req->crtc_w, NULL) ||drm_mode_plane_set_obj_prop(plane, state,config->prop_crtc_h, plane_req->crtc_h, NULL) ||drm_mode_plane_set_obj_prop(plane, state,config->prop_src_w, plane_req->src_w, NULL) ||drm_mode_plane_set_obj_prop(plane, state,config->prop_src_h, plane_req->src_h, NULL) ||drm_mode_plane_set_obj_prop(plane, state,config->prop_src_x, plane_req->src_x, NULL) ||drm_mode_plane_set_obj_prop(plane, state,config->prop_src_y, plane_req->src_y, NULL) ||dev->driver->atomic_check(dev, state);if (ret) goto out;
}drm_modeset_lock_all(dev);ret = plane->funcs->update_plane(plane, crtc, fb,plane_req->crtc_x, plane_req->crtc_y,plane_req->crtc_w, plane_req->crtc_h,plane_req->src_x, plane_req->src_y,plane_req->src_w, plane_req->src_h);if (!ret) {old_fb = plane->fb;plane->crtc = crtc;plane->fb = fb;fb = NULL;}drm_modeset_unlock_all(dev);
ret = dev->driver->atomic_commit(dev, state);out:
if (fb)drm_framebuffer_unreference(fb);if (old_fb)drm_framebuffer_unreference(old_fb);
dev->driver->atomic_end(dev, state);if (ret == -EDEADLK)goto retry; return ret;}
<snip>