Re: [PATCH] drm/udl: Make page_flip asynchronous
Quoting Dawid Kurek (2017-07-07 06:48:49)
> In page_flip vblank is sent with no delay. Driver does not know when the
> actual update is present on the display and has no means for getting
> this information from a device. It is practically impossible to say
> exactly *when* as there is also i.e. a usb delay.
>
> When we are unable to determine when the vblank actually happens we may
> assume it will behave accordingly, i.e. it will present frames with
> proper timing. In the worst case scenario it should take up to duration
> of one frame (we may get new frame in the device just after presenting
> current one so we would need to wait for the whole frame).
>
> Because of the asynchronous nature of the delay we need to synchronize:
> * read/write vrefresh/page_flip data when changing mode and
>preparing/executing vblank
> * USB requests to prevent interleaved access to URBs for two different
>frame buffers
>
> All those changes are backports from ChromeOS:
> 1. https://chromium-review.googlesource.com/250622
> 2. https://chromium-review.googlesource.com/249450
> partially, only change in udl_modeset.c for 'udl_flip_queue'
> 3. https://chromium-review.googlesource.com/321378
> 4. https://chromium-review.googlesource.com/324119
> + fixes for checkpatch and latest drm changes
>
> Cc: h...@chromium.org
> Cc: marc...@chromium.org
> Cc: za...@chromium.org
> Cc: db...@google.com
> Signed-off-by: Dawid Kurek
> +struct udl_flip_queue {
> + struct mutex lock;
> + struct workqueue_struct *wq;
> + struct delayed_work work;
> + struct drm_crtc *crtc;
> + struct drm_pending_vblank_event *event;
> + u64 flip_time; /*in jiffies */
> + u64 vblank_interval; /*in jiffies */
jiffies are unsigned long. That avoids the complication of the reader
having to consider whether all the divides are 32bit safe.
mallocing this struct seems a little overkill as opposed to embedding it
tino the udl_device, flip_queue.wq provides the safety check after
initialisation.
I couldn't spot anything drastically wrong, certainly it looks complete,
I would have structured it such that everything went through the same
worker with a hrtimer emulating the vblank. That model is then but a
stone's throw away from atomic.
Reviewed-by: Chris Wilson
-Chris
RE: [PATCH] drm/udl: Make page_flip asynchronous
On Thursday, July 13, 2017 7:11 PM, Daniel Vetter wrote: >>> Can't we roll this driver over to the atomic helpers instead? There >>> you get nonblocking pretty much for free ... I'm not sure extending >>> the old modeset code has all that much benefit really. >> This code certainly has value by itself; it makes the driver more >> efficient. I think the best can sometimes be the enemy of the good -- >> this code is here and written, but I don't think any of us is going to >> tackle atomic for udl. > Sure, I guess I should have clarified this with "If you want me to > review and merge this, then pls look into atomic, since that seems > actually beneficial for my own interest". I'm not paid by intel to > review driver patches at random, but to keep overall drm in nice > shape. Moving drivers to atomic and using more shared infrastructure > is in that interest, reviewing random driver patches isn't. So, what's the next step here then? This code makes udl work better, without investing a lot of effort to move everything to atomics. Btw, this is already causing modesetting not to support page flips (while Chrome OS, having this code uses page flips with udl): https://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/drivers/modesetting/driver.c#n1198 Regards, Michal Lukaszek
Re: [PATCH] drm/udl: Make page_flip asynchronous
On Thu, Jul 13, 2017 at 6:25 PM, Stéphane Marchesin wrote: >> Can't we roll this driver over to the atomic helpers instead? There >> you get nonblocking pretty much for free ... I'm not sure extending >> the old modeset code has all that much benefit really. > > This code certainly has value by itself; it makes the driver more > efficient. I think the best can sometimes be the enemy of the good -- > this code is here and written, but I don't think any of us is going to > tackle atomic for udl. Sure, I guess I should have clarified this with "If you want me to review and merge this, then pls look into atomic, since that seems actually beneficial for my own interest". I'm not paid by intel to review driver patches at random, but to keep overall drm in nice shape. Moving drivers to atomic and using more shared infrastructure is in that interest, reviewing random driver patches isn't. Sorry for not making clear, I kinda have that as my implicit context. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH] drm/udl: Make page_flip asynchronous
On Mon, Jul 10, 2017 at 11:58 PM, Daniel Vetter wrote: > On Fri, Jul 7, 2017 at 7:48 AM, Dawid Kurek > wrote: >> In page_flip vblank is sent with no delay. Driver does not know when the >> actual update is present on the display and has no means for getting >> this information from a device. It is practically impossible to say >> exactly *when* as there is also i.e. a usb delay. >> >> When we are unable to determine when the vblank actually happens we may >> assume it will behave accordingly, i.e. it will present frames with >> proper timing. In the worst case scenario it should take up to duration >> of one frame (we may get new frame in the device just after presenting >> current one so we would need to wait for the whole frame). >> >> Because of the asynchronous nature of the delay we need to synchronize: >> * read/write vrefresh/page_flip data when changing mode and >>preparing/executing vblank >> * USB requests to prevent interleaved access to URBs for two different >>frame buffers >> >> All those changes are backports from ChromeOS: >> 1. https://chromium-review.googlesource.com/250622 >> 2. https://chromium-review.googlesource.com/249450 >> partially, only change in udl_modeset.c for 'udl_flip_queue' >> 3. https://chromium-review.googlesource.com/321378 >> 4. https://chromium-review.googlesource.com/324119 >> + fixes for checkpatch and latest drm changes >> >> Cc: h...@chromium.org >> Cc: marc...@chromium.org >> Cc: za...@chromium.org >> Cc: db...@google.com >> Signed-off-by: Dawid Kurek > > Can't we roll this driver over to the atomic helpers instead? There > you get nonblocking pretty much for free ... I'm not sure extending > the old modeset code has all that much benefit really. This code certainly has value by itself; it makes the driver more efficient. I think the best can sometimes be the enemy of the good -- this code is here and written, but I don't think any of us is going to tackle atomic for udl. Stéphane > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/udl: Make page_flip asynchronous
On Fri, Jul 7, 2017 at 7:48 AM, Dawid Kurek wrote: > In page_flip vblank is sent with no delay. Driver does not know when the > actual update is present on the display and has no means for getting > this information from a device. It is practically impossible to say > exactly *when* as there is also i.e. a usb delay. > > When we are unable to determine when the vblank actually happens we may > assume it will behave accordingly, i.e. it will present frames with > proper timing. In the worst case scenario it should take up to duration > of one frame (we may get new frame in the device just after presenting > current one so we would need to wait for the whole frame). > > Because of the asynchronous nature of the delay we need to synchronize: > * read/write vrefresh/page_flip data when changing mode and >preparing/executing vblank > * USB requests to prevent interleaved access to URBs for two different >frame buffers > > All those changes are backports from ChromeOS: > 1. https://chromium-review.googlesource.com/250622 > 2. https://chromium-review.googlesource.com/249450 > partially, only change in udl_modeset.c for 'udl_flip_queue' > 3. https://chromium-review.googlesource.com/321378 > 4. https://chromium-review.googlesource.com/324119 > + fixes for checkpatch and latest drm changes > > Cc: h...@chromium.org > Cc: marc...@chromium.org > Cc: za...@chromium.org > Cc: db...@google.com > Signed-off-by: Dawid Kurek Can't we roll this driver over to the atomic helpers instead? There you get nonblocking pretty much for free ... I'm not sure extending the old modeset code has all that much benefit really. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH] drm/udl: Make page_flip asynchronous
In page_flip vblank is sent with no delay. Driver does not know when the
actual update is present on the display and has no means for getting
this information from a device. It is practically impossible to say
exactly *when* as there is also i.e. a usb delay.
When we are unable to determine when the vblank actually happens we may
assume it will behave accordingly, i.e. it will present frames with
proper timing. In the worst case scenario it should take up to duration
of one frame (we may get new frame in the device just after presenting
current one so we would need to wait for the whole frame).
Because of the asynchronous nature of the delay we need to synchronize:
* read/write vrefresh/page_flip data when changing mode and
preparing/executing vblank
* USB requests to prevent interleaved access to URBs for two different
frame buffers
All those changes are backports from ChromeOS:
1. https://chromium-review.googlesource.com/250622
2. https://chromium-review.googlesource.com/249450
partially, only change in udl_modeset.c for 'udl_flip_queue'
3. https://chromium-review.googlesource.com/321378
4. https://chromium-review.googlesource.com/324119
+ fixes for checkpatch and latest drm changes
Cc: h...@chromium.org
Cc: marc...@chromium.org
Cc: za...@chromium.org
Cc: db...@google.com
Signed-off-by: Dawid Kurek
---
drivers/gpu/drm/udl/udl_drv.h | 4 +
drivers/gpu/drm/udl/udl_fb.c | 28 ---
drivers/gpu/drm/udl/udl_main.c| 3 +
drivers/gpu/drm/udl/udl_modeset.c | 160 ++
4 files changed, 171 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 2a75ab8..b93fb8d 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -47,6 +47,7 @@ struct urb_list {
};
struct udl_fbdev;
+struct udl_flip_queue;
struct udl_device {
struct device *dev;
@@ -66,6 +67,9 @@ struct udl_device {
atomic_t bytes_identical; /* saved effort with backbuffer comparison */
atomic_t bytes_sent; /* to usb, after compression including overhead */
atomic_t cpu_kcycles_used; /* transpired during pixel processing */
+
+ struct udl_flip_queue *flip_queue;
+ struct mutex transfer_lock; /* to serialize transfers */
};
struct udl_gem_object {
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 4a65003..6dd9a49 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -82,7 +82,7 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int
y,
{
struct drm_device *dev = fb->base.dev;
struct udl_device *udl = dev->dev_private;
- int i, ret;
+ int i, ret = 0;
char *cmd;
cycles_t start_cycles, end_cycles;
int bytes_sent = 0;
@@ -91,18 +91,22 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x,
int y,
int aligned_x;
int bpp = fb->base.format->cpp[0];
+ mutex_lock(&udl->transfer_lock);
+
if (!fb->active_16)
- return 0;
+ goto out;
if (!fb->obj->vmapping) {
ret = udl_gem_vmap(fb->obj);
if (ret == -ENOMEM) {
DRM_ERROR("failed to vmap fb\n");
- return 0;
+ ret = 0;
+ goto out;
}
if (!fb->obj->vmapping) {
DRM_ERROR("failed to vmapping\n");
- return 0;
+ ret = 0;
+ goto out;
}
}
@@ -112,14 +116,18 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x,
int y,
if ((width <= 0) ||
(x + width > fb->base.width) ||
- (y + height > fb->base.height))
- return -EINVAL;
+ (y + height > fb->base.height)) {
+ ret = -EINVAL;
+ goto out;
+ }
start_cycles = get_cycles();
urb = udl_get_urb(dev);
- if (!urb)
- return 0;
+ if (!urb) {
+ ret = 0;
+ goto out;
+ }
cmd = urb->transfer_buffer;
for (i = y; i < y + height ; i++) {
@@ -151,7 +159,9 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x,
int y,
>> 10)), /* Kcycles */
&udl->cpu_kcycles_used);
- return 0;
+out:
+ mutex_unlock(&udl->transfer_lock);
+ return ret;
}
static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index a9d93b8..d376233 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -319,6 +319,8 @@ int udl_driver_load(struct drm_device *dev, unsigned long
flags)
if (!udl)
return -ENOMEM;
+ mutex_init(&udl->transfer_lock);
+
udl->udev = ud
[PATCH] drm/udl: Make page_flip asynchronous
In page_flip vblank is sent with no delay. Driver does not know when the
actual update is present on the display and has no means for getting
this information from a device. It is practically impossible to say
exactly *when* as there is also i.e. a usb delay.
When we are unable to determine when the vblank actually happens we may
assume it will behave accordingly, i.e. it will present frames with
proper timing. In the worst case scenario it should take up to duration
of one frame (we may get new frame in the device just after presenting
current one so we would need to wait for the whole frame).
Because of the asynchronous nature of the delay we need to synchronize:
* read/write vrefresh/page_flip data when changing mode and
preparing/executing vblank
* USB requests to prevent interleaved access to URBs for two different
frame buffers
All those changes are backports from ChromeOS:
1. https://chromium-review.googlesource.com/250622
2. https://chromium-review.googlesource.com/249450
partially, only change in udl_modeset.c for 'udl_flip_queue'
3. https://chromium-review.googlesource.com/321378
4. https://chromium-review.googlesource.com/324119
+ fixes for checkpatch and latest drm changes
Cc: h...@chromium.org
Cc: marc...@chromium.org
Cc: za...@chromium.org
Cc: db...@google.com
Signed-off-by: Dawid Kurek
---
drivers/gpu/drm/udl/udl_drv.h | 4 +
drivers/gpu/drm/udl/udl_fb.c | 28 ---
drivers/gpu/drm/udl/udl_main.c| 3 +
drivers/gpu/drm/udl/udl_modeset.c | 160 ++
4 files changed, 171 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 2a75ab8..b93fb8d 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -47,6 +47,7 @@ struct urb_list {
};
struct udl_fbdev;
+struct udl_flip_queue;
struct udl_device {
struct device *dev;
@@ -66,6 +67,9 @@ struct udl_device {
atomic_t bytes_identical; /* saved effort with backbuffer comparison */
atomic_t bytes_sent; /* to usb, after compression including overhead */
atomic_t cpu_kcycles_used; /* transpired during pixel processing */
+
+ struct udl_flip_queue *flip_queue;
+ struct mutex transfer_lock; /* to serialize transfers */
};
struct udl_gem_object {
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 4a65003..6dd9a49 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -82,7 +82,7 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int
y,
{
struct drm_device *dev = fb->base.dev;
struct udl_device *udl = dev->dev_private;
- int i, ret;
+ int i, ret = 0;
char *cmd;
cycles_t start_cycles, end_cycles;
int bytes_sent = 0;
@@ -91,18 +91,22 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x,
int y,
int aligned_x;
int bpp = fb->base.format->cpp[0];
+ mutex_lock(&udl->transfer_lock);
+
if (!fb->active_16)
- return 0;
+ goto out;
if (!fb->obj->vmapping) {
ret = udl_gem_vmap(fb->obj);
if (ret == -ENOMEM) {
DRM_ERROR("failed to vmap fb\n");
- return 0;
+ ret = 0;
+ goto out;
}
if (!fb->obj->vmapping) {
DRM_ERROR("failed to vmapping\n");
- return 0;
+ ret = 0;
+ goto out;
}
}
@@ -112,14 +116,18 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x,
int y,
if ((width <= 0) ||
(x + width > fb->base.width) ||
- (y + height > fb->base.height))
- return -EINVAL;
+ (y + height > fb->base.height)) {
+ ret = -EINVAL;
+ goto out;
+ }
start_cycles = get_cycles();
urb = udl_get_urb(dev);
- if (!urb)
- return 0;
+ if (!urb) {
+ ret = 0;
+ goto out;
+ }
cmd = urb->transfer_buffer;
for (i = y; i < y + height ; i++) {
@@ -151,7 +159,9 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x,
int y,
>> 10)), /* Kcycles */
&udl->cpu_kcycles_used);
- return 0;
+out:
+ mutex_unlock(&udl->transfer_lock);
+ return ret;
}
static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index a9d93b8..d376233 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -319,6 +319,8 @@ int udl_driver_load(struct drm_device *dev, unsigned long
flags)
if (!udl)
return -ENOMEM;
+ mutex_init(&udl->transfer_lock);
+
udl->udev = ud
