[PATCH 05/17] drm/mgag200: Clean up mga_set_start_address()
Thomas Zimmermann
tzimmermann at suse.de
Thu Apr 30 08:23:03 UTC 2020
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200430/c7c9585e/attachment-0001.sig>
More information about the dri-devel
mailing list