Re: [PATCH 10/17] drm/mgag200: Set pitch in a separate helper function

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi

Am 03.05.20 um 17:42 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Wed, Apr 29, 2020 at 04:32:31PM +0200, Thomas Zimmermann wrote:
>> The framebuffer's pitch is now set in mgag200_set_offset().
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> I failed to follow this code.

Is it because of the line-offset calculation? MGAs want offsets in
multiples of pixels, not bytes, and that factor depends on the value of
cpp. I guess putting the calculation into a separate function will help
with readability.

> 
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_mode.c | 41 +++++++++++++++++---------
>>  1 file changed, 27 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 92dee31f16c42..eb83e471d72fc 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -1003,6 +1003,32 @@ static void mgag200_set_mode_regs(struct mga_device *mdev,
>>  	mga_crtc_set_plls(mdev, mode->clock);
>>  }
>>  
>> +static void mgag200_set_offset(struct mga_device *mdev,
>> +			       const struct drm_framebuffer *fb)
>> +{
>> +	unsigned int offset;
>> +	uint8_t crtc13, crtcext0;
>> +	u8 bppshift;
>> +
>> +	bppshift = mdev->bpp_shifts[fb->format->cpp[0] - 1];
> Hmm, use of cpp is deprecated. But is continue to be used all over.

I try to move code without modifying it too much. Replacing cpp with
some newer interface would obfuscate the moving to some extend. It's
always a balancing act.

> 
>> +
>> +	offset = fb->pitches[0] / fb->format->cpp[0];
>> +	if (fb->format->cpp[0] * 8 == 24)
>> +		offset = (offset * 3) >> (4 - bppshift);
>> +	else
>> +		offset = offset >> (4 - bppshift);
>> +
>> +	RREG_ECRT(0, crtcext0);
>> +
>> +	crtc13 = offset & 0xff;
>> +
>> +	crtcext0 &= ~GENMASK(5, 4);
>> +	crtcext0 |= (offset & GENMASK(9, 8)) >> 4;
> Lot's of hardcoded numbers.
> Could the reg file include these so you had more readable defined names?

Sure.

Best regards
Thomas

> 
>> +
>> +	WREG_CRT(0x13, crtc13);
>> +	WREG_ECRT(0x00, crtcext0);
>> +}
>> +
>>  static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  				struct drm_display_mode *mode,
>>  				struct drm_display_mode *adjusted_mode,
>> @@ -1011,7 +1037,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  	struct drm_device *dev = crtc->dev;
>>  	struct mga_device *mdev = dev->dev_private;
>>  	const struct drm_framebuffer *fb = crtc->primary->fb;
>> -	int pitch;
>>  	int option = 0, option2 = 0;
>>  	int i;
>>  	unsigned char misc = 0;
>> @@ -1122,12 +1147,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  	WREG_SEQ(3, 0);
>>  	WREG_SEQ(4, 0xe);
>>  
>> -	pitch = fb->pitches[0] / fb->format->cpp[0];
>> -	if (fb->format->cpp[0] * 8 == 24)
>> -		pitch = (pitch * 3) >> (4 - bppshift);
>> -	else
>> -		pitch = pitch >> (4 - bppshift);
>> -
>>  	WREG_GFX(0, 0);
>>  	WREG_GFX(1, 0);
>>  	WREG_GFX(2, 0);
>> @@ -1144,20 +1163,15 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  	WREG_CRT(13, 0);
>>  	WREG_CRT(14, 0);
>>  	WREG_CRT(15, 0);
>> -	WREG_CRT(19, pitch & 0xFF);
>> -
>> -	ext_vga[0] = 0;
>>  
>>  	/* TODO interlace */
>>  
>> -	ext_vga[0] |= (pitch & 0x300) >> 4;
>>  	if (fb->format->cpp[0] * 8 == 24)
>>  		ext_vga[3] = (((1 << bppshift) * 3) - 1) | 0x80;
>>  	else
>>  		ext_vga[3] = ((1 << bppshift) - 1) | 0x80;
>>  	ext_vga[4] = 0;
>>  
>> -	WREG_ECRT(0, ext_vga[0]);
>>  	WREG_ECRT(3, ext_vga[3]);
>>  	WREG_ECRT(4, ext_vga[4]);
>>  
>> @@ -1171,8 +1185,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  		WREG_ECRT(6, 0);
>>  	}
>>  
>> -	WREG_ECRT(0, ext_vga[0]);
>> -
>>  	misc = RREG8(MGA_MISC_IN);
>>  	misc |= MGAREG_MISC_IOADSEL |
>>  		MGAREG_MISC_RAMMAPEN |
>> @@ -1180,6 +1192,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  	WREG8(MGA_MISC_OUT, misc);
>>  
>>  	mga_crtc_do_set_base(mdev, fb, old_fb);
>> +	mgag200_set_offset(mdev, fb);
>>  
>>  	mgag200_set_mode_regs(mdev, mode);
> 
> 	Sam
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


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

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

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