search for: drm_gem_vram_placement

...et; > + struct ttm_operation_ctx ctx = { false, false }; > + > + if (!gbo->pin_count) > + return 0; Likewise. > + --gbo->pin_count; > + if (gbo->pin_count) > + return 0; > + > + if (gbo->kmap.virtual) > + ttm_bo_kunmap(&gbo->kmap); > + > + drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM); > + for (i = 0; i < gbo->placement.num_placement ; ++i) > + gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; > + > + ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx); > + if (ret) > + return ret; > + > + return 0;...

...et; > + struct ttm_operation_ctx ctx = { false, false }; > + > + if (!gbo->pin_count) > + return 0; Likewise. > + --gbo->pin_count; > + if (gbo->pin_count) > + return 0; > + > + if (gbo->kmap.virtual) > + ttm_bo_kunmap(&gbo->kmap); > + > + drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM); > + for (i = 0; i < gbo->placement.num_placement ; ++i) > + gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; > + > + ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx); > + if (ret) > + return ret; > + > + return 0;...

Hi, > This misses the call to drm_gem_vram_placement(), where > drm_gem_vram_push_to_system() enforces placement in system memory. Ah, missed that detail. > We > could build a common implementation out of both interfaces, but that > would obfuscate the code IMHO. I'd just leave it as it is. Ok. > >> +struct drm_gem_vram_o...

Hi thanks for reviewing the patches. Am 06.05.19 um 14:31 schrieb Gerd Hoffmann: >> + --gbo->pin_count; >> + if (gbo->pin_count) >> + return 0; >> + >> + if (gbo->kmap.virtual) >> + ttm_bo_kunmap(&gbo->kmap); >> + >> + drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM); >> + for (i = 0; i < gbo->placement.num_placement ; ++i) >> + gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; >> + >> + ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx); >> + if (ret) >> + return ret;...

...o_driver_evict_flags(struct ttm_buffer_object *bo, > + struct ttm_placement *pl) > +{ > + struct drm_gem_vram_object *gbo; > + > + /* TTM may pass BOs that are not GEM VRAM BOs. */ > + if (!drm_is_gem_vram(bo)) > + return; > + > + gbo = drm_gem_vram_of_bo(bo); > + drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM); > + *pl = gbo->placement; > +} For drm_is_gem_vram(), I'm not quite sure what else to test for. So there are still a few things I'd like to discuss. 1) If this test is about the placement flags, then it's unrelated to the actual DRM driver. All buff...

...ect *gbo) > +{ > + drm_gem_vram_cleanup(gbo); > + kfree(gbo); > +} > + > +static void ttm_buffer_object_destroy(struct ttm_buffer_object *bo) > +{ > + struct drm_gem_vram_object *gbo = drm_gem_vram_of_bo(bo); > + drm_gem_vram_destroy(gbo); > +} > + > +static void drm_gem_vram_placement(struct drm_gem_vram_object *gbo, int pl_flag) > +{ > + unsigned int i; > + unsigned int c = 0; > + > + gbo->placement.placement = gbo->placements; > + gbo->placement.busy_placement = gbo->placements; > + > + if (pl_flag & TTM_PL_FLAG_VRAM) > + gbo->pla...

...ect *gbo) > +{ > + drm_gem_vram_cleanup(gbo); > + kfree(gbo); > +} > + > +static void ttm_buffer_object_destroy(struct ttm_buffer_object *bo) > +{ > + struct drm_gem_vram_object *gbo = drm_gem_vram_of_bo(bo); > + drm_gem_vram_destroy(gbo); > +} > + > +static void drm_gem_vram_placement(struct drm_gem_vram_object *gbo, int pl_flag) > +{ > + unsigned int i; > + unsigned int c = 0; > + > + gbo->placement.placement = gbo->placements; > + gbo->placement.busy_placement = gbo->placements; > + > + if (pl_flag & TTM_PL_FLAG_VRAM) > + gbo->pla...

...+static void drm_gem_vram_destroy(struct drm_gem_vram_object *gbo) +{ + drm_gem_vram_cleanup(gbo); + kfree(gbo); +} + +static void ttm_buffer_object_destroy(struct ttm_buffer_object *bo) +{ + struct drm_gem_vram_object *gbo = drm_gem_vram_of_bo(bo); + + drm_gem_vram_destroy(gbo); +} + +static void drm_gem_vram_placement(struct drm_gem_vram_object *gbo, + unsigned long pl_flag) +{ + unsigned int i; + unsigned int c = 0; + + gbo->placement.placement = gbo->placements; + gbo->placement.busy_placement = gbo->placements; + + if (pl_flag & TTM_PL_FLAG_VRAM) + gbo->placements[c++].flags = TTM_PL...

...+ +static void drm_gem_vram_destroy(struct drm_gem_vram_object *gbo) +{ + drm_gem_vram_cleanup(gbo); + kfree(gbo); +} + +static void ttm_buffer_object_destroy(struct ttm_buffer_object *bo) +{ + struct drm_gem_vram_object *gbo = drm_gem_vram_of_bo(bo); + drm_gem_vram_destroy(gbo); +} + +static void drm_gem_vram_placement(struct drm_gem_vram_object *gbo, int pl_flag) +{ + unsigned int i; + unsigned int c = 0; + + gbo->placement.placement = gbo->placements; + gbo->placement.busy_placement = gbo->placements; + + if (pl_flag & TTM_PL_FLAG_VRAM) + gbo->placements[c++].flags = TTM_PL_FLAG_WC | +...

...M placement information. + */ +void drm_gem_vram_bo_driver_evict_flags(struct ttm_buffer_object *bo, + struct ttm_placement *pl) +{ + struct drm_gem_vram_object *gbo; + + /* TTM may pass BOs that are not GEM VRAM BOs. */ + if (!drm_is_gem_vram(bo)) + return; + + gbo = drm_gem_vram_of_bo(bo); + drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM); + *pl = gbo->placement; +} +EXPORT_SYMBOL(drm_gem_vram_bo_driver_evict_flags); + +/** + * drm_gem_vram_bo_driver_verify_access() - \ + Implements &struct ttm_bo_driver.verify_access + * @bo: TTM buffer object. Refers to &struct drm_gem_vram_object.bo + * @filp:...

...M placement information. + */ +void drm_gem_vram_bo_driver_evict_flags(struct ttm_buffer_object *bo, + struct ttm_placement *pl) +{ + struct drm_gem_vram_object *gbo; + + /* TTM may pass BOs that are not GEM VRAM BOs. */ + if (!drm_is_gem_vram(bo)) + return; + + gbo = drm_gem_vram_of_bo(bo); + drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM); + *pl = gbo->placement; +} +EXPORT_SYMBOL(drm_gem_vram_bo_driver_evict_flags); + +/** + * drm_gem_vram_bo_driver_verify_access() - \ + Implements &struct ttm_bo_driver.verify_access + * @bo: TTM buffer object. Refers to &struct drm_gem_vram_object.bo + * @filp:...

...M placement information. + */ +void drm_gem_vram_bo_driver_evict_flags(struct ttm_buffer_object *bo, + struct ttm_placement *pl) +{ + struct drm_gem_vram_object *gbo; + + /* TTM may pass BOs that are not GEM VRAM BOs. */ + if (!drm_is_gem_vram(bo)) + return; + + gbo = drm_gem_vram_of_bo(bo); + drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM); + *pl = gbo->placement; +} +EXPORT_SYMBOL(drm_gem_vram_bo_driver_evict_flags); + +/** + * drm_gem_vram_bo_driver_verify_access() - \ + Implements &struct ttm_bo_driver.verify_access + * @bo: TTM buffer object. Refers to &struct drm_gem_vram_object.bo + * @filp:...

...replace them with tiny ttm_bo_lock() wrappers. Christian. > -Daniel > >> Cheers, Daniel >> >>> + >>> + if (gbo->pin_count) { >>> + ++gbo->pin_count; >>> + return 0; >>> + } >>> + >>> + drm_gem_vram_placement(gbo, pl_flag); >>> + for (i = 0; i < gbo->placement.num_placement; ++i) >>> + gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; >>> + >>> + ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx); >>> + if (ret &...

A kernel test bot reported a problem with the locktorture testcase that was triggered by the GEM VRAM helpers. ... [ 10.004734] RIP: 0010:ttm_bo_validate+0x41/0x141 [ttm] ... [ 10.015669] ? kvm_sched_clock_read+0x5/0xd [ 10.016157] ? get_lock_stats+0x11/0x3f [ 10.016607] drm_gem_vram_pin+0x77/0xa2 [drm_vram_helper] [ 10.017229]

A kernel test bot reported a problem with the locktorture testcase that was triggered by the GEM VRAM helpers. ... [ 10.004734] RIP: 0010:ttm_bo_validate+0x41/0x141 [ttm] ... [ 10.015669] ? kvm_sched_clock_read+0x5/0xd [ 10.016157] ? get_lock_stats+0x11/0x3f [ 10.016607] drm_gem_vram_pin+0x77/0xa2 [drm_vram_helper] [ 10.017229]

...;s going to lead to less wtf moments by driver writers :-) Maybe we should also do a large-scale s/reserve/lock/ within ttm, to align more with what we now have in dma-buf. Cheers, Daniel > + > + if (gbo->pin_count) { > + ++gbo->pin_count; > + return 0; > + } > + > + drm_gem_vram_placement(gbo, pl_flag); > + for (i = 0; i < gbo->placement.num_placement; ++i) > + gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; > + > + ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx); > + if (ret < 0) > + return ret; > + > + gbo->pin_co...

...;s going to lead to less wtf moments by driver writers :-) Maybe we should also do a large-scale s/reserve/lock/ within ttm, to align more with what we now have in dma-buf. Cheers, Daniel > + > + if (gbo->pin_count) { > + ++gbo->pin_count; > + return 0; > + } > + > + drm_gem_vram_placement(gbo, pl_flag); > + for (i = 0; i < gbo->placement.num_placement; ++i) > + gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; > + > + ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx); > + if (ret < 0) > + return ret; > + > + gbo->pin_co...

...* 0 on success, or + * a negative error code otherwise. + */ +int drm_gem_vram_pin_reserved(struct drm_gem_vram_object *gbo, + unsigned long pl_flag) +{ + int i, ret; + struct ttm_operation_ctx ctx = { false, false }; + + if (gbo->pin_count) { + ++gbo->pin_count; + return 0; + } + + drm_gem_vram_placement(gbo, pl_flag); + for (i = 0; i < gbo->placement.num_placement; ++i) + gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; + + ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx); + if (ret < 0) + return ret; + + gbo->pin_count = 1; + + return 0; +} +EXPORT_SYMBOL...

...hether that aligns with Christian's plans. TODO.rst patch might be a good step to get that discussion started. -Daniel > > Cheers, Daniel > > > + > > + if (gbo->pin_count) { > > + ++gbo->pin_count; > > + return 0; > > + } > > + > > + drm_gem_vram_placement(gbo, pl_flag); > > + for (i = 0; i < gbo->placement.num_placement; ++i) > > + gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; > > + > > + ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx); > > + if (ret < 0) > > + return ret...

Several simple framebuffer drivers copy most of the TTM code from each other. The implementation is always the same; except for the name of some data structures. As recently discussed, this patch set provides generic memory-management code for simple framebuffers with dedicated video memory. It further converts the respective drivers to the generic code. The shared code is basically the same