| Message ID | 20201023122216.2373294-28-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/65] drm/vc4: Drop legacy_cursor_update override | expand |
Am 23.10.20 um 14:21 schrieb Daniel Vetter: > ttm_resource_manager->use_type is only used for runtime changes by > vmwgfx. I think ideally we'd push this functionality into drivers - > ttm itself does not provide any locking to guarantee this is safe, so > the only way this can work at runtime is if the driver does provide > additional guarantees. vwmgfx does that through the > vmw_private->reservation_sem. Therefore supporting this feature in > shared code feels a bit misplaced. > > As a first step add a WARN_ON to make sure the resource manager is > empty. This is just to make sure I actually understand correctly what > vmwgfx is doing, and to make sure an eventual subsequent refactor > doesn't break anything. > > This check should also be useful for other drivers, to make sure they > haven't leaked anything. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Christian Koenig <christian.koenig@amd.com> > Cc: Huang Rui <ray.huang@amd.com> I'm pretty sure that this will trigger for vmwgfx. But that's what it is supposed to do, isn't it? Reviewed-by: Christian König <christian.koenig@amd.com> > --- > include/drm/ttm/ttm_resource.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h > index f48a70d39ac5..789ec477b607 100644 > --- a/include/drm/ttm/ttm_resource.h > +++ b/include/drm/ttm/ttm_resource.h > @@ -191,6 +191,10 @@ struct ttm_resource { > static inline void > ttm_resource_manager_set_used(struct ttm_resource_manager *man, bool used) > { > + int i; > + > + for (i = 0; i < TTM_MAX_BO_PRIORITY; i++) > + WARN_ON(!list_empty(&man->lru[i])); > man->use_type = used; > } >
On Fri, Oct 23, 2020 at 4:54 PM Christian König <christian.koenig@amd.com> wrote: > > Am 23.10.20 um 14:21 schrieb Daniel Vetter: > > ttm_resource_manager->use_type is only used for runtime changes by > > vmwgfx. I think ideally we'd push this functionality into drivers - > > ttm itself does not provide any locking to guarantee this is safe, so > > the only way this can work at runtime is if the driver does provide > > additional guarantees. vwmgfx does that through the > > vmw_private->reservation_sem. Therefore supporting this feature in > > shared code feels a bit misplaced. > > > > As a first step add a WARN_ON to make sure the resource manager is > > empty. This is just to make sure I actually understand correctly what > > vmwgfx is doing, and to make sure an eventual subsequent refactor > > doesn't break anything. > > > > This check should also be useful for other drivers, to make sure they > > haven't leaked anything. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Christian Koenig <christian.koenig@amd.com> > > Cc: Huang Rui <ray.huang@amd.com> > > I'm pretty sure that this will trigger for vmwgfx. But that's what it is > supposed to do, isn't it? Yeah, this is an accidental dump of my wip pile, and it's not done yet at all. Please disregard (at least for now). -Daniel > Reviewed-by: Christian König <christian.koenig@amd.com> > > > --- > > include/drm/ttm/ttm_resource.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h > > index f48a70d39ac5..789ec477b607 100644 > > --- a/include/drm/ttm/ttm_resource.h > > +++ b/include/drm/ttm/ttm_resource.h > > @@ -191,6 +191,10 @@ struct ttm_resource { > > static inline void > > ttm_resource_manager_set_used(struct ttm_resource_manager *man, bool used) > > { > > + int i; > > + > > + for (i = 0; i < TTM_MAX_BO_PRIORITY; i++) > > + WARN_ON(!list_empty(&man->lru[i])); > > man->use_type = used; > > } > > >
On Fri, Oct 23, 2020 at 04:56:20PM +0200, Daniel Vetter wrote: > On Fri, Oct 23, 2020 at 4:54 PM Christian König > <christian.koenig@amd.com> wrote: > > > > Am 23.10.20 um 14:21 schrieb Daniel Vetter: > > > ttm_resource_manager->use_type is only used for runtime changes by > > > vmwgfx. I think ideally we'd push this functionality into drivers - > > > ttm itself does not provide any locking to guarantee this is safe, so > > > the only way this can work at runtime is if the driver does provide > > > additional guarantees. vwmgfx does that through the > > > vmw_private->reservation_sem. Therefore supporting this feature in > > > shared code feels a bit misplaced. > > > > > > As a first step add a WARN_ON to make sure the resource manager is > > > empty. This is just to make sure I actually understand correctly what > > > vmwgfx is doing, and to make sure an eventual subsequent refactor > > > doesn't break anything. > > > > > > This check should also be useful for other drivers, to make sure they > > > haven't leaked anything. > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Christian Koenig <christian.koenig@amd.com> > > > Cc: Huang Rui <ray.huang@amd.com> > > > > I'm pretty sure that this will trigger for vmwgfx. But that's what it is > > supposed to do, isn't it? > > Yeah, this is an accidental dump of my wip pile, and it's not done yet > at all. Please disregard (at least for now). > -Daniel > > > Reviewed-by: Christian König <christian.koenig@amd.com> Ok decided to submit these 3 patches finally, including the 2 vmwgfx fixes which should avoid the splat. I included your r-b, pls complain if that's not ok anymore. Thanks, Daniel > > > > > --- > > > include/drm/ttm/ttm_resource.h | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h > > > index f48a70d39ac5..789ec477b607 100644 > > > --- a/include/drm/ttm/ttm_resource.h > > > +++ b/include/drm/ttm/ttm_resource.h > > > @@ -191,6 +191,10 @@ struct ttm_resource { > > > static inline void > > > ttm_resource_manager_set_used(struct ttm_resource_manager *man, bool used) > > > { > > > + int i; > > > + > > > + for (i = 0; i < TTM_MAX_BO_PRIORITY; i++) > > > + WARN_ON(!list_empty(&man->lru[i])); > > > man->use_type = used; > > > } > > > > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h index f48a70d39ac5..789ec477b607 100644 --- a/include/drm/ttm/ttm_resource.h +++ b/include/drm/ttm/ttm_resource.h @@ -191,6 +191,10 @@ struct ttm_resource { static inline void ttm_resource_manager_set_used(struct ttm_resource_manager *man, bool used) { + int i; + + for (i = 0; i < TTM_MAX_BO_PRIORITY; i++) + WARN_ON(!list_empty(&man->lru[i])); man->use_type = used; }
ttm_resource_manager->use_type is only used for runtime changes by vmwgfx. I think ideally we'd push this functionality into drivers - ttm itself does not provide any locking to guarantee this is safe, so the only way this can work at runtime is if the driver does provide additional guarantees. vwmgfx does that through the vmw_private->reservation_sem. Therefore supporting this feature in shared code feels a bit misplaced. As a first step add a WARN_ON to make sure the resource manager is empty. This is just to make sure I actually understand correctly what vmwgfx is doing, and to make sure an eventual subsequent refactor doesn't break anything. This check should also be useful for other drivers, to make sure they haven't leaked anything. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Christian Koenig <christian.koenig@amd.com> Cc: Huang Rui <ray.huang@amd.com> --- include/drm/ttm/ttm_resource.h | 4 ++++ 1 file changed, 4 insertions(+)