On Mon, Mar 3, 2014 at 2:22 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
> 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?
I *think* so.. although to be safe you should probably ask me again
after I rebase atomic and have my head back in it..
tbh, I'm not completely happy with the way fb refcnt'ing is working at
the moment with atomic. In particular to deal with edge cases like
userspace setting same fb property twice in one ioctl. I'm thinking
about switching to have two ref's held to the fb, so the crtc/plane
holds one ref for active scanout buffer (as it does currently), and
the state object holds it's own ref.
BR,
-R
>
> 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
