[RFCv2 10/13] drm: convert plane to properties/state
Rob Clark
robdclark at gmail.com
Fri Oct 25 01:43:38 CEST 2013
On Thu, Oct 24, 2013 at 5:48 PM, Matt Roper <matthew.d.roper at intel.com> wrote:
> On Mon, Oct 14, 2013 at 01:26:45PM -0400, Rob Clark 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).
>> ---
>> drivers/gpu/drm/drm_atomic_helper.c | 137 +++++++++-
>> drivers/gpu/drm/drm_crtc.c | 399 +++++++++++++++++++---------
>> drivers/gpu/drm/drm_fb_helper.c | 17 +-
>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 4 +-
>> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 6 +-
>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 13 +-
>> drivers/gpu/drm/i915/intel_sprite.c | 19 +-
>> drivers/gpu/drm/msm/mdp4/mdp4_crtc.c | 2 +-
>> drivers/gpu/drm/msm/mdp4/mdp4_plane.c | 18 +-
>> drivers/gpu/drm/omapdrm/omap_crtc.c | 4 +-
>> drivers/gpu/drm/omapdrm/omap_drv.c | 2 +-
>> drivers/gpu/drm/omapdrm/omap_plane.c | 30 ++-
>> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 5 +-
>> drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 2 +-
>> drivers/gpu/drm/shmobile/shmob_drm_plane.c | 6 +-
>> drivers/gpu/host1x/drm/dc.c | 16 +-
>> include/drm/drm_atomic_helper.h | 37 ++-
>> include/drm/drm_crtc.h | 88 +++++-
>> 18 files changed, 615 insertions(+), 190 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 46c67b8..0618113 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -39,10 +39,12 @@
>> void *drm_atomic_helper_begin(struct drm_device *dev, uint32_t flags)
>> {
>> struct drm_atomic_helper_state *state;
>> + int nplanes = dev->mode_config.num_plane;
>> int sz;
>> void *ptr;
>>
>> sz = sizeof(*state);
>> + sz += (sizeof(state->planes) + sizeof(state->pstates)) * nplanes;
>>
>> ptr = kzalloc(sz, GFP_KERNEL);
>>
>> @@ -52,6 +54,13 @@ void *drm_atomic_helper_begin(struct drm_device *dev, uint32_t flags)
>> kref_init(&state->refcount);
>> state->dev = dev;
>> state->flags = flags;
>> +
>> + state->planes = ptr;
>> + ptr = &state->planes[nplanes];
>> +
>> + state->pstates = ptr;
>> + ptr = &state->pstates[nplanes];
>> +
>> return state;
>> }
>> EXPORT_SYMBOL(drm_atomic_helper_begin);
>> @@ -87,7 +96,19 @@ EXPORT_SYMBOL(drm_atomic_helper_set_event);
>> */
>> int drm_atomic_helper_check(struct drm_device *dev, void *state)
>> {
>> - return 0; /* for now */
>> + struct drm_atomic_helper_state *a = state;
>> + int nplanes = dev->mode_config.num_plane;
>> + int i, ret = 0;
>> +
>> + for (i = 0; i < nplanes; i++) {
>> + if (a->planes[i]) {
>> + ret = drm_atomic_check_plane_state(a->planes[i], a->pstates[i]);
>> + if (ret)
>> + break;
>> + }
>> + }
>> +
>> + return ret;
>> }
>> EXPORT_SYMBOL(drm_atomic_helper_check);
>>
>> @@ -104,7 +125,19 @@ EXPORT_SYMBOL(drm_atomic_helper_check);
>> */
>> int drm_atomic_helper_commit(struct drm_device *dev, void *state)
>> {
>> - return 0; /* for now */
>> + struct drm_atomic_helper_state *a = state;
>> + int nplanes = dev->mode_config.num_plane;
>> + int i, ret = 0;
>> +
>> + for (i = 0; i < nplanes; i++) {
>> + if (a->planes[i]) {
>> + ret = drm_atomic_commit_plane_state(a->planes[i], a->pstates[i]);
>> + if (ret)
>> + break;
>> + }
>> + }
>> +
>> + return ret;
>> }
>> EXPORT_SYMBOL(drm_atomic_helper_commit);
>>
>> @@ -125,11 +158,111 @@ void _drm_atomic_helper_state_free(struct kref *kref)
>> {
>> struct drm_atomic_helper_state *state =
>> container_of(kref, struct drm_atomic_helper_state, refcount);
>> + struct drm_device *dev = state->dev;
>> + int nplanes = dev->mode_config.num_plane;
>> + int i;
>> +
>> + for (i = 0; i < nplanes; i++) {
>> + if (state->pstates[i]) {
>> + state->planes[i]->state->state = NULL;
>> + kfree(state->pstates[i]);
>> + }
>> + }
>> +
>> kfree(state);
>> }
>> EXPORT_SYMBOL(_drm_atomic_helper_state_free);
>>
>> +int drm_atomic_helper_plane_set_property(struct drm_plane *plane, void *state,
>> + struct drm_property *property, uint64_t val, void *blob_data)
>> +{
>> + return drm_plane_set_property(plane,
>> + drm_atomic_get_plane_state(plane, state),
>> + property, val, blob_data);
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_plane_set_property);
>> +
>> +void drm_atomic_helper_init_plane_state(struct drm_plane *plane,
>> + struct drm_plane_state *pstate, void *state)
>> +{
>> + /* snapshot current state: */
>> + *pstate = *plane->state;
>> + pstate->state = state;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_init_plane_state);
>> +
>> +static struct drm_plane_state *
>> +drm_atomic_helper_get_plane_state(struct drm_plane *plane, void *state)
>> +{
>> + struct drm_atomic_helper_state *a = state;
>> + struct drm_plane_state *pstate = a->pstates[plane->id];
>> + if (!pstate) {
>> + pstate = kzalloc(sizeof(*pstate), GFP_KERNEL);
>> + drm_atomic_helper_init_plane_state(plane, pstate, state);
>> + a->planes[plane->id] = plane;
>> + a->pstates[plane->id] = pstate;
>> + }
>> + return pstate;
>> +}
>> +
>> +static void
>> +swap_plane_state(struct drm_plane *plane, struct drm_atomic_helper_state *a)
>> +{
>> + swap(plane->state, a->pstates[plane->id]);
>> + plane->base.propvals = &plane->state->propvals;
>> +}
>> +
>> +static int
>> +drm_atomic_helper_commit_plane_state(struct drm_plane *plane,
>> + struct drm_plane_state *pstate)
>> +{
>> + struct drm_device *dev = plane->dev;
>> + struct drm_framebuffer *old_fb = NULL, *fb = NULL;
>> + int ret = 0;
>> +
>> + /* probably more fine grain locking would be ok of old crtc
>> + * and new crtc were same..
>> + */
>> + drm_modeset_lock_all(dev);
>> +
>> + 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;
>> + 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);
>> + }
>> +
>> + drm_modeset_unlock_all(dev);
>> +
>> + if (fb)
>> + drm_framebuffer_unreference(fb);
>> + if (old_fb)
>> + drm_framebuffer_unreference(old_fb);
>> +
>> + return ret;
>> +}
>>
>> const struct drm_atomic_helper_funcs drm_atomic_helper_funcs = {
>> + .get_plane_state = drm_atomic_helper_get_plane_state,
>> + .check_plane_state = drm_plane_check_state,
>> + .commit_plane_state = drm_atomic_helper_commit_plane_state,
>> };
>> EXPORT_SYMBOL(drm_atomic_helper_funcs);
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 9f46f3b..3cf235e 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -595,7 +595,20 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>> * in this manner.
>> */
>> if (atomic_read(&fb->refcount.refcount) > 1) {
>> + void *state;
>> +
>> + state = dev->driver->atomic_begin(dev, 0);
>> + if (IS_ERR(state)) {
>> + DRM_ERROR("failed to disable crtc and/or plane when fb was deleted\n");
>> + return;
>> + }
>> +
>> + /* TODO once CRTC is converted to state/properties, we can push the
>> + * locking down into drm_atomic_helper_commit(), since that is where
>> + * the actual changes take place..
>> + */
>> drm_modeset_lock_all(dev);
>> +
>> /* remove from any CRTC */
>> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>> if (crtc->fb == fb) {
>> @@ -610,9 +623,18 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>> }
>>
>> list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
>> - if (plane->fb == fb)
>> - drm_plane_force_disable(plane);
>> + if (plane->state->fb == fb)
>> + drm_plane_force_disable(plane, state);
>> }
>> +
>> + /* just disabling stuff shouldn't fail, hopefully: */
>> + if(dev->driver->atomic_check(dev, state))
>> + DRM_ERROR("failed to disable crtc and/or plane when fb was deleted\n");
>> + else
>> + dev->driver->atomic_commit(dev, state);
>> +
>> + dev->driver->atomic_end(dev, state);
>> +
>> drm_modeset_unlock_all(dev);
>> }
>>
>> @@ -904,8 +926,12 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>> const uint32_t *formats, uint32_t format_count,
>> bool priv)
>> {
>> + struct drm_mode_config *config = &dev->mode_config;
>> int ret;
>>
>> + if (!plane->state)
>> + plane->state = kzalloc(sizeof(plane->state), GFP_KERNEL);
>> +
>> drm_modeset_lock_all(dev);
>>
>> ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
>> @@ -913,7 +939,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>> goto out;
>>
>> plane->base.properties = &plane->properties;
>> - plane->base.propvals = &plane->propvals;
>> + plane->base.propvals = &plane->state->propvals;
>> plane->dev = dev;
>> plane->funcs = funcs;
>> plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
>> @@ -935,11 +961,23 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>> */
>> if (!priv) {
>> list_add_tail(&plane->head, &dev->mode_config.plane_list);
>> + plane->id = dev->mode_config.num_plane;
>> dev->mode_config.num_plane++;
>> } else {
>> INIT_LIST_HEAD(&plane->head);
>> }
>>
>> + drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
>> + drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
>> + drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
>> + drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
>> + drm_object_attach_property(&plane->base, config->prop_crtc_w, 0);
>> + drm_object_attach_property(&plane->base, config->prop_crtc_h, 0);
>> + drm_object_attach_property(&plane->base, config->prop_src_x, 0);
>> + drm_object_attach_property(&plane->base, config->prop_src_y, 0);
>> + drm_object_attach_property(&plane->base, config->prop_src_w, 0);
>> + drm_object_attach_property(&plane->base, config->prop_src_h, 0);
>> +
>> out:
>> drm_modeset_unlock_all(dev);
>>
>> @@ -971,6 +1009,111 @@ void drm_plane_cleanup(struct drm_plane *plane)
>> }
>> EXPORT_SYMBOL(drm_plane_cleanup);
>>
>> +int drm_plane_check_state(struct drm_plane *plane,
>> + struct drm_plane_state *state)
>> +{
>> + unsigned int fb_width, fb_height;
>> + struct drm_framebuffer *fb = state->fb;
>> + int i;
>> +
>> + /* disabling the plane is allowed: */
>> + if (!fb)
>> + return 0;
>> +
>> + fb_width = fb->width << 16;
>> + fb_height = fb->height << 16;
>> +
>> + /* 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 0x%08x\n", fb->pixel_format);
>> + return -EINVAL;
>> + }
>> +
>> + /* Make sure source coordinates are inside the fb. */
>> + if (state->src_w > fb_width ||
>> + state->src_x > fb_width - state->src_w ||
>> + state->src_h > fb_height ||
>> + state->src_y > fb_height - state->src_h) {
>> + DRM_DEBUG_KMS("Invalid source coordinates "
>> + "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
>> + state->src_w >> 16,
>> + ((state->src_w & 0xffff) * 15625) >> 10,
>> + state->src_h >> 16,
>> + ((state->src_h & 0xffff) * 15625) >> 10,
>> + state->src_x >> 16,
>> + ((state->src_x & 0xffff) * 15625) >> 10,
>> + state->src_y >> 16,
>> + ((state->src_y & 0xffff) * 15625) >> 10);
>> + return -ENOSPC;
>> + }
>> +
>> + /* Give drivers some help against integer overflows */
>> + if (state->crtc_w > INT_MAX ||
>> + state->crtc_x > INT_MAX - (int32_t) state->crtc_w ||
>> + state->crtc_h > INT_MAX ||
>> + state->crtc_y > INT_MAX - (int32_t) state->crtc_h) {
>> + DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
>> + state->crtc_w, state->crtc_h,
>> + state->crtc_x, state->crtc_y);
>> + return -ERANGE;
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_plane_check_state);
>> +
>> +void drm_plane_commit_state(struct drm_plane *plane,
>> + struct drm_plane_state *state)
>> +{
>> + plane->state = state;
>> + plane->base.propvals = &state->propvals;
>> +}
>> +EXPORT_SYMBOL(drm_plane_commit_state);
>> +
>> +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);
>> + state->crtc = obj ? obj_to_crtc(obj) : NULL;
>> + } 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);
>> +
>> /**
>> * drm_plane_force_disable - Forcibly disable a plane
>> * @plane: plane to disable
>> @@ -980,20 +1123,15 @@ EXPORT_SYMBOL(drm_plane_cleanup);
>> * Used when the plane's current framebuffer is destroyed,
>> * and when restoring fbdev mode.
>> */
>> -void drm_plane_force_disable(struct drm_plane *plane)
>> +void drm_plane_force_disable(struct drm_plane *plane, void *state)
>> {
>> - int ret;
>> + struct drm_mode_config *config = &plane->dev->mode_config;
>>
>> - if (!plane->fb)
>> - return;
>> -
>> - ret = plane->funcs->disable_plane(plane);
>> - if (ret)
>> - DRM_ERROR("failed to disable plane with busy fb\n");
>> - /* disconnect the plane from the fb and crtc: */
>> - __drm_framebuffer_unreference(plane->fb);
>> - plane->fb = NULL;
>> - plane->crtc = NULL;
>> + /* should turn off the crtc */
>> + drm_mode_plane_set_obj_prop(plane, state,
>> + config->prop_crtc_id, 0, NULL);
>> + drm_mode_plane_set_obj_prop(plane, state,
>> + config->prop_fb_id, 0, NULL);
>> }
>> EXPORT_SYMBOL(drm_plane_force_disable);
>>
>> @@ -1043,21 +1181,89 @@ EXPORT_SYMBOL(drm_mode_destroy);
>>
>> static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
>> {
>> - struct drm_property *edid;
>> - struct drm_property *dpms;
>> + struct drm_property *prop;
>>
>> /*
>> * Standard properties (apply to all connectors)
>> */
>> - edid = drm_property_create(dev, DRM_MODE_PROP_BLOB |
>> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB |
>> DRM_MODE_PROP_IMMUTABLE,
>> "EDID", 0);
>> - dev->mode_config.edid_property = edid;
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.edid_property = prop;
>>
>> - dpms = drm_property_create_enum(dev, 0,
>> + prop = drm_property_create_enum(dev, 0,
>> "DPMS", drm_dpms_enum_list,
>> ARRAY_SIZE(drm_dpms_enum_list));
>> - dev->mode_config.dpms_property = dpms;
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.dpms_property = prop;
>> +
>> +
>> + /* TODO we need the driver to control which of these are dynamic
>> + * and which are not.. or maybe we should just set all to zero
>> + * and let the individual drivers frob the prop->flags for the
>> + * properties they can support dynamic changes on..
>> + */
>> +
>> + prop = drm_property_create_range(dev, DRM_MODE_PROP_DYNAMIC,
>> + "SRC_X", 0, UINT_MAX);
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_src_x = prop;
>> +
>> + prop = drm_property_create_range(dev, DRM_MODE_PROP_DYNAMIC,
>> + "SRC_Y", 0, UINT_MAX);
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_src_y = prop;
>> +
>> + prop = drm_property_create_range(dev, 0, "SRC_W", 0, UINT_MAX);
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_src_w = prop;
>> +
>> + prop = drm_property_create_range(dev, 0, "SRC_H", 0, UINT_MAX);
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_src_h = prop;
>> +
>> + prop = drm_property_create_range(dev,
>> + DRM_MODE_PROP_DYNAMIC | DRM_MODE_PROP_SIGNED,
>> + "CRTC_X", I642U64(INT_MIN), I642U64(INT_MAX));
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_crtc_x = prop;
>> +
>> + prop = drm_property_create_range(dev,
>> + DRM_MODE_PROP_DYNAMIC | DRM_MODE_PROP_SIGNED,
>> + "CRTC_Y", I642U64(INT_MIN), I642U64(INT_MAX));
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_crtc_y = prop;
>> +
>> + prop = drm_property_create_range(dev, 0, "CRTC_W", 0, INT_MAX);
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_crtc_w = prop;
>> +
>> + prop = drm_property_create_range(dev, 0, "CRTC_H", 0, INT_MAX);
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_crtc_h = prop;
>> +
>> + prop = drm_property_create_object(dev, DRM_MODE_PROP_DYNAMIC,
>> + "FB_ID", DRM_MODE_OBJECT_FB);
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_fb_id = prop;
>> +
>> + prop = drm_property_create_object(dev, 0,
>> + "CRTC_ID", DRM_MODE_OBJECT_CRTC);
>> + if (!prop)
>> + return -ENOMEM;
>> + dev->mode_config.prop_crtc_id = prop;
>>
>> return 0;
>> }
>> @@ -1842,13 +2048,13 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>> }
>> plane = obj_to_plane(obj);
>>
>> - if (plane->crtc)
>> - plane_resp->crtc_id = plane->crtc->base.id;
>> + if (plane->state->crtc)
>> + plane_resp->crtc_id = plane->state->crtc->base.id;
>> else
>> plane_resp->crtc_id = 0;
>>
>> - if (plane->fb)
>> - plane_resp->fb_id = plane->fb->base.id;
>> + if (plane->state->fb)
>> + plane_resp->fb_id = plane->state->fb->base.id;
>> else
>> plane_resp->fb_id = 0;
>>
>> @@ -1890,21 +2096,18 @@ 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_mode_object *obj;
>> - 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.
>> - */
>> + state = dev->driver->atomic_begin(dev, 0);
>> + if (IS_ERR(state))
>> + return PTR_ERR(state);
>> +
>> obj = drm_mode_object_find(dev, plane_req->plane_id,
>> DRM_MODE_OBJECT_PLANE);
>> if (!obj) {
>> @@ -1912,102 +2115,36 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>> plane_req->plane_id);
>> return -ENOENT;
>> }
>> - plane = obj_to_plane(obj);
>> -
>> - /* 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;
>> - }
>>
>> - obj = drm_mode_object_find(dev, plane_req->crtc_id,
>> - DRM_MODE_OBJECT_CRTC);
>> - if (!obj) {
>> - DRM_DEBUG_KMS("Unknown crtc ID %d\n",
>> - plane_req->crtc_id);
>> - ret = -ENOENT;
>> - goto out;
>> - }
>> - crtc = obj_to_crtc(obj);
>> -
>> - 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_set_obj_prop(obj, state,
>> + config->prop_crtc_id, plane_req->crtc_id, NULL) ||
>> + drm_mode_set_obj_prop(obj, state,
>> + config->prop_fb_id, plane_req->fb_id, NULL) ||
>> + drm_mode_set_obj_prop(obj, state,
>> + config->prop_crtc_x, I642U64(plane_req->crtc_x), NULL) ||
>> + drm_mode_set_obj_prop(obj, state,
>> + config->prop_crtc_y, I642U64(plane_req->crtc_y), NULL) ||
>> + drm_mode_set_obj_prop(obj, state,
>> + config->prop_crtc_w, plane_req->crtc_w, NULL) ||
>> + drm_mode_set_obj_prop(obj, state,
>> + config->prop_crtc_h, plane_req->crtc_h, NULL) ||
>> + drm_mode_set_obj_prop(obj, state,
>> + config->prop_src_w, plane_req->src_w, NULL) ||
>> + drm_mode_set_obj_prop(obj, state,
>> + config->prop_src_h, plane_req->src_h, NULL) ||
>> + drm_mode_set_obj_prop(obj, state,
>> + config->prop_src_x, plane_req->src_x, NULL) ||
>> + drm_mode_set_obj_prop(obj, 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);
>> return ret;
>> }
>>
>> @@ -3296,7 +3433,7 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
>> return drm_mode_obj_set_property_ioctl(dev, &obj_set_prop, file_priv);
>> }
>>
>> -static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
>> +int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
>> void *state, struct drm_property *property,
>> uint64_t value, void *blob_data)
>> {
>> @@ -3319,8 +3456,9 @@ static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
>>
>> return ret;
>> }
>> +EXPORT_SYMBOL(drm_mode_connector_set_obj_prop);
>>
>> -static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
>> +int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
>> void *state, struct drm_property *property,
>> uint64_t value, void *blob_data)
>> {
>> @@ -3335,8 +3473,9 @@ static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
>>
>> return ret;
>> }
>> +EXPORT_SYMBOL(drm_mode_crtc_set_obj_prop);
>>
>> -static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>> +int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>> void *state, struct drm_property *property,
>> uint64_t value, void *blob_data)
>> {
>> @@ -3345,12 +3484,10 @@ static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>> if (plane->funcs->set_property)
>> ret = plane->funcs->set_property(plane, state, property,
>> value, blob_data);
>> - if (!ret)
>> - drm_object_property_set_value(&plane->base, &plane->propvals,
>> - property, value, NULL);
>>
>> return ret;
>> }
>> +EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
>>
>> static int drm_mode_set_obj_prop(struct drm_mode_object *obj,
>> void *state, struct drm_property *property,
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 24898dc..f1fc605 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -290,12 +290,27 @@ bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>> struct drm_device *dev = fb_helper->dev;
>> struct drm_plane *plane;
>> bool error = false;
>> + void *state;
>> int i;
>>
>> drm_warn_on_modeset_not_all_locked(dev);
>>
>> + state = dev->driver->atomic_begin(dev, 0);
>> + if (IS_ERR(state)) {
>> + DRM_ERROR("failed to restore fbdev mode\n");
>> + return true;
>> + }
>> +
>> list_for_each_entry(plane, &dev->mode_config.plane_list, head)
>> - drm_plane_force_disable(plane);
>> + drm_plane_force_disable(plane, state);
>> +
>> + /* just disabling stuff shouldn't fail, hopefully: */
>> + if(dev->driver->atomic_check(dev, state))
>> + DRM_ERROR("failed to restore fbdev mode\n");
>> + else
>> + dev->driver->atomic_commit(dev, state);
>
> I'm seeing some deadlocks here on i915 when we trigger the lastclose
> handler. We're already holding mode_config.mutex when we enter this
> function, but drm_atomic_helper_commit_plane_state does another
> drm_modeset_lock_all() call.
oh, sorry, I should have mentioned that for now I got rid of the locks
around the call to drm_fb_helper_restore_fbdev_mode().. that hack is
intended to be cleaned up somehow by (I think) pushing the lock down
into _restore_fbdev_mode(), plus ww_mutex conversion, which will be
part of next next round of RFC (as soon as I have time to implement
it).
BR,
-R
More information about the dri-devel
mailing list