Re: [PATCH 05/17] drm/mgag200: Clean up mga_set_start_address()
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
>>> ---
>>> 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 > 0x1f);
>>> - 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 > 0x1f);
>>> +
>>> + 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
Re: [PATCH 05/17] drm/mgag200: Clean up mga_set_start_address()
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
>> ---
>> 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 > 0x1f);
>> -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 > 0x1f);
>> +
>> +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);
>> +
Re: [PATCH 05/17] drm/mgag200: Clean up mga_set_start_address()
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
> ---
> 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 > 0x1f);
> - 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 > 0x1f);
> +
> + 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.
> + ((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
