Re: [PATCH 07/17] drm/mgag200: Move mode-setting code into separate helper function
Hi
Am 29.04.20 um 20:24 schrieb Sam Ravnborg:
> On Wed, Apr 29, 2020 at 04:32:28PM +0200, Thomas Zimmermann wrote:
>> The mode-setting code is now located in mgag200_set_mode_regs(), sans
>> a few flags that will be moved in a later patch for clarity.
>>
>> Signed-off-by: Thomas Zimmermann
> Looked like trivial code movements, did not look carefully.
It's often not that trivial.
The complicated thing about these functions is to not override bits that
have been set by another functions already. For example, MISC has a
number of bits that control PLL, mode, and hsync/vsync. Each is set in a
different place. I've been careful to read-back MISC each time and
update each setting individually.
Best regards
Thomas
> Acked-by: Sam Ravnborg
>
>> ---
>> drivers/gpu/drm/mgag200/mgag200_mode.c | 140 ++---
>> 1 file changed, 78 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 12df809d64f7c..749ba6e420ac7 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -911,6 +911,79 @@ static int mga_crtc_mode_set_base(struct drm_crtc
>> *crtc, int x, int y,
>> return mga_crtc_do_set_base(mdev, fb, old_fb);
>> }
>>
>> +static void mgag200_set_mode_regs(struct mga_device *mdev,
>> + const struct drm_display_mode *mode)
>> +{
>> +unsigned int hdisplay, hsyncstart, hsyncend, htotal;
>> +unsigned int vdisplay, vsyncstart, vsyncend, vtotal;
>> +uint8_t misc = 0;
>> +uint8_t crtcext1, crtcext2, crtcext5;
>> +
>> +hdisplay = mode->hdisplay / 8 - 1;
>> +hsyncstart = mode->hsync_start / 8 - 1;
>> +hsyncend = mode->hsync_end / 8 - 1;
>> +htotal = mode->htotal / 8 - 1;
>> +
>> +/* Work around hardware quirk */
>> +if ((htotal & 0x07) == 0x06 || (htotal & 0x07) == 0x04)
>> +htotal++;
>> +
>> +vdisplay = mode->vdisplay - 1;
>> +vsyncstart = mode->vsync_start - 1;
>> +vsyncend = mode->vsync_end - 1;
>> +vtotal = mode->vtotal - 2;
>> +
>> +if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>> +misc |= 0x40;
>> +if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>> +misc |= 0x80;
>> +
>> +crtcext1 = (((htotal - 4) & 0x100) >> 8) |
>> + ((hdisplay & 0x100) >> 7) |
>> + ((hsyncstart & 0x100) >> 6) |
>> +(htotal & 0x40);
>> +if (mdev->type == G200_WB || mdev->type == G200_EW3)
>> +crtcext1 |= BIT(7) | /* vrsten */
>> +BIT(3); /* hrsten */
>> +
>> +crtcext2 = ((vtotal & 0xc00) >> 10) |
>> + ((vdisplay & 0x400) >> 8) |
>> + ((vdisplay & 0xc00) >> 7) |
>> + ((vsyncstart & 0xc00) >> 5) |
>> + ((vdisplay & 0x400) >> 3);
>> +crtcext5 = 0x00;
>> +
>> +WREG_CRT(0, htotal - 4);
>> +WREG_CRT(1, hdisplay);
>> +WREG_CRT(2, hdisplay);
>> +WREG_CRT(3, (htotal & 0x1F) | 0x80);
>> +WREG_CRT(4, hsyncstart);
>> +WREG_CRT(5, ((htotal & 0x20) << 2) | (hsyncend & 0x1F));
>> +WREG_CRT(6, vtotal & 0xFF);
>> +WREG_CRT(7, ((vtotal & 0x100) >> 8) |
>> + ((vdisplay & 0x100) >> 7) |
>> + ((vsyncstart & 0x100) >> 6) |
>> + ((vdisplay & 0x100) >> 5) |
>> + ((vdisplay & 0x100) >> 4) | /* linecomp */
>> + ((vtotal & 0x200) >> 4) |
>> + ((vdisplay & 0x200) >> 3) |
>> + ((vsyncstart & 0x200) >> 2));
>> +WREG_CRT(9, ((vdisplay & 0x200) >> 4) |
>> + ((vdisplay & 0x200) >> 3));
>> +WREG_CRT(16, vsyncstart & 0xFF);
>> +WREG_CRT(17, (vsyncend & 0x0F) | 0x20);
>> +WREG_CRT(18, vdisplay & 0xFF);
>> +WREG_CRT(20, 0);
>> +WREG_CRT(21, vdisplay & 0xFF);
>> +WREG_CRT(22, (vtotal + 1) & 0xFF);
>> +WREG_CRT(23, 0xc3);
>> +WREG_CRT(24, vdisplay & 0xFF);
>> +
>> +WREG_ECRT(0x01, crtcext1);
>> +WREG_ECRT(0x02, crtcext2);
>> +WREG_ECRT(0x05, crtcext5);
>> +}
>> +
>> static int mga_crtc_mode_set(struct drm_crtc *crtc,
>> struct drm_display_mode *mode,
>> struct drm_display_mode *adjusted_mode,
>> @@ -919,8 +992,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>> struct drm_device *dev = crtc->dev;
>> struct mga_device *mdev = dev->dev_private;
>> const struct drm_framebuffer *fb = crtc->primary->fb;
>> -int hdisplay, hsyncstart, hsyncend, htotal;
>> -int vdisplay, vsyncstart, vsyncend, vtotal;
>> int pitch;
>> int option = 0, option2 = 0;
>> int i;
>> @@ -999,12 +1070,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>> break;
>> }
>>
>> -if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>> -misc |= 0x40;
>> -if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>> -misc |= 0x80;
>> -
>> -
>> for (i = 0; i < sizeof(dacvalue); i++) {
>>
Re: [PATCH 07/17] drm/mgag200: Move mode-setting code into separate helper function
On Wed, Apr 29, 2020 at 04:32:28PM +0200, Thomas Zimmermann wrote:
> The mode-setting code is now located in mgag200_set_mode_regs(), sans
> a few flags that will be moved in a later patch for clarity.
>
> Signed-off-by: Thomas Zimmermann
Looked like trivial code movements, did not look carefully.
Acked-by: Sam Ravnborg
> ---
> drivers/gpu/drm/mgag200/mgag200_mode.c | 140 ++---
> 1 file changed, 78 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
> b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 12df809d64f7c..749ba6e420ac7 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -911,6 +911,79 @@ static int mga_crtc_mode_set_base(struct drm_crtc *crtc,
> int x, int y,
> return mga_crtc_do_set_base(mdev, fb, old_fb);
> }
>
> +static void mgag200_set_mode_regs(struct mga_device *mdev,
> + const struct drm_display_mode *mode)
> +{
> + unsigned int hdisplay, hsyncstart, hsyncend, htotal;
> + unsigned int vdisplay, vsyncstart, vsyncend, vtotal;
> + uint8_t misc = 0;
> + uint8_t crtcext1, crtcext2, crtcext5;
> +
> + hdisplay = mode->hdisplay / 8 - 1;
> + hsyncstart = mode->hsync_start / 8 - 1;
> + hsyncend = mode->hsync_end / 8 - 1;
> + htotal = mode->htotal / 8 - 1;
> +
> + /* Work around hardware quirk */
> + if ((htotal & 0x07) == 0x06 || (htotal & 0x07) == 0x04)
> + htotal++;
> +
> + vdisplay = mode->vdisplay - 1;
> + vsyncstart = mode->vsync_start - 1;
> + vsyncend = mode->vsync_end - 1;
> + vtotal = mode->vtotal - 2;
> +
> + if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> + misc |= 0x40;
> + if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> + misc |= 0x80;
> +
> + crtcext1 = (((htotal - 4) & 0x100) >> 8) |
> +((hdisplay & 0x100) >> 7) |
> +((hsyncstart & 0x100) >> 6) |
> + (htotal & 0x40);
> + if (mdev->type == G200_WB || mdev->type == G200_EW3)
> + crtcext1 |= BIT(7) | /* vrsten */
> + BIT(3); /* hrsten */
> +
> + crtcext2 = ((vtotal & 0xc00) >> 10) |
> +((vdisplay & 0x400) >> 8) |
> +((vdisplay & 0xc00) >> 7) |
> +((vsyncstart & 0xc00) >> 5) |
> +((vdisplay & 0x400) >> 3);
> + crtcext5 = 0x00;
> +
> + WREG_CRT(0, htotal - 4);
> + WREG_CRT(1, hdisplay);
> + WREG_CRT(2, hdisplay);
> + WREG_CRT(3, (htotal & 0x1F) | 0x80);
> + WREG_CRT(4, hsyncstart);
> + WREG_CRT(5, ((htotal & 0x20) << 2) | (hsyncend & 0x1F));
> + WREG_CRT(6, vtotal & 0xFF);
> + WREG_CRT(7, ((vtotal & 0x100) >> 8) |
> + ((vdisplay & 0x100) >> 7) |
> + ((vsyncstart & 0x100) >> 6) |
> + ((vdisplay & 0x100) >> 5) |
> + ((vdisplay & 0x100) >> 4) | /* linecomp */
> + ((vtotal & 0x200) >> 4) |
> + ((vdisplay & 0x200) >> 3) |
> + ((vsyncstart & 0x200) >> 2));
> + WREG_CRT(9, ((vdisplay & 0x200) >> 4) |
> + ((vdisplay & 0x200) >> 3));
> + WREG_CRT(16, vsyncstart & 0xFF);
> + WREG_CRT(17, (vsyncend & 0x0F) | 0x20);
> + WREG_CRT(18, vdisplay & 0xFF);
> + WREG_CRT(20, 0);
> + WREG_CRT(21, vdisplay & 0xFF);
> + WREG_CRT(22, (vtotal + 1) & 0xFF);
> + WREG_CRT(23, 0xc3);
> + WREG_CRT(24, vdisplay & 0xFF);
> +
> + WREG_ECRT(0x01, crtcext1);
> + WREG_ECRT(0x02, crtcext2);
> + WREG_ECRT(0x05, crtcext5);
> +}
> +
> static int mga_crtc_mode_set(struct drm_crtc *crtc,
> struct drm_display_mode *mode,
> struct drm_display_mode *adjusted_mode,
> @@ -919,8 +992,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
> struct drm_device *dev = crtc->dev;
> struct mga_device *mdev = dev->dev_private;
> const struct drm_framebuffer *fb = crtc->primary->fb;
> - int hdisplay, hsyncstart, hsyncend, htotal;
> - int vdisplay, vsyncstart, vsyncend, vtotal;
> int pitch;
> int option = 0, option2 = 0;
> int i;
> @@ -999,12 +1070,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
> break;
> }
>
> - if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> - misc |= 0x40;
> - if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> - misc |= 0x80;
> -
> -
> for (i = 0; i < sizeof(dacvalue); i++) {
> if ((i <= 0x17) ||
> (i == 0x1b) ||
> @@ -1044,20 +1109,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
> else
> pitch = pitch >> (4 - bppshift);
>
> - hdisplay = mode->hdisplay / 8 - 1;
> - hsyncstart = mode->hsync_start / 8 - 1;
> - hsyncend = mode->hsync_end / 8 - 1;
> - htotal = mode->htotal / 8 - 1;
> -
> - /* Work around hardware quirk */
> - if ((htotal & 0x07) == 0x06 |
[PATCH 07/17] drm/mgag200: Move mode-setting code into separate helper function
The mode-setting code is now located in mgag200_set_mode_regs(), sans
a few flags that will be moved in a later patch for clarity.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_mode.c | 140 ++---
1 file changed, 78 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 12df809d64f7c..749ba6e420ac7 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -911,6 +911,79 @@ static int mga_crtc_mode_set_base(struct drm_crtc *crtc,
int x, int y,
return mga_crtc_do_set_base(mdev, fb, old_fb);
}
+static void mgag200_set_mode_regs(struct mga_device *mdev,
+ const struct drm_display_mode *mode)
+{
+ unsigned int hdisplay, hsyncstart, hsyncend, htotal;
+ unsigned int vdisplay, vsyncstart, vsyncend, vtotal;
+ uint8_t misc = 0;
+ uint8_t crtcext1, crtcext2, crtcext5;
+
+ hdisplay = mode->hdisplay / 8 - 1;
+ hsyncstart = mode->hsync_start / 8 - 1;
+ hsyncend = mode->hsync_end / 8 - 1;
+ htotal = mode->htotal / 8 - 1;
+
+ /* Work around hardware quirk */
+ if ((htotal & 0x07) == 0x06 || (htotal & 0x07) == 0x04)
+ htotal++;
+
+ vdisplay = mode->vdisplay - 1;
+ vsyncstart = mode->vsync_start - 1;
+ vsyncend = mode->vsync_end - 1;
+ vtotal = mode->vtotal - 2;
+
+ if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+ misc |= 0x40;
+ if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+ misc |= 0x80;
+
+ crtcext1 = (((htotal - 4) & 0x100) >> 8) |
+ ((hdisplay & 0x100) >> 7) |
+ ((hsyncstart & 0x100) >> 6) |
+ (htotal & 0x40);
+ if (mdev->type == G200_WB || mdev->type == G200_EW3)
+ crtcext1 |= BIT(7) | /* vrsten */
+ BIT(3); /* hrsten */
+
+ crtcext2 = ((vtotal & 0xc00) >> 10) |
+ ((vdisplay & 0x400) >> 8) |
+ ((vdisplay & 0xc00) >> 7) |
+ ((vsyncstart & 0xc00) >> 5) |
+ ((vdisplay & 0x400) >> 3);
+ crtcext5 = 0x00;
+
+ WREG_CRT(0, htotal - 4);
+ WREG_CRT(1, hdisplay);
+ WREG_CRT(2, hdisplay);
+ WREG_CRT(3, (htotal & 0x1F) | 0x80);
+ WREG_CRT(4, hsyncstart);
+ WREG_CRT(5, ((htotal & 0x20) << 2) | (hsyncend & 0x1F));
+ WREG_CRT(6, vtotal & 0xFF);
+ WREG_CRT(7, ((vtotal & 0x100) >> 8) |
+((vdisplay & 0x100) >> 7) |
+((vsyncstart & 0x100) >> 6) |
+((vdisplay & 0x100) >> 5) |
+((vdisplay & 0x100) >> 4) | /* linecomp */
+((vtotal & 0x200) >> 4) |
+((vdisplay & 0x200) >> 3) |
+((vsyncstart & 0x200) >> 2));
+ WREG_CRT(9, ((vdisplay & 0x200) >> 4) |
+((vdisplay & 0x200) >> 3));
+ WREG_CRT(16, vsyncstart & 0xFF);
+ WREG_CRT(17, (vsyncend & 0x0F) | 0x20);
+ WREG_CRT(18, vdisplay & 0xFF);
+ WREG_CRT(20, 0);
+ WREG_CRT(21, vdisplay & 0xFF);
+ WREG_CRT(22, (vtotal + 1) & 0xFF);
+ WREG_CRT(23, 0xc3);
+ WREG_CRT(24, vdisplay & 0xFF);
+
+ WREG_ECRT(0x01, crtcext1);
+ WREG_ECRT(0x02, crtcext2);
+ WREG_ECRT(0x05, crtcext5);
+}
+
static int mga_crtc_mode_set(struct drm_crtc *crtc,
struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode,
@@ -919,8 +992,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
struct drm_device *dev = crtc->dev;
struct mga_device *mdev = dev->dev_private;
const struct drm_framebuffer *fb = crtc->primary->fb;
- int hdisplay, hsyncstart, hsyncend, htotal;
- int vdisplay, vsyncstart, vsyncend, vtotal;
int pitch;
int option = 0, option2 = 0;
int i;
@@ -999,12 +1070,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
break;
}
- if (mode->flags & DRM_MODE_FLAG_NHSYNC)
- misc |= 0x40;
- if (mode->flags & DRM_MODE_FLAG_NVSYNC)
- misc |= 0x80;
-
-
for (i = 0; i < sizeof(dacvalue); i++) {
if ((i <= 0x17) ||
(i == 0x1b) ||
@@ -1044,20 +1109,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
else
pitch = pitch >> (4 - bppshift);
- hdisplay = mode->hdisplay / 8 - 1;
- hsyncstart = mode->hsync_start / 8 - 1;
- hsyncend = mode->hsync_end / 8 - 1;
- htotal = mode->htotal / 8 - 1;
-
- /* Work around hardware quirk */
- if ((htotal & 0x07) == 0x06 || (htotal & 0x07) == 0x04)
- htotal++;
-
- vdisplay = mode->vdisplay - 1;
- vsyncstart = mode->vsync_start - 1;
- vsyncend = mode->vsync_end - 1;
- vtotal = mode->vtotal - 2;
-
WREG_
