Re: [PATCH v2 5/9] drm/ast: Pin framebuffer BO during dirty update

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3134 bytes --]

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel