| Message ID | 20190611130344.18988-6-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove explicit locking and kmap arguments from GEM VRAM interface | expand |
Hi Thomas. On Tue, Jun 11, 2019 at 03:03:40PM +0200, Thomas Zimmermann wrote: > Another explicit lock operation of a GEM VRAM BO is located in AST's > framebuffer update code. Instead of locking the BO, we pin it to wherever > it is. > > v2: > * update with pin flag of 0 > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/ast/ast_fb.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c > index 05f45222b702..b404b51c2c55 100644 > --- a/drivers/gpu/drm/ast/ast_fb.c > +++ b/drivers/gpu/drm/ast/ast_fb.c > @@ -48,32 +48,31 @@ static void ast_dirty_update(struct ast_fbdev *afbdev, > int x, int y, int width, int height) > { > int i; > - struct drm_gem_object *obj; > struct drm_gem_vram_object *gbo; > int src_offset, dst_offset; > int bpp = afbdev->afb.base.format->cpp[0]; > - int ret = -EBUSY; > + int ret; > u8 *dst; > bool unmap = false; > bool store_for_later = false; > int x2, y2; > unsigned long flags; > > - obj = afbdev->afb.obj; > - gbo = drm_gem_vram_of_gem(obj); > - > - /* Try to lock the BO. If we fail with -EBUSY then > - * the BO is being moved and we should store up the > - * damage until later. > - */ > - if (drm_can_sleep()) > - ret = drm_gem_vram_lock(gbo, true); > - if (ret) { > - if (ret != -EBUSY) > - return; > - > + gbo = drm_gem_vram_of_gem(afbdev->afb.obj); > + > + if (drm_can_sleep()) { While touching this code, anyway we could get rid of drm_can_sleep()? I only ask because Daniel V. said that we should not use it. This was some months ago during some ehader file clean-up, so nothing in particular related to the ast driver. Sam > + /* We pin the BO so it won't be moved during the > + * update. The actual location, video RAM or system > + * memory, is not important. > + */ > + ret = drm_gem_vram_pin(gbo, 0); > + if (ret) { > + if (ret != -EBUSY) > + return; > + store_for_later = true; > + } > + } else > store_for_later = true; > - } > > x2 = x + width - 1; > y2 = y + height - 1; > @@ -126,7 +125,7 @@ static void ast_dirty_update(struct ast_fbdev *afbdev, > drm_gem_vram_kunmap(gbo); > > out: > - drm_gem_vram_unlock(gbo); > + drm_gem_vram_unpin(gbo); > } > > static void ast_fillrect(struct fb_info *info, > -- > 2.21.0
Hi Am 11.06.19 um 22:29 schrieb Sam Ravnborg: > Hi Thomas. > > On Tue, Jun 11, 2019 at 03:03:40PM +0200, Thomas Zimmermann wrote: >> Another explicit lock operation of a GEM VRAM BO is located in AST's >> framebuffer update code. Instead of locking the BO, we pin it to wherever >> it is. >> >> v2: >> * update with pin flag of 0 >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/ast/ast_fb.c | 33 ++++++++++++++++----------------- >> 1 file changed, 16 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c >> index 05f45222b702..b404b51c2c55 100644 >> --- a/drivers/gpu/drm/ast/ast_fb.c >> +++ b/drivers/gpu/drm/ast/ast_fb.c >> @@ -48,32 +48,31 @@ static void ast_dirty_update(struct ast_fbdev *afbdev, >> int x, int y, int width, int height) >> { >> int i; >> - struct drm_gem_object *obj; >> struct drm_gem_vram_object *gbo; >> int src_offset, dst_offset; >> int bpp = afbdev->afb.base.format->cpp[0]; >> - int ret = -EBUSY; >> + int ret; >> u8 *dst; >> bool unmap = false; >> bool store_for_later = false; >> int x2, y2; >> unsigned long flags; >> >> - obj = afbdev->afb.obj; >> - gbo = drm_gem_vram_of_gem(obj); >> - >> - /* Try to lock the BO. If we fail with -EBUSY then >> - * the BO is being moved and we should store up the >> - * damage until later. >> - */ >> - if (drm_can_sleep()) >> - ret = drm_gem_vram_lock(gbo, true); >> - if (ret) { >> - if (ret != -EBUSY) >> - return; >> - >> + gbo = drm_gem_vram_of_gem(afbdev->afb.obj); >> + >> + if (drm_can_sleep()) { > > While touching this code, anyway we could get rid of drm_can_sleep()? > I only ask because Daniel V. said that we should not use it. > This was some months ago during some ehader file clean-up, so nothing > in particular related to the ast driver. I'm aware of what is commented in the header and the todo file. Do you have a link to this discussion? In any case, I'd prefer to fix this in a separate patch set. I have patches that replace the ast and mgag200 fbdev consoles with generic code. That might be the right place. Best regards Thomas > > Sam > >> + /* We pin the BO so it won't be moved during the >> + * update. The actual location, video RAM or system >> + * memory, is not important. >> + */ >> + ret = drm_gem_vram_pin(gbo, 0); >> + if (ret) { >> + if (ret != -EBUSY) >> + return; >> + store_for_later = true; >> + } >> + } else >> store_for_later = true; >> - } >> >> x2 = x + width - 1; >> y2 = y + height - 1; >> @@ -126,7 +125,7 @@ static void ast_dirty_update(struct ast_fbdev *afbdev, >> drm_gem_vram_kunmap(gbo); >> >> out: >> - drm_gem_vram_unlock(gbo); >> + drm_gem_vram_unpin(gbo); >> } >> >> static void ast_fillrect(struct fb_info *info, >> -- >> 2.21.0
Hi Thomas. > > > > While touching this code, anyway we could get rid of drm_can_sleep()? > > I only ask because Daniel V. said that we should not use it. > > This was some months ago during some ehader file clean-up, so nothing > > in particular related to the ast driver. > > I'm aware of what is commented in the header and the todo file. Do you > have a link to this discussion? I managed to dig up this link: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1887389.html But that does not provide any additional technical details. > In any case, I'd prefer to fix this in a separate patch set. I have > patches that replace the ast and mgag200 fbdev consoles with generic > code. That might be the right place. Using generic code, and thus deleting the old code seems like a good plan. Sam
diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c index 05f45222b702..b404b51c2c55 100644 --- a/drivers/gpu/drm/ast/ast_fb.c +++ b/drivers/gpu/drm/ast/ast_fb.c @@ -48,32 +48,31 @@ static void ast_dirty_update(struct ast_fbdev *afbdev, int x, int y, int width, int height) { int i; - struct drm_gem_object *obj; struct drm_gem_vram_object *gbo; int src_offset, dst_offset; int bpp = afbdev->afb.base.format->cpp[0]; - int ret = -EBUSY; + int ret; u8 *dst; bool unmap = false; bool store_for_later = false; int x2, y2; unsigned long flags; - obj = afbdev->afb.obj; - gbo = drm_gem_vram_of_gem(obj); - - /* Try to lock the BO. If we fail with -EBUSY then - * the BO is being moved and we should store up the - * damage until later. - */ - if (drm_can_sleep()) - ret = drm_gem_vram_lock(gbo, true); - if (ret) { - if (ret != -EBUSY) - return; - + gbo = drm_gem_vram_of_gem(afbdev->afb.obj); + + if (drm_can_sleep()) { + /* We pin the BO so it won't be moved during the + * update. The actual location, video RAM or system + * memory, is not important. + */ + ret = drm_gem_vram_pin(gbo, 0); + if (ret) { + if (ret != -EBUSY) + return; + store_for_later = true; + } + } else store_for_later = true; - } x2 = x + width - 1; y2 = y + height - 1; @@ -126,7 +125,7 @@ static void ast_dirty_update(struct ast_fbdev *afbdev, drm_gem_vram_kunmap(gbo); out: - drm_gem_vram_unlock(gbo); + drm_gem_vram_unpin(gbo); } static void ast_fillrect(struct fb_info *info,
Another explicit lock operation of a GEM VRAM BO is located in AST's framebuffer update code. Instead of locking the BO, we pin it to wherever it is. v2: * update with pin flag of 0 Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/ast/ast_fb.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)