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: 7729 bytes --]

Hi

Am 30.04.20 um 10:23 schrieb Thomas Zimmermann:
> 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.

Reading the code again, I think it's better to keep it as it is. At
least it's consistent with the rest of the function.

Best regards
Thomas

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