Re: [PATCH 05/17] drm/mgag200: Clean up mga_set_start_address()

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi

Am 29.04.20 um 20:20 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Wed, Apr 29, 2020 at 04:32:26PM +0200, Thomas Zimmermann wrote:
>> All register names and fields are now named according to the
>> MGA programming manuals. The function doesn't need the CRTC, so
>> callers pass in the device structure directly. The logging now
>> uses device-specific macros.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  5 ++
>>  drivers/gpu/drm/mgag200/mgag200_mode.c | 82 +++++++++++++++-----------
>>  2 files changed, 53 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index 4403145e3593c..9b957d9fc7e04 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -61,6 +61,11 @@
>>  		WREG8(MGAREG_CRTC_DATA, v);			\
>>  	} while (0)						\
>>  
>> +#define RREG_ECRT(reg, v)					\
>> +	do {							\
>> +		WREG8(MGAREG_CRTCEXT_INDEX, reg);		\
>> +		v = RREG8(MGAREG_CRTCEXT_DATA);			\
>> +	} while (0)						\
>>  
>>  #define WREG_ECRT(reg, v)					\
>>  	do {							\
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 3d894b37a0812..b16a73c8617d6 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -819,49 +819,53 @@ static void mga_g200wb_commit(struct drm_crtc *crtc)
>>  }
>>  
>>  /*
>> -   This is how the framebuffer base address is stored in g200 cards:
>> -   * Assume @offset is the gpu_addr variable of the framebuffer object
>> -   * Then addr is the number of _pixels_ (not bytes) from the start of
>> -     VRAM to the first pixel we want to display. (divided by 2 for 32bit
>> -     framebuffers)
>> -   * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers
>> -   addr<20> -> CRTCEXT0<6>
>> -   addr<19-16> -> CRTCEXT0<3-0>
>> -   addr<15-8> -> CRTCC<7-0>
>> -   addr<7-0> -> CRTCD<7-0>
>> -   CRTCEXT0 has to be programmed last to trigger an update and make the
>> -   new addr variable take effect.
>> + * This is how the framebuffer base address is stored in g200 cards:
>> + *   * Assume @offset is the gpu_addr variable of the framebuffer object
>> + *   * Then addr is the number of _pixels_ (not bytes) from the start of
>> + *     VRAM to the first pixel we want to display. (divided by 2 for 32bit
>> + *     framebuffers)
>> + *   * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers
>> + *      addr<20> -> CRTCEXT0<6>
>> + *      addr<19-16> -> CRTCEXT0<3-0>
>> + *      addr<15-8> -> CRTCC<7-0>
>> + *      addr<7-0> -> CRTCD<7-0>
>> + *
>> + *  CRTCEXT0 has to be programmed last to trigger an update and make the
>> + *  new addr variable take effect.
>>   */
>> -static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset)
>> +static void mgag200_set_startadd(struct mga_device *mdev,
>> +				 unsigned long offset)
>>  {
>> -	struct mga_device *mdev = crtc->dev->dev_private;
>> -	u32 addr;
>> -	int count;
>> -	u8 crtcext0;
>> +	struct drm_device *dev = mdev->dev;
>> +	uint32_t startadd;
>> +	uint8_t crtcc, crtcd, crtcext0;
>>  
>> -	while (RREG8(0x1fda) & 0x08);
>> -	while (!(RREG8(0x1fda) & 0x08));
>> +	startadd = offset / 8;
>>  
>> -	count = RREG8(MGAREG_VCOUNT) + 2;
>> -	while (RREG8(MGAREG_VCOUNT) < count);
>> -
>> -	WREG8(MGAREG_CRTCEXT_INDEX, 0);
>> -	crtcext0 = RREG8(MGAREG_CRTCEXT_DATA);
>> -	crtcext0 &= 0xB0;
>> -	addr = offset / 8;
>> -	/* Can't store addresses any higher than that...
>> -	   but we also don't have more than 16MB of memory, so it should be fine. */
>> -	WARN_ON(addr > 0x1fffff);
>> -	crtcext0 |= (!!(addr & (1<<20)))<<6;
>> -	WREG_CRT(0x0d, (u8)(addr & 0xff));
>> -	WREG_CRT(0x0c, (u8)(addr >> 8) & 0xff);
>> -	WREG_ECRT(0x0, ((u8)(addr >> 16) & 0xf) | crtcext0);
>> +	/*
>> +	 * Can't store addresses any higher than that, but we also
>> +	 * don't have more than 16MB of memory, so it should be fine.
>> +	 */
>> +	drm_WARN_ON(dev, startadd > 0x1fffff);
>> +
>> +	RREG_ECRT(0x00, crtcext0);
>> +
>> +	crtcc = (startadd >> 8) & 0xff;
>> +	crtcd = startadd & 0xff;
>> +	crtcext0 &= 0xb0;
> 
>> +	crtcext0 |= ((startadd >> 14) & BIT(6)) |
> It is not so obvious that the value of bit 20 is stored in bit 6 here.
> 
> Maybe:
> 	crtcext0 |= ((startadd & BIT(20) >> 14) |
> 
> I would find the above easier to parse.

Ok. I can change that.

> 
>> +		    ((startadd >> 16) & 0x0f);
> 
>> +
>> +	WREG_CRT(0x0c, crtcc);
>> +	WREG_CRT(0x0d, crtcd);
>> +	WREG_ECRT(0x00, crtcext0);
>>  }
>>  
>>  static int mga_crtc_do_set_base(struct drm_crtc *crtc,
>>  				struct drm_framebuffer *fb,
>>  				int x, int y, int atomic)
>>  {
>> +	struct mga_device *mdev = crtc->dev->dev_private;
> Could you use a crtc_to_mdev() macro here.
> So we avoid adding new users of dev_private?

I introduce such a macro in a later patch. I guess I'll add a separate
patch for the macro and the conversion of all these dev_private references.


> 
>>  	struct drm_gem_vram_object *gbo;
>>  	int ret;
>>  	s64 gpu_addr;
> Make this unsigned long and..

The function that returns gpu_addr can fail (but shouldn't) with a
negative error. That's why it's signed.

> 
>> @@ -882,7 +886,7 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc,
>>  		goto err_drm_gem_vram_unpin;
>>  	}
>>  
>> -	mga_set_start_address(crtc, (u32)gpu_addr);
>> +	mgag200_set_startadd(mdev, (unsigned long)gpu_addr);
> drop this cast.
> 
> 
>>  
>>  	return 0;
>>  
>> @@ -894,6 +898,16 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc,
>>  static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
>>  				  struct drm_framebuffer *old_fb)
>>  {
>> +	struct drm_device *dev = crtc->dev;
>> +	struct mga_device *mdev = dev->dev_private;
>> +	unsigned int count;
>> +
>> +	while (RREG8(0x1fda) & 0x08) { }
>> +	while (!(RREG8(0x1fda) & 0x08)) { }
>> +
>> +	count = RREG8(MGAREG_VCOUNT) + 2;
>> +	while (RREG8(MGAREG_VCOUNT) < count) { }
>> +
>>  	return mga_crtc_do_set_base(crtc, old_fb, x, y, 0);
>>  }
> I do not really see why this code was lifter two functions up.
> Before is was deep in mga_set_start_address(), now it is in
> mga_crtc_mode_set_base().
> Puzzeled?

Ah, that should have probably been explained in the commit message. The
above code waits for the vsync flag to go up plus two scanlines. That
way the pageflip happens during a vblank period.

It's fairly inefficient AFAICT. I don't want this code in atomic
modesetting, but didn't want to throw it away yet. So it's now in the
non-atomic callback. Later when the atomic code calls
mgag200_set_startadd(), it shouldn't busy-wait for vblanks.

I still have a patch that adds vblank irq support to mgag200. I'd rather
merge that instead of keeping this busy waiting in the driver.

Best regards
Thomas


> 
> 	Sam
> 
> 
> 
>>  
>> -- 
>> 2.26.0

-- 
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