| Message ID | 20190611130344.18988-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove explicit locking and kmap arguments from GEM VRAM interface | expand |
On Tue, Jun 11, 2019 at 03:03:36PM +0200, Thomas Zimmermann wrote: > Pinning a buffer prevents it from being moved to a different memory > location. For some operations, such as buffer updates, it is not > important where the buffer is located. Setting the pin function's > pl_flag argument to 0 will pin the buffer to whereever it is stored. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_gem_vram_helper.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > index 42ad80888df7..214f54b8920b 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -224,7 +224,9 @@ EXPORT_SYMBOL(drm_gem_vram_offset); > * > * Pinning a buffer object ensures that it is not evicted from > * a memory region. A pinned buffer object has to be unpinned before > - * it can be pinned to another region. > + * it can be pinned to another region. If the pl_flag argument is 0, > + * the buffer is pinned at its current location (video RAM or system > + * memory). > * > * Returns: > * 0 on success, or > @@ -242,7 +244,9 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag) > if (gbo->pin_count) > goto out; > > - drm_gem_vram_placement(gbo, pl_flag); > + if (pl_flag) > + 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; > > @@ -691,7 +695,7 @@ int drm_gem_vram_driver_gem_prime_pin(struct drm_gem_object *gem) > { > struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem); > > - return drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM); > + return drm_gem_vram_pin(gbo, 0); Not sure this is a good idea here. If the bo happens to be in sysram it can't be displayed any more. > - ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM); > + ret = drm_gem_vram_pin(gbo, 0); Likewise. cheers, Gerd
Hi Am 12.06.19 um 10:13 schrieb Gerd Hoffmann: > On Tue, Jun 11, 2019 at 03:03:36PM +0200, Thomas Zimmermann wrote: >> Pinning a buffer prevents it from being moved to a different memory >> location. For some operations, such as buffer updates, it is not >> important where the buffer is located. Setting the pin function's >> pl_flag argument to 0 will pin the buffer to whereever it is stored. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_gem_vram_helper.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c >> index 42ad80888df7..214f54b8920b 100644 >> --- a/drivers/gpu/drm/drm_gem_vram_helper.c >> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c >> @@ -224,7 +224,9 @@ EXPORT_SYMBOL(drm_gem_vram_offset); >> * >> * Pinning a buffer object ensures that it is not evicted from >> * a memory region. A pinned buffer object has to be unpinned before >> - * it can be pinned to another region. >> + * it can be pinned to another region. If the pl_flag argument is 0, >> + * the buffer is pinned at its current location (video RAM or system >> + * memory). >> * >> * Returns: >> * 0 on success, or >> @@ -242,7 +244,9 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag) >> if (gbo->pin_count) >> goto out; >> >> - drm_gem_vram_placement(gbo, pl_flag); >> + if (pl_flag) >> + 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; >> >> @@ -691,7 +695,7 @@ int drm_gem_vram_driver_gem_prime_pin(struct drm_gem_object *gem) >> { >> struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem); >> >> - return drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM); >> + return drm_gem_vram_pin(gbo, 0); The only use case for these Prime helpers is fbdev console emulation. I have another patch set that replaces the ast and mgag200 consoles with generic code. During the console updates it temporarily pins the BO via this Prime funcation, which might move the BO into scarce VRAM unnecessarily. Can we leave it like this and add a comment explaining the decision? Best regards Thomas > Not sure this is a good idea here. If the bo happens to be in sysram > it can't be displayed any more. > >> - ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM); >> + ret = drm_gem_vram_pin(gbo, 0); > > Likewise. > > cheers, > Gerd >
On Wed, Jun 12, 2019 at 10:29:29AM +0200, Thomas Zimmermann wrote: > Hi > > Am 12.06.19 um 10:13 schrieb Gerd Hoffmann: > > On Tue, Jun 11, 2019 at 03:03:36PM +0200, Thomas Zimmermann wrote: > >> Pinning a buffer prevents it from being moved to a different memory > >> location. For some operations, such as buffer updates, it is not > >> important where the buffer is located. Setting the pin function's > >> pl_flag argument to 0 will pin the buffer to whereever it is stored. > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> --- > >> drivers/gpu/drm/drm_gem_vram_helper.c | 12 ++++++++---- > >> 1 file changed, 8 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > >> index 42ad80888df7..214f54b8920b 100644 > >> --- a/drivers/gpu/drm/drm_gem_vram_helper.c > >> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > >> @@ -224,7 +224,9 @@ EXPORT_SYMBOL(drm_gem_vram_offset); > >> * > >> * Pinning a buffer object ensures that it is not evicted from > >> * a memory region. A pinned buffer object has to be unpinned before > >> - * it can be pinned to another region. > >> + * it can be pinned to another region. If the pl_flag argument is 0, > >> + * the buffer is pinned at its current location (video RAM or system > >> + * memory). > >> * > >> * Returns: > >> * 0 on success, or > >> @@ -242,7 +244,9 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag) > >> if (gbo->pin_count) > >> goto out; > >> > >> - drm_gem_vram_placement(gbo, pl_flag); > >> + if (pl_flag) > >> + 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; > >> > >> @@ -691,7 +695,7 @@ int drm_gem_vram_driver_gem_prime_pin(struct drm_gem_object *gem) > >> { > >> struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem); > >> > >> - return drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM); > >> + return drm_gem_vram_pin(gbo, 0); > > The only use case for these Prime helpers is fbdev console emulation. I > have another patch set that replaces the ast and mgag200 consoles with > generic code. During the console updates it temporarily pins the BO via > this Prime funcation, which might move the BO into scarce VRAM > unnecessarily. Ok, if the pin is temporary only for the update this should be fine. > Can we leave it like this and add a comment explaining > the decision? Yes, a comment is a good idea. cheers, Gerd
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 42ad80888df7..214f54b8920b 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -224,7 +224,9 @@ EXPORT_SYMBOL(drm_gem_vram_offset); * * Pinning a buffer object ensures that it is not evicted from * a memory region. A pinned buffer object has to be unpinned before - * it can be pinned to another region. + * it can be pinned to another region. If the pl_flag argument is 0, + * the buffer is pinned at its current location (video RAM or system + * memory). * * Returns: * 0 on success, or @@ -242,7 +244,9 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag) if (gbo->pin_count) goto out; - drm_gem_vram_placement(gbo, pl_flag); + if (pl_flag) + 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; @@ -691,7 +695,7 @@ int drm_gem_vram_driver_gem_prime_pin(struct drm_gem_object *gem) { struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem); - return drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM); + return drm_gem_vram_pin(gbo, 0); } EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_pin); @@ -723,7 +727,7 @@ void *drm_gem_vram_driver_gem_prime_vmap(struct drm_gem_object *gem) int ret; void *base; - ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM); + ret = drm_gem_vram_pin(gbo, 0); if (ret) return NULL; base = drm_gem_vram_kmap(gbo, true, NULL);
Pinning a buffer prevents it from being moved to a different memory location. For some operations, such as buffer updates, it is not important where the buffer is located. Setting the pin function's pl_flag argument to 0 will pin the buffer to whereever it is stored. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_gem_vram_helper.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)