[RFCv4 09/14] drm: convert plane to properties/state
Sean Paul
seanpaul at chromium.org
Wed Feb 26 13:30:45 PST 2014
On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark <robdclark at 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.
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.
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>
More information about the dri-devel
mailing list