On Sat, Jul 26, 2014 at 12:51 AM, Rob Clark <robdclark@xxxxxxxxx> wrote:
> We're going to need this for atomic.
>
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
I disagree. Iiui correctly Rob's concern is that the additional stuff
to keep track of mode lists (list_head and the idr stuff) could
confuse driver writers into doing stupid stuff when they embed
drm_display_mode into some other stuff. Imo the right fix would be to
just remove them (but that's fairly invasive to the mode list code).
Now wrt atomic we only need refcounting because currently
drm_atomic_state is refcounted. And we don't need that afaics (and I'm
working on the draft code to show how). So without a clear need for
refcounting I really prefer we don't add this complexity - doing
refcounting for fbs wasn't fun at all ;-)
-Daniel
> ---
> drivers/gpu/drm/drm_crtc.c | 4 +--
> drivers/gpu/drm/drm_crtc_helper.c | 2 +-
> drivers/gpu/drm/drm_edid.c | 6 ++--
> drivers/gpu/drm/drm_fb_helper.c | 6 ++--
> drivers/gpu/drm/drm_modes.c | 41 ++++++++++++++++++++-------
> drivers/gpu/drm/exynos/exynos_drm_connector.c | 2 +-
> drivers/gpu/drm/gma500/psb_intel_sdvo.c | 3 +-
> drivers/gpu/drm/i915/intel_panel.c | 8 ++----
> drivers/gpu/drm/i915/intel_sdvo.c | 3 +-
> drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
> drivers/gpu/drm/omapdrm/omap_connector.c | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +--
> include/drm/drm_modes.h | 5 +++-
> 13 files changed, 52 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 1ccf5cb..7a7fced 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -822,7 +822,7 @@ static void drm_mode_remove(struct drm_connector *connector,
> struct drm_display_mode *mode)
> {
> list_del(&mode->head);
> - drm_mode_destroy(connector->dev, mode);
> + drm_mode_unreference(mode);
> }
>
> /**
> @@ -2602,7 +2602,7 @@ out:
> drm_framebuffer_unreference(fb);
>
> kfree(connector_set);
> - drm_mode_destroy(dev, mode);
> + drm_mode_unreference(mode);
> drm_modeset_unlock_all(dev);
> return ret;
> }
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 6c65a0a..757de8b 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -384,7 +384,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>
> /* FIXME: add subpixel order */
> done:
> - drm_mode_destroy(dev, adjusted_mode);
> + drm_mode_unreference(adjusted_mode);
> if (!ret) {
> crtc->enabled = saved_enabled;
> crtc->mode = saved_mode;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 087d608..cbc021d 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1705,7 +1705,7 @@ drm_mode_std(struct drm_connector *connector, struct edid *edid,
> if (!mode)
> return NULL;
> if (drm_mode_hsync(mode) > drm_gtf2_hbreak(edid)) {
> - drm_mode_destroy(dev, mode);
> + drm_mode_unreference(mode);
> mode = drm_gtf_mode_complex(dev, hsize, vsize,
> vrefresh_rate, 0, 0,
> drm_gtf2_m(edid),
> @@ -2021,7 +2021,7 @@ drm_gtf_modes_for_range(struct drm_connector *connector, struct edid *edid,
> fixup_mode_1366x768(newmode);
> if (!mode_in_range(newmode, edid, timing) ||
> !valid_inferred_mode(connector, newmode)) {
> - drm_mode_destroy(dev, newmode);
> + drm_mode_unreference(newmode);
> continue;
> }
>
> @@ -2050,7 +2050,7 @@ drm_cvt_modes_for_range(struct drm_connector *connector, struct edid *edid,
> fixup_mode_1366x768(newmode);
> if (!mode_in_range(newmode, edid, timing) ||
> !valid_inferred_mode(connector, newmode)) {
> - drm_mode_destroy(dev, newmode);
> + drm_mode_unreference(newmode);
> continue;
> }
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 3144db9..5c96a6a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -588,7 +588,7 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
> for (i = 0; i < helper->crtc_count; i++) {
> kfree(helper->crtc_info[i].mode_set.connectors);
> if (helper->crtc_info[i].mode_set.mode)
> - drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode);
> + drm_mode_unreference(helper->crtc_info[i].mode_set.mode);
> }
> kfree(helper->crtc_info);
> }
> @@ -1606,7 +1606,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> mode->name, fb_crtc->mode_set.crtc->base.id);
> fb_crtc->desired_mode = mode;
> if (modeset->mode)
> - drm_mode_destroy(dev, modeset->mode);
> + drm_mode_unreference(modeset->mode);
> modeset->mode = drm_mode_duplicate(dev,
> fb_crtc->desired_mode);
> modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector;
> @@ -1621,7 +1621,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> BUG_ON(modeset->fb);
> BUG_ON(modeset->num_connectors);
> if (modeset->mode)
> - drm_mode_destroy(dev, modeset->mode);
> + drm_mode_unreference(modeset->mode);
> modeset->mode = NULL;
> }
> }
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index bedf189..55d51ed 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -82,27 +82,46 @@ struct drm_display_mode *drm_mode_create(struct drm_device *dev)
> return NULL;
> }
>
> + kref_init(&nmode->refcount);
> + nmode->dev = dev;
> +
> return nmode;
> }
> EXPORT_SYMBOL(drm_mode_create);
>
> /**
> - * drm_mode_destroy - remove a mode
> - * @dev: DRM device
> + * drm_mode_reference - incr the mode's refcnt
> + * @mode: display mode
> + *
> + * This functions increments the mode's refcount.
> + */
> +void drm_mode_reference(struct drm_display_mode *mode)
> +{
> + kref_get(&mode->refcount);
> +}
> +EXPORT_SYMBOL(drm_mode_reference);
> +
> +static void drm_mode_free(struct kref *kref)
> +{
> + struct drm_display_mode *mode =
> + container_of(kref, struct drm_display_mode, refcount);
> + drm_mode_object_put(mode->dev, &mode->base);
> + kfree(mode);
> +}
> +
> +/**
> + * drm_mode_unreference - decrement the mode's refcnt
> * @mode: mode to remove
> *
> - * Release @mode's unique ID, then free it @mode structure itself using kfree.
> + * Decrement the mode's refcount, freeing it after last ref is dropped.
> */
> -void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode)
> +void drm_mode_unreference(struct drm_display_mode *mode)
> {
> if (!mode)
> return;
> -
> - drm_mode_object_put(dev, &mode->base);
> -
> - kfree(mode);
> + kref_put(&mode->refcount, drm_mode_free);
> }
> -EXPORT_SYMBOL(drm_mode_destroy);
> +EXPORT_SYMBOL(drm_mode_unreference);
>
> /**
> * drm_mode_probed_add - add a mode to a connector's probed_mode list
> @@ -957,7 +976,7 @@ void drm_mode_prune_invalid(struct drm_device *dev,
> DRM_DEBUG_KMS("Not using %s mode %d\n",
> mode->name, mode->status);
> }
> - drm_mode_destroy(dev, mode);
> + drm_mode_unreference(mode);
> }
> }
> }
> @@ -1046,7 +1065,7 @@ void drm_mode_connector_list_update(struct drm_connector *connector,
> else
> mode->type = pmode->type;
> list_del(&pmode->head);
> - drm_mode_destroy(connector->dev, pmode);
> + drm_mode_unreference(pmode);
> break;
> }
> }
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c
> index ba9b3d5..3f9d44a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c
> @@ -72,7 +72,7 @@ static int exynos_drm_connector_get_modes(struct drm_connector *connector)
> if (display->ops->get_panel)
> panel = display->ops->get_panel(display);
> else {
> - drm_mode_destroy(connector->dev, mode);
> + drm_mode_unreference(mode);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> index 0be96fd..00eb0f63 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> @@ -1905,8 +1905,7 @@ static void psb_intel_sdvo_enc_destroy(struct drm_encoder *encoder)
> struct psb_intel_sdvo *psb_intel_sdvo = to_psb_intel_sdvo(encoder);
>
> if (psb_intel_sdvo->sdvo_lvds_fixed_mode != NULL)
> - drm_mode_destroy(encoder->dev,
> - psb_intel_sdvo->sdvo_lvds_fixed_mode);
> + drm_mode_unreference(psb_intel_sdvo->sdvo_lvds_fixed_mode);
>
> i2c_del_adapter(&psb_intel_sdvo->ddc);
> gma_encoder_destroy(encoder);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 38a9857..cbf63e0 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1205,13 +1205,9 @@ int intel_panel_init(struct intel_panel *panel,
>
> void intel_panel_fini(struct intel_panel *panel)
> {
> - struct intel_connector *intel_connector =
> - container_of(panel, struct intel_connector, panel);
> -
> if (panel->fixed_mode)
> - drm_mode_destroy(intel_connector->base.dev, panel->fixed_mode);
> + drm_mode_unreference(panel->fixed_mode);
>
> if (panel->downclock_mode)
> - drm_mode_destroy(intel_connector->base.dev,
> - panel->downclock_mode);
> + drm_mode_unreference(panel->downclock_mode);
> }
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 9350edd..7049ac7 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2248,8 +2248,7 @@ static void intel_sdvo_enc_destroy(struct drm_encoder *encoder)
> struct intel_sdvo *intel_sdvo = to_sdvo(to_intel_encoder(encoder));
>
> if (intel_sdvo->sdvo_lvds_fixed_mode != NULL)
> - drm_mode_destroy(encoder->dev,
> - intel_sdvo->sdvo_lvds_fixed_mode);
> + drm_mode_unreference(intel_sdvo->sdvo_lvds_fixed_mode);
>
> i2c_del_adapter(&intel_sdvo->ddc);
> intel_encoder_destroy(encoder);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index dbdc9ad..876ae75 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -739,7 +739,7 @@ nouveau_connector_get_modes(struct drm_connector *connector)
> /* destroy the native mode, the attached monitor could have changed.
> */
> if (nv_connector->native_mode) {
> - drm_mode_destroy(dev, nv_connector->native_mode);
> + drm_mode_unreference(nv_connector->native_mode);
> nv_connector->native_mode = NULL;
> }
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c
> index 36bc5cc..222d42a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_connector.c
> +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
> @@ -224,7 +224,7 @@ static int omap_connector_mode_valid(struct drm_connector *connector,
> new_mode->vrefresh = 0;
> if (mode->vrefresh == drm_mode_vrefresh(new_mode))
> ret = MODE_OK;
> - drm_mode_destroy(dev, new_mode);
> + drm_mode_unreference(new_mode);
> }
>
> DBG("connector: mode %s: "
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index d2bc2b0..d726a81 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -1964,13 +1964,13 @@ int vmw_du_connector_fill_modes(struct drm_connector *connector,
> mode->vdisplay)) {
> drm_mode_probed_add(connector, mode);
> } else {
> - drm_mode_destroy(dev, mode);
> + drm_mode_unreference(mode);
> mode = NULL;
> }
>
> if (du->pref_mode) {
> list_del_init(&du->pref_mode->head);
> - drm_mode_destroy(dev, du->pref_mode);
> + drm_mode_unreference(du->pref_mode);
> }
>
> /* mode might be null here, this is intended */
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 91d0582..ef552c9 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -97,6 +97,8 @@ struct drm_display_mode {
> /* Header */
> struct list_head head;
> struct drm_mode_object base;
> + struct kref refcount;
> + struct drm_device *dev;
>
> char name[DRM_DISPLAY_MODE_LEN];
>
> @@ -178,7 +180,8 @@ struct drm_connector;
> struct drm_cmdline_mode;
>
> struct drm_display_mode *drm_mode_create(struct drm_device *dev);
> -void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode);
> +void drm_mode_reference(struct drm_display_mode *mode);
> +void drm_mode_unreference(struct drm_display_mode *mode);
> void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
> void drm_mode_debug_printmodeline(const struct drm_display_mode *mode);
>
> --
> 1.9.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
