[RFC PATCH 0/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[RFC PATCH 0/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[Jocelyn Falempe]

This series adds DMA and IRQ for the mgag200 driver.
Unfortunately the DMA doesn't make the driver faster.
But it's still a big improvement regarding CPU usage and latency.

CPU usage goes from 100% of 1 CPU to 3% (using top and refreshing the screen continuously).

top without DMA, and a bash script to refresh the screen continuously
    PID S  %CPU     TIME+ COMMAND
   1536 R 100.0   4:02.78 kworker/1:0+events
   1612 S   3.0   0:03.82 bash
     16 I   0.3   0:01.56 rcu_preempt
   1467 I   0.3   0:00.11 kworker/u64:1-events_unbound
   3650 R   0.3   0:00.02 top

top with DMA, and the same bash script:
    PID S  %CPU     TIME+ COMMAND
   1335 D   3.0   0:01.26 kworker/2:0+events
   1486 S   0.3   0:00.14 bash
   1846 R   0.3   0:00.03 top
      1 S   0.0   0:01.87 systemd
      2 S   0.0   0:00.00 kthreadd

Latency, measured with cyclictest -s -l 10000:
Without DMA:
# /dev/cpu_dma_latency set to 0us
policy: other/other: loadavg: 1.52 0.52 0.33 3/358 2025          
T: 0 ( 1977) P: 0 I:1000 C:  10000 Min:      7 Act:   56 Avg:   85 Max:    2542

With DMA:
# /dev/cpu_dma_latency set to 0us
policy: other/other: loadavg: 1.27 0.48 0.18 2/363 2498          
T: 0 ( 2403) P: 0 I:1000 C:  10000 Min:      8 Act:   62 Avg:   59 Max:     339

Last benchmark is glxgears. It's still software rendering, but on my 2 core CPU,
freeing one CPU constantly doing memcpy(), allows it to draw more frames.
Without DMA:
415 frames in 5.0 seconds = 82.973 FPS
356 frames in 5.0 seconds = 71.167 FPS
with DMA:
717 frames in 5.0 seconds = 143.343 FPS
720 frames in 5.0 seconds = 143.993 FPS

Regarding the implementation:
The driver uses primary DMA to send drawing engine commands, and secondary DMA to send the pixels to an ILOAD command.
You can directly program the ILOAD command, and use Primary DMA to send the pixels, but in this case, you can't use the softrap interrupt to wait for the DMA completion.
The pixels are copied from the gem framebuffer to the DMA buffer, but as system memory is much faster than VRAM, it has a negligible impact.

DMA buffer size:
On my test machine, I can allocate only 4MB of dma coherent memory, and the framebuffer is 5MB.
So the driver has to cut it into small chunks when the full framebuffer is refreshed.
My implementation tries to allocate 4MB, and then smaller allocation until it succeeds.
If it fails to allocate, DMA will be disabled. That's probably not perfect, but at least it's simple.
It's also possible to do some kind of scatter gather DMA, by sending multiple ILOAD/SECDMA, but that increases the complexity a bit.

Adding a module parameter to disable DMA:
I think before merging this work, I will add a module parameter to disable DMA, so that if
something goes wrong it's easy to turn it off.

Pixel width:
I tested this in 16 bits per pixels RGB565 and 32 bits per pixels (XRGB8888).
I didn't find a userspace able to use 24 bits (RGB888), Xorg uses XRGB8888 when specifying
"DefaultDepth" to 24.

Big endian:
The DMA can be configured to handle the be->le conversion, but I can't test it, so it's not done yet.
As I don't know if there are still big endian systems with mgag200, maybe disabling DMA for big endian is the safest option ?

I think the complexity is low, as it only adds ~350 lines, less than 10% of the whole mgag200 driver (~5000 lines).

 drivers/gpu/drm/mgag200/Makefile       |   3 +-
 drivers/gpu/drm/mgag200/mgag200_dma.c  | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_drv.c  |  43 +++++++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  28 ++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 200 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
 drivers/gpu/drm/mgag200/mgag200_reg.h  |  30 ++++++++++++++++++-
 6 files changed, 362 insertions(+), 56 deletions(-)

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>

base-commit: 457391b0380335d5 (tag: v6.3)

[PATCH 1/4] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS

[Jocelyn Falempe]

From: Thomas Zimmermann <tzimmermann@suse.de>

Register constants are upper case. Fix MGAREG_Status accordingly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 6 +++---
 drivers/gpu/drm/mgag200/mgag200_reg.h  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 0a5aaf78172a..9a24b9c00745 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -109,12 +109,12 @@ static inline void mga_wait_vsync(struct mga_device *mdev)
 	unsigned int status = 0;
 
 	do {
-		status = RREG32(MGAREG_Status);
+		status = RREG32(MGAREG_STATUS);
 	} while ((status & 0x08) && time_before(jiffies, timeout));
 	timeout = jiffies + HZ/10;
 	status = 0;
 	do {
-		status = RREG32(MGAREG_Status);
+		status = RREG32(MGAREG_STATUS);
 	} while (!(status & 0x08) && time_before(jiffies, timeout));
 }
 
@@ -123,7 +123,7 @@ static inline void mga_wait_busy(struct mga_device *mdev)
 	unsigned long timeout = jiffies + HZ;
 	unsigned int status = 0;
 	do {
-		status = RREG8(MGAREG_Status + 2);
+		status = RREG8(MGAREG_STATUS + 2);
 	} while ((status & 0x01) && time_before(jiffies, timeout));
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
index 1019ffd6c260..aa73463674e4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_reg.h
+++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
@@ -102,7 +102,7 @@
 #define MGAREG_EXEC		0x0100
 
 #define	MGAREG_FIFOSTATUS	0x1e10
-#define	MGAREG_Status		0x1e14
+#define	MGAREG_STATUS		0x1e14
 #define MGAREG_CACHEFLUSH       0x1fff
 #define	MGAREG_ICLEAR		0x1e18
 #define	MGAREG_IEN		0x1e1c
-- 
2.39.2

[PATCH 2/4] drm/mgag200: Simplify offset and scale computation.

[Jocelyn Falempe]

Now that the driver handles only 16, 24 and 32-bit framebuffer,
it can be simplified.

No functional changes.

offset:
16bit: (bppshift = 1)
offset = width >> (4 - bppshift) => width / 8 => pitch / 16

24bit:  (bppshift = 0)
offset = (width * 3) >> (4 - bppshift)  => width * 3 / 16 => pitch / 16

32bit:  (bppshift = 2)
offset = width >> (4 - bppshift) => width / 4 => pitch / 16

scale:
16bit:
scale = (1 << bppshift) - 1 => 1
24bit:
scale = ((1 << bppshift) * 3) - 1 => 2
32bit:
scale = (1 << bppshift) - 1 => 3

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 63 +++++++-------------------
 1 file changed, 16 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 9a24b9c00745..7d8c65372ac4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -280,30 +280,16 @@ void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mod
 	WREG8(MGA_MISC_OUT, misc);
 }
 
-static u8 mgag200_get_bpp_shift(const struct drm_format_info *format)
-{
-	static const u8 bpp_shift[] = {0, 1, 0, 2};
-
-	return bpp_shift[format->cpp[0] - 1];
-}
-
 /*
  * Calculates the HW offset value from the framebuffer's pitch. The
  * offset is a multiple of the pixel size and depends on the display
- * format.
+ * format. With width in pixels and pitch in bytes, the formula is:
+ * offset = width * bpp / 128 = pitch / 16
  */
 static u32 mgag200_calculate_offset(struct mga_device *mdev,
 				    const struct drm_framebuffer *fb)
 {
-	u32 offset = fb->pitches[0] / fb->format->cpp[0];
-	u8 bppshift = mgag200_get_bpp_shift(fb->format);
-
-	if (fb->format->cpp[0] * 8 == 24)
-		offset = (offset * 3) >> (4 - bppshift);
-	else
-		offset = offset >> (4 - bppshift);
-
-	return offset;
+	return fb->pitches[0] >> 4;
 }
 
 static void mgag200_set_offset(struct mga_device *mdev,
@@ -326,48 +312,25 @@ static void mgag200_set_offset(struct mga_device *mdev,
 void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_info *format)
 {
 	struct drm_device *dev = &mdev->base;
-	unsigned int bpp, bppshift, scale;
+	unsigned int scale;
 	u8 crtcext3, xmulctrl;
 
-	bpp = format->cpp[0] * 8;
-
-	bppshift = mgag200_get_bpp_shift(format);
-	switch (bpp) {
-	case 24:
-		scale = ((1 << bppshift) * 3) - 1;
-		break;
-	default:
-		scale = (1 << bppshift) - 1;
-		break;
-	}
-
-	RREG_ECRT(3, crtcext3);
-
-	switch (bpp) {
-	case 8:
-		xmulctrl = MGA1064_MUL_CTL_8bits;
-		break;
-	case 16:
-		if (format->depth == 15)
-			xmulctrl = MGA1064_MUL_CTL_15bits;
-		else
-			xmulctrl = MGA1064_MUL_CTL_16bits;
+	switch (format->format) {
+	case DRM_FORMAT_RGB565:
+		xmulctrl = MGA1064_MUL_CTL_16bits;
 		break;
-	case 24:
+	case DRM_FORMAT_RGB888:
 		xmulctrl = MGA1064_MUL_CTL_24bits;
 		break;
-	case 32:
+	case DRM_FORMAT_XRGB8888:
 		xmulctrl = MGA1064_MUL_CTL_32_24bits;
 		break;
 	default:
 		/* BUG: We should have caught this problem already. */
-		drm_WARN_ON(dev, "invalid format depth\n");
+		drm_WARN_ON(dev, "invalid drm format\n");
 		return;
 	}
 
-	crtcext3 &= ~GENMASK(2, 0);
-	crtcext3 |= scale;
-
 	WREG_DAC(MGA1064_MUL_CTL, xmulctrl);
 
 	WREG_GFX(0, 0x00);
@@ -383,6 +346,12 @@ void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_in
 	WREG_GFX(7, 0x0f);
 	WREG_GFX(8, 0x0f);
 
+	/* scale is the number of bytes per pixels - 1 */
+	scale = format->cpp[0] - 1;
+
+	RREG_ECRT(3, crtcext3);
+	crtcext3 &= ~GENMASK(2, 0);
+	crtcext3 |= scale;
 	WREG_ECRT(3, crtcext3);
 }
 
-- 
2.39.2

Re: [PATCH 2/4] drm/mgag200: Simplify offset and scale computation.

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4538 bytes --]

Hi

Am 05.05.23 um 14:43 schrieb Jocelyn Falempe:
> Now that the driver handles only 16, 24 and 32-bit framebuffer,
> it can be simplified.

I think it should say that the driver never really handled 8-bit colors. 
Or at least I'm not aware of.

> 
> No functional changes.
> 
> offset:
> 16bit: (bppshift = 1)
> offset = width >> (4 - bppshift) => width / 8 => pitch / 16
> 
> 24bit:  (bppshift = 0)
> offset = (width * 3) >> (4 - bppshift)  => width * 3 / 16 => pitch / 16
> 
> 32bit:  (bppshift = 2)
> offset = width >> (4 - bppshift) => width / 4 => pitch / 16
> 
> scale:
> 16bit:
> scale = (1 << bppshift) - 1 => 1
> 24bit:
> scale = ((1 << bppshift) * 3) - 1 => 2
> 32bit:
> scale = (1 << bppshift) - 1 => 3
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/mgag200/mgag200_mode.c | 63 +++++++-------------------
>   1 file changed, 16 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 9a24b9c00745..7d8c65372ac4 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -280,30 +280,16 @@ void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mod
>   	WREG8(MGA_MISC_OUT, misc);
>   }
>   
> -static u8 mgag200_get_bpp_shift(const struct drm_format_info *format)
> -{
> -	static const u8 bpp_shift[] = {0, 1, 0, 2};
> -
> -	return bpp_shift[format->cpp[0] - 1];
> -}
> -
>   /*
>    * Calculates the HW offset value from the framebuffer's pitch. The
>    * offset is a multiple of the pixel size and depends on the display
> - * format.
> + * format. With width in pixels and pitch in bytes, the formula is:
> + * offset = width * bpp / 128 = pitch / 16
>    */
>   static u32 mgag200_calculate_offset(struct mga_device *mdev,
>   				    const struct drm_framebuffer *fb)
>   {
> -	u32 offset = fb->pitches[0] / fb->format->cpp[0];
> -	u8 bppshift = mgag200_get_bpp_shift(fb->format);
> -
> -	if (fb->format->cpp[0] * 8 == 24)
> -		offset = (offset * 3) >> (4 - bppshift);
> -	else
> -		offset = offset >> (4 - bppshift);
> -
> -	return offset;
> +	return fb->pitches[0] >> 4;

24-bit is different from the rest. I don't understand how you simplified 
this code.

Best regards
Thomas

>   }
>   
>   static void mgag200_set_offset(struct mga_device *mdev,
> @@ -326,48 +312,25 @@ static void mgag200_set_offset(struct mga_device *mdev,
>   void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_info *format)
>   {
>   	struct drm_device *dev = &mdev->base;
> -	unsigned int bpp, bppshift, scale;
> +	unsigned int scale;
>   	u8 crtcext3, xmulctrl;
>   
> -	bpp = format->cpp[0] * 8;
> -
> -	bppshift = mgag200_get_bpp_shift(format);
> -	switch (bpp) {
> -	case 24:
> -		scale = ((1 << bppshift) * 3) - 1;
> -		break;
> -	default:
> -		scale = (1 << bppshift) - 1;
> -		break;
> -	}
> -
> -	RREG_ECRT(3, crtcext3);
> -
> -	switch (bpp) {
> -	case 8:
> -		xmulctrl = MGA1064_MUL_CTL_8bits;
> -		break;
> -	case 16:
> -		if (format->depth == 15)
> -			xmulctrl = MGA1064_MUL_CTL_15bits;
> -		else
> -			xmulctrl = MGA1064_MUL_CTL_16bits;
> +	switch (format->format) {
> +	case DRM_FORMAT_RGB565:
> +		xmulctrl = MGA1064_MUL_CTL_16bits;
>   		break;
> -	case 24:
> +	case DRM_FORMAT_RGB888:
>   		xmulctrl = MGA1064_MUL_CTL_24bits;
>   		break;
> -	case 32:
> +	case DRM_FORMAT_XRGB8888:
>   		xmulctrl = MGA1064_MUL_CTL_32_24bits;
>   		break;
>   	default:
>   		/* BUG: We should have caught this problem already. */
> -		drm_WARN_ON(dev, "invalid format depth\n");
> +		drm_WARN_ON(dev, "invalid drm format\n");
>   		return;
>   	}
>   
> -	crtcext3 &= ~GENMASK(2, 0);
> -	crtcext3 |= scale;
> -
>   	WREG_DAC(MGA1064_MUL_CTL, xmulctrl);
>   
>   	WREG_GFX(0, 0x00);
> @@ -383,6 +346,12 @@ void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_in
>   	WREG_GFX(7, 0x0f);
>   	WREG_GFX(8, 0x0f);
>   
> +	/* scale is the number of bytes per pixels - 1 */
> +	scale = format->cpp[0] - 1;
> +
> +	RREG_ECRT(3, crtcext3);
> +	crtcext3 &= ~GENMASK(2, 0);
> +	crtcext3 |= scale;
>   	WREG_ECRT(3, crtcext3);
>   }
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/4] drm/mgag200: Simplify offset and scale computation.

[Jocelyn Falempe]

On 08/05/2023 09:44, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.05.23 um 14:43 schrieb Jocelyn Falempe:
>> Now that the driver handles only 16, 24 and 32-bit framebuffer,
>> it can be simplified.
> 
> I think it should say that the driver never really handled 8-bit colors. 
> Or at least I'm not aware of.

Ok I can change that. This patch is just a cleanup, and is not really 
necessary for DMA.
> 
>>
>> No functional changes.
>>
>> offset:
>> 16bit: (bppshift = 1)
>> offset = width >> (4 - bppshift) => width / 8 => pitch / 16
>>
>> 24bit:  (bppshift = 0)
>> offset = (width * 3) >> (4 - bppshift)  => width * 3 / 16 => pitch / 16
>>
>> 32bit:  (bppshift = 2)
>> offset = width >> (4 - bppshift) => width / 4 => pitch / 16
>>
>> scale:
>> 16bit:
>> scale = (1 << bppshift) - 1 => 1
>> 24bit:
>> scale = ((1 << bppshift) * 3) - 1 => 2
>> 32bit:
>> scale = (1 << bppshift) - 1 => 3
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_mode.c | 63 +++++++-------------------
>>   1 file changed, 16 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c 
>> b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 9a24b9c00745..7d8c65372ac4 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -280,30 +280,16 @@ void mgag200_set_mode_regs(struct mga_device 
>> *mdev, const struct drm_display_mod
>>       WREG8(MGA_MISC_OUT, misc);
>>   }
>> -static u8 mgag200_get_bpp_shift(const struct drm_format_info *format)
>> -{
>> -    static const u8 bpp_shift[] = {0, 1, 0, 2};
>> -
>> -    return bpp_shift[format->cpp[0] - 1];
>> -}
>> -
>>   /*
>>    * Calculates the HW offset value from the framebuffer's pitch. The
>>    * offset is a multiple of the pixel size and depends on the display
>> - * format.
>> + * format. With width in pixels and pitch in bytes, the formula is:
>> + * offset = width * bpp / 128 = pitch / 16
>>    */
>>   static u32 mgag200_calculate_offset(struct mga_device *mdev,
>>                       const struct drm_framebuffer *fb)
>>   {
>> -    u32 offset = fb->pitches[0] / fb->format->cpp[0];
>> -    u8 bppshift = mgag200_get_bpp_shift(fb->format);
>> -
>> -    if (fb->format->cpp[0] * 8 == 24)
>> -        offset = (offset * 3) >> (4 - bppshift);
>> -    else
>> -        offset = offset >> (4 - bppshift);
>> -
>> -    return offset;
>> +    return fb->pitches[0] >> 4;
> 
> 24-bit is different from the rest. I don't understand how you simplified 
> this code.

This code was overly complex, but this special case is compensated by 
the "bpp_shift" thing. The formula width * bpp / 128 is in the G200 
documentation 4.6.5

u32 offset = fb->pitches[0] / fb->format->cpp[0];

so offset is now the width in pixels. (pitches[0] is the width in bytes)
bppshift is 0 for 24 bits, 1 for 16 bits and 2 for 32 bits

offset:
16bit: (bppshift = 1) ; pitch = width * 2
offset = width >> (4 - bppshift) => width / 8 => pitch / 16

24bit:  (bppshift = 0) ; pitch = width * 3
offset = (width * 3) >> (4 - bppshift)  => width * 3 / 16 => pitch / 16

32bit:  (bppshift = 2) ; pith = width * 4
offset = width >> (4 - bppshift) => width / 4 => pitch / 16

> 
> Best regards
> Thomas
> 
>>   }
>>   static void mgag200_set_offset(struct mga_device *mdev,
>> @@ -326,48 +312,25 @@ static void mgag200_set_offset(struct mga_device 
>> *mdev,
>>   void mgag200_set_format_regs(struct mga_device *mdev, const struct 
>> drm_format_info *format)
>>   {
>>       struct drm_device *dev = &mdev->base;
>> -    unsigned int bpp, bppshift, scale;
>> +    unsigned int scale;
>>       u8 crtcext3, xmulctrl;
>> -    bpp = format->cpp[0] * 8;
>> -
>> -    bppshift = mgag200_get_bpp_shift(format);
>> -    switch (bpp) {
>> -    case 24:
>> -        scale = ((1 << bppshift) * 3) - 1;
>> -        break;
>> -    default:
>> -        scale = (1 << bppshift) - 1;
>> -        break;
>> -    }
>> -
>> -    RREG_ECRT(3, crtcext3);
>> -
>> -    switch (bpp) {
>> -    case 8:
>> -        xmulctrl = MGA1064_MUL_CTL_8bits;
>> -        break;
>> -    case 16:
>> -        if (format->depth == 15)
>> -            xmulctrl = MGA1064_MUL_CTL_15bits;
>> -        else
>> -            xmulctrl = MGA1064_MUL_CTL_16bits;
>> +    switch (format->format) {
>> +    case DRM_FORMAT_RGB565:
>> +        xmulctrl = MGA1064_MUL_CTL_16bits;
>>           break;
>> -    case 24:
>> +    case DRM_FORMAT_RGB888:
>>           xmulctrl = MGA1064_MUL_CTL_24bits;
>>           break;
>> -    case 32:
>> +    case DRM_FORMAT_XRGB8888:
>>           xmulctrl = MGA1064_MUL_CTL_32_24bits;
>>           break;
>>       default:
>>           /* BUG: We should have caught this problem already. */
>> -        drm_WARN_ON(dev, "invalid format depth\n");
>> +        drm_WARN_ON(dev, "invalid drm format\n");
>>           return;
>>       }
>> -    crtcext3 &= ~GENMASK(2, 0);
>> -    crtcext3 |= scale;
>> -
>>       WREG_DAC(MGA1064_MUL_CTL, xmulctrl);
>>       WREG_GFX(0, 0x00);
>> @@ -383,6 +346,12 @@ void mgag200_set_format_regs(struct mga_device 
>> *mdev, const struct drm_format_in
>>       WREG_GFX(7, 0x0f);
>>       WREG_GFX(8, 0x0f);
>> +    /* scale is the number of bytes per pixels - 1 */
>> +    scale = format->cpp[0] - 1;
>> +
>> +    RREG_ECRT(3, crtcext3);
>> +    crtcext3 &= ~GENMASK(2, 0);
>> +    crtcext3 |= scale;
>>       WREG_ECRT(3, crtcext3);
>>   }
> 


-- 

Jocelyn

[PATCH 3/4] drm/mgag200: Add IRQ support

[Jocelyn Falempe]

Register irq, and enable the softrap irq.
This patch has no functional impact since softrap
irq can't be triggered without DMA.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c | 41 +++++++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_drv.h |  3 ++
 drivers/gpu/drm/mgag200/mgag200_reg.h |  3 ++
 3 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 976f0ab2006b..3566fcdfe1e4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -110,12 +110,43 @@ resource_size_t mgag200_device_probe_vram(struct mga_device *mdev)
 	return mgag200_probe_vram(mdev->vram, resource_size(mdev->vram_res));
 }
 
+irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
+{
+	struct mga_device *mdev = (struct mga_device *) arg;
+	u32 status;
+
+	status = RREG32(MGAREG_STATUS);
+
+	if (status & MGAIRQ_SOFTRAP) {
+		WREG32(MGAREG_ICLEAR, MGAIRQ_SOFTRAP);
+		mdev->dma_in_use = 0;
+		wake_up(&mdev->waitq);
+		return IRQ_HANDLED;
+	}
+	return IRQ_NONE;
+}
+
+void mgag200_init_irq(struct mga_device *mdev)
+{
+	/* Disable *all* interrupts */
+	WREG32(MGAREG_IEN, 0);
+	/* Clear bits if they're already high */
+	WREG32(MGAREG_ICLEAR, 0xf);
+}
+
+void mgag200_enable_irq(struct mga_device *mdev)
+{
+	/* Enable only Softrap IRQ */
+	WREG32(MGAREG_IEN, MGAIRQ_SOFTRAP);
+}
+
 int mgag200_device_preinit(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	resource_size_t start, len;
 	struct resource *res;
+	int ret;
 
 	/* BAR 1 contains registers */
 
@@ -153,6 +184,16 @@ int mgag200_device_preinit(struct mga_device *mdev)
 	if (!mdev->vram)
 		return -ENOMEM;
 
+	mgag200_init_irq(mdev);
+	ret = devm_request_irq(dev->dev, pdev->irq, mgag200_driver_irq_handler,
+			       IRQF_SHARED, "mgag200_irq", mdev);
+	if (ret < 0) {
+		drm_err(dev, "devm_request_irq(VRAM) failed %d\n", ret);
+		return -ENXIO;
+	}
+	init_waitqueue_head(&mdev->waitq);
+
+	mgag200_enable_irq(mdev);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 9e604dbb8e44..02175bfaf5a8 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -291,6 +291,9 @@ struct mga_device {
 	void __iomem			*vram;
 	resource_size_t			vram_available;
 
+	wait_queue_head_t waitq;
+	int dma_in_use;
+
 	struct drm_plane primary_plane;
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
index aa73463674e4..748c8e18e938 100644
--- a/drivers/gpu/drm/mgag200/mgag200_reg.h
+++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
@@ -107,6 +107,9 @@
 #define	MGAREG_ICLEAR		0x1e18
 #define	MGAREG_IEN		0x1e1c
 
+/* same bit shift for MGAREG_IEN, MGAREG_ICLEAR and MGAREG_STATUS */
+#define MGAIRQ_SOFTRAP		BIT(0)
+
 #define	MGAREG_VCOUNT		0x1e20
 
 #define	MGAREG_Reset		0x1e40
-- 
2.39.2

Re: [PATCH 3/4] drm/mgag200: Add IRQ support

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 457391b0380335d5e9a5babdec90ac53928b23b4]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
base:   457391b0380335d5e9a5babdec90ac53928b23b4
patch link:    https://lore.kernel.org/r/20230505124337.854845-4-jfalempe%40redhat.com
patch subject: [PATCH 3/4] drm/mgag200: Add IRQ support
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20230505/202305052227.4o72gpi8-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/cbbd69ea02ffdcee64621b76bf22cb360d943294
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
        git checkout cbbd69ea02ffdcee64621b76bf22cb360d943294
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/gpu/drm/mgag200/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305052227.4o72gpi8-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mgag200/mgag200_drv.c:113:13: warning: no previous prototype for 'mgag200_driver_irq_handler' [-Wmissing-prototypes]
     113 | irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/mgag200/mgag200_drv.c:129:6: warning: no previous prototype for 'mgag200_init_irq' [-Wmissing-prototypes]
     129 | void mgag200_init_irq(struct mga_device *mdev)
         |      ^~~~~~~~~~~~~~~~
>> drivers/gpu/drm/mgag200/mgag200_drv.c:137:6: warning: no previous prototype for 'mgag200_enable_irq' [-Wmissing-prototypes]
     137 | void mgag200_enable_irq(struct mga_device *mdev)
         |      ^~~~~~~~~~~~~~~~~~


vim +/mgag200_driver_irq_handler +113 drivers/gpu/drm/mgag200/mgag200_drv.c

   112	
 > 113	irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
   114	{
   115		struct mga_device *mdev = (struct mga_device *) arg;
   116		u32 status;
   117	
   118		status = RREG32(MGAREG_STATUS);
   119	
   120		if (status & MGAIRQ_SOFTRAP) {
   121			WREG32(MGAREG_ICLEAR, MGAIRQ_SOFTRAP);
   122			mdev->dma_in_use = 0;
   123			wake_up(&mdev->waitq);
   124			return IRQ_HANDLED;
   125		}
   126		return IRQ_NONE;
   127	}
   128	
 > 129	void mgag200_init_irq(struct mga_device *mdev)
   130	{
   131		/* Disable *all* interrupts */
   132		WREG32(MGAREG_IEN, 0);
   133		/* Clear bits if they're already high */
   134		WREG32(MGAREG_ICLEAR, 0xf);
   135	}
   136	
 > 137	void mgag200_enable_irq(struct mga_device *mdev)
   138	{
   139		/* Enable only Softrap IRQ */
   140		WREG32(MGAREG_IEN, MGAIRQ_SOFTRAP);
   141	}
   142	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 3/4] drm/mgag200: Add IRQ support

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 457391b0380335d5e9a5babdec90ac53928b23b4]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
base:   457391b0380335d5e9a5babdec90ac53928b23b4
patch link:    https://lore.kernel.org/r/20230505124337.854845-4-jfalempe%40redhat.com
patch subject: [PATCH 3/4] drm/mgag200: Add IRQ support
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20230505/202305052227.4o72gpi8-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/cbbd69ea02ffdcee64621b76bf22cb360d943294
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
        git checkout cbbd69ea02ffdcee64621b76bf22cb360d943294
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/gpu/drm/mgag200/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305052227.4o72gpi8-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mgag200/mgag200_drv.c:113:13: warning: no previous prototype for 'mgag200_driver_irq_handler' [-Wmissing-prototypes]
     113 | irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/mgag200/mgag200_drv.c:129:6: warning: no previous prototype for 'mgag200_init_irq' [-Wmissing-prototypes]
     129 | void mgag200_init_irq(struct mga_device *mdev)
         |      ^~~~~~~~~~~~~~~~
>> drivers/gpu/drm/mgag200/mgag200_drv.c:137:6: warning: no previous prototype for 'mgag200_enable_irq' [-Wmissing-prototypes]
     137 | void mgag200_enable_irq(struct mga_device *mdev)
         |      ^~~~~~~~~~~~~~~~~~


vim +/mgag200_driver_irq_handler +113 drivers/gpu/drm/mgag200/mgag200_drv.c

   112	
 > 113	irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
   114	{
   115		struct mga_device *mdev = (struct mga_device *) arg;
   116		u32 status;
   117	
   118		status = RREG32(MGAREG_STATUS);
   119	
   120		if (status & MGAIRQ_SOFTRAP) {
   121			WREG32(MGAREG_ICLEAR, MGAIRQ_SOFTRAP);
   122			mdev->dma_in_use = 0;
   123			wake_up(&mdev->waitq);
   124			return IRQ_HANDLED;
   125		}
   126		return IRQ_NONE;
   127	}
   128	
 > 129	void mgag200_init_irq(struct mga_device *mdev)
   130	{
   131		/* Disable *all* interrupts */
   132		WREG32(MGAREG_IEN, 0);
   133		/* Clear bits if they're already high */
   134		WREG32(MGAREG_ICLEAR, 0xf);
   135	}
   136	
 > 137	void mgag200_enable_irq(struct mga_device *mdev)
   138	{
   139		/* Enable only Softrap IRQ */
   140		WREG32(MGAREG_IEN, MGAIRQ_SOFTRAP);
   141	}
   142	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 3/4] drm/mgag200: Add IRQ support

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 457391b0380335d5e9a5babdec90ac53928b23b4]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
base:   457391b0380335d5e9a5babdec90ac53928b23b4
patch link:    https://lore.kernel.org/r/20230505124337.854845-4-jfalempe%40redhat.com
patch subject: [PATCH 3/4] drm/mgag200: Add IRQ support
config: i386-randconfig-a013-20230501 (https://download.01.org/0day-ci/archive/20230505/202305052227.GGoicr9j-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/cbbd69ea02ffdcee64621b76bf22cb360d943294
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
        git checkout cbbd69ea02ffdcee64621b76bf22cb360d943294
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/mgag200/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305052227.GGoicr9j-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mgag200/mgag200_drv.c:113:13: warning: no previous prototype for function 'mgag200_driver_irq_handler' [-Wmissing-prototypes]
   irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
               ^
   drivers/gpu/drm/mgag200/mgag200_drv.c:113:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
   ^
   static 
>> drivers/gpu/drm/mgag200/mgag200_drv.c:129:6: warning: no previous prototype for function 'mgag200_init_irq' [-Wmissing-prototypes]
   void mgag200_init_irq(struct mga_device *mdev)
        ^
   drivers/gpu/drm/mgag200/mgag200_drv.c:129:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void mgag200_init_irq(struct mga_device *mdev)
   ^
   static 
>> drivers/gpu/drm/mgag200/mgag200_drv.c:137:6: warning: no previous prototype for function 'mgag200_enable_irq' [-Wmissing-prototypes]
   void mgag200_enable_irq(struct mga_device *mdev)
        ^
   drivers/gpu/drm/mgag200/mgag200_drv.c:137:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void mgag200_enable_irq(struct mga_device *mdev)
   ^
   static 
   3 warnings generated.


vim +/mgag200_driver_irq_handler +113 drivers/gpu/drm/mgag200/mgag200_drv.c

   112	
 > 113	irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
   114	{
   115		struct mga_device *mdev = (struct mga_device *) arg;
   116		u32 status;
   117	
   118		status = RREG32(MGAREG_STATUS);
   119	
   120		if (status & MGAIRQ_SOFTRAP) {
   121			WREG32(MGAREG_ICLEAR, MGAIRQ_SOFTRAP);
   122			mdev->dma_in_use = 0;
   123			wake_up(&mdev->waitq);
   124			return IRQ_HANDLED;
   125		}
   126		return IRQ_NONE;
   127	}
   128	
 > 129	void mgag200_init_irq(struct mga_device *mdev)
   130	{
   131		/* Disable *all* interrupts */
   132		WREG32(MGAREG_IEN, 0);
   133		/* Clear bits if they're already high */
   134		WREG32(MGAREG_ICLEAR, 0xf);
   135	}
   136	
 > 137	void mgag200_enable_irq(struct mga_device *mdev)
   138	{
   139		/* Enable only Softrap IRQ */
   140		WREG32(MGAREG_IEN, MGAIRQ_SOFTRAP);
   141	}
   142	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 3/4] drm/mgag200: Add IRQ support

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 457391b0380335d5e9a5babdec90ac53928b23b4]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
base:   457391b0380335d5e9a5babdec90ac53928b23b4
patch link:    https://lore.kernel.org/r/20230505124337.854845-4-jfalempe%40redhat.com
patch subject: [PATCH 3/4] drm/mgag200: Add IRQ support
config: i386-randconfig-a013-20230501 (https://download.01.org/0day-ci/archive/20230505/202305052227.GGoicr9j-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/cbbd69ea02ffdcee64621b76bf22cb360d943294
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
        git checkout cbbd69ea02ffdcee64621b76bf22cb360d943294
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/mgag200/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305052227.GGoicr9j-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mgag200/mgag200_drv.c:113:13: warning: no previous prototype for function 'mgag200_driver_irq_handler' [-Wmissing-prototypes]
   irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
               ^
   drivers/gpu/drm/mgag200/mgag200_drv.c:113:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
   ^
   static 
>> drivers/gpu/drm/mgag200/mgag200_drv.c:129:6: warning: no previous prototype for function 'mgag200_init_irq' [-Wmissing-prototypes]
   void mgag200_init_irq(struct mga_device *mdev)
        ^
   drivers/gpu/drm/mgag200/mgag200_drv.c:129:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void mgag200_init_irq(struct mga_device *mdev)
   ^
   static 
>> drivers/gpu/drm/mgag200/mgag200_drv.c:137:6: warning: no previous prototype for function 'mgag200_enable_irq' [-Wmissing-prototypes]
   void mgag200_enable_irq(struct mga_device *mdev)
        ^
   drivers/gpu/drm/mgag200/mgag200_drv.c:137:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void mgag200_enable_irq(struct mga_device *mdev)
   ^
   static 
   3 warnings generated.


vim +/mgag200_driver_irq_handler +113 drivers/gpu/drm/mgag200/mgag200_drv.c

   112	
 > 113	irqreturn_t mgag200_driver_irq_handler(int irq, void *arg)
   114	{
   115		struct mga_device *mdev = (struct mga_device *) arg;
   116		u32 status;
   117	
   118		status = RREG32(MGAREG_STATUS);
   119	
   120		if (status & MGAIRQ_SOFTRAP) {
   121			WREG32(MGAREG_ICLEAR, MGAIRQ_SOFTRAP);
   122			mdev->dma_in_use = 0;
   123			wake_up(&mdev->waitq);
   124			return IRQ_HANDLED;
   125		}
   126		return IRQ_NONE;
   127	}
   128	
 > 129	void mgag200_init_irq(struct mga_device *mdev)
   130	{
   131		/* Disable *all* interrupts */
   132		WREG32(MGAREG_IEN, 0);
   133		/* Clear bits if they're already high */
   134		WREG32(MGAREG_ICLEAR, 0xf);
   135	}
   136	
 > 137	void mgag200_enable_irq(struct mga_device *mdev)
   138	{
   139		/* Enable only Softrap IRQ */
   140		WREG32(MGAREG_IEN, MGAIRQ_SOFTRAP);
   141	}
   142	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[Jocelyn Falempe]

Even if the transfer is not faster, it brings significant
improvement in latencies and CPU usage.

CPU usage drops from 100% of one core to 3% when continuously
refreshing the screen.

The primary DMA is used to send commands (register write), and
the secondary DMA to send the pixel data.
It uses the ILOAD command (chapter 4.5.7 in G200 specification),
which allows to load an image, or a part of an image from system
memory to VRAM.
The last command sent, is a softrap command, which triggers an IRQ
when the DMA transfer is complete.
For 16bits and 24bits pixel width, each line is padded to make sure,
the DMA transfer is a multiple of 32bits. The padded bytes won't be
drawn on the screen, so they don't need to be initialized.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/mgag200/Makefile       |   3 +-
 drivers/gpu/drm/mgag200/mgag200_dma.c  | 114 +++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_drv.c  |   2 +
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  25 +++++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 131 ++++++++++++++++++++++++-
 drivers/gpu/drm/mgag200/mgag200_reg.h  |  25 +++++
 6 files changed, 295 insertions(+), 5 deletions(-)
 create mode 100644 drivers/gpu/drm/mgag200/mgag200_dma.c

diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile
index 182e224c460d..96e23dc5572c 100644
--- a/drivers/gpu/drm/mgag200/Makefile
+++ b/drivers/gpu/drm/mgag200/Makefile
@@ -11,6 +11,7 @@ mgag200-y := \
 	mgag200_g200se.o \
 	mgag200_g200wb.o \
 	mgag200_i2c.o \
-	mgag200_mode.o
+	mgag200_mode.o \
+	mgag200_dma.o
 
 obj-$(CONFIG_DRM_MGAG200) += mgag200.o
diff --git a/drivers/gpu/drm/mgag200/mgag200_dma.c b/drivers/gpu/drm/mgag200/mgag200_dma.c
new file mode 100644
index 000000000000..fe063fa2b5f1
--- /dev/null
+++ b/drivers/gpu/drm/mgag200/mgag200_dma.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2023 Red Hat
+ *
+ * Authors: Jocelyn Falempe
+ *
+ */
+
+#include <linux/wait.h>
+#include <linux/dma-mapping.h>
+
+#include "mgag200_drv.h"
+#include "mgag200_reg.h"
+
+/* Maximum number of command block for one DMA transfert
+ * iload should only use 4 blocks
+ */
+#define MGA_MAX_CMD		50
+
+#define MGA_DMA_SIZE		(4 * 1024 * 1024)
+#define MGA_MIN_DMA_SIZE	(64 * 1024)
+
+/*
+ * Allocate coherent buffers for DMA transfert.
+ * We need two buffers, one for the commands, and one for the data.
+ * If allocation fails, mdev->dma_buf will be NULL, and the driver will
+ * fallback to memcpy().
+ */
+void mgag200_dma_allocate(struct mga_device *mdev)
+{
+	struct drm_device *dev = &mdev->base;
+	int size;
+	/* Allocate the command buffer */
+	mdev->cmd = dmam_alloc_coherent(dev->dev, MGA_MAX_CMD * sizeof(*mdev->cmd),
+					&mdev->cmd_handle, GFP_KERNEL);
+
+	if (!mdev->cmd) {
+		drm_warn(dev, "DMA command buffer allocation failed, fallback to cpu copy\n");
+		return;
+	}
+
+	for (size = MGA_DMA_SIZE; size >= MGA_MIN_DMA_SIZE; size = size >> 1) {
+		mdev->dma_buf = dmam_alloc_coherent(dev->dev, size, &mdev->dma_handle, GFP_KERNEL);
+		if (mdev->dma_buf)
+			break;
+	}
+	if (!mdev->dma_buf) {
+		drm_warn(dev, "DMA data buffer allocation failed, fallback to cpu copy\n");
+		return;
+	}
+	mdev->dma_size = size;
+	drm_info(dev, "Using DMA with a %dk data buffer\n", size / 1024);
+}
+
+/*
+ * Matrox uses commands block to program the hardware through DMA.
+ * Each command is a register write, and each block contains 4 commands
+ * packed in 5 words(u32).
+ * First word is the 4 register index (8bit) to write for the 4 commands,
+ * followed by the four values to be written.
+ */
+void mgag200_dma_add_block(struct mga_device *mdev,
+			   u32 reg0, u32 val0,
+			   u32 reg1, u32 val1,
+			   u32 reg2, u32 val2,
+			   u32 reg3, u32 val3)
+{
+	if (mdev->cmd_idx >= MGA_MAX_CMD) {
+		pr_err("mgag200: exceeding the DMA command buffer size\n");
+		return;
+	}
+
+	mdev->cmd[mdev->cmd_idx] = (struct mga_cmd_block) {
+		.cmd = DMAREG(reg0) | DMAREG(reg1) << 8 | DMAREG(reg2) << 16 | DMAREG(reg3) << 24,
+		.v0 = val0,
+		.v1 = val1,
+		.v2 = val2,
+		.v3 = val3};
+	mdev->cmd_idx++;
+}
+
+void mgag200_dma_run_cmd(struct mga_device *mdev)
+{
+	struct drm_device *dev = &mdev->base;
+	u32 primend;
+	int ret;
+
+	/* Add a last block to trigger the softrap interrupt */
+	mgag200_dma_add_block(mdev,
+			MGAREG_DMAPAD, 0,
+			MGAREG_DMAPAD, 0,
+			MGAREG_DMAPAD, 0,
+			MGAREG_SOFTRAP, 0);
+
+	primend = mdev->cmd_handle + mdev->cmd_idx * sizeof(struct mga_cmd_block);
+
+	// Use primary DMA to send the commands
+	WREG32(MGAREG_PRIMADDR, (u32) mdev->cmd_handle);
+	WREG32(MGAREG_PRIMEND, primend);
+	mdev->dma_in_use = 1;
+
+	ret = wait_event_timeout(mdev->waitq, mdev->dma_in_use == 0, HZ);
+
+	if (mdev->dma_in_use) {
+		drm_err(dev, "DMA transfert timed out\n");
+		/* something goes wrong, reset the DMA engine */
+		WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
+		mdev->dma_in_use = 0;
+	}
+
+	/* reset command index to start a new sequence */
+	mdev->cmd_idx = 0;
+}
+
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 3566fcdfe1e4..c863487526e7 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -184,6 +184,8 @@ int mgag200_device_preinit(struct mga_device *mdev)
 	if (!mdev->vram)
 		return -ENOMEM;
 
+	mgag200_dma_allocate(mdev);
+
 	mgag200_init_irq(mdev);
 	ret = devm_request_irq(dev->dev, pdev->irq, mgag200_driver_irq_handler,
 			       IRQF_SHARED, "mgag200_irq", mdev);
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 02175bfaf5a8..f5060fdc16f9 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -277,6 +277,14 @@ struct mgag200_device_funcs {
 	void (*pixpllc_atomic_update)(struct drm_crtc *crtc, struct drm_atomic_state *old_state);
 };
 
+struct mga_cmd_block {
+	u32 cmd;
+	u32 v0;
+	u32 v1;
+	u32 v2;
+	u32 v3;
+} __packed;
+
 struct mga_device {
 	struct drm_device base;
 
@@ -291,6 +299,14 @@ struct mga_device {
 	void __iomem			*vram;
 	resource_size_t			vram_available;
 
+	void *dma_buf;
+	size_t dma_size;
+	dma_addr_t dma_handle;
+
+	struct mga_cmd_block *cmd;
+	int cmd_idx;
+	dma_addr_t cmd_handle;
+
 	wait_queue_head_t waitq;
 	int dma_in_use;
 
@@ -446,4 +462,13 @@ void mgag200_bmc_enable_vidrst(struct mga_device *mdev);
 				/* mgag200_i2c.c */
 int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c);
 
+/* mgag200_dma.c */
+void mgag200_dma_allocate(struct mga_device *mdev);
+void mgag200_dma_add_block(struct mga_device *mdev,
+			   u32 reg0, u32 val0,
+			   u32 reg1, u32 val1,
+			   u32 reg2, u32 val2,
+			   u32 reg3, u32 val3);
+void mgag200_dma_run_cmd(struct mga_device *mdev);
+
 #endif				/* __MGAG200_DRV_H__ */
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 7d8c65372ac4..7825ec4323d2 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -398,13 +398,132 @@ static void mgag200_disable_display(struct mga_device *mdev)
 	WREG_ECRT(0x01, crtcext1);
 }
 
+static void mgag200_dwg_setup(struct mga_device *mdev, struct drm_framebuffer *fb)
+{
+	u32 maccess;
+
+	drm_dbg(&mdev->base, "Setup DWG with %dx%d %p4cc\n",
+		fb->width, fb->height, &fb->format->format);
+
+	switch (fb->format->format) {
+	case DRM_FORMAT_RGB565:
+		maccess = MGAMAC_PW16;
+		break;
+	case DRM_FORMAT_RGB888:
+		maccess = MGAMAC_PW24;
+		break;
+	case DRM_FORMAT_XRGB8888:
+		maccess = MGAMAC_PW32;
+		break;
+	}
+	WREG32(MGAREG_MACCESS, maccess);
+
+	/* Framebuffer width in pixel */
+	WREG32(MGAREG_PITCH, fb->width);
+
+	/* Sane default value for the drawing engine registers */
+	WREG32(MGAREG_DSTORG, 0);
+	WREG32(MGAREG_YDSTORG, 0);
+	WREG32(MGAREG_SRCORG, 0);
+	WREG32(MGAREG_CXBNDRY, 0x0FFF0000);
+	WREG32(MGAREG_YTOP, 0);
+	WREG32(MGAREG_YBOT, 0x00FFFFFF);
+
+	/* Activate blit mode DMA, only write the low part of the register */
+	WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
+}
+
+/*
+ * ILOAD allows to load an image from system memory to the VRAM, and with FXBNDRY, YDST and YDSTLEN,
+ * you can transfert a rectangle, so it's perfect when used with a damage clip.
+ */
+static void mgag200_iload_cmd(struct mga_device *mdev, int x, int y, int width, int height,
+			      int width_padded, int cpp)
+{
+	int size = width_padded * height;
+	u32 iload;
+
+	iload = MGADWG_ILOAD | MGADWG_SGNZERO | MGADWG_SHIFTZERO | MGADWG_REPLACE | MGADWG_CLIPDIS
+		| MGADWG_BFCOL;
+
+	mgag200_dma_add_block(mdev,
+		MGAREG_DWGCTL, iload,
+		MGAREG_FXBNDRY, (((x + width - 1) << 16) | x),
+		MGAREG_AR0, (width_padded / cpp) - 1,
+		MGAREG_AR3, 0);
+
+	mgag200_dma_add_block(mdev,
+		MGAREG_AR5, 0,
+		MGAREG_YDST, y,
+		MGAREG_DMAPAD, 0,
+		MGAREG_DMAPAD, 0);
+
+	mgag200_dma_add_block(mdev,
+		MGAREG_DMAPAD, 0,
+		MGAREG_LEN | MGAREG_EXEC, height,
+		MGAREG_SECADDR, mdev->dma_handle | 1,
+		/* Writing SECEND should always be the last command of a block */
+		MGAREG_SECEND, mdev->dma_handle + size);
+}
+
+static void mgag200_dma_copy(struct mga_device *mdev, const void *src, u32 pitch,
+				struct drm_rect *clip, int cpp)
+{
+	int i;
+	int width = drm_rect_width(clip);
+	int height = drm_rect_height(clip);
+
+	/* pad each line to 32bits boundaries see section 4.5.7 of G200 Specification */
+	int width_padded = round_up(width * cpp, 4);
+
+	for (i = 0; i < height; i++)
+		memcpy(mdev->dma_buf + width_padded * i,
+		       src + (((clip->y1 + i) * pitch) + clip->x1 * cpp),
+		       width * cpp);
+
+	mgag200_iload_cmd(mdev, clip->x1, clip->y1, width, height, width_padded, cpp);
+	mgag200_dma_run_cmd(mdev);
+}
+
+/*
+ * If the DMA coherent buffer is smaller than damage rectangle, we need to
+ * split it into multiple DMA transfert.
+ */
+static void mgag200_dma_damage(struct mga_device *mdev, const struct iosys_map *vmap,
+			       struct drm_framebuffer *fb, struct drm_rect *clip)
+{
+	u32 pitch = fb->pitches[0];
+	const void *src = vmap[0].vaddr;
+	struct drm_rect subclip;
+	int y1;
+	int lines;
+	int cpp = fb->format->cpp[0];
+
+	/* Number of lines that fits in one DMA buffer */
+	lines = min(drm_rect_height(clip), (int) mdev->dma_size / (drm_rect_width(clip) * cpp));
+
+	subclip.x1 = clip->x1;
+	subclip.x2 = clip->x2;
+
+	for (y1 = clip->y1; y1 < clip->y2; y1 += lines) {
+		subclip.y1 = y1;
+		subclip.y2 = min(clip->y2, y1 + lines);
+		mgag200_dma_copy(mdev, src, pitch, &subclip, cpp);
+	}
+}
+
 static void mgag200_handle_damage(struct mga_device *mdev, const struct iosys_map *vmap,
 				  struct drm_framebuffer *fb, struct drm_rect *clip)
 {
-	struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
-
-	iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], fb->format, clip));
-	drm_fb_memcpy(&dst, fb->pitches, vmap, fb, clip);
+	if (mdev->dma_buf) {
+		/* Fast path, use DMA */
+		mgag200_dma_damage(mdev, vmap, fb, clip);
+	} else {
+		struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
+
+		iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], fb->format, clip));
+		drm_fb_memcpy(&dst, fb->pitches, vmap, fb, clip);
+	}
 }
 
 /*
@@ -475,6 +594,10 @@ void mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	if (!fb)
 		return;
 
+	if (!old_plane_state->fb || fb->format != old_plane_state->fb->format
+	    || fb->width != old_plane_state->fb->width)
+		mgag200_dwg_setup(mdev, fb);
+
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
 		mgag200_handle_damage(mdev, shadow_plane_state->data, fb, &damage);
diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
index 748c8e18e938..256ac92dae56 100644
--- a/drivers/gpu/drm/mgag200/mgag200_reg.h
+++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
@@ -116,6 +116,9 @@
 
 #define	MGAREG_OPMODE		0x1e54
 
+#define MGAREG_PRIMADDR		0x1e58
+#define MGAREG_PRIMEND		0x1e5c
+
 /* Warp Registers */
 #define MGAREG_WIADDR           0x1dc0
 #define MGAREG_WIADDR2          0x1dd8
@@ -200,6 +203,8 @@
 
 /* See table on 4-43 for bop ALU operations */
 
+#define MGADWG_REPLACE	(0xC << 16)
+
 /* See table on 4-44 for translucidity masks */
 
 #define MGADWG_BMONOLEF		( 0x00 << 25 )
@@ -218,6 +223,8 @@
 
 #define MGADWG_PATTERN		( 0x01 << 29 )
 #define MGADWG_TRANSC		( 0x01 << 30 )
+#define MGADWG_CLIPDIS		( 0x01 << 31 )
+
 #define MGAREG_MISC_WRITE	0x3c2
 #define MGAREG_MISC_READ	0x3cc
 #define MGAREG_MEM_MISC_WRITE       0x1fc2
@@ -605,6 +612,9 @@
 #    define MGA_TC2_SELECT_TMU1                 (0x80000000)
 #define MGAREG_TEXTRANS		0x2c34
 #define MGAREG_TEXTRANSHIGH	0x2c38
+#define MGAREG_SECADDR		0x2c40
+#define MGAREG_SECEND		0x2c44
+#define MGAREG_SOFTRAP		0x2c48
 #define MGAREG_TEXFILTER	0x2c58
 #    define MGA_MIN_NRST                        (0x00000000)
 #    define MGA_MIN_BILIN                       (0x00000002)
@@ -691,4 +701,19 @@
 #define MGA_AGP2XPLL_ENABLE		0x1
 #define MGA_AGP2XPLL_DISABLE		0x0
 
+
+#define DWGREG0		0x1c00
+#define DWGREG0_END	0x1dff
+#define DWGREG1		0x2c00
+#define DWGREG1_END	0x2dff
+
+/* These macros convert register address to the 8 bit command index used with DMA
+ * It remaps 0x1c00-0x1dff to 0x00-0x7f (REG0)
+ * and 0x2c00-0x2dff to 0x80-0xff (REG1)
+ */
+#define ISREG0(r)	(r >= DWGREG0 && r <= DWGREG0_END)
+#define DMAREG0(r)	((u8)((r - DWGREG0) >> 2))
+#define DMAREG1(r)	((u8)(((r - DWGREG1) >> 2) | 0x80))
+#define DMAREG(r)	(ISREG0((r)) ? DMAREG0((r)) : DMAREG1((r)))
+
 #endif
-- 
2.39.2

Re: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 457391b0380335d5e9a5babdec90ac53928b23b4]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
base:   457391b0380335d5e9a5babdec90ac53928b23b4
patch link:    https://lore.kernel.org/r/20230505124337.854845-5-jfalempe%40redhat.com
patch subject: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM
config: x86_64-randconfig-a001-20230501 (https://download.01.org/0day-ci/archive/20230505/202305052238.7oFMzU5Q-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/b3cab0043427adee56c6f3169cdfa15526dd331c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
        git checkout b3cab0043427adee56c6f3169cdfa15526dd331c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/mgag200/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305052238.7oFMzU5Q-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/mgag200/mgag200_dma.c: In function 'mgag200_dma_run_cmd':
>> drivers/gpu/drm/mgag200/mgag200_dma.c:86:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
      86 |         int ret;
         |             ^~~


vim +/ret +86 drivers/gpu/drm/mgag200/mgag200_dma.c

    81	
    82	void mgag200_dma_run_cmd(struct mga_device *mdev)
    83	{
    84		struct drm_device *dev = &mdev->base;
    85		u32 primend;
  > 86		int ret;
    87	
    88		/* Add a last block to trigger the softrap interrupt */
    89		mgag200_dma_add_block(mdev,
    90				MGAREG_DMAPAD, 0,
    91				MGAREG_DMAPAD, 0,
    92				MGAREG_DMAPAD, 0,
    93				MGAREG_SOFTRAP, 0);
    94	
    95		primend = mdev->cmd_handle + mdev->cmd_idx * sizeof(struct mga_cmd_block);
    96	
    97		// Use primary DMA to send the commands
    98		WREG32(MGAREG_PRIMADDR, (u32) mdev->cmd_handle);
    99		WREG32(MGAREG_PRIMEND, primend);
   100		mdev->dma_in_use = 1;
   101	
   102		ret = wait_event_timeout(mdev->waitq, mdev->dma_in_use == 0, HZ);
   103	
   104		if (mdev->dma_in_use) {
   105			drm_err(dev, "DMA transfert timed out\n");
   106			/* something goes wrong, reset the DMA engine */
   107			WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
   108			mdev->dma_in_use = 0;
   109		}
   110	
   111		/* reset command index to start a new sequence */
   112		mdev->cmd_idx = 0;
   113	}
   114	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 457391b0380335d5e9a5babdec90ac53928b23b4]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
base:   457391b0380335d5e9a5babdec90ac53928b23b4
patch link:    https://lore.kernel.org/r/20230505124337.854845-5-jfalempe%40redhat.com
patch subject: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM
config: x86_64-randconfig-a001-20230501 (https://download.01.org/0day-ci/archive/20230505/202305052238.7oFMzU5Q-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/b3cab0043427adee56c6f3169cdfa15526dd331c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
        git checkout b3cab0043427adee56c6f3169cdfa15526dd331c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/mgag200/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305052238.7oFMzU5Q-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/mgag200/mgag200_dma.c: In function 'mgag200_dma_run_cmd':
>> drivers/gpu/drm/mgag200/mgag200_dma.c:86:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
      86 |         int ret;
         |             ^~~


vim +/ret +86 drivers/gpu/drm/mgag200/mgag200_dma.c

    81	
    82	void mgag200_dma_run_cmd(struct mga_device *mdev)
    83	{
    84		struct drm_device *dev = &mdev->base;
    85		u32 primend;
  > 86		int ret;
    87	
    88		/* Add a last block to trigger the softrap interrupt */
    89		mgag200_dma_add_block(mdev,
    90				MGAREG_DMAPAD, 0,
    91				MGAREG_DMAPAD, 0,
    92				MGAREG_DMAPAD, 0,
    93				MGAREG_SOFTRAP, 0);
    94	
    95		primend = mdev->cmd_handle + mdev->cmd_idx * sizeof(struct mga_cmd_block);
    96	
    97		// Use primary DMA to send the commands
    98		WREG32(MGAREG_PRIMADDR, (u32) mdev->cmd_handle);
    99		WREG32(MGAREG_PRIMEND, primend);
   100		mdev->dma_in_use = 1;
   101	
   102		ret = wait_event_timeout(mdev->waitq, mdev->dma_in_use == 0, HZ);
   103	
   104		if (mdev->dma_in_use) {
   105			drm_err(dev, "DMA transfert timed out\n");
   106			/* something goes wrong, reset the DMA engine */
   107			WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
   108			mdev->dma_in_use = 0;
   109		}
   110	
   111		/* reset command index to start a new sequence */
   112		mdev->cmd_idx = 0;
   113	}
   114	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 457391b0380335d5e9a5babdec90ac53928b23b4]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
base:   457391b0380335d5e9a5babdec90ac53928b23b4
patch link:    https://lore.kernel.org/r/20230505124337.854845-5-jfalempe%40redhat.com
patch subject: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM
config: i386-randconfig-a013-20230501 (https://download.01.org/0day-ci/archive/20230505/202305052344.TAIFBCMZ-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/b3cab0043427adee56c6f3169cdfa15526dd331c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
        git checkout b3cab0043427adee56c6f3169cdfa15526dd331c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/mgag200/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305052344.TAIFBCMZ-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mgag200/mgag200_dma.c:86:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
           int ret;
               ^
   1 warning generated.


vim +/ret +86 drivers/gpu/drm/mgag200/mgag200_dma.c

    81	
    82	void mgag200_dma_run_cmd(struct mga_device *mdev)
    83	{
    84		struct drm_device *dev = &mdev->base;
    85		u32 primend;
  > 86		int ret;
    87	
    88		/* Add a last block to trigger the softrap interrupt */
    89		mgag200_dma_add_block(mdev,
    90				MGAREG_DMAPAD, 0,
    91				MGAREG_DMAPAD, 0,
    92				MGAREG_DMAPAD, 0,
    93				MGAREG_SOFTRAP, 0);
    94	
    95		primend = mdev->cmd_handle + mdev->cmd_idx * sizeof(struct mga_cmd_block);
    96	
    97		// Use primary DMA to send the commands
    98		WREG32(MGAREG_PRIMADDR, (u32) mdev->cmd_handle);
    99		WREG32(MGAREG_PRIMEND, primend);
   100		mdev->dma_in_use = 1;
   101	
   102		ret = wait_event_timeout(mdev->waitq, mdev->dma_in_use == 0, HZ);
   103	
   104		if (mdev->dma_in_use) {
   105			drm_err(dev, "DMA transfert timed out\n");
   106			/* something goes wrong, reset the DMA engine */
   107			WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
   108			mdev->dma_in_use = 0;
   109		}
   110	
   111		/* reset command index to start a new sequence */
   112		mdev->cmd_idx = 0;
   113	}
   114	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 457391b0380335d5e9a5babdec90ac53928b23b4]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
base:   457391b0380335d5e9a5babdec90ac53928b23b4
patch link:    https://lore.kernel.org/r/20230505124337.854845-5-jfalempe%40redhat.com
patch subject: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM
config: i386-randconfig-a013-20230501 (https://download.01.org/0day-ci/archive/20230505/202305052344.TAIFBCMZ-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/b3cab0043427adee56c6f3169cdfa15526dd331c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
        git checkout b3cab0043427adee56c6f3169cdfa15526dd331c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/mgag200/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305052344.TAIFBCMZ-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mgag200/mgag200_dma.c:86:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
           int ret;
               ^
   1 warning generated.


vim +/ret +86 drivers/gpu/drm/mgag200/mgag200_dma.c

    81	
    82	void mgag200_dma_run_cmd(struct mga_device *mdev)
    83	{
    84		struct drm_device *dev = &mdev->base;
    85		u32 primend;
  > 86		int ret;
    87	
    88		/* Add a last block to trigger the softrap interrupt */
    89		mgag200_dma_add_block(mdev,
    90				MGAREG_DMAPAD, 0,
    91				MGAREG_DMAPAD, 0,
    92				MGAREG_DMAPAD, 0,
    93				MGAREG_SOFTRAP, 0);
    94	
    95		primend = mdev->cmd_handle + mdev->cmd_idx * sizeof(struct mga_cmd_block);
    96	
    97		// Use primary DMA to send the commands
    98		WREG32(MGAREG_PRIMADDR, (u32) mdev->cmd_handle);
    99		WREG32(MGAREG_PRIMEND, primend);
   100		mdev->dma_in_use = 1;
   101	
   102		ret = wait_event_timeout(mdev->waitq, mdev->dma_in_use == 0, HZ);
   103	
   104		if (mdev->dma_in_use) {
   105			drm_err(dev, "DMA transfert timed out\n");
   106			/* something goes wrong, reset the DMA engine */
   107			WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
   108			mdev->dma_in_use = 0;
   109		}
   110	
   111		/* reset command index to start a new sequence */
   112		mdev->cmd_idx = 0;
   113	}
   114	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 16330 bytes --]

Hi Jocelyn

Am 05.05.23 um 14:43 schrieb Jocelyn Falempe:
> Even if the transfer is not faster, it brings significant
> improvement in latencies and CPU usage.
> 

I think I shot down this patch already. I don't want to maintain the 
extra code for DMA support.


> CPU usage drops from 100% of one core to 3% when continuously
> refreshing the screen.

But this result is nice, so mabe I'd reconsider. Let's take this patch 
as an RFC to discuss possible ways forward.

There seems to be some fallback still in place in this patch. I 
definately don't want multiple codepaths for copying; so the DMA code 
needs to work on all our hardware.

> 
> The primary DMA is used to send commands (register write), and
> the secondary DMA to send the pixel data.
> It uses the ILOAD command (chapter 4.5.7 in G200 specification),
> which allows to load an image, or a part of an image from system
> memory to VRAM.
> The last command sent, is a softrap command, which triggers an IRQ
> when the DMA transfer is complete.
> For 16bits and 24bits pixel width, each line is padded to make sure,
> the DMA transfer is a multiple of 32bits. The padded bytes won't be
> drawn on the screen, so they don't need to be initialized.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/mgag200/Makefile       |   3 +-
>   drivers/gpu/drm/mgag200/mgag200_dma.c  | 114 +++++++++++++++++++++
>   drivers/gpu/drm/mgag200/mgag200_drv.c  |   2 +
>   drivers/gpu/drm/mgag200/mgag200_drv.h  |  25 +++++
>   drivers/gpu/drm/mgag200/mgag200_mode.c | 131 ++++++++++++++++++++++++-
>   drivers/gpu/drm/mgag200/mgag200_reg.h  |  25 +++++
>   6 files changed, 295 insertions(+), 5 deletions(-)
>   create mode 100644 drivers/gpu/drm/mgag200/mgag200_dma.c
> 
> diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile
> index 182e224c460d..96e23dc5572c 100644
> --- a/drivers/gpu/drm/mgag200/Makefile
> +++ b/drivers/gpu/drm/mgag200/Makefile
> @@ -11,6 +11,7 @@ mgag200-y := \
>   	mgag200_g200se.o \
>   	mgag200_g200wb.o \
>   	mgag200_i2c.o \
> -	mgag200_mode.o
> +	mgag200_mode.o \
> +	mgag200_dma.o
>   
>   obj-$(CONFIG_DRM_MGAG200) += mgag200.o
> diff --git a/drivers/gpu/drm/mgag200/mgag200_dma.c b/drivers/gpu/drm/mgag200/mgag200_dma.c
> new file mode 100644
> index 000000000000..fe063fa2b5f1
> --- /dev/null
> +++ b/drivers/gpu/drm/mgag200/mgag200_dma.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2023 Red Hat
> + *
> + * Authors: Jocelyn Falempe
> + *
> + */
> +
> +#include <linux/wait.h>
> +#include <linux/dma-mapping.h>
> +
> +#include "mgag200_drv.h"
> +#include "mgag200_reg.h"
> +
> +/* Maximum number of command block for one DMA transfert
> + * iload should only use 4 blocks
> + */
> +#define MGA_MAX_CMD		50
> +
> +#define MGA_DMA_SIZE		(4 * 1024 * 1024)
> +#define MGA_MIN_DMA_SIZE	(64 * 1024)
> +
> +/*
> + * Allocate coherent buffers for DMA transfert.
> + * We need two buffers, one for the commands, and one for the data.
> + * If allocation fails, mdev->dma_buf will be NULL, and the driver will
> + * fallback to memcpy().
> + */
> +void mgag200_dma_allocate(struct mga_device *mdev)
> +{
> +	struct drm_device *dev = &mdev->base;
> +	int size;
> +	/* Allocate the command buffer */
> +	mdev->cmd = dmam_alloc_coherent(dev->dev, MGA_MAX_CMD * sizeof(*mdev->cmd),
> +					&mdev->cmd_handle, GFP_KERNEL);
> +
> +	if (!mdev->cmd) {
> +		drm_warn(dev, "DMA command buffer allocation failed, fallback to cpu copy\n");
> +		return;
> +	}
> +
> +	for (size = MGA_DMA_SIZE; size >= MGA_MIN_DMA_SIZE; size = size >> 1) {
> +		mdev->dma_buf = dmam_alloc_coherent(dev->dev, size, &mdev->dma_handle, GFP_KERNEL);
> +		if (mdev->dma_buf)
> +			break;
> +	}
> +	if (!mdev->dma_buf) {
> +		drm_warn(dev, "DMA data buffer allocation failed, fallback to cpu copy\n");
> +		return;
> +	}
> +	mdev->dma_size = size;
> +	drm_info(dev, "Using DMA with a %dk data buffer\n", size / 1024);
> +}
> +
> +/*
> + * Matrox uses commands block to program the hardware through DMA.
> + * Each command is a register write, and each block contains 4 commands
> + * packed in 5 words(u32).
> + * First word is the 4 register index (8bit) to write for the 4 commands,
> + * followed by the four values to be written.
> + */
> +void mgag200_dma_add_block(struct mga_device *mdev,
> +			   u32 reg0, u32 val0,
> +			   u32 reg1, u32 val1,
> +			   u32 reg2, u32 val2,
> +			   u32 reg3, u32 val3)
> +{
> +	if (mdev->cmd_idx >= MGA_MAX_CMD) {
> +		pr_err("mgag200: exceeding the DMA command buffer size\n");
> +		return;
> +	}
> +
> +	mdev->cmd[mdev->cmd_idx] = (struct mga_cmd_block) {
> +		.cmd = DMAREG(reg0) | DMAREG(reg1) << 8 | DMAREG(reg2) << 16 | DMAREG(reg3) << 24,
> +		.v0 = val0,
> +		.v1 = val1,
> +		.v2 = val2,
> +		.v3 = val3};
> +	mdev->cmd_idx++;
> +}
> +
> +void mgag200_dma_run_cmd(struct mga_device *mdev)
> +{
> +	struct drm_device *dev = &mdev->base;
> +	u32 primend;
> +	int ret;
> +
> +	/* Add a last block to trigger the softrap interrupt */
> +	mgag200_dma_add_block(mdev,
> +			MGAREG_DMAPAD, 0,
> +			MGAREG_DMAPAD, 0,
> +			MGAREG_DMAPAD, 0,
> +			MGAREG_SOFTRAP, 0);
> +
> +	primend = mdev->cmd_handle + mdev->cmd_idx * sizeof(struct mga_cmd_block);
> +
> +	// Use primary DMA to send the commands
> +	WREG32(MGAREG_PRIMADDR, (u32) mdev->cmd_handle);
> +	WREG32(MGAREG_PRIMEND, primend);
> +	mdev->dma_in_use = 1;
> +
> +	ret = wait_event_timeout(mdev->waitq, mdev->dma_in_use == 0, HZ);
> +
> +	if (mdev->dma_in_use) {
> +		drm_err(dev, "DMA transfert timed out\n");
> +		/* something goes wrong, reset the DMA engine */
> +		WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
> +		mdev->dma_in_use = 0;
> +	}
> +
> +	/* reset command index to start a new sequence */
> +	mdev->cmd_idx = 0;
> +}

Can the DMA code be build around Linux' struct drm_device?

> +
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 3566fcdfe1e4..c863487526e7 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -184,6 +184,8 @@ int mgag200_device_preinit(struct mga_device *mdev)
>   	if (!mdev->vram)
>   		return -ENOMEM;
>   
> +	mgag200_dma_allocate(mdev);
> +
>   	mgag200_init_irq(mdev);
>   	ret = devm_request_irq(dev->dev, pdev->irq, mgag200_driver_irq_handler,
>   			       IRQF_SHARED, "mgag200_irq", mdev);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 02175bfaf5a8..f5060fdc16f9 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -277,6 +277,14 @@ struct mgag200_device_funcs {
>   	void (*pixpllc_atomic_update)(struct drm_crtc *crtc, struct drm_atomic_state *old_state);
>   };
>   
> +struct mga_cmd_block {
> +	u32 cmd;
> +	u32 v0;
> +	u32 v1;
> +	u32 v2;
> +	u32 v3;
> +} __packed;
> +
>   struct mga_device {
>   	struct drm_device base;
>   
> @@ -291,6 +299,14 @@ struct mga_device {
>   	void __iomem			*vram;
>   	resource_size_t			vram_available;
>   
> +	void *dma_buf;
> +	size_t dma_size;
> +	dma_addr_t dma_handle;
> +
> +	struct mga_cmd_block *cmd;
> +	int cmd_idx;
> +	dma_addr_t cmd_handle;
> +
>   	wait_queue_head_t waitq;
>   	int dma_in_use;
>   
> @@ -446,4 +462,13 @@ void mgag200_bmc_enable_vidrst(struct mga_device *mdev);
>   				/* mgag200_i2c.c */
>   int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c);
>   
> +/* mgag200_dma.c */
> +void mgag200_dma_allocate(struct mga_device *mdev);
> +void mgag200_dma_add_block(struct mga_device *mdev,
> +			   u32 reg0, u32 val0,
> +			   u32 reg1, u32 val1,
> +			   u32 reg2, u32 val2,
> +			   u32 reg3, u32 val3);
> +void mgag200_dma_run_cmd(struct mga_device *mdev);
> +
>   #endif				/* __MGAG200_DRV_H__ */
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 7d8c65372ac4..7825ec4323d2 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -398,13 +398,132 @@ static void mgag200_disable_display(struct mga_device *mdev)
>   	WREG_ECRT(0x01, crtcext1);
>   }
>   
> +static void mgag200_dwg_setup(struct mga_device *mdev, struct drm_framebuffer *fb)
> +{
> +	u32 maccess;
> +
> +	drm_dbg(&mdev->base, "Setup DWG with %dx%d %p4cc\n",
> +		fb->width, fb->height, &fb->format->format);
> +
> +	switch (fb->format->format) {
> +	case DRM_FORMAT_RGB565:
> +		maccess = MGAMAC_PW16;
> +		break;
> +	case DRM_FORMAT_RGB888:
> +		maccess = MGAMAC_PW24;
> +		break;
> +	case DRM_FORMAT_XRGB8888:
> +		maccess = MGAMAC_PW32;
> +		break;
> +	}
> +	WREG32(MGAREG_MACCESS, maccess);
> +
> +	/* Framebuffer width in pixel */
> +	WREG32(MGAREG_PITCH, fb->width);
> +
> +	/* Sane default value for the drawing engine registers */
> +	WREG32(MGAREG_DSTORG, 0);
> +	WREG32(MGAREG_YDSTORG, 0);
> +	WREG32(MGAREG_SRCORG, 0);
> +	WREG32(MGAREG_CXBNDRY, 0x0FFF0000);
> +	WREG32(MGAREG_YTOP, 0);
> +	WREG32(MGAREG_YBOT, 0x00FFFFFF);
> +
> +	/* Activate blit mode DMA, only write the low part of the register */
> +	WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
> +}
> +
> +/*
> + * ILOAD allows to load an image from system memory to the VRAM, and with FXBNDRY, YDST and YDSTLEN,
> + * you can transfert a rectangle, so it's perfect when used with a damage clip.
> + */
> +static void mgag200_iload_cmd(struct mga_device *mdev, int x, int y, int width, int height,
> +			      int width_padded, int cpp)
> +{
> +	int size = width_padded * height;
> +	u32 iload;
> +
> +	iload = MGADWG_ILOAD | MGADWG_SGNZERO | MGADWG_SHIFTZERO | MGADWG_REPLACE | MGADWG_CLIPDIS
> +		| MGADWG_BFCOL;
> +
> +	mgag200_dma_add_block(mdev,
> +		MGAREG_DWGCTL, iload,
> +		MGAREG_FXBNDRY, (((x + width - 1) << 16) | x),
> +		MGAREG_AR0, (width_padded / cpp) - 1,
> +		MGAREG_AR3, 0);
> +
> +	mgag200_dma_add_block(mdev,
> +		MGAREG_AR5, 0,
> +		MGAREG_YDST, y,
> +		MGAREG_DMAPAD, 0,
> +		MGAREG_DMAPAD, 0);
> +
> +	mgag200_dma_add_block(mdev,
> +		MGAREG_DMAPAD, 0,
> +		MGAREG_LEN | MGAREG_EXEC, height,
> +		MGAREG_SECADDR, mdev->dma_handle | 1,
> +		/* Writing SECEND should always be the last command of a block */
> +		MGAREG_SECEND, mdev->dma_handle + size);
> +}
> +
> +static void mgag200_dma_copy(struct mga_device *mdev, const void *src, u32 pitch,
> +				struct drm_rect *clip, int cpp)
> +{
> +	int i;
> +	int width = drm_rect_width(clip);
> +	int height = drm_rect_height(clip);
> +
> +	/* pad each line to 32bits boundaries see section 4.5.7 of G200 Specification */
> +	int width_padded = round_up(width * cpp, 4);
> +
> +	for (i = 0; i < height; i++)
> +		memcpy(mdev->dma_buf + width_padded * i,
> +		       src + (((clip->y1 + i) * pitch) + clip->x1 * cpp),
> +		       width * cpp);

This memcpy() should probably go away.  Instead of SHMEM, mgag200 should 
allocate with GEM DMA helpers. We have a number of drivers that use DMA 
helpers with DRM format helpers, so the conversion should be 
strait-forward and can be done independently from the other patches.

Afterward, it should be possible to DMA-copy directly from the GEM 
buffer object.

> +
> +	mgag200_iload_cmd(mdev, clip->x1, clip->y1, width, height, width_padded, cpp);
> +	mgag200_dma_run_cmd(mdev);
> +}
> +
> +/*
> + * If the DMA coherent buffer is smaller than damage rectangle, we need to
> + * split it into multiple DMA transfert.
> + */
> +static void mgag200_dma_damage(struct mga_device *mdev, const struct iosys_map *vmap,
> +			       struct drm_framebuffer *fb, struct drm_rect *clip)
> +{
> +	u32 pitch = fb->pitches[0];
> +	const void *src = vmap[0].vaddr;
> +	struct drm_rect subclip;
> +	int y1;
> +	int lines;
> +	int cpp = fb->format->cpp[0];
> +
> +	/* Number of lines that fits in one DMA buffer */
> +	lines = min(drm_rect_height(clip), (int) mdev->dma_size / (drm_rect_width(clip) * cpp));
> +
> +	subclip.x1 = clip->x1;
> +	subclip.x2 = clip->x2;
> +
> +	for (y1 = clip->y1; y1 < clip->y2; y1 += lines) {
> +		subclip.y1 = y1;
> +		subclip.y2 = min(clip->y2, y1 + lines);
> +		mgag200_dma_copy(mdev, src, pitch, &subclip, cpp);
> +	}
> +}
> +
>   static void mgag200_handle_damage(struct mga_device *mdev, const struct iosys_map *vmap,
>   				  struct drm_framebuffer *fb, struct drm_rect *clip)
>   {
> -	struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
> -
> -	iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], fb->format, clip));
> -	drm_fb_memcpy(&dst, fb->pitches, vmap, fb, clip);
> +	if (mdev->dma_buf) {
> +		/* Fast path, use DMA */
> +		mgag200_dma_damage(mdev, vmap, fb, clip);
> +	} else {
> +		struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
> +
> +		iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], fb->format, clip));
> +		drm_fb_memcpy(&dst, fb->pitches, vmap, fb, clip);
> +	}

This branching needs to go. As I said, DMA is either the standard, or 
not there at all. I doubt that the !mdev->dmabuf case can easily happen 
in practice. AFAICT it's all in the 32-bit address range and we're on 
systems with enough physical memory.

Best regards
Thomas

>   }
>   
>   /*
> @@ -475,6 +594,10 @@ void mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   	if (!fb)
>   		return;
>   
> +	if (!old_plane_state->fb || fb->format != old_plane_state->fb->format
> +	    || fb->width != old_plane_state->fb->width)
> +		mgag200_dwg_setup(mdev, fb);
> +
>   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>   	drm_atomic_for_each_plane_damage(&iter, &damage) {
>   		mgag200_handle_damage(mdev, shadow_plane_state->data, fb, &damage);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
> index 748c8e18e938..256ac92dae56 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_reg.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
> @@ -116,6 +116,9 @@
>   
>   #define	MGAREG_OPMODE		0x1e54
>   
> +#define MGAREG_PRIMADDR		0x1e58
> +#define MGAREG_PRIMEND		0x1e5c
> +
>   /* Warp Registers */
>   #define MGAREG_WIADDR           0x1dc0
>   #define MGAREG_WIADDR2          0x1dd8
> @@ -200,6 +203,8 @@
>   
>   /* See table on 4-43 for bop ALU operations */
>   
> +#define MGADWG_REPLACE	(0xC << 16)
> +
>   /* See table on 4-44 for translucidity masks */
>   
>   #define MGADWG_BMONOLEF		( 0x00 << 25 )
> @@ -218,6 +223,8 @@
>   
>   #define MGADWG_PATTERN		( 0x01 << 29 )
>   #define MGADWG_TRANSC		( 0x01 << 30 )
> +#define MGADWG_CLIPDIS		( 0x01 << 31 )
> +
>   #define MGAREG_MISC_WRITE	0x3c2
>   #define MGAREG_MISC_READ	0x3cc
>   #define MGAREG_MEM_MISC_WRITE       0x1fc2
> @@ -605,6 +612,9 @@
>   #    define MGA_TC2_SELECT_TMU1                 (0x80000000)
>   #define MGAREG_TEXTRANS		0x2c34
>   #define MGAREG_TEXTRANSHIGH	0x2c38
> +#define MGAREG_SECADDR		0x2c40
> +#define MGAREG_SECEND		0x2c44
> +#define MGAREG_SOFTRAP		0x2c48
>   #define MGAREG_TEXFILTER	0x2c58
>   #    define MGA_MIN_NRST                        (0x00000000)
>   #    define MGA_MIN_BILIN                       (0x00000002)
> @@ -691,4 +701,19 @@
>   #define MGA_AGP2XPLL_ENABLE		0x1
>   #define MGA_AGP2XPLL_DISABLE		0x0
>   
> +
> +#define DWGREG0		0x1c00
> +#define DWGREG0_END	0x1dff
> +#define DWGREG1		0x2c00
> +#define DWGREG1_END	0x2dff
> +
> +/* These macros convert register address to the 8 bit command index used with DMA
> + * It remaps 0x1c00-0x1dff to 0x00-0x7f (REG0)
> + * and 0x2c00-0x2dff to 0x80-0xff (REG1)
> + */
> +#define ISREG0(r)	(r >= DWGREG0 && r <= DWGREG0_END)
> +#define DMAREG0(r)	((u8)((r - DWGREG0) >> 2))
> +#define DMAREG1(r)	((u8)(((r - DWGREG1) >> 2) | 0x80))
> +#define DMAREG(r)	(ISREG0((r)) ? DMAREG0((r)) : DMAREG1((r)))
> +
>   #endif

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[Jocelyn Falempe]

On 08/05/2023 10:04, Thomas Zimmermann wrote:
> Hi Jocelyn
> 
> Am 05.05.23 um 14:43 schrieb Jocelyn Falempe:
>> Even if the transfer is not faster, it brings significant
>> improvement in latencies and CPU usage.
>>
> 
> I think I shot down this patch already. I don't want to maintain the 
> extra code for DMA support.

Yes, thank you for still taking a look at it !
I will also be there to fix regressions, if any.
> 
> 
>> CPU usage drops from 100% of one core to 3% when continuously
>> refreshing the screen.
> 
> But this result is nice, so mabe I'd reconsider. Let's take this patch 
> as an RFC to discuss possible ways forward.
> 
> There seems to be some fallback still in place in this patch. I 
> definately don't want multiple codepaths for copying; so the DMA code 
> needs to work on all our hardware.

As it is new code, and I can't test on every hardware, I let a fallback 
in case it breaks on some configuration. But I agree, it can be DMA 
only, as all G200 should work with this code.

> 
>>
>> The primary DMA is used to send commands (register write), and
>> the secondary DMA to send the pixel data.
>> It uses the ILOAD command (chapter 4.5.7 in G200 specification),
>> which allows to load an image, or a part of an image from system
>> memory to VRAM.
>> The last command sent, is a softrap command, which triggers an IRQ
>> when the DMA transfer is complete.
>> For 16bits and 24bits pixel width, each line is padded to make sure,
>> the DMA transfer is a multiple of 32bits. The padded bytes won't be
>> drawn on the screen, so they don't need to be initialized.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/mgag200/Makefile       |   3 +-
>>   drivers/gpu/drm/mgag200/mgag200_dma.c  | 114 +++++++++++++++++++++
>>   drivers/gpu/drm/mgag200/mgag200_drv.c  |   2 +
>>   drivers/gpu/drm/mgag200/mgag200_drv.h  |  25 +++++
>>   drivers/gpu/drm/mgag200/mgag200_mode.c | 131 ++++++++++++++++++++++++-
>>   drivers/gpu/drm/mgag200/mgag200_reg.h  |  25 +++++
>>   6 files changed, 295 insertions(+), 5 deletions(-)
>>   create mode 100644 drivers/gpu/drm/mgag200/mgag200_dma.c
>>
>> diff --git a/drivers/gpu/drm/mgag200/Makefile 
>> b/drivers/gpu/drm/mgag200/Makefile
>> index 182e224c460d..96e23dc5572c 100644
>> --- a/drivers/gpu/drm/mgag200/Makefile
>> +++ b/drivers/gpu/drm/mgag200/Makefile
>> @@ -11,6 +11,7 @@ mgag200-y := \
>>       mgag200_g200se.o \
>>       mgag200_g200wb.o \
>>       mgag200_i2c.o \
>> -    mgag200_mode.o
>> +    mgag200_mode.o \
>> +    mgag200_dma.o
>>   obj-$(CONFIG_DRM_MGAG200) += mgag200.o
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_dma.c 
>> b/drivers/gpu/drm/mgag200/mgag200_dma.c
>> new file mode 100644
>> index 000000000000..fe063fa2b5f1
>> --- /dev/null
>> +++ b/drivers/gpu/drm/mgag200/mgag200_dma.c
>> @@ -0,0 +1,114 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright 2023 Red Hat
>> + *
>> + * Authors: Jocelyn Falempe
>> + *
>> + */
>> +
>> +#include <linux/wait.h>
>> +#include <linux/dma-mapping.h>
>> +
>> +#include "mgag200_drv.h"
>> +#include "mgag200_reg.h"
>> +
>> +/* Maximum number of command block for one DMA transfert
>> + * iload should only use 4 blocks
>> + */
>> +#define MGA_MAX_CMD        50
>> +
>> +#define MGA_DMA_SIZE        (4 * 1024 * 1024)
>> +#define MGA_MIN_DMA_SIZE    (64 * 1024)
>> +
>> +/*
>> + * Allocate coherent buffers for DMA transfert.
>> + * We need two buffers, one for the commands, and one for the data.
>> + * If allocation fails, mdev->dma_buf will be NULL, and the driver will
>> + * fallback to memcpy().
>> + */
>> +void mgag200_dma_allocate(struct mga_device *mdev)
>> +{
>> +    struct drm_device *dev = &mdev->base;
>> +    int size;
>> +    /* Allocate the command buffer */
>> +    mdev->cmd = dmam_alloc_coherent(dev->dev, MGA_MAX_CMD * 
>> sizeof(*mdev->cmd),
>> +                    &mdev->cmd_handle, GFP_KERNEL);
>> +
>> +    if (!mdev->cmd) {
>> +        drm_warn(dev, "DMA command buffer allocation failed, fallback 
>> to cpu copy\n");
>> +        return;
>> +    }
>> +
>> +    for (size = MGA_DMA_SIZE; size >= MGA_MIN_DMA_SIZE; size = size 
>> >> 1) {
>> +        mdev->dma_buf = dmam_alloc_coherent(dev->dev, size, 
>> &mdev->dma_handle, GFP_KERNEL);
>> +        if (mdev->dma_buf)
>> +            break;
>> +    }
>> +    if (!mdev->dma_buf) {
>> +        drm_warn(dev, "DMA data buffer allocation failed, fallback to 
>> cpu copy\n");
>> +        return;
>> +    }
>> +    mdev->dma_size = size;
>> +    drm_info(dev, "Using DMA with a %dk data buffer\n", size / 1024);
>> +}
>> +
>> +/*
>> + * Matrox uses commands block to program the hardware through DMA.
>> + * Each command is a register write, and each block contains 4 commands
>> + * packed in 5 words(u32).
>> + * First word is the 4 register index (8bit) to write for the 4 
>> commands,
>> + * followed by the four values to be written.
>> + */
>> +void mgag200_dma_add_block(struct mga_device *mdev,
>> +               u32 reg0, u32 val0,
>> +               u32 reg1, u32 val1,
>> +               u32 reg2, u32 val2,
>> +               u32 reg3, u32 val3)
>> +{
>> +    if (mdev->cmd_idx >= MGA_MAX_CMD) {
>> +        pr_err("mgag200: exceeding the DMA command buffer size\n");
>> +        return;
>> +    }
>> +
>> +    mdev->cmd[mdev->cmd_idx] = (struct mga_cmd_block) {
>> +        .cmd = DMAREG(reg0) | DMAREG(reg1) << 8 | DMAREG(reg2) << 16 
>> | DMAREG(reg3) << 24,
>> +        .v0 = val0,
>> +        .v1 = val1,
>> +        .v2 = val2,
>> +        .v3 = val3};
>> +    mdev->cmd_idx++;
>> +}
>> +
>> +void mgag200_dma_run_cmd(struct mga_device *mdev)
>> +{
>> +    struct drm_device *dev = &mdev->base;
>> +    u32 primend;
>> +    int ret;
>> +
>> +    /* Add a last block to trigger the softrap interrupt */
>> +    mgag200_dma_add_block(mdev,
>> +            MGAREG_DMAPAD, 0,
>> +            MGAREG_DMAPAD, 0,
>> +            MGAREG_DMAPAD, 0,
>> +            MGAREG_SOFTRAP, 0);
>> +
>> +    primend = mdev->cmd_handle + mdev->cmd_idx * sizeof(struct 
>> mga_cmd_block);
>> +
>> +    // Use primary DMA to send the commands
>> +    WREG32(MGAREG_PRIMADDR, (u32) mdev->cmd_handle);
>> +    WREG32(MGAREG_PRIMEND, primend);
>> +    mdev->dma_in_use = 1;
>> +
>> +    ret = wait_event_timeout(mdev->waitq, mdev->dma_in_use == 0, HZ);
>> +
>> +    if (mdev->dma_in_use) {
>> +        drm_err(dev, "DMA transfert timed out\n");
>> +        /* something goes wrong, reset the DMA engine */
>> +        WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
>> +        mdev->dma_in_use = 0;
>> +    }
>> +
>> +    /* reset command index to start a new sequence */
>> +    mdev->cmd_idx = 0;
>> +}
> 
> Can the DMA code be build around Linux' struct drm_device?

I just took a look at which field of the struct drm_device I can use 
there. "struct drm_device_dma * dma" is defined in drm_legacy.h, does 
that mean I shouldn't use it for a new work ?

> 
>> +
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c 
>> b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index 3566fcdfe1e4..c863487526e7 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -184,6 +184,8 @@ int mgag200_device_preinit(struct mga_device *mdev)
>>       if (!mdev->vram)
>>           return -ENOMEM;
>> +    mgag200_dma_allocate(mdev);
>> +
>>       mgag200_init_irq(mdev);
>>       ret = devm_request_irq(dev->dev, pdev->irq, 
>> mgag200_driver_irq_handler,
>>                      IRQF_SHARED, "mgag200_irq", mdev);
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h 
>> b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index 02175bfaf5a8..f5060fdc16f9 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -277,6 +277,14 @@ struct mgag200_device_funcs {
>>       void (*pixpllc_atomic_update)(struct drm_crtc *crtc, struct 
>> drm_atomic_state *old_state);
>>   };
>> +struct mga_cmd_block {
>> +    u32 cmd;
>> +    u32 v0;
>> +    u32 v1;
>> +    u32 v2;
>> +    u32 v3;
>> +} __packed;
>> +
>>   struct mga_device {
>>       struct drm_device base;
>> @@ -291,6 +299,14 @@ struct mga_device {
>>       void __iomem            *vram;
>>       resource_size_t            vram_available;
>> +    void *dma_buf;
>> +    size_t dma_size;
>> +    dma_addr_t dma_handle;
>> +
>> +    struct mga_cmd_block *cmd;
>> +    int cmd_idx;
>> +    dma_addr_t cmd_handle;
>> +
>>       wait_queue_head_t waitq;
>>       int dma_in_use;
>> @@ -446,4 +462,13 @@ void mgag200_bmc_enable_vidrst(struct mga_device 
>> *mdev);
>>                   /* mgag200_i2c.c */
>>   int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan 
>> *i2c);
>> +/* mgag200_dma.c */
>> +void mgag200_dma_allocate(struct mga_device *mdev);
>> +void mgag200_dma_add_block(struct mga_device *mdev,
>> +               u32 reg0, u32 val0,
>> +               u32 reg1, u32 val1,
>> +               u32 reg2, u32 val2,
>> +               u32 reg3, u32 val3);
>> +void mgag200_dma_run_cmd(struct mga_device *mdev);
>> +
>>   #endif                /* __MGAG200_DRV_H__ */
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c 
>> b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 7d8c65372ac4..7825ec4323d2 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -398,13 +398,132 @@ static void mgag200_disable_display(struct 
>> mga_device *mdev)
>>       WREG_ECRT(0x01, crtcext1);
>>   }
>> +static void mgag200_dwg_setup(struct mga_device *mdev, struct 
>> drm_framebuffer *fb)
>> +{
>> +    u32 maccess;
>> +
>> +    drm_dbg(&mdev->base, "Setup DWG with %dx%d %p4cc\n",
>> +        fb->width, fb->height, &fb->format->format);
>> +
>> +    switch (fb->format->format) {
>> +    case DRM_FORMAT_RGB565:
>> +        maccess = MGAMAC_PW16;
>> +        break;
>> +    case DRM_FORMAT_RGB888:
>> +        maccess = MGAMAC_PW24;
>> +        break;
>> +    case DRM_FORMAT_XRGB8888:
>> +        maccess = MGAMAC_PW32;
>> +        break;
>> +    }
>> +    WREG32(MGAREG_MACCESS, maccess);
>> +
>> +    /* Framebuffer width in pixel */
>> +    WREG32(MGAREG_PITCH, fb->width);
>> +
>> +    /* Sane default value for the drawing engine registers */
>> +    WREG32(MGAREG_DSTORG, 0);
>> +    WREG32(MGAREG_YDSTORG, 0);
>> +    WREG32(MGAREG_SRCORG, 0);
>> +    WREG32(MGAREG_CXBNDRY, 0x0FFF0000);
>> +    WREG32(MGAREG_YTOP, 0);
>> +    WREG32(MGAREG_YBOT, 0x00FFFFFF);
>> +
>> +    /* Activate blit mode DMA, only write the low part of the 
>> register */
>> +    WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
>> +}
>> +
>> +/*
>> + * ILOAD allows to load an image from system memory to the VRAM, and 
>> with FXBNDRY, YDST and YDSTLEN,
>> + * you can transfert a rectangle, so it's perfect when used with a 
>> damage clip.
>> + */
>> +static void mgag200_iload_cmd(struct mga_device *mdev, int x, int y, 
>> int width, int height,
>> +                  int width_padded, int cpp)
>> +{
>> +    int size = width_padded * height;
>> +    u32 iload;
>> +
>> +    iload = MGADWG_ILOAD | MGADWG_SGNZERO | MGADWG_SHIFTZERO | 
>> MGADWG_REPLACE | MGADWG_CLIPDIS
>> +        | MGADWG_BFCOL;
>> +
>> +    mgag200_dma_add_block(mdev,
>> +        MGAREG_DWGCTL, iload,
>> +        MGAREG_FXBNDRY, (((x + width - 1) << 16) | x),
>> +        MGAREG_AR0, (width_padded / cpp) - 1,
>> +        MGAREG_AR3, 0);
>> +
>> +    mgag200_dma_add_block(mdev,
>> +        MGAREG_AR5, 0,
>> +        MGAREG_YDST, y,
>> +        MGAREG_DMAPAD, 0,
>> +        MGAREG_DMAPAD, 0);
>> +
>> +    mgag200_dma_add_block(mdev,
>> +        MGAREG_DMAPAD, 0,
>> +        MGAREG_LEN | MGAREG_EXEC, height,
>> +        MGAREG_SECADDR, mdev->dma_handle | 1,
>> +        /* Writing SECEND should always be the last command of a 
>> block */
>> +        MGAREG_SECEND, mdev->dma_handle + size);
>> +}
>> +
>> +static void mgag200_dma_copy(struct mga_device *mdev, const void 
>> *src, u32 pitch,
>> +                struct drm_rect *clip, int cpp)
>> +{
>> +    int i;
>> +    int width = drm_rect_width(clip);
>> +    int height = drm_rect_height(clip);
>> +
>> +    /* pad each line to 32bits boundaries see section 4.5.7 of G200 
>> Specification */
>> +    int width_padded = round_up(width * cpp, 4);
>> +
>> +    for (i = 0; i < height; i++)
>> +        memcpy(mdev->dma_buf + width_padded * i,
>> +               src + (((clip->y1 + i) * pitch) + clip->x1 * cpp),
>> +               width * cpp);
> 
> This memcpy() should probably go away.  Instead of SHMEM, mgag200 should 
> allocate with GEM DMA helpers. We have a number of drivers that use DMA 
> helpers with DRM format helpers, so the conversion should be 
> strait-forward and can be done independently from the other patches.

This is something I tried to do.
It will also make the driver a bit more complex, because when the damage 
rectangle is not the full screen, I will need to do one ILOAD per line, 
instead of one for the whole rectangle, but that's still manageable.
Do you know which driver I can take as an example ?

> 
> Afterward, it should be possible to DMA-copy directly from the GEM 
> buffer object.
> 
>> +
>> +    mgag200_iload_cmd(mdev, clip->x1, clip->y1, width, height, 
>> width_padded, cpp);
>> +    mgag200_dma_run_cmd(mdev);
>> +}
>> +
>> +/*
>> + * If the DMA coherent buffer is smaller than damage rectangle, we 
>> need to
>> + * split it into multiple DMA transfert.
>> + */
>> +static void mgag200_dma_damage(struct mga_device *mdev, const struct 
>> iosys_map *vmap,
>> +                   struct drm_framebuffer *fb, struct drm_rect *clip)
>> +{
>> +    u32 pitch = fb->pitches[0];
>> +    const void *src = vmap[0].vaddr;
>> +    struct drm_rect subclip;
>> +    int y1;
>> +    int lines;
>> +    int cpp = fb->format->cpp[0];
>> +
>> +    /* Number of lines that fits in one DMA buffer */
>> +    lines = min(drm_rect_height(clip), (int) mdev->dma_size / 
>> (drm_rect_width(clip) * cpp));
>> +
>> +    subclip.x1 = clip->x1;
>> +    subclip.x2 = clip->x2;
>> +
>> +    for (y1 = clip->y1; y1 < clip->y2; y1 += lines) {
>> +        subclip.y1 = y1;
>> +        subclip.y2 = min(clip->y2, y1 + lines);
>> +        mgag200_dma_copy(mdev, src, pitch, &subclip, cpp);
>> +    }
>> +}
>> +
>>   static void mgag200_handle_damage(struct mga_device *mdev, const 
>> struct iosys_map *vmap,
>>                     struct drm_framebuffer *fb, struct drm_rect *clip)
>>   {
>> -    struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
>> -
>> -    iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], 
>> fb->format, clip));
>> -    drm_fb_memcpy(&dst, fb->pitches, vmap, fb, clip);
>> +    if (mdev->dma_buf) {
>> +        /* Fast path, use DMA */
>> +        mgag200_dma_damage(mdev, vmap, fb, clip);
>> +    } else {
>> +        struct iosys_map dst = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
>> +
>> +        iosys_map_incr(&dst, drm_fb_clip_offset(fb->pitches[0], 
>> fb->format, clip));
>> +        drm_fb_memcpy(&dst, fb->pitches, vmap, fb, clip);
>> +    }
> 
> This branching needs to go. As I said, DMA is either the standard, or 
> not there at all. I doubt that the !mdev->dmabuf case can easily happen 
> in practice. AFAICT it's all in the 32-bit address range and we're on 
> systems with enough physical memory.

I put that in place because on my system, I can't allocate more than 4MB 
with dmam_alloc_coherent(). (And my framebuffer is 5MB, so my first 
attempt was unsuccessful).
I'm not sure what is the reasonable amount of dma memory that we are 
almost guarantee to allocate.

> 
> Best regards
> Thomas
> 
>>   }
>>   /*
>> @@ -475,6 +594,10 @@ void 
>> mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>       if (!fb)
>>           return;
>> +    if (!old_plane_state->fb || fb->format != 
>> old_plane_state->fb->format
>> +        || fb->width != old_plane_state->fb->width)
>> +        mgag200_dwg_setup(mdev, fb);
>> +
>>       drm_atomic_helper_damage_iter_init(&iter, old_plane_state, 
>> plane_state);
>>       drm_atomic_for_each_plane_damage(&iter, &damage) {
>>           mgag200_handle_damage(mdev, shadow_plane_state->data, fb, 
>> &damage);
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h 
>> b/drivers/gpu/drm/mgag200/mgag200_reg.h
>> index 748c8e18e938..256ac92dae56 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_reg.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
>> @@ -116,6 +116,9 @@
>>   #define    MGAREG_OPMODE        0x1e54
>> +#define MGAREG_PRIMADDR        0x1e58
>> +#define MGAREG_PRIMEND        0x1e5c
>> +
>>   /* Warp Registers */
>>   #define MGAREG_WIADDR           0x1dc0
>>   #define MGAREG_WIADDR2          0x1dd8
>> @@ -200,6 +203,8 @@
>>   /* See table on 4-43 for bop ALU operations */
>> +#define MGADWG_REPLACE    (0xC << 16)
>> +
>>   /* See table on 4-44 for translucidity masks */
>>   #define MGADWG_BMONOLEF        ( 0x00 << 25 )
>> @@ -218,6 +223,8 @@
>>   #define MGADWG_PATTERN        ( 0x01 << 29 )
>>   #define MGADWG_TRANSC        ( 0x01 << 30 )
>> +#define MGADWG_CLIPDIS        ( 0x01 << 31 )
>> +
>>   #define MGAREG_MISC_WRITE    0x3c2
>>   #define MGAREG_MISC_READ    0x3cc
>>   #define MGAREG_MEM_MISC_WRITE       0x1fc2
>> @@ -605,6 +612,9 @@
>>   #    define MGA_TC2_SELECT_TMU1                 (0x80000000)
>>   #define MGAREG_TEXTRANS        0x2c34
>>   #define MGAREG_TEXTRANSHIGH    0x2c38
>> +#define MGAREG_SECADDR        0x2c40
>> +#define MGAREG_SECEND        0x2c44
>> +#define MGAREG_SOFTRAP        0x2c48
>>   #define MGAREG_TEXFILTER    0x2c58
>>   #    define MGA_MIN_NRST                        (0x00000000)
>>   #    define MGA_MIN_BILIN                       (0x00000002)
>> @@ -691,4 +701,19 @@
>>   #define MGA_AGP2XPLL_ENABLE        0x1
>>   #define MGA_AGP2XPLL_DISABLE        0x0
>> +
>> +#define DWGREG0        0x1c00
>> +#define DWGREG0_END    0x1dff
>> +#define DWGREG1        0x2c00
>> +#define DWGREG1_END    0x2dff
>> +
>> +/* These macros convert register address to the 8 bit command index 
>> used with DMA
>> + * It remaps 0x1c00-0x1dff to 0x00-0x7f (REG0)
>> + * and 0x2c00-0x2dff to 0x80-0xff (REG1)
>> + */
>> +#define ISREG0(r)    (r >= DWGREG0 && r <= DWGREG0_END)
>> +#define DMAREG0(r)    ((u8)((r - DWGREG0) >> 2))
>> +#define DMAREG1(r)    ((u8)(((r - DWGREG1) >> 2) | 0x80))
>> +#define DMAREG(r)    (ISREG0((r)) ? DMAREG0((r)) : DMAREG1((r)))
>> +
>>   #endif
> 

Best regards,

-- 

Jocelyn

Re: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[Jocelyn Falempe]

On 09/05/2023 11:49, Jocelyn Falempe wrote:
> On 08/05/2023 10:04, Thomas Zimmermann wrote:
>> Hi Jocelyn
>>
>> Am 05.05.23 um 14:43 schrieb Jocelyn Falempe:

[cut]
>>> +    /* pad each line to 32bits boundaries see section 4.5.7 of G200 
>>> Specification */
>>> +    int width_padded = round_up(width * cpp, 4);
>>> +
>>> +    for (i = 0; i < height; i++)
>>> +        memcpy(mdev->dma_buf + width_padded * i,
>>> +               src + (((clip->y1 + i) * pitch) + clip->x1 * cpp),
>>> +               width * cpp);
>>
>> This memcpy() should probably go away.  Instead of SHMEM, mgag200 
>> should allocate with GEM DMA helpers. We have a number of drivers that 
>> use DMA helpers with DRM format helpers, so the conversion should be 
>> strait-forward and can be done independently from the other patches.
> 
> This is something I tried to do.
> It will also make the driver a bit more complex, because when the damage 
> rectangle is not the full screen, I will need to do one ILOAD per line, 
> instead of one for the whole rectangle, but that's still manageable.
> Do you know which driver I can take as an example ?

I've tested using the GEM DMA helper, but on my test machine, I have the 
same issue that it fails to allocate the framebuffer when it is more 
than 4MB.
So it works in 1024x768 (3MB) but fails for 1280x1024 (5MB).

Adding cma=128M to the kernel command line makes it work (in 1280x1024) 
sometime but not reliably. (it fails to allocate in loop for 30min, and 
then suddenly the allocation succeed, and graphics start working). Also 
enabling iommu seems to have no effect.

I'm probably missing something, as other drivers are using this 
successfully ?

I just used DEFINE_DRM_GEM_DMA_FOPS instead of DEFINE_DRM_GEM_FOPS and
DRM_GEM_DMA_DRIVER_OPS instead of DRM_GEM_SHMEM_DRIVER_OPS in mgag200_drv.c

Then I call drm_fb_dma_get_gem_obj() to get the physical address for the 
Matrox's DMA.

Best regards,

-- 

Jocelyn

Re: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20230505124337.854845-5-jfalempe@redhat.com>
References: <20230505124337.854845-5-jfalempe@redhat.com>
TO: Jocelyn Falempe <jfalempe@redhat.com>
TO: dri-devel@lists.freedesktop.org
TO: tzimmermann@suse.de
TO: airlied@redhat.com
TO: javierm@redhat.com
TO: lyude@redhat.com
CC: Jocelyn Falempe <jfalempe@redhat.com>

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 457391b0380335d5e9a5babdec90ac53928b23b4]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-mgag200-Rename-constant-MGAREG_Status-to-MGAREG_STATUS/20230505-204705
base:   457391b0380335d5e9a5babdec90ac53928b23b4
patch link:    https://lore.kernel.org/r/20230505124337.854845-5-jfalempe%40redhat.com
patch subject: [PATCH 4/4] drm/mgag200: Use DMA to copy the framebuffer to the VRAM
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20230510/202305100554.Q6f2fDlx-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202305100554.Q6f2fDlx-lkp@intel.com/

smatch warnings:
drivers/gpu/drm/mgag200/mgag200_mode.c:419 mgag200_dwg_setup() error: uninitialized symbol 'maccess'.

vim +/maccess +419 drivers/gpu/drm/mgag200/mgag200_mode.c

414c453106255b Dave Airlie     2012-04-17  400  
b3cab0043427ad Jocelyn Falempe 2023-05-05  401  static void mgag200_dwg_setup(struct mga_device *mdev, struct drm_framebuffer *fb)
b3cab0043427ad Jocelyn Falempe 2023-05-05  402  {
b3cab0043427ad Jocelyn Falempe 2023-05-05  403  	u32 maccess;
b3cab0043427ad Jocelyn Falempe 2023-05-05  404  
b3cab0043427ad Jocelyn Falempe 2023-05-05  405  	drm_dbg(&mdev->base, "Setup DWG with %dx%d %p4cc\n",
b3cab0043427ad Jocelyn Falempe 2023-05-05  406  		fb->width, fb->height, &fb->format->format);
b3cab0043427ad Jocelyn Falempe 2023-05-05  407  
b3cab0043427ad Jocelyn Falempe 2023-05-05  408  	switch (fb->format->format) {
b3cab0043427ad Jocelyn Falempe 2023-05-05  409  	case DRM_FORMAT_RGB565:
b3cab0043427ad Jocelyn Falempe 2023-05-05  410  		maccess = MGAMAC_PW16;
b3cab0043427ad Jocelyn Falempe 2023-05-05  411  		break;
b3cab0043427ad Jocelyn Falempe 2023-05-05  412  	case DRM_FORMAT_RGB888:
b3cab0043427ad Jocelyn Falempe 2023-05-05  413  		maccess = MGAMAC_PW24;
b3cab0043427ad Jocelyn Falempe 2023-05-05  414  		break;
b3cab0043427ad Jocelyn Falempe 2023-05-05  415  	case DRM_FORMAT_XRGB8888:
b3cab0043427ad Jocelyn Falempe 2023-05-05  416  		maccess = MGAMAC_PW32;
b3cab0043427ad Jocelyn Falempe 2023-05-05  417  		break;
b3cab0043427ad Jocelyn Falempe 2023-05-05  418  	}
b3cab0043427ad Jocelyn Falempe 2023-05-05 @419  	WREG32(MGAREG_MACCESS, maccess);
b3cab0043427ad Jocelyn Falempe 2023-05-05  420  
b3cab0043427ad Jocelyn Falempe 2023-05-05  421  	/* Framebuffer width in pixel */
b3cab0043427ad Jocelyn Falempe 2023-05-05  422  	WREG32(MGAREG_PITCH, fb->width);
b3cab0043427ad Jocelyn Falempe 2023-05-05  423  
b3cab0043427ad Jocelyn Falempe 2023-05-05  424  	/* Sane default value for the drawing engine registers */
b3cab0043427ad Jocelyn Falempe 2023-05-05  425  	WREG32(MGAREG_DSTORG, 0);
b3cab0043427ad Jocelyn Falempe 2023-05-05  426  	WREG32(MGAREG_YDSTORG, 0);
b3cab0043427ad Jocelyn Falempe 2023-05-05  427  	WREG32(MGAREG_SRCORG, 0);
b3cab0043427ad Jocelyn Falempe 2023-05-05  428  	WREG32(MGAREG_CXBNDRY, 0x0FFF0000);
b3cab0043427ad Jocelyn Falempe 2023-05-05  429  	WREG32(MGAREG_YTOP, 0);
b3cab0043427ad Jocelyn Falempe 2023-05-05  430  	WREG32(MGAREG_YBOT, 0x00FFFFFF);
b3cab0043427ad Jocelyn Falempe 2023-05-05  431  
b3cab0043427ad Jocelyn Falempe 2023-05-05  432  	/* Activate blit mode DMA, only write the low part of the register */
b3cab0043427ad Jocelyn Falempe 2023-05-05  433  	WREG8(MGAREG_OPMODE, MGAOPM_DMA_BLIT);
b3cab0043427ad Jocelyn Falempe 2023-05-05  434  }
b3cab0043427ad Jocelyn Falempe 2023-05-05  435  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests