[PATCH 0/2] Rate-limit shadow-FB-to-console-update to screen refresh

[PATCH 0/2] Rate-limit shadow-FB-to-console-update to screen refresh

[Thomas Zimmermann]

A full-screen memcpy() moves the console's shadow buffer to hardware; with
possibly significant runtime overhead. [1]

The console's dirty worker now waits for the vblank to rate limit the
output frequency. Screen output can pile up while waiting and there's a
chance that multiple screen updates can be handled with a single memcpy().
Note that this has no effect on tearing: while the dirty worker updates
the hardware buffer, new data can still arrive in the shadow buffer. This
can create a tearing effcet, even though console output is synchronized
to vblank.

The patchset adds vblank support to mgag200, because the problem was first
reported with Matrox hardware.

[1] https://lists.freedesktop.org/archives/dri-devel/2019-July/228663.html

Thomas Zimmermann (2):
  drm/fb-helper: Synchronize dirty worker with vblank
  drm/mgag200: Add vblank support

 drivers/gpu/drm/drm_fb_helper.c        | 12 ++++++++
 drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
 drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++---
 5 files changed, 85 insertions(+), 4 deletions(-)

--
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/fb-helper: Synchronize dirty worker with vblank

[Thomas Zimmermann]

Before updating the display from the console's shadow buffer, the dirty
worker now waits for vblank. This allows several screen updates to pile
up and acts as a rate limiter.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a7ba5b4902d6..017e2f6bd1b9 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -402,8 +402,20 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
 						    dirty_work);
 	struct drm_clip_rect *clip = &helper->dirty_clip;
 	struct drm_clip_rect clip_copy;
+	struct drm_crtc *crtc;
 	unsigned long flags;
 	void *vaddr;
+	int ret;
+
+	/* rate-limit update frequency */
+	mutex_lock(&helper->lock);
+	crtc = helper->client.modesets[0].crtc;
+	ret = drm_crtc_vblank_get(crtc);
+	if (!ret) {
+		drm_crtc_wait_one_vblank(crtc);
+		drm_crtc_vblank_put(crtc);
+	}
+	mutex_unlock(&helper->lock);
 
 	spin_lock_irqsave(&helper->dirty_lock, flags);
 	clip_copy = *clip;
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/mgag200: Add vblank support

[Thomas Zimmermann]

Support for vblank requires VSYNC to signal an interrupt, which is broken
on Matrox chipsets. The workaround that is used here and in other free
Matrox drivers is to program <linecomp> to the value of <vdisplay> and
enable the VLINE interrupt. This triggers an interrupt at the same time
when VSYNC begins.

VLINE uses separate registers for enabling and clearing pending interrupts.
No extra syncronization between irq handler and the rest of the driver is
required.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
 drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++---
 4 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 4f9df3b93598..cff265973154 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -67,6 +67,7 @@ static struct drm_driver driver = {
 	.driver_features = DRIVER_GEM | DRIVER_MODESET,
 	.load = mgag200_driver_load,
 	.unload = mgag200_driver_unload,
+	.irq_handler = mgag200_irq_handler,
 	.fops = &mgag200_driver_fops,
 	.name = DRIVER_NAME,
 	.desc = DRIVER_DESC,
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 1c93f8dc08c7..88cf256d135f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -195,6 +195,7 @@ void mgag200_modeset_fini(struct mga_device *mdev);
 				/* mgag200_main.c */
 int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
 void mgag200_driver_unload(struct drm_device *dev);
+irqreturn_t mgag200_irq_handler(int irq, void *arg);
 
 				/* mgag200_i2c.c */
 struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index a9773334dedf..5941607796e8 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -10,7 +10,9 @@
 
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_irq.h>
 #include <drm/drm_pci.h>
+#include <drm/drm_vblank.h>
 
 #include "mgag200_drv.h"
 
@@ -186,10 +188,18 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 	mdev->cursor.pixels_current = NULL;
 
+	r = drm_vblank_init(dev, 1);
+	if (r)
+		goto err_modeset;
+
 	r = drm_fbdev_generic_setup(mdev->dev, 0);
 	if (r)
 		goto err_modeset;
 
+	r = drm_irq_install(dev, dev->pdev->irq);
+	if (r)
+		goto err_modeset;
+
 	return 0;
 
 err_modeset:
@@ -207,8 +217,31 @@ void mgag200_driver_unload(struct drm_device *dev)
 
 	if (mdev == NULL)
 		return;
+	drm_irq_uninstall(dev);
 	mgag200_modeset_fini(mdev);
 	drm_mode_config_cleanup(dev);
 	mgag200_mm_fini(mdev);
 	dev->dev_private = NULL;
 }
+
+irqreturn_t mgag200_irq_handler(int irq, void *arg)
+{
+	struct drm_device *dev = arg;
+	struct mga_device *mdev = dev->dev_private;
+	struct drm_crtc *crtc;
+	u32 status, iclear;
+
+	status = RREG32(0x1e14);
+
+	if (status & 0x00000020) { /* test <vlinepen> */
+		drm_for_each_crtc(crtc, dev) {
+			drm_crtc_handle_vblank(crtc);
+		}
+		iclear = RREG32(0x1e18);
+		iclear |= 0x00000020; /* set <vlineiclr> */
+		WREG32(0x1e18, iclear);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+};
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 5e778b5f1a10..ffe5f15d0a7d 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -905,6 +905,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 	const struct drm_framebuffer *fb = crtc->primary->fb;
 	int hdisplay, hsyncstart, hsyncend, htotal;
 	int vdisplay, vsyncstart, vsyncend, vtotal;
+	int linecomp;
 	int pitch;
 	int option = 0, option2 = 0;
 	int i;
@@ -1042,6 +1043,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 	vsyncend = mode->vsync_end - 1;
 	vtotal = mode->vtotal - 2;
 
+	/* The VSYNC interrupt is broken on Matrox chipsets. We use
+	 * the VLINE interrupt instead. It triggers when the current
+	 * linecomp has been reached. Therefore keep <linecomp> in
+	 * sync with <vdisplay>.
+	 */
+	linecomp = vdisplay;
+
 	WREG_GFX(0, 0);
 	WREG_GFX(1, 0);
 	WREG_GFX(2, 0);
@@ -1063,12 +1071,12 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 		 ((vdisplay & 0x100) >> 7) |
 		 ((vsyncstart & 0x100) >> 6) |
 		 ((vdisplay & 0x100) >> 5) |
-		 ((vdisplay & 0x100) >> 4) | /* linecomp */
+		 ((linecomp & 0x100) >> 4) |
 		 ((vtotal & 0x200) >> 4)|
 		 ((vdisplay & 0x200) >> 3) |
 		 ((vsyncstart & 0x200) >> 2));
 	WREG_CRT(9, ((vdisplay & 0x200) >> 4) |
-		 ((vdisplay & 0x200) >> 3));
+		 ((linecomp & 0x200) >> 3));
 	WREG_CRT(10, 0);
 	WREG_CRT(11, 0);
 	WREG_CRT(12, 0);
@@ -1083,7 +1091,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 	WREG_CRT(21, vdisplay & 0xFF);
 	WREG_CRT(22, (vtotal + 1) & 0xFF);
 	WREG_CRT(23, 0xc3);
-	WREG_CRT(24, vdisplay & 0xFF);
+	WREG_CRT(24, linecomp & 0xff);
 
 	ext_vga[0] = 0;
 	ext_vga[5] = 0;
@@ -1099,7 +1107,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 		((vdisplay & 0x400) >> 8) |
 		((vdisplay & 0xc00) >> 7) |
 		((vsyncstart & 0xc00) >> 5) |
-		((vdisplay & 0x400) >> 3);
+		((linecomp & 0x400) >> 3);
 	if (fb->format->cpp[0] * 8 == 24)
 		ext_vga[3] = (((1 << bppshift) * 3) - 1) | 0x80;
 	else
@@ -1411,6 +1419,30 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
 	crtc->primary->fb = NULL;
 }
 
+static int mga_crtc_enable_vblank(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct mga_device *mdev = dev->dev_private;
+	u32 ien;
+
+	ien = RREG32(0x1e1c);
+	ien |= 0x00000020; /* set <vlineien> */
+	WREG32(0x1e1c, ien);
+
+	return 0;
+}
+
+static void mga_crtc_disable_vblank(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct mga_device *mdev = dev->dev_private;
+	u32 ien;
+
+	ien = RREG32(0x1e1c);
+	ien &= 0xffffffdf; /* clear <vlineien> */
+	WREG32(0x1e1c, ien);
+}
+
 /* These provide the minimum set of functions required to handle a CRTC */
 static const struct drm_crtc_funcs mga_crtc_funcs = {
 	.cursor_set = mga_crtc_cursor_set,
@@ -1418,6 +1450,8 @@ static const struct drm_crtc_funcs mga_crtc_funcs = {
 	.gamma_set = mga_crtc_gamma_set,
 	.set_config = drm_crtc_helper_set_config,
 	.destroy = mga_crtc_destroy,
+	.enable_vblank = mga_crtc_enable_vblank,
+	.disable_vblank = mga_crtc_disable_vblank,
 };
 
 static const struct drm_crtc_helper_funcs mga_helper_funcs = {
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/mgag200: Add vblank support

[Gerd Hoffmann]

On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote:
> Support for vblank requires VSYNC to signal an interrupt, which is broken
> on Matrox chipsets. The workaround that is used here and in other free
> Matrox drivers is to program <linecomp> to the value of <vdisplay> and
> enable the VLINE interrupt. This triggers an interrupt at the same time
> when VSYNC begins.
> 
> VLINE uses separate registers for enabling and clearing pending interrupts.
> No extra syncronization between irq handler and the rest of the driver is
> required.

Looks good overall, some minor nits ...

> +irqreturn_t mgag200_irq_handler(int irq, void *arg)
> +{
> +	struct drm_device *dev = arg;
> +	struct mga_device *mdev = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	u32 status, iclear;
> +
> +	status = RREG32(0x1e14);
> +
> +	if (status & 0x00000020) { /* test <vlinepen> */
> +		drm_for_each_crtc(crtc, dev) {
> +			drm_crtc_handle_vblank(crtc);
> +		}
> +		iclear = RREG32(0x1e18);
> +		iclear |= 0x00000020; /* set <vlineiclr> */

#define for this would be good (you also don't need the comment then).

> +	/* The VSYNC interrupt is broken on Matrox chipsets. We use

Codestyle.  "/*" should be on a separate line.

> +static void mga_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct mga_device *mdev = dev->dev_private;
> +	u32 ien;
> +
> +	ien = RREG32(0x1e1c);
> +	ien &= 0xffffffdf; /* clear <vlineien> */

That is typically written as value &= ~(bit);

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Synchronize dirty worker with vblank

[Gerd Hoffmann]

On Mon, Sep 09, 2019 at 04:06:32PM +0200, Thomas Zimmermann wrote:
> Before updating the display from the console's shadow buffer, the dirty
> worker now waits for vblank. This allows several screen updates to pile
> up and acts as a rate limiter.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a7ba5b4902d6..017e2f6bd1b9 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -402,8 +402,20 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>  						    dirty_work);
>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>  	struct drm_clip_rect clip_copy;
> +	struct drm_crtc *crtc;
>  	unsigned long flags;
>  	void *vaddr;
> +	int ret;
> +
> +	/* rate-limit update frequency */
> +	mutex_lock(&helper->lock);
> +	crtc = helper->client.modesets[0].crtc;
> +	ret = drm_crtc_vblank_get(crtc);
> +	if (!ret) {
> +		drm_crtc_wait_one_vblank(crtc);
> +		drm_crtc_vblank_put(crtc);
> +	}
> +	mutex_unlock(&helper->lock);

Hmm, not sure it is the best plan to sleep for a while in the worker,
especially while holding the lock.

What does the lock protect against here?  Accessing
helper->client.modesets?  If so then you can unlock before going to
sleep in drm_crtc_wait_one_vblank() I think.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Synchronize dirty worker with vblank

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2127 bytes --]

Hi

Am 10.09.19 um 13:52 schrieb Gerd Hoffmann:
> On Mon, Sep 09, 2019 at 04:06:32PM +0200, Thomas Zimmermann wrote:
>> Before updating the display from the console's shadow buffer, the dirty
>> worker now waits for vblank. This allows several screen updates to pile
>> up and acts as a rate limiter.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index a7ba5b4902d6..017e2f6bd1b9 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -402,8 +402,20 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>>  						    dirty_work);
>>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>>  	struct drm_clip_rect clip_copy;
>> +	struct drm_crtc *crtc;
>>  	unsigned long flags;
>>  	void *vaddr;
>> +	int ret;
>> +
>> +	/* rate-limit update frequency */
>> +	mutex_lock(&helper->lock);
>> +	crtc = helper->client.modesets[0].crtc;
>> +	ret = drm_crtc_vblank_get(crtc);
>> +	if (!ret) {
>> +		drm_crtc_wait_one_vblank(crtc);
>> +		drm_crtc_vblank_put(crtc);
>> +	}
>> +	mutex_unlock(&helper->lock);
> 
> Hmm, not sure it is the best plan to sleep for a while in the worker,
> especially while holding the lock.
> 
> What does the lock protect against here?  Accessing

This lock is hold by the fbdev code during mode-setting operations (but
not drawing operations). So *crtc might be gone if we don't hold it here.

> helper->client.modesets?  If so then you can unlock before going to
> sleep in drm_crtc_wait_one_vblank() I think.

I looked, but I cannot find any code that protects crtc while vblank is
active. I'd rather not change the current code until someone can clarify.

Best regards
Thomas

> 
> cheers,
>   Gerd
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Synchronize dirty worker with vblank

[Noralf Trønnes]

Den 10.09.2019 14.48, skrev Thomas Zimmermann:
> Hi
> 
> Am 10.09.19 um 13:52 schrieb Gerd Hoffmann:
>> On Mon, Sep 09, 2019 at 04:06:32PM +0200, Thomas Zimmermann wrote:
>>> Before updating the display from the console's shadow buffer, the dirty
>>> worker now waits for vblank. This allows several screen updates to pile
>>> up and acts as a rate limiter.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>  drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>> index a7ba5b4902d6..017e2f6bd1b9 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -402,8 +402,20 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>>>  						    dirty_work);
>>>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>>>  	struct drm_clip_rect clip_copy;
>>> +	struct drm_crtc *crtc;
>>>  	unsigned long flags;
>>>  	void *vaddr;
>>> +	int ret;
>>> +
>>> +	/* rate-limit update frequency */
>>> +	mutex_lock(&helper->lock);
>>> +	crtc = helper->client.modesets[0].crtc;
>>> +	ret = drm_crtc_vblank_get(crtc);
>>> +	if (!ret) {
>>> +		drm_crtc_wait_one_vblank(crtc);
>>> +		drm_crtc_vblank_put(crtc);
>>> +	}
>>> +	mutex_unlock(&helper->lock);
>>
>> Hmm, not sure it is the best plan to sleep for a while in the worker,
>> especially while holding the lock.
>>
>> What does the lock protect against here?  Accessing
> 
> This lock is hold by the fbdev code during mode-setting operations (but
> not drawing operations). So *crtc might be gone if we don't hold it here.
> 
>> helper->client.modesets?  If so then you can unlock before going to
>> sleep in drm_crtc_wait_one_vblank() I think.
> 
> I looked, but I cannot find any code that protects crtc while vblank is
> active. I'd rather not change the current code until someone can clarify.
> 

The client->modesets array and the crtc struct member are invariant over
the lifetime of client (drm_client_modeset_create()). No need to take a
lock for access. See drm_client_modeset_release() for the things that
_can_ change and needs protection (protected by client->modeset_mutex).

I don't see a problem with sleeping in the worker though, but I might
miss out on something. AFAICS changes will just pile up in >dirty_clip
and the worker will be scheduled for a new run to happen when it's done
with the current update.

Noralf.

> Best regards
> Thomas
> 
>>
>> cheers,
>>   Gerd
>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Synchronize dirty worker with vblank

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3199 bytes --]

Hi

Am 10.09.19 um 15:34 schrieb Noralf Trønnes:
> 
> 
> Den 10.09.2019 14.48, skrev Thomas Zimmermann:
>> Hi
>>
>> Am 10.09.19 um 13:52 schrieb Gerd Hoffmann:
>>> On Mon, Sep 09, 2019 at 04:06:32PM +0200, Thomas Zimmermann wrote:
>>>> Before updating the display from the console's shadow buffer, the dirty
>>>> worker now waits for vblank. This allows several screen updates to pile
>>>> up and acts as a rate limiter.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>  drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>> index a7ba5b4902d6..017e2f6bd1b9 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -402,8 +402,20 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>>>>  						    dirty_work);
>>>>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>>>>  	struct drm_clip_rect clip_copy;
>>>> +	struct drm_crtc *crtc;
>>>>  	unsigned long flags;
>>>>  	void *vaddr;
>>>> +	int ret;
>>>> +
>>>> +	/* rate-limit update frequency */
>>>> +	mutex_lock(&helper->lock);
>>>> +	crtc = helper->client.modesets[0].crtc;
>>>> +	ret = drm_crtc_vblank_get(crtc);
>>>> +	if (!ret) {
>>>> +		drm_crtc_wait_one_vblank(crtc);
>>>> +		drm_crtc_vblank_put(crtc);
>>>> +	}
>>>> +	mutex_unlock(&helper->lock);
>>>
>>> Hmm, not sure it is the best plan to sleep for a while in the worker,
>>> especially while holding the lock.
>>>
>>> What does the lock protect against here?  Accessing
>>
>> This lock is hold by the fbdev code during mode-setting operations (but
>> not drawing operations). So *crtc might be gone if we don't hold it here.
>>
>>> helper->client.modesets?  If so then you can unlock before going to
>>> sleep in drm_crtc_wait_one_vblank() I think.
>>
>> I looked, but I cannot find any code that protects crtc while vblank is
>> active. I'd rather not change the current code until someone can clarify.
>>
> 
> The client->modesets array and the crtc struct member are invariant over
> the lifetime of client (drm_client_modeset_create()). No need to take a
> lock for access. See drm_client_modeset_release() for the things that
> _can_ change and needs protection (protected by client->modeset_mutex).

Thanks for the reply. So we don't need the lock? But why does
drm_fb_helper_ioctl() take it? ioctl exclusiveness?

> I don't see a problem with sleeping in the worker though, but I might
> miss out on something. AFAICS changes will just pile up in >dirty_clip
> and the worker will be scheduled for a new run to happen when it's done
> with the current update.

Yes, that's the intention of the patch. We hope to reduce the number of
memcpys by handling several of them at once.

Best regards
Thomas

> 
> Noralf.
> 
>> Best regards
>> Thomas
>>
>>>
>>> cheers,
>>>   Gerd
>>>
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/mgag200: Add vblank support

[Ville Syrjälä]

On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote:
> Support for vblank requires VSYNC to signal an interrupt, which is broken
> on Matrox chipsets.

I don't remember there being anything wrong with the vsync interrupt.
What makes you think it's broken?

> The workaround that is used here and in other free
> Matrox drivers is to program <linecomp> to the value of <vdisplay> and
> enable the VLINE interrupt. This triggers an interrupt at the same time
> when VSYNC begins.

You're programming it to fire at start of vblank, not start of vsync.

> 
> VLINE uses separate registers for enabling and clearing pending interrupts.
> No extra syncronization between irq handler and the rest of the driver is
> required.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
>  drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++---
>  4 files changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 4f9df3b93598..cff265973154 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -67,6 +67,7 @@ static struct drm_driver driver = {
>  	.driver_features = DRIVER_GEM | DRIVER_MODESET,
>  	.load = mgag200_driver_load,
>  	.unload = mgag200_driver_unload,
> +	.irq_handler = mgag200_irq_handler,
>  	.fops = &mgag200_driver_fops,
>  	.name = DRIVER_NAME,
>  	.desc = DRIVER_DESC,
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 1c93f8dc08c7..88cf256d135f 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -195,6 +195,7 @@ void mgag200_modeset_fini(struct mga_device *mdev);
>  				/* mgag200_main.c */
>  int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
>  void mgag200_driver_unload(struct drm_device *dev);
> +irqreturn_t mgag200_irq_handler(int irq, void *arg);
>  
>  				/* mgag200_i2c.c */
>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index a9773334dedf..5941607796e8 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -10,7 +10,9 @@
>  
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_irq.h>
>  #include <drm/drm_pci.h>
> +#include <drm/drm_vblank.h>
>  
>  #include "mgag200_drv.h"
>  
> @@ -186,10 +188,18 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	}
>  	mdev->cursor.pixels_current = NULL;
>  
> +	r = drm_vblank_init(dev, 1);
> +	if (r)
> +		goto err_modeset;
> +
>  	r = drm_fbdev_generic_setup(mdev->dev, 0);
>  	if (r)
>  		goto err_modeset;
>  
> +	r = drm_irq_install(dev, dev->pdev->irq);
> +	if (r)
> +		goto err_modeset;
> +
>  	return 0;
>  
>  err_modeset:
> @@ -207,8 +217,31 @@ void mgag200_driver_unload(struct drm_device *dev)
>  
>  	if (mdev == NULL)
>  		return;
> +	drm_irq_uninstall(dev);
>  	mgag200_modeset_fini(mdev);
>  	drm_mode_config_cleanup(dev);
>  	mgag200_mm_fini(mdev);
>  	dev->dev_private = NULL;
>  }
> +
> +irqreturn_t mgag200_irq_handler(int irq, void *arg)
> +{
> +	struct drm_device *dev = arg;
> +	struct mga_device *mdev = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	u32 status, iclear;
> +
> +	status = RREG32(0x1e14);
> +
> +	if (status & 0x00000020) { /* test <vlinepen> */
> +		drm_for_each_crtc(crtc, dev) {
> +			drm_crtc_handle_vblank(crtc);
> +		}

A bit odd way to write that but as long this driver doesn't support
crtc2 it should be fine.

> +		iclear = RREG32(0x1e18);
> +		iclear |= 0x00000020; /* set <vlineiclr> */
> +		WREG32(0x1e18, iclear);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +};
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 5e778b5f1a10..ffe5f15d0a7d 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -905,6 +905,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  	const struct drm_framebuffer *fb = crtc->primary->fb;
>  	int hdisplay, hsyncstart, hsyncend, htotal;
>  	int vdisplay, vsyncstart, vsyncend, vtotal;
> +	int linecomp;
>  	int pitch;
>  	int option = 0, option2 = 0;
>  	int i;
> @@ -1042,6 +1043,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  	vsyncend = mode->vsync_end - 1;
>  	vtotal = mode->vtotal - 2;
>  
> +	/* The VSYNC interrupt is broken on Matrox chipsets. We use
> +	 * the VLINE interrupt instead. It triggers when the current
> +	 * linecomp has been reached. Therefore keep <linecomp> in
> +	 * sync with <vdisplay>.
> +	 */
> +	linecomp = vdisplay;

I have an odd recollection that you want vdisplay+1 here if you
want the interrupt to fire at the correct time.

Sine linecomp also resets the memory address counter to 0 you
should probably see one bogus line at the bottom of the screen
if my recollection of that +1 is correct.

But maybe my memory is wrong.

> +
>  	WREG_GFX(0, 0);
>  	WREG_GFX(1, 0);
>  	WREG_GFX(2, 0);
> @@ -1063,12 +1071,12 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  		 ((vdisplay & 0x100) >> 7) |
>  		 ((vsyncstart & 0x100) >> 6) |
>  		 ((vdisplay & 0x100) >> 5) |
> -		 ((vdisplay & 0x100) >> 4) | /* linecomp */
> +		 ((linecomp & 0x100) >> 4) |
>  		 ((vtotal & 0x200) >> 4)|
>  		 ((vdisplay & 0x200) >> 3) |
>  		 ((vsyncstart & 0x200) >> 2));
>  	WREG_CRT(9, ((vdisplay & 0x200) >> 4) |
> -		 ((vdisplay & 0x200) >> 3));
> +		 ((linecomp & 0x200) >> 3));
>  	WREG_CRT(10, 0);
>  	WREG_CRT(11, 0);
>  	WREG_CRT(12, 0);
> @@ -1083,7 +1091,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  	WREG_CRT(21, vdisplay & 0xFF);
>  	WREG_CRT(22, (vtotal + 1) & 0xFF);
>  	WREG_CRT(23, 0xc3);
> -	WREG_CRT(24, vdisplay & 0xFF);
> +	WREG_CRT(24, linecomp & 0xff);
>  
>  	ext_vga[0] = 0;
>  	ext_vga[5] = 0;
> @@ -1099,7 +1107,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  		((vdisplay & 0x400) >> 8) |
>  		((vdisplay & 0xc00) >> 7) |
>  		((vsyncstart & 0xc00) >> 5) |
> -		((vdisplay & 0x400) >> 3);
> +		((linecomp & 0x400) >> 3);
>  	if (fb->format->cpp[0] * 8 == 24)
>  		ext_vga[3] = (((1 << bppshift) * 3) - 1) | 0x80;
>  	else
> @@ -1411,6 +1419,30 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
>  	crtc->primary->fb = NULL;
>  }
>  
> +static int mga_crtc_enable_vblank(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct mga_device *mdev = dev->dev_private;
> +	u32 ien;
> +
> +	ien = RREG32(0x1e1c);

MGAREG_IEN?

> +	ien |= 0x00000020; /* set <vlineien> */
> +	WREG32(0x1e1c, ien);
> +
> +	return 0;
> +}
> +
> +static void mga_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct mga_device *mdev = dev->dev_private;
> +	u32 ien;
> +
> +	ien = RREG32(0x1e1c);
> +	ien &= 0xffffffdf; /* clear <vlineien> */
> +	WREG32(0x1e1c, ien);
> +}
> +
>  /* These provide the minimum set of functions required to handle a CRTC */
>  static const struct drm_crtc_funcs mga_crtc_funcs = {
>  	.cursor_set = mga_crtc_cursor_set,
> @@ -1418,6 +1450,8 @@ static const struct drm_crtc_funcs mga_crtc_funcs = {
>  	.gamma_set = mga_crtc_gamma_set,
>  	.set_config = drm_crtc_helper_set_config,
>  	.destroy = mga_crtc_destroy,
> +	.enable_vblank = mga_crtc_enable_vblank,
> +	.disable_vblank = mga_crtc_disable_vblank,
>  };
>  
>  static const struct drm_crtc_helper_funcs mga_helper_funcs = {
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/mgag200: Add vblank support

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 9572 bytes --]

Hi

thanks for the feedback.

Am 10.09.19 um 16:01 schrieb Ville Syrjälä:
> On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote:
>> Support for vblank requires VSYNC to signal an interrupt, which is broken
>> on Matrox chipsets.
> 
> I don't remember there being anything wrong with the vsync interrupt.
> What makes you think it's broken?

I tested vsync with the same code as in this patch, with extra spin
locks to protect against concurrent register access (vsync uses crtc11h).

A few things constantly went wrong: I never received it when setting
linecomp to vdisplay. However it fired when linecomp was ~0. Then I
regularly missed the first interrupt (but only the first), and
occasionally my test system crashed when I entered commands on the
serial terminal. The vsync irq is maybe not entirely broken, but appears
to be much harder to get right.

>> The workaround that is used here and in other free
>> Matrox drivers is to program <linecomp> to the value of <vdisplay> and
>> enable the VLINE interrupt. This triggers an interrupt at the same time
>> when VSYNC begins.
> 
> You're programming it to fire at start of vblank, not start of vsync.

Ah, sorry for messing up terminology; will be fixed. No matter what's
wrong with the vsync irq: there's no vblank irq, so using linecomp
should be the way to go in any case.

> 
>>
>> VLINE uses separate registers for enabling and clearing pending interrupts.
>> No extra syncronization between irq handler and the rest of the driver is
>> required.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
>>  drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++
>>  drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++---
>>  4 files changed, 73 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index 4f9df3b93598..cff265973154 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -67,6 +67,7 @@ static struct drm_driver driver = {
>>  	.driver_features = DRIVER_GEM | DRIVER_MODESET,
>>  	.load = mgag200_driver_load,
>>  	.unload = mgag200_driver_unload,
>> +	.irq_handler = mgag200_irq_handler,
>>  	.fops = &mgag200_driver_fops,
>>  	.name = DRIVER_NAME,
>>  	.desc = DRIVER_DESC,
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index 1c93f8dc08c7..88cf256d135f 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -195,6 +195,7 @@ void mgag200_modeset_fini(struct mga_device *mdev);
>>  				/* mgag200_main.c */
>>  int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
>>  void mgag200_driver_unload(struct drm_device *dev);
>> +irqreturn_t mgag200_irq_handler(int irq, void *arg);
>>  
>>  				/* mgag200_i2c.c */
>>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
>> index a9773334dedf..5941607796e8 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
>> @@ -10,7 +10,9 @@
>>  
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_irq.h>
>>  #include <drm/drm_pci.h>
>> +#include <drm/drm_vblank.h>
>>  
>>  #include "mgag200_drv.h"
>>  
>> @@ -186,10 +188,18 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  	}
>>  	mdev->cursor.pixels_current = NULL;
>>  
>> +	r = drm_vblank_init(dev, 1);
>> +	if (r)
>> +		goto err_modeset;
>> +
>>  	r = drm_fbdev_generic_setup(mdev->dev, 0);
>>  	if (r)
>>  		goto err_modeset;
>>  
>> +	r = drm_irq_install(dev, dev->pdev->irq);
>> +	if (r)
>> +		goto err_modeset;
>> +
>>  	return 0;
>>  
>>  err_modeset:
>> @@ -207,8 +217,31 @@ void mgag200_driver_unload(struct drm_device *dev)
>>  
>>  	if (mdev == NULL)
>>  		return;
>> +	drm_irq_uninstall(dev);
>>  	mgag200_modeset_fini(mdev);
>>  	drm_mode_config_cleanup(dev);
>>  	mgag200_mm_fini(mdev);
>>  	dev->dev_private = NULL;
>>  }
>> +
>> +irqreturn_t mgag200_irq_handler(int irq, void *arg)
>> +{
>> +	struct drm_device *dev = arg;
>> +	struct mga_device *mdev = dev->dev_private;
>> +	struct drm_crtc *crtc;
>> +	u32 status, iclear;
>> +
>> +	status = RREG32(0x1e14);
>> +
>> +	if (status & 0x00000020) { /* test <vlinepen> */
>> +		drm_for_each_crtc(crtc, dev) {
>> +			drm_crtc_handle_vblank(crtc);
>> +		}
> 
> A bit odd way to write that but as long this driver doesn't support
> crtc2 it should be fine.
> 
>> +		iclear = RREG32(0x1e18);
>> +		iclear |= 0x00000020; /* set <vlineiclr> */
>> +		WREG32(0x1e18, iclear);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	return IRQ_NONE;
>> +};
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 5e778b5f1a10..ffe5f15d0a7d 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -905,6 +905,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  	const struct drm_framebuffer *fb = crtc->primary->fb;
>>  	int hdisplay, hsyncstart, hsyncend, htotal;
>>  	int vdisplay, vsyncstart, vsyncend, vtotal;
>> +	int linecomp;
>>  	int pitch;
>>  	int option = 0, option2 = 0;
>>  	int i;
>> @@ -1042,6 +1043,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  	vsyncend = mode->vsync_end - 1;
>>  	vtotal = mode->vtotal - 2;
>>  
>> +	/* The VSYNC interrupt is broken on Matrox chipsets. We use
>> +	 * the VLINE interrupt instead. It triggers when the current
>> +	 * linecomp has been reached. Therefore keep <linecomp> in
>> +	 * sync with <vdisplay>.
>> +	 */
>> +	linecomp = vdisplay;
> 
> I have an odd recollection that you want vdisplay+1 here if you
> want the interrupt to fire at the correct time.
> 
> Sine linecomp also resets the memory address counter to 0 you
> should probably see one bogus line at the bottom of the screen
> if my recollection of that +1 is correct.

It works reliably on my test system, but I'll double-check against
existing drivers.

> But maybe my memory is wrong.
> 
>> +
>>  	WREG_GFX(0, 0);
>>  	WREG_GFX(1, 0);
>>  	WREG_GFX(2, 0);
>> @@ -1063,12 +1071,12 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  		 ((vdisplay & 0x100) >> 7) |
>>  		 ((vsyncstart & 0x100) >> 6) |
>>  		 ((vdisplay & 0x100) >> 5) |
>> -		 ((vdisplay & 0x100) >> 4) | /* linecomp */
>> +		 ((linecomp & 0x100) >> 4) |
>>  		 ((vtotal & 0x200) >> 4)|
>>  		 ((vdisplay & 0x200) >> 3) |
>>  		 ((vsyncstart & 0x200) >> 2));
>>  	WREG_CRT(9, ((vdisplay & 0x200) >> 4) |
>> -		 ((vdisplay & 0x200) >> 3));
>> +		 ((linecomp & 0x200) >> 3));
>>  	WREG_CRT(10, 0);
>>  	WREG_CRT(11, 0);
>>  	WREG_CRT(12, 0);
>> @@ -1083,7 +1091,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  	WREG_CRT(21, vdisplay & 0xFF);
>>  	WREG_CRT(22, (vtotal + 1) & 0xFF);
>>  	WREG_CRT(23, 0xc3);
>> -	WREG_CRT(24, vdisplay & 0xFF);
>> +	WREG_CRT(24, linecomp & 0xff);
>>  
>>  	ext_vga[0] = 0;
>>  	ext_vga[5] = 0;
>> @@ -1099,7 +1107,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  		((vdisplay & 0x400) >> 8) |
>>  		((vdisplay & 0xc00) >> 7) |
>>  		((vsyncstart & 0xc00) >> 5) |
>> -		((vdisplay & 0x400) >> 3);
>> +		((linecomp & 0x400) >> 3);
>>  	if (fb->format->cpp[0] * 8 == 24)
>>  		ext_vga[3] = (((1 << bppshift) * 3) - 1) | 0x80;
>>  	else
>> @@ -1411,6 +1419,30 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
>>  	crtc->primary->fb = NULL;
>>  }
>>  
>> +static int mga_crtc_enable_vblank(struct drm_crtc *crtc)
>> +{
>> +	struct drm_device *dev = crtc->dev;
>> +	struct mga_device *mdev = dev->dev_private;
>> +	u32 ien;
>> +
>> +	ien = RREG32(0x1e1c);
> 
> MGAREG_IEN?

I'll go through the changes and use constants everywhere.

Best regards
Thomas

>> +	ien |= 0x00000020; /* set <vlineien> */
>> +	WREG32(0x1e1c, ien);
>> +
>> +	return 0;
>> +}
>> +
>> +static void mga_crtc_disable_vblank(struct drm_crtc *crtc)
>> +{
>> +	struct drm_device *dev = crtc->dev;
>> +	struct mga_device *mdev = dev->dev_private;
>> +	u32 ien;
>> +
>> +	ien = RREG32(0x1e1c);
>> +	ien &= 0xffffffdf; /* clear <vlineien> */
>> +	WREG32(0x1e1c, ien);
>> +}
>> +
>>  /* These provide the minimum set of functions required to handle a CRTC */
>>  static const struct drm_crtc_funcs mga_crtc_funcs = {
>>  	.cursor_set = mga_crtc_cursor_set,
>> @@ -1418,6 +1450,8 @@ static const struct drm_crtc_funcs mga_crtc_funcs = {
>>  	.gamma_set = mga_crtc_gamma_set,
>>  	.set_config = drm_crtc_helper_set_config,
>>  	.destroy = mga_crtc_destroy,
>> +	.enable_vblank = mga_crtc_enable_vblank,
>> +	.disable_vblank = mga_crtc_disable_vblank,
>>  };
>>  
>>  static const struct drm_crtc_helper_funcs mga_helper_funcs = {
>> -- 
>> 2.23.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Synchronize dirty worker with vblank

[Noralf Trønnes]

Den 10.09.2019 15.51, skrev Thomas Zimmermann:
> Hi
> 
> Am 10.09.19 um 15:34 schrieb Noralf Trønnes:
>>
>>
>> Den 10.09.2019 14.48, skrev Thomas Zimmermann:
>>> Hi
>>>
>>> Am 10.09.19 um 13:52 schrieb Gerd Hoffmann:
>>>> On Mon, Sep 09, 2019 at 04:06:32PM +0200, Thomas Zimmermann wrote:
>>>>> Before updating the display from the console's shadow buffer, the dirty
>>>>> worker now waits for vblank. This allows several screen updates to pile
>>>>> up and acts as a rate limiter.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++++
>>>>>  1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>>> index a7ba5b4902d6..017e2f6bd1b9 100644
>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>> @@ -402,8 +402,20 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>>>>>  						    dirty_work);
>>>>>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>>>>>  	struct drm_clip_rect clip_copy;
>>>>> +	struct drm_crtc *crtc;
>>>>>  	unsigned long flags;
>>>>>  	void *vaddr;
>>>>> +	int ret;
>>>>> +
>>>>> +	/* rate-limit update frequency */
>>>>> +	mutex_lock(&helper->lock);
>>>>> +	crtc = helper->client.modesets[0].crtc;
>>>>> +	ret = drm_crtc_vblank_get(crtc);
>>>>> +	if (!ret) {
>>>>> +		drm_crtc_wait_one_vblank(crtc);
>>>>> +		drm_crtc_vblank_put(crtc);
>>>>> +	}
>>>>> +	mutex_unlock(&helper->lock);
>>>>
>>>> Hmm, not sure it is the best plan to sleep for a while in the worker,
>>>> especially while holding the lock.
>>>>
>>>> What does the lock protect against here?  Accessing
>>>
>>> This lock is hold by the fbdev code during mode-setting operations (but
>>> not drawing operations). So *crtc might be gone if we don't hold it here.
>>>
>>>> helper->client.modesets?  If so then you can unlock before going to
>>>> sleep in drm_crtc_wait_one_vblank() I think.
>>>
>>> I looked, but I cannot find any code that protects crtc while vblank is
>>> active. I'd rather not change the current code until someone can clarify.
>>>
>>
>> The client->modesets array and the crtc struct member are invariant over
>> the lifetime of client (drm_client_modeset_create()). No need to take a
>> lock for access. See drm_client_modeset_release() for the things that
>> _can_ change and needs protection (protected by client->modeset_mutex).
> 
> Thanks for the reply. So we don't need the lock? But why does
> drm_fb_helper_ioctl() take it? ioctl exclusiveness?
> 

Because of drm_master_internal_acquire() it's necessary to take the lock
first. That's the locking rules of drm_fb_helper. First take the fb
helper lock, then the dev->master_mutex. We stay away if there's a
userspace master and if there's none, we prevent userspace from becoming
master while we do stuff.

But looking at drm_fb_helper_ioctl() now, I see that it's not strictly
necessary to do this since all this function can do is vblank wait and
that's fine even if userspace is master. The locking was necessary
before I refactored and moved stuff to drm_client, because at that time
the modeset array was deleted and recreated when probing connectors.
But it doesn't hurt either in case more functionality is added to the
ioctl. One wouldn't think that would ever happen, since fbdev is going
away soon, but still we keep polishing it ;)

Noralf.

>> I don't see a problem with sleeping in the worker though, but I might
>> miss out on something. AFAICS changes will just pile up in >dirty_clip
>> and the worker will be scheduled for a new run to happen when it's done
>> with the current update.
> 
> Yes, that's the intention of the patch. We hope to reduce the number of
> memcpys by handling several of them at once.
> 
> Best regards
> Thomas
> 
>>
>> Noralf.
>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> cheers,
>>>>   Gerd
>>>>
>>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/mgag200: Add vblank support

[Ville Syrjälä]

On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote:
> Support for vblank requires VSYNC to signal an interrupt, which is broken
> on Matrox chipsets. The workaround that is used here and in other free
> Matrox drivers is to program <linecomp> to the value of <vdisplay> and
> enable the VLINE interrupt. This triggers an interrupt at the same time
> when VSYNC begins.
> 
> VLINE uses separate registers for enabling and clearing pending interrupts.
> No extra syncronization between irq handler and the rest of the driver is
> required.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
>  drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++---
>  4 files changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 4f9df3b93598..cff265973154 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -67,6 +67,7 @@ static struct drm_driver driver = {
>  	.driver_features = DRIVER_GEM | DRIVER_MODESET,
>  	.load = mgag200_driver_load,
>  	.unload = mgag200_driver_unload,
> +	.irq_handler = mgag200_irq_handler,
>  	.fops = &mgag200_driver_fops,
>  	.name = DRIVER_NAME,
>  	.desc = DRIVER_DESC,
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 1c93f8dc08c7..88cf256d135f 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -195,6 +195,7 @@ void mgag200_modeset_fini(struct mga_device *mdev);
>  				/* mgag200_main.c */
>  int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
>  void mgag200_driver_unload(struct drm_device *dev);
> +irqreturn_t mgag200_irq_handler(int irq, void *arg);
>  
>  				/* mgag200_i2c.c */
>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index a9773334dedf..5941607796e8 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -10,7 +10,9 @@
>  
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_irq.h>
>  #include <drm/drm_pci.h>
> +#include <drm/drm_vblank.h>
>  
>  #include "mgag200_drv.h"
>  
> @@ -186,10 +188,18 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	}
>  	mdev->cursor.pixels_current = NULL;
>  
> +	r = drm_vblank_init(dev, 1);
> +	if (r)
> +		goto err_modeset;
> +
>  	r = drm_fbdev_generic_setup(mdev->dev, 0);
>  	if (r)
>  		goto err_modeset;
>  
> +	r = drm_irq_install(dev, dev->pdev->irq);
> +	if (r)
> +		goto err_modeset;
> +
>  	return 0;
>  
>  err_modeset:
> @@ -207,8 +217,31 @@ void mgag200_driver_unload(struct drm_device *dev)
>  
>  	if (mdev == NULL)
>  		return;
> +	drm_irq_uninstall(dev);
>  	mgag200_modeset_fini(mdev);
>  	drm_mode_config_cleanup(dev);
>  	mgag200_mm_fini(mdev);
>  	dev->dev_private = NULL;
>  }
> +
> +irqreturn_t mgag200_irq_handler(int irq, void *arg)
> +{
> +	struct drm_device *dev = arg;
> +	struct mga_device *mdev = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	u32 status, iclear;
> +
> +	status = RREG32(0x1e14);
> +
> +	if (status & 0x00000020) { /* test <vlinepen> */

On further inspection this looks a bit iffy. IIRC the
status bits aren't masked out by IEN, so I would do
something like this:

irq_handler() {
	status = read(STATUS) & read(IEN):
	if (status == 0)
		return IRQ_NONE;

	write(ICLEAR, status);
	drm_handle_vblank();
	return IRQ_HANDLED;
}

vblank_enable() {
	write(ICLEAR, vline);
	write(IEN, vline);
}

vblank_disable() {
	write(IEN, 0);
}

Otherwise you maybe try to handle a stale interrupt when
enabling the vblank irq, and also could accidentally
handle bogus interrupts from other devices on the same irq
line.

And probably clear IEN on before registering the irq handler
for good measure.


> +		drm_for_each_crtc(crtc, dev) {
> +			drm_crtc_handle_vblank(crtc);
> +		}
> +		iclear = RREG32(0x1e18);
> +		iclear |= 0x00000020; /* set <vlineiclr> */
> +		WREG32(0x1e18, iclear);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +};

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/mgag200: Add vblank support

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 8827 bytes --]

Hi

Am 10.09.19 um 16:01 schrieb Ville Syrjälä:
> On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote:
>> Support for vblank requires VSYNC to signal an interrupt, which is broken
>> on Matrox chipsets.
> 
> I don't remember there being anything wrong with the vsync interrupt.
> What makes you think it's broken?
> 
>> The workaround that is used here and in other free
>> Matrox drivers is to program <linecomp> to the value of <vdisplay> and
>> enable the VLINE interrupt. This triggers an interrupt at the same time
>> when VSYNC begins.
> 
> You're programming it to fire at start of vblank, not start of vsync.
> 
>>
>> VLINE uses separate registers for enabling and clearing pending interrupts.
>> No extra syncronization between irq handler and the rest of the driver is
>> required.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
>>  drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++
>>  drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++---
>>  4 files changed, 73 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index 4f9df3b93598..cff265973154 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -67,6 +67,7 @@ static struct drm_driver driver = {
>>  	.driver_features = DRIVER_GEM | DRIVER_MODESET,
>>  	.load = mgag200_driver_load,
>>  	.unload = mgag200_driver_unload,
>> +	.irq_handler = mgag200_irq_handler,
>>  	.fops = &mgag200_driver_fops,
>>  	.name = DRIVER_NAME,
>>  	.desc = DRIVER_DESC,
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index 1c93f8dc08c7..88cf256d135f 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -195,6 +195,7 @@ void mgag200_modeset_fini(struct mga_device *mdev);
>>  				/* mgag200_main.c */
>>  int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
>>  void mgag200_driver_unload(struct drm_device *dev);
>> +irqreturn_t mgag200_irq_handler(int irq, void *arg);
>>  
>>  				/* mgag200_i2c.c */
>>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
>> index a9773334dedf..5941607796e8 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
>> @@ -10,7 +10,9 @@
>>  
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_irq.h>
>>  #include <drm/drm_pci.h>
>> +#include <drm/drm_vblank.h>
>>  
>>  #include "mgag200_drv.h"
>>  
>> @@ -186,10 +188,18 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  	}
>>  	mdev->cursor.pixels_current = NULL;
>>  
>> +	r = drm_vblank_init(dev, 1);
>> +	if (r)
>> +		goto err_modeset;
>> +
>>  	r = drm_fbdev_generic_setup(mdev->dev, 0);
>>  	if (r)
>>  		goto err_modeset;
>>  
>> +	r = drm_irq_install(dev, dev->pdev->irq);
>> +	if (r)
>> +		goto err_modeset;
>> +
>>  	return 0;
>>  
>>  err_modeset:
>> @@ -207,8 +217,31 @@ void mgag200_driver_unload(struct drm_device *dev)
>>  
>>  	if (mdev == NULL)
>>  		return;
>> +	drm_irq_uninstall(dev);
>>  	mgag200_modeset_fini(mdev);
>>  	drm_mode_config_cleanup(dev);
>>  	mgag200_mm_fini(mdev);
>>  	dev->dev_private = NULL;
>>  }
>> +
>> +irqreturn_t mgag200_irq_handler(int irq, void *arg)
>> +{
>> +	struct drm_device *dev = arg;
>> +	struct mga_device *mdev = dev->dev_private;
>> +	struct drm_crtc *crtc;
>> +	u32 status, iclear;
>> +
>> +	status = RREG32(0x1e14);
>> +
>> +	if (status & 0x00000020) { /* test <vlinepen> */
>> +		drm_for_each_crtc(crtc, dev) {
>> +			drm_crtc_handle_vblank(crtc);
>> +		}
> 
> A bit odd way to write that but as long this driver doesn't support
> crtc2 it should be fine.
> 
>> +		iclear = RREG32(0x1e18);
>> +		iclear |= 0x00000020; /* set <vlineiclr> */
>> +		WREG32(0x1e18, iclear);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	return IRQ_NONE;
>> +};
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 5e778b5f1a10..ffe5f15d0a7d 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -905,6 +905,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  	const struct drm_framebuffer *fb = crtc->primary->fb;
>>  	int hdisplay, hsyncstart, hsyncend, htotal;
>>  	int vdisplay, vsyncstart, vsyncend, vtotal;
>> +	int linecomp;
>>  	int pitch;
>>  	int option = 0, option2 = 0;
>>  	int i;
>> @@ -1042,6 +1043,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  	vsyncend = mode->vsync_end - 1;
>>  	vtotal = mode->vtotal - 2;
>>  
>> +	/* The VSYNC interrupt is broken on Matrox chipsets. We use
>> +	 * the VLINE interrupt instead. It triggers when the current
>> +	 * linecomp has been reached. Therefore keep <linecomp> in
>> +	 * sync with <vdisplay>.
>> +	 */
>> +	linecomp = vdisplay;
> 
> I have an odd recollection that you want vdisplay+1 here if you
> want the interrupt to fire at the correct time.

I double-checked and found that vdisplay is the value originally used by
this driver, by the fbdev driver and by Xorg's mga driver.

Best regards
Thomas

> Sine linecomp also resets the memory address counter to 0 you
> should probably see one bogus line at the bottom of the screen
> if my recollection of that +1 is correct.
> 
> But maybe my memory is wrong.
> 
>> +
>>  	WREG_GFX(0, 0);
>>  	WREG_GFX(1, 0);
>>  	WREG_GFX(2, 0);
>> @@ -1063,12 +1071,12 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  		 ((vdisplay & 0x100) >> 7) |
>>  		 ((vsyncstart & 0x100) >> 6) |
>>  		 ((vdisplay & 0x100) >> 5) |
>> -		 ((vdisplay & 0x100) >> 4) | /* linecomp */
>> +		 ((linecomp & 0x100) >> 4) |
>>  		 ((vtotal & 0x200) >> 4)|
>>  		 ((vdisplay & 0x200) >> 3) |
>>  		 ((vsyncstart & 0x200) >> 2));
>>  	WREG_CRT(9, ((vdisplay & 0x200) >> 4) |
>> -		 ((vdisplay & 0x200) >> 3));
>> +		 ((linecomp & 0x200) >> 3));
>>  	WREG_CRT(10, 0);
>>  	WREG_CRT(11, 0);
>>  	WREG_CRT(12, 0);
>> @@ -1083,7 +1091,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  	WREG_CRT(21, vdisplay & 0xFF);
>>  	WREG_CRT(22, (vtotal + 1) & 0xFF);
>>  	WREG_CRT(23, 0xc3);
>> -	WREG_CRT(24, vdisplay & 0xFF);
>> +	WREG_CRT(24, linecomp & 0xff);
>>  
>>  	ext_vga[0] = 0;
>>  	ext_vga[5] = 0;
>> @@ -1099,7 +1107,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>  		((vdisplay & 0x400) >> 8) |
>>  		((vdisplay & 0xc00) >> 7) |
>>  		((vsyncstart & 0xc00) >> 5) |
>> -		((vdisplay & 0x400) >> 3);
>> +		((linecomp & 0x400) >> 3);
>>  	if (fb->format->cpp[0] * 8 == 24)
>>  		ext_vga[3] = (((1 << bppshift) * 3) - 1) | 0x80;
>>  	else
>> @@ -1411,6 +1419,30 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
>>  	crtc->primary->fb = NULL;
>>  }
>>  
>> +static int mga_crtc_enable_vblank(struct drm_crtc *crtc)
>> +{
>> +	struct drm_device *dev = crtc->dev;
>> +	struct mga_device *mdev = dev->dev_private;
>> +	u32 ien;
>> +
>> +	ien = RREG32(0x1e1c);
> 
> MGAREG_IEN?
> 
>> +	ien |= 0x00000020; /* set <vlineien> */
>> +	WREG32(0x1e1c, ien);
>> +
>> +	return 0;
>> +}
>> +
>> +static void mga_crtc_disable_vblank(struct drm_crtc *crtc)
>> +{
>> +	struct drm_device *dev = crtc->dev;
>> +	struct mga_device *mdev = dev->dev_private;
>> +	u32 ien;
>> +
>> +	ien = RREG32(0x1e1c);
>> +	ien &= 0xffffffdf; /* clear <vlineien> */
>> +	WREG32(0x1e1c, ien);
>> +}
>> +
>>  /* These provide the minimum set of functions required to handle a CRTC */
>>  static const struct drm_crtc_funcs mga_crtc_funcs = {
>>  	.cursor_set = mga_crtc_cursor_set,
>> @@ -1418,6 +1450,8 @@ static const struct drm_crtc_funcs mga_crtc_funcs = {
>>  	.gamma_set = mga_crtc_gamma_set,
>>  	.set_config = drm_crtc_helper_set_config,
>>  	.destroy = mga_crtc_destroy,
>> +	.enable_vblank = mga_crtc_enable_vblank,
>> +	.disable_vblank = mga_crtc_disable_vblank,
>>  };
>>  
>>  static const struct drm_crtc_helper_funcs mga_helper_funcs = {
>> -- 
>> 2.23.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/mgag200: Add vblank support

[Ville Syrjälä]

On Wed, Sep 11, 2019 at 05:08:45PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 10.09.19 um 16:01 schrieb Ville Syrjälä:
> > On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote:
> >> Support for vblank requires VSYNC to signal an interrupt, which is broken
> >> on Matrox chipsets.
> > 
> > I don't remember there being anything wrong with the vsync interrupt.
> > What makes you think it's broken?
> > 
> >> The workaround that is used here and in other free
> >> Matrox drivers is to program <linecomp> to the value of <vdisplay> and
> >> enable the VLINE interrupt. This triggers an interrupt at the same time
> >> when VSYNC begins.
> > 
> > You're programming it to fire at start of vblank, not start of vsync.
> > 
> >>
> >> VLINE uses separate registers for enabling and clearing pending interrupts.
> >> No extra syncronization between irq handler and the rest of the driver is
> >> required.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >>  drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
> >>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
> >>  drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++
> >>  drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++---
> >>  4 files changed, 73 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> >> index 4f9df3b93598..cff265973154 100644
> >> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> >> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> >> @@ -67,6 +67,7 @@ static struct drm_driver driver = {
> >>  	.driver_features = DRIVER_GEM | DRIVER_MODESET,
> >>  	.load = mgag200_driver_load,
> >>  	.unload = mgag200_driver_unload,
> >> +	.irq_handler = mgag200_irq_handler,
> >>  	.fops = &mgag200_driver_fops,
> >>  	.name = DRIVER_NAME,
> >>  	.desc = DRIVER_DESC,
> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> >> index 1c93f8dc08c7..88cf256d135f 100644
> >> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> >> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> >> @@ -195,6 +195,7 @@ void mgag200_modeset_fini(struct mga_device *mdev);
> >>  				/* mgag200_main.c */
> >>  int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
> >>  void mgag200_driver_unload(struct drm_device *dev);
> >> +irqreturn_t mgag200_irq_handler(int irq, void *arg);
> >>  
> >>  				/* mgag200_i2c.c */
> >>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> >> index a9773334dedf..5941607796e8 100644
> >> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> >> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> >> @@ -10,7 +10,9 @@
> >>  
> >>  #include <drm/drm_crtc_helper.h>
> >>  #include <drm/drm_gem_framebuffer_helper.h>
> >> +#include <drm/drm_irq.h>
> >>  #include <drm/drm_pci.h>
> >> +#include <drm/drm_vblank.h>
> >>  
> >>  #include "mgag200_drv.h"
> >>  
> >> @@ -186,10 +188,18 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
> >>  	}
> >>  	mdev->cursor.pixels_current = NULL;
> >>  
> >> +	r = drm_vblank_init(dev, 1);
> >> +	if (r)
> >> +		goto err_modeset;
> >> +
> >>  	r = drm_fbdev_generic_setup(mdev->dev, 0);
> >>  	if (r)
> >>  		goto err_modeset;
> >>  
> >> +	r = drm_irq_install(dev, dev->pdev->irq);
> >> +	if (r)
> >> +		goto err_modeset;
> >> +
> >>  	return 0;
> >>  
> >>  err_modeset:
> >> @@ -207,8 +217,31 @@ void mgag200_driver_unload(struct drm_device *dev)
> >>  
> >>  	if (mdev == NULL)
> >>  		return;
> >> +	drm_irq_uninstall(dev);
> >>  	mgag200_modeset_fini(mdev);
> >>  	drm_mode_config_cleanup(dev);
> >>  	mgag200_mm_fini(mdev);
> >>  	dev->dev_private = NULL;
> >>  }
> >> +
> >> +irqreturn_t mgag200_irq_handler(int irq, void *arg)
> >> +{
> >> +	struct drm_device *dev = arg;
> >> +	struct mga_device *mdev = dev->dev_private;
> >> +	struct drm_crtc *crtc;
> >> +	u32 status, iclear;
> >> +
> >> +	status = RREG32(0x1e14);
> >> +
> >> +	if (status & 0x00000020) { /* test <vlinepen> */
> >> +		drm_for_each_crtc(crtc, dev) {
> >> +			drm_crtc_handle_vblank(crtc);
> >> +		}
> > 
> > A bit odd way to write that but as long this driver doesn't support
> > crtc2 it should be fine.
> > 
> >> +		iclear = RREG32(0x1e18);
> >> +		iclear |= 0x00000020; /* set <vlineiclr> */
> >> +		WREG32(0x1e18, iclear);
> >> +		return IRQ_HANDLED;
> >> +	}
> >> +
> >> +	return IRQ_NONE;
> >> +};
> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> >> index 5e778b5f1a10..ffe5f15d0a7d 100644
> >> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> >> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> >> @@ -905,6 +905,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
> >>  	const struct drm_framebuffer *fb = crtc->primary->fb;
> >>  	int hdisplay, hsyncstart, hsyncend, htotal;
> >>  	int vdisplay, vsyncstart, vsyncend, vtotal;
> >> +	int linecomp;
> >>  	int pitch;
> >>  	int option = 0, option2 = 0;
> >>  	int i;
> >> @@ -1042,6 +1043,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
> >>  	vsyncend = mode->vsync_end - 1;
> >>  	vtotal = mode->vtotal - 2;
> >>  
> >> +	/* The VSYNC interrupt is broken on Matrox chipsets. We use
> >> +	 * the VLINE interrupt instead. It triggers when the current
> >> +	 * linecomp has been reached. Therefore keep <linecomp> in
> >> +	 * sync with <vdisplay>.
> >> +	 */
> >> +	linecomp = vdisplay;
> > 
> > I have an odd recollection that you want vdisplay+1 here if you
> > want the interrupt to fire at the correct time.
> 
> I double-checked and found that vdisplay is the value originally used by
> this driver, by the fbdev driver and by Xorg's mga driver.

Sure, but what does the hardware actually want?

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/mgag200: Add vblank support

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 6339 bytes --]

Hi

Am 11.09.19 um 17:21 schrieb Ville Syrjälä:
> On Wed, Sep 11, 2019 at 05:08:45PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 10.09.19 um 16:01 schrieb Ville Syrjälä:
>>> On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote:
>>>> Support for vblank requires VSYNC to signal an interrupt, which is broken
>>>> on Matrox chipsets.
>>>
>>> I don't remember there being anything wrong with the vsync interrupt.
>>> What makes you think it's broken?
>>>
>>>> The workaround that is used here and in other free
>>>> Matrox drivers is to program <linecomp> to the value of <vdisplay> and
>>>> enable the VLINE interrupt. This triggers an interrupt at the same time
>>>> when VSYNC begins.
>>>
>>> You're programming it to fire at start of vblank, not start of vsync.
>>>
>>>>
>>>> VLINE uses separate registers for enabling and clearing pending interrupts.
>>>> No extra syncronization between irq handler and the rest of the driver is
>>>> required.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>  drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
>>>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
>>>>  drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++
>>>>  drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++---
>>>>  4 files changed, 73 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
>>>> index 4f9df3b93598..cff265973154 100644
>>>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>>>> @@ -67,6 +67,7 @@ static struct drm_driver driver = {
>>>>  	.driver_features = DRIVER_GEM | DRIVER_MODESET,
>>>>  	.load = mgag200_driver_load,
>>>>  	.unload = mgag200_driver_unload,
>>>> +	.irq_handler = mgag200_irq_handler,
>>>>  	.fops = &mgag200_driver_fops,
>>>>  	.name = DRIVER_NAME,
>>>>  	.desc = DRIVER_DESC,
>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
>>>> index 1c93f8dc08c7..88cf256d135f 100644
>>>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>>>> @@ -195,6 +195,7 @@ void mgag200_modeset_fini(struct mga_device *mdev);
>>>>  				/* mgag200_main.c */
>>>>  int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
>>>>  void mgag200_driver_unload(struct drm_device *dev);
>>>> +irqreturn_t mgag200_irq_handler(int irq, void *arg);
>>>>  
>>>>  				/* mgag200_i2c.c */
>>>>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
>>>> index a9773334dedf..5941607796e8 100644
>>>> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
>>>> @@ -10,7 +10,9 @@
>>>>  
>>>>  #include <drm/drm_crtc_helper.h>
>>>>  #include <drm/drm_gem_framebuffer_helper.h>
>>>> +#include <drm/drm_irq.h>
>>>>  #include <drm/drm_pci.h>
>>>> +#include <drm/drm_vblank.h>
>>>>  
>>>>  #include "mgag200_drv.h"
>>>>  
>>>> @@ -186,10 +188,18 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>>>  	}
>>>>  	mdev->cursor.pixels_current = NULL;
>>>>  
>>>> +	r = drm_vblank_init(dev, 1);
>>>> +	if (r)
>>>> +		goto err_modeset;
>>>> +
>>>>  	r = drm_fbdev_generic_setup(mdev->dev, 0);
>>>>  	if (r)
>>>>  		goto err_modeset;
>>>>  
>>>> +	r = drm_irq_install(dev, dev->pdev->irq);
>>>> +	if (r)
>>>> +		goto err_modeset;
>>>> +
>>>>  	return 0;
>>>>  
>>>>  err_modeset:
>>>> @@ -207,8 +217,31 @@ void mgag200_driver_unload(struct drm_device *dev)
>>>>  
>>>>  	if (mdev == NULL)
>>>>  		return;
>>>> +	drm_irq_uninstall(dev);
>>>>  	mgag200_modeset_fini(mdev);
>>>>  	drm_mode_config_cleanup(dev);
>>>>  	mgag200_mm_fini(mdev);
>>>>  	dev->dev_private = NULL;
>>>>  }
>>>> +
>>>> +irqreturn_t mgag200_irq_handler(int irq, void *arg)
>>>> +{
>>>> +	struct drm_device *dev = arg;
>>>> +	struct mga_device *mdev = dev->dev_private;
>>>> +	struct drm_crtc *crtc;
>>>> +	u32 status, iclear;
>>>> +
>>>> +	status = RREG32(0x1e14);
>>>> +
>>>> +	if (status & 0x00000020) { /* test <vlinepen> */
>>>> +		drm_for_each_crtc(crtc, dev) {
>>>> +			drm_crtc_handle_vblank(crtc);
>>>> +		}
>>>
>>> A bit odd way to write that but as long this driver doesn't support
>>> crtc2 it should be fine.
>>>
>>>> +		iclear = RREG32(0x1e18);
>>>> +		iclear |= 0x00000020; /* set <vlineiclr> */
>>>> +		WREG32(0x1e18, iclear);
>>>> +		return IRQ_HANDLED;
>>>> +	}
>>>> +
>>>> +	return IRQ_NONE;
>>>> +};
>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>>>> index 5e778b5f1a10..ffe5f15d0a7d 100644
>>>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>>>> @@ -905,6 +905,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>>>  	const struct drm_framebuffer *fb = crtc->primary->fb;
>>>>  	int hdisplay, hsyncstart, hsyncend, htotal;
>>>>  	int vdisplay, vsyncstart, vsyncend, vtotal;
>>>> +	int linecomp;
>>>>  	int pitch;
>>>>  	int option = 0, option2 = 0;
>>>>  	int i;
>>>> @@ -1042,6 +1043,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>>>>  	vsyncend = mode->vsync_end - 1;
>>>>  	vtotal = mode->vtotal - 2;
>>>>  
>>>> +	/* The VSYNC interrupt is broken on Matrox chipsets. We use
>>>> +	 * the VLINE interrupt instead. It triggers when the current
>>>> +	 * linecomp has been reached. Therefore keep <linecomp> in
>>>> +	 * sync with <vdisplay>.
>>>> +	 */
>>>> +	linecomp = vdisplay;
>>>
>>> I have an odd recollection that you want vdisplay+1 here if you
>>> want the interrupt to fire at the correct time.
>>
>> I double-checked and found that vdisplay is the value originally used by
>> this driver, by the fbdev driver and by Xorg's mga driver.
> 
> Sure, but what does the hardware actually want?
> 

I set linecomp to vdisplay+1 and got a DRM warning about missing a
vblank event.

Best regards
Thomas


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)


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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Synchronize dirty worker with vblank

[Daniel Vetter]

On Tue, Sep 10, 2019 at 04:59:57PM +0200, Noralf Trønnes wrote:
> 
> 
> Den 10.09.2019 15.51, skrev Thomas Zimmermann:
> > Hi
> > 
> > Am 10.09.19 um 15:34 schrieb Noralf Trønnes:
> >>
> >>
> >> Den 10.09.2019 14.48, skrev Thomas Zimmermann:
> >>> Hi
> >>>
> >>> Am 10.09.19 um 13:52 schrieb Gerd Hoffmann:
> >>>> On Mon, Sep 09, 2019 at 04:06:32PM +0200, Thomas Zimmermann wrote:
> >>>>> Before updating the display from the console's shadow buffer, the dirty
> >>>>> worker now waits for vblank. This allows several screen updates to pile
> >>>>> up and acts as a rate limiter.
> >>>>>
> >>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> ---
> >>>>>  drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++++
> >>>>>  1 file changed, 12 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >>>>> index a7ba5b4902d6..017e2f6bd1b9 100644
> >>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>>>> @@ -402,8 +402,20 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
> >>>>>  						    dirty_work);
> >>>>>  	struct drm_clip_rect *clip = &helper->dirty_clip;
> >>>>>  	struct drm_clip_rect clip_copy;
> >>>>> +	struct drm_crtc *crtc;
> >>>>>  	unsigned long flags;
> >>>>>  	void *vaddr;
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	/* rate-limit update frequency */
> >>>>> +	mutex_lock(&helper->lock);
> >>>>> +	crtc = helper->client.modesets[0].crtc;
> >>>>> +	ret = drm_crtc_vblank_get(crtc);
> >>>>> +	if (!ret) {
> >>>>> +		drm_crtc_wait_one_vblank(crtc);
> >>>>> +		drm_crtc_vblank_put(crtc);
> >>>>> +	}
> >>>>> +	mutex_unlock(&helper->lock);
> >>>>
> >>>> Hmm, not sure it is the best plan to sleep for a while in the worker,
> >>>> especially while holding the lock.
> >>>>
> >>>> What does the lock protect against here?  Accessing
> >>>
> >>> This lock is hold by the fbdev code during mode-setting operations (but
> >>> not drawing operations). So *crtc might be gone if we don't hold it here.
> >>>
> >>>> helper->client.modesets?  If so then you can unlock before going to
> >>>> sleep in drm_crtc_wait_one_vblank() I think.
> >>>
> >>> I looked, but I cannot find any code that protects crtc while vblank is
> >>> active. I'd rather not change the current code until someone can clarify.
> >>>
> >>
> >> The client->modesets array and the crtc struct member are invariant over
> >> the lifetime of client (drm_client_modeset_create()). No need to take a
> >> lock for access. See drm_client_modeset_release() for the things that
> >> _can_ change and needs protection (protected by client->modeset_mutex).
> > 
> > Thanks for the reply. So we don't need the lock? But why does
> > drm_fb_helper_ioctl() take it? ioctl exclusiveness?
> > 
> 
> Because of drm_master_internal_acquire() it's necessary to take the lock
> first. That's the locking rules of drm_fb_helper. First take the fb
> helper lock, then the dev->master_mutex. We stay away if there's a
> userspace master and if there's none, we prevent userspace from becoming
> master while we do stuff.
> 
> But looking at drm_fb_helper_ioctl() now, I see that it's not strictly
> necessary to do this since all this function can do is vblank wait and
> that's fine even if userspace is master. The locking was necessary
> before I refactored and moved stuff to drm_client, because at that time
> the modeset array was deleted and recreated when probing connectors.
> But it doesn't hurt either in case more functionality is added to the
> ioctl. One wouldn't think that would ever happen, since fbdev is going
> away soon, but still we keep polishing it ;)

fbdev drivers are hopefully disappearing, I don't think fbdev as the uapi
interface will disappear soon. Hence why it's still somewhat reasonable to
keep polishing it imo. It should actually help in convincing people to
move their fbdev driver over to drm, if that gives them a more polished
fbdev implementation :-)
-Daniel

> 
> Noralf.
> 
> >> I don't see a problem with sleeping in the worker though, but I might
> >> miss out on something. AFAICS changes will just pile up in >dirty_clip
> >> and the worker will be scheduled for a new run to happen when it's done
> >> with the current update.
> > 
> > Yes, that's the intention of the patch. We hope to reduce the number of
> > memcpys by handling several of them at once.
> > 
> > Best regards
> > Thomas
> > 
> >>
> >> Noralf.
> >>
> >>> Best regards
> >>> Thomas
> >>>
> >>>>
> >>>> cheers,
> >>>>   Gerd
> >>>>
> >>>
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Synchronize dirty worker with vblank

[Noralf Trønnes]

Den 17.09.2019 14.55, skrev Daniel Vetter:
> On Tue, Sep 10, 2019 at 04:59:57PM +0200, Noralf Trønnes wrote:
>>
>>
>> Den 10.09.2019 15.51, skrev Thomas Zimmermann:
>>> Hi
>>>
>>> Am 10.09.19 um 15:34 schrieb Noralf Trønnes:
>>>>
>>>>
>>>> Den 10.09.2019 14.48, skrev Thomas Zimmermann:
>>>>> Hi
>>>>>
>>>>> Am 10.09.19 um 13:52 schrieb Gerd Hoffmann:
>>>>>> On Mon, Sep 09, 2019 at 04:06:32PM +0200, Thomas Zimmermann wrote:
>>>>>>> Before updating the display from the console's shadow buffer, the dirty
>>>>>>> worker now waits for vblank. This allows several screen updates to pile
>>>>>>> up and acts as a rate limiter.
>>>>>>>
>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++++
>>>>>>>  1 file changed, 12 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>>>>> index a7ba5b4902d6..017e2f6bd1b9 100644
>>>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>>>> @@ -402,8 +402,20 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>>>>>>>  						    dirty_work);
>>>>>>>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>>>>>>>  	struct drm_clip_rect clip_copy;
>>>>>>> +	struct drm_crtc *crtc;
>>>>>>>  	unsigned long flags;
>>>>>>>  	void *vaddr;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	/* rate-limit update frequency */
>>>>>>> +	mutex_lock(&helper->lock);
>>>>>>> +	crtc = helper->client.modesets[0].crtc;
>>>>>>> +	ret = drm_crtc_vblank_get(crtc);
>>>>>>> +	if (!ret) {
>>>>>>> +		drm_crtc_wait_one_vblank(crtc);
>>>>>>> +		drm_crtc_vblank_put(crtc);
>>>>>>> +	}
>>>>>>> +	mutex_unlock(&helper->lock);
>>>>>>
>>>>>> Hmm, not sure it is the best plan to sleep for a while in the worker,
>>>>>> especially while holding the lock.
>>>>>>
>>>>>> What does the lock protect against here?  Accessing
>>>>>
>>>>> This lock is hold by the fbdev code during mode-setting operations (but
>>>>> not drawing operations). So *crtc might be gone if we don't hold it here.
>>>>>
>>>>>> helper->client.modesets?  If so then you can unlock before going to
>>>>>> sleep in drm_crtc_wait_one_vblank() I think.
>>>>>
>>>>> I looked, but I cannot find any code that protects crtc while vblank is
>>>>> active. I'd rather not change the current code until someone can clarify.
>>>>>
>>>>
>>>> The client->modesets array and the crtc struct member are invariant over
>>>> the lifetime of client (drm_client_modeset_create()). No need to take a
>>>> lock for access. See drm_client_modeset_release() for the things that
>>>> _can_ change and needs protection (protected by client->modeset_mutex).
>>>
>>> Thanks for the reply. So we don't need the lock? But why does
>>> drm_fb_helper_ioctl() take it? ioctl exclusiveness?
>>>
>>
>> Because of drm_master_internal_acquire() it's necessary to take the lock
>> first. That's the locking rules of drm_fb_helper. First take the fb
>> helper lock, then the dev->master_mutex. We stay away if there's a
>> userspace master and if there's none, we prevent userspace from becoming
>> master while we do stuff.
>>
>> But looking at drm_fb_helper_ioctl() now, I see that it's not strictly
>> necessary to do this since all this function can do is vblank wait and
>> that's fine even if userspace is master. The locking was necessary
>> before I refactored and moved stuff to drm_client, because at that time
>> the modeset array was deleted and recreated when probing connectors.
>> But it doesn't hurt either in case more functionality is added to the
>> ioctl. One wouldn't think that would ever happen, since fbdev is going
>> away soon, but still we keep polishing it ;)
> 
> fbdev drivers are hopefully disappearing, I don't think fbdev as the uapi
> interface will disappear soon. Hence why it's still somewhat reasonable to
> keep polishing it imo. It should actually help in convincing people to
> move their fbdev driver over to drm, if that gives them a more polished
> fbdev implementation :-)

Oh right, the uapi stays, in that light it makes sense to keep polishing
the emulation and get fbdev drivers ported over.

Noralf.

> -Daniel
> 
>>
>> Noralf.
>>
>>>> I don't see a problem with sleeping in the worker though, but I might
>>>> miss out on something. AFAICS changes will just pile up in >dirty_clip
>>>> and the worker will be scheduled for a new run to happen when it's done
>>>> with the current update.
>>>
>>> Yes, that's the intention of the patch. We hope to reduce the number of
>>> memcpys by handling several of them at once.
>>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> Noralf.
>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>>>
>>>>>> cheers,
>>>>>>   Gerd
>>>>>>
>>>>>
>>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel