On Sun, Jul 27, 2014 at 11:17 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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).
bleh, all the deep copies seem ugly either way, I still think
refcnt'ing and shallow copies is the better idea
> 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 ;-)
Well, that was somewhat different, because there were some side-effects of rmfb
BR,
-R
> -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
