On Fri, Apr 15, 2016 at 03:10:43PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@xxxxxxxxxx>
>
> This uses the previous changes to add reference counting to the
> drm connector objects.
>
> Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx>
Bunch of nitpicks in this one. And I noticed a pretty serious issue we
have with the generic prop code since forever. I'll write testcases and
bugfixes for that (hopefully today).
> ---
> drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++++++--
> drivers/gpu/drm/drm_crtc.c | 39 ++++++++++++++++++++++++++++++---------
> drivers/gpu/drm/drm_fb_helper.c | 38 ++++++--------------------------------
> include/drm/drm_crtc.h | 13 ++++++++++++-
> 4 files changed, 65 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8ee1db8..9d5e3c8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -154,6 +154,7 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> state->connector_states[i]);
> state->connectors[i] = NULL;
> state->connector_states[i] = NULL;
> + drm_connector_unreference(connector);
> }
>
> for (i = 0; i < config->num_crtc; i++) {
> @@ -924,6 +925,7 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
> if (!connector_state)
> return ERR_PTR(-ENOMEM);
>
> + drm_connector_reference(connector);
> state->connector_states[index] = connector_state;
> state->connectors[index] = connector;
> connector_state->state = state;
> @@ -1614,12 +1616,19 @@ retry:
> }
>
> obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
> - if (!obj || !obj->properties) {
> + if (!obj) {
> + ret = -ENOENT;
> + goto out;
> + }
> +
> + if (!obj->properties) {
> + drm_mode_object_unreference(obj);
> ret = -ENOENT;
> goto out;
> }
>
> if (get_user(count_props, count_props_ptr + copied_objs)) {
> + drm_mode_object_unreference(obj);
> ret = -EFAULT;
> goto out;
> }
> @@ -1632,12 +1641,14 @@ retry:
> struct drm_property *prop;
>
> if (get_user(prop_id, props_ptr + copied_props)) {
> + drm_mode_object_unreference(obj);
> ret = -EFAULT;
> goto out;
> }
>
> prop = drm_property_find(dev, prop_id);
> if (!prop) {
> + drm_mode_object_unreference(obj);
> ret = -ENOENT;
> goto out;
> }
> @@ -1645,13 +1656,16 @@ retry:
> if (copy_from_user(&prop_value,
> prop_values_ptr + copied_props,
> sizeof(prop_value))) {
> + drm_mode_object_unreference(obj);
> ret = -EFAULT;
> goto out;
> }
>
> ret = atomic_set_prop(state, obj, prop, prop_value);
> - if (ret)
> + if (ret) {
> + drm_mode_object_unreference(obj);
> goto out;
> + }
>
> copied_props++;
> }
> @@ -1662,6 +1676,7 @@ retry:
> plane_mask |= (1 << drm_plane_index(plane));
> plane->old_fb = plane->fb;
> }
> + drm_mode_object_unreference(obj);
Could we maybe do something like this to share the unreference calls:
continue;
fail_loop:
drm_mode_object_unreference(obj);
goto out;
And then change all the goto out; in the loop to goto fail_loop;? Just a
suggestion, either is fine with me. I just like common exit code a lot.
> }
>
> if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f6bf828..90bc597 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -861,6 +861,16 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
> mode->interlace ? " interlaced" : "");
> }
>
> +static void drm_connector_free(struct kref *kref)
> +{
> + struct drm_connector *connector =
> + container_of(kref, struct drm_connector, base.refcount);
> + struct drm_device *dev = connector->dev;
> +
> + drm_mode_object_unregister(dev, &connector->base);
> + connector->funcs->destroy(connector);
> +}
> +
> /**
> * drm_connector_init - Init a preallocated connector
> * @dev: DRM device
> @@ -886,7 +896,7 @@ int drm_connector_init(struct drm_device *dev,
>
> drm_modeset_lock_all(dev);
>
> - ret = drm_mode_object_get_reg(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR, false, NULL);
> + ret = drm_mode_object_get_reg(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR, false, drm_connector_free);
Too long line.
> if (ret)
> goto out_unlock;
>
> @@ -2106,7 +2116,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>
> mutex_lock(&dev->mode_config.mutex);
>
> - connector = drm_connector_find(dev, out_resp->connector_id);
> + connector = drm_connector_lookup(dev, out_resp->connector_id);
> if (!connector) {
> ret = -ENOENT;
> goto out_unlock;
> @@ -2190,6 +2200,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> out:
> drm_modeset_unlock(&dev->mode_config.connection_mutex);
>
> + drm_connector_unreference(connector);
> out_unlock:
> mutex_unlock(&dev->mode_config.mutex);
>
> @@ -2834,13 +2845,14 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> }
>
> for (i = 0; i < crtc_req->count_connectors; i++) {
> + connector_set[i] = NULL;
> set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
> if (get_user(out_id, &set_connectors_ptr[i])) {
> ret = -EFAULT;
> goto out;
> }
>
> - connector = drm_connector_find(dev, out_id);
> + connector = drm_connector_lookup(dev, out_id);
> if (!connector) {
> DRM_DEBUG_KMS("Connector id %d unknown\n",
> out_id);
> @@ -2868,6 +2880,12 @@ out:
> if (fb)
> drm_framebuffer_unreference(fb);
>
> + if (connector_set) {
> + for (i = 0; i < crtc_req->count_connectors; i++) {
> + if (connector_set[i])
> + drm_connector_unreference(connector_set[i]);
> + }
> + }
> kfree(connector_set);
> drm_mode_destroy(dev, mode);
> drm_modeset_unlock_all(dev);
> @@ -4957,7 +4975,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
Preexisting bug, but we don't filter out FB and BLOB properties here.
Which means at least we'll blow up on the WARN_ON, but I think we can also
nicely leak references with your patches now (because at least fb
references are grabbed in _object_find now) :(
We do filter out FB and BLOB obj with the obj->properties check, but
that's a use-after-free in 4.6 (and the ioctl will leak with your patches
now in 4.7).
> }
> if (!obj->properties) {
> ret = -EINVAL;
> - goto out;
> + goto out_unref;
> }
>
> ret = get_properties(obj, file_priv->atomic,
> @@ -4965,6 +4983,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> (uint64_t __user *)(unsigned long)(arg->prop_values_ptr),
> &arg->count_props);
>
> +out_unref:
> + drm_mode_object_unreference(obj);
> out:
> drm_modeset_unlock_all(dev);
> return ret;
> @@ -5007,25 +5027,25 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
Same issue with FB and BLOB here as with get_property_ioctl.
> goto out;
> }
> if (!arg_obj->properties)
> - goto out;
> + goto out_unref;
>
> for (i = 0; i < arg_obj->properties->count; i++)
> if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
> break;
>
> if (i == arg_obj->properties->count)
> - goto out;
> + goto out_unref;
>
> prop_obj = drm_mode_object_find(dev, arg->prop_id,
> DRM_MODE_OBJECT_PROPERTY);
> if (!prop_obj) {
> ret = -ENOENT;
> - goto out;
> + goto out_unref;
> }
> property = obj_to_property(prop_obj);
>
> if (!drm_property_change_valid_get(property, arg->value, &ref))
> - goto out;
> + goto out_unref;
>
> switch (arg_obj->type) {
> case DRM_MODE_OBJECT_CONNECTOR:
> @@ -5042,7 +5062,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
> }
>
> drm_property_change_valid_put(property, ref);
> -
> +out_unref:
> + drm_mode_object_unreference(arg_obj);
> out:
> drm_modeset_unlock_all(dev);
> return ret;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 855108e..3691565 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -153,40 +153,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
> if (!fb_helper_connector)
> return -ENOMEM;
>
> + drm_connector_reference(connector);
I think it'd be good to split the changes for the fbdev helper into a
separate patch, for easier reviewing, and it seems inconsistent: You grab
a ref here, but don't drop it in remove_one_connector.
Also I think we need to grab references for all the intial connectors too,
and drop them on unload, in case you boot up (or reload) with an mst
connector plugged in. You do some of that in drm_setup_crtcs, but I think
it's better to do that in drm_fb_helper_single_add_all_connectors and the
final cleanup code, and not have any refcounting in drm_setup_crtc (but
instead just inherit the refcount from the connector_info array for the
fbdev modeset config structures).
> fb_helper_connector->connector = connector;
> fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
> return 0;
> }
> EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
>
> -static void remove_from_modeset(struct drm_mode_set *set,
> - struct drm_connector *connector)
> -{
> - int i, j;
> -
> - for (i = 0; i < set->num_connectors; i++) {
> - if (set->connectors[i] == connector)
> - break;
> - }
> -
> - if (i == set->num_connectors)
> - return;
> -
> - for (j = i + 1; j < set->num_connectors; j++) {
> - set->connectors[j - 1] = set->connectors[j];
> - }
> - set->num_connectors--;
> -
> - /*
> - * TODO maybe need to makes sure we set it back to !=NULL somewhere?
> - */
> - if (set->num_connectors == 0) {
> - set->fb = NULL;
> - drm_mode_destroy(connector->dev, set->mode);
> - set->mode = NULL;
> - }
> -}
> -
> int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
> struct drm_connector *connector)
> {
> @@ -213,10 +186,6 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
> fb_helper->connector_count--;
> kfree(fb_helper_connector);
>
> - /* also cleanup dangling references to the connector: */
> - for (i = 0; i < fb_helper->crtc_count; i++)
> - remove_from_modeset(&fb_helper->crtc_info[i].mode_set, connector);
> -
> return 0;
> }
> EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
> @@ -2024,7 +1993,11 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> /* need to set the modesets up here for use later */
> /* fill out the connector<->crtc mappings into the modesets */
> for (i = 0; i < fb_helper->crtc_count; i++) {
> + int j;
> modeset = &fb_helper->crtc_info[i].mode_set;
> +
> + for (j = 0; j < modeset->num_connectors; j++)
> + drm_connector_unreference(modeset->connectors[j]);
> modeset->num_connectors = 0;
> modeset->fb = NULL;
> }
> @@ -2045,6 +2018,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> drm_mode_destroy(dev, modeset->mode);
> modeset->mode = drm_mode_duplicate(dev,
> fb_crtc->desired_mode);
> + drm_connector_reference(fb_helper->connector_info[i]->connector);
> modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
> modeset->fb = fb_helper->fb;
> modeset->x = offset->x;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 576faf4..7e13127 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2579,7 +2579,8 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
> return mo ? obj_to_encoder(mo) : NULL;
> }
>
> -static inline struct drm_connector *drm_connector_find(struct drm_device *dev,
> +/* takes a reference */
> +static inline struct drm_connector *drm_connector_lookup(struct drm_device *dev,
> uint32_t id)
> {
> struct drm_mode_object *mo;
> @@ -2625,6 +2626,16 @@ static inline uint32_t drm_framebuffer_read_refcount(struct drm_framebuffer *fb)
> return atomic_read(&fb->base.refcount.refcount);
> }
>
> +static inline void drm_connector_reference(struct drm_connector *connector)
> +{
> + drm_mode_object_reference(&connector->base);
> +}
> +
> +static inline void drm_connector_unreference(struct drm_connector *connector)
kerneldoc for these two plus drm_connector_lookup would be good with the
usual few lines about the refcounting. That also gives you a good place to
put the "takes a reference" comment above.
> +{
> + drm_mode_object_unreference(&connector->base);
> +}
> +
> /* Plane list iterator for legacy (overlay only) planes. */
> #define drm_for_each_legacy_plane(plane, dev) \
> list_for_each_entry(plane, &(dev)->mode_config.plane_list, head) \
> --
> 2.5.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
