On Wed, Feb 26, 2014 at 7:18 PM, Rob Clark <robdclark@xxxxxxxxx> wrote:
> On Wed, Feb 26, 2014 at 4:30 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
>> On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark <robdclark@xxxxxxxxx> 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.
>
Hey Rob,
Sorry for the delay getting back to you. Indeed, I was confused in my
last email, thanks for straightening me out.
I traced through things a little more carefully and it looks like my
fb is being unreferenced every time we call set_property on the plane.
On exynos, we set a private zpos property via the set_property ioctl.
In this case, drm_mode_set_property_ioctl does not take a reference on
the plane's current fb, but we put a reference in atomic_commit
(assuming it's got a valid crtc/fb). Eventually, this runs the
refcount down to 0 and we end up freeing the fb early.
I think the fix here is to only unreference the plane's current fb in
the case where new_fb is true. ie:
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 14e0571..ec60d4e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -394,7 +394,8 @@ drm_atomic_helper_commit_plane_state(struct
drm_plane *plane,
* the slightly different ordering in the two
cases in the
* original code.
*/
- old_fb = plane->state->fb;
+ if (pstate->new_fb)
+ old_fb = plane->state->fb;
swap_plane_state(plane, pstate->state);
fb = NULL;
}
Seem reasonable?
Sean
> 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>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
