[PATCH 05/17] drm/mgag200: Clean up mga_set_start_address()
Thomas Zimmermann
tzimmermann at suse.de
Mon May 11 12:41:53 UTC 2020
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 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.
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
-------------- 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/20200511/9f3545f4/attachment.sig>
More information about the dri-devel
mailing list