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.
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
