DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl can change the framebuffer of plane but user can't know completion of changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is added, we can also do pageflip of a plane.
Signed-off-by: Joonyoung Shim jy0922.shim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/gpu/drm/drm_crtc.c | 45 ++++++++++++++++++++++++++++++++++++++++--- include/drm/drm.h | 1 + include/drm/drm_crtc.h | 3 +- include/drm/drm_mode.h | 3 ++ 4 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d3aaeb6..4c4fa03 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1690,8 +1690,10 @@ int drm_mode_setplane(struct drm_device *dev, void *data, struct drm_plane *plane; struct drm_crtc *crtc; struct drm_framebuffer *fb; + struct drm_pending_vblank_event *e = NULL; int ret = 0; unsigned int fb_width, fb_height; + unsigned long flags; int i;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) @@ -1785,16 +1787,51 @@ int drm_mode_setplane(struct drm_device *dev, void *data, goto out; }
+ if (plane_req->flags & DRM_MODE_PLANE_EVENT) { + ret = -ENOMEM; + spin_lock_irqsave(&dev->event_lock, flags); + if (file_priv->event_space < sizeof e->event) { + spin_unlock_irqrestore(&dev->event_lock, flags); + goto out; + } + file_priv->event_space -= sizeof e->event; + spin_unlock_irqrestore(&dev->event_lock, flags); + + e = kzalloc(sizeof *e, GFP_KERNEL); + if (e == NULL) { + spin_lock_irqsave(&dev->event_lock, flags); + file_priv->event_space += sizeof e->event; + spin_unlock_irqrestore(&dev->event_lock, flags); + goto out; + } + + e->event.base.type = DRM_EVENT_SET_PLANE_COMPLETE; + e->event.base.length = sizeof e->event; + e->event.user_data = plane_req->user_data; + e->base.event = &e->event.base; + e->base.file_priv = file_priv; + e->base.destroy = (void (*) (struct drm_pending_event *)) kfree; + } + ret = plane->funcs->update_plane(plane, crtc, fb, plane_req->crtc_x, plane_req->crtc_y, plane_req->crtc_w, plane_req->crtc_h, plane_req->src_x, plane_req->src_y, - plane_req->src_w, plane_req->src_h); - if (!ret) { - plane->crtc = crtc; - plane->fb = fb; + plane_req->src_w, plane_req->src_h, + e); + if (ret) { + if (plane_req->flags & DRM_MODE_PLANE_EVENT) { + spin_lock_irqsave(&dev->event_lock, flags); + file_priv->event_space += sizeof e->event; + spin_unlock_irqrestore(&dev->event_lock, flags); + kfree(e); + } + goto out; }
+ plane->crtc = crtc; + plane->fb = fb; + out: mutex_unlock(&dev->mode_config.mutex);
diff --git a/include/drm/drm.h b/include/drm/drm.h index 64ff02d..8e2d385 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -761,6 +761,7 @@ struct drm_event {
#define DRM_EVENT_VBLANK 0x01 #define DRM_EVENT_FLIP_COMPLETE 0x02 +#define DRM_EVENT_SET_PLANE_COMPLETE 0x03
struct drm_event_vblank { struct drm_event base; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e250eda..19fb9ea 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -602,7 +602,8 @@ struct drm_plane_funcs { int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h); + uint32_t src_w, uint32_t src_h, + struct drm_pending_vblank_event *event); int (*disable_plane)(struct drm_plane *plane); void (*destroy)(struct drm_plane *plane); }; diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h index 4a0aae3..5ebdbdd 100644 --- a/include/drm/drm_mode.h +++ b/include/drm/drm_mode.h @@ -124,6 +124,7 @@ struct drm_mode_crtc {
#define DRM_MODE_PRESENT_TOP_FIELD (1<<0) #define DRM_MODE_PRESENT_BOTTOM_FIELD (1<<1) +#define DRM_MODE_PLANE_EVENT (1<<2)
/* Planes blend with or override other bits on the CRTC */ struct drm_mode_set_plane { @@ -139,6 +140,8 @@ struct drm_mode_set_plane { /* Source values are 16.16 fixed point */ __u32 src_x, src_y; __u32 src_h, src_w; + + __u64 user_data; };
struct drm_mode_get_plane {
On Wed, Apr 18, 2012 at 01:31:59PM +0900, Joonyoung Shim wrote:
DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl can change the framebuffer of plane but user can't know completion of changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is added, we can also do pageflip of a plane.
Signed-off-by: Joonyoung Shim jy0922.shim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
If I understand the current kms api correctly, set_plane is akin to set_base and should not generate an asynchronous flip completion event. To do that we need a new pageflip ioctl which changes a complete set of fb + planes + any crtc attributes that might be in an atomic fashion. At which point we can just reuse the existing page flip event mechanism.
Yours, Daniel
drivers/gpu/drm/drm_crtc.c | 45 ++++++++++++++++++++++++++++++++++++++++--- include/drm/drm.h | 1 + include/drm/drm_crtc.h | 3 +- include/drm/drm_mode.h | 3 ++ 4 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d3aaeb6..4c4fa03 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1690,8 +1690,10 @@ int drm_mode_setplane(struct drm_device *dev, void *data, struct drm_plane *plane; struct drm_crtc *crtc; struct drm_framebuffer *fb;
struct drm_pending_vblank_event *e = NULL; int ret = 0; unsigned int fb_width, fb_height;
unsigned long flags; int i;
if (!drm_core_check_feature(dev, DRIVER_MODESET))
@@ -1785,16 +1787,51 @@ int drm_mode_setplane(struct drm_device *dev, void *data, goto out; }
- if (plane_req->flags & DRM_MODE_PLANE_EVENT) {
ret = -ENOMEM;spin_lock_irqsave(&dev->event_lock, flags);if (file_priv->event_space < sizeof e->event) {spin_unlock_irqrestore(&dev->event_lock, flags);goto out;}file_priv->event_space -= sizeof e->event;spin_unlock_irqrestore(&dev->event_lock, flags);e = kzalloc(sizeof *e, GFP_KERNEL);if (e == NULL) {spin_lock_irqsave(&dev->event_lock, flags);file_priv->event_space += sizeof e->event;spin_unlock_irqrestore(&dev->event_lock, flags);goto out;}e->event.base.type = DRM_EVENT_SET_PLANE_COMPLETE;e->event.base.length = sizeof e->event;e->event.user_data = plane_req->user_data;e->base.event = &e->event.base;e->base.file_priv = file_priv;e->base.destroy = (void (*) (struct drm_pending_event *)) kfree;- }
- ret = plane->funcs->update_plane(plane, crtc, fb, plane_req->crtc_x, plane_req->crtc_y, plane_req->crtc_w, plane_req->crtc_h, plane_req->src_x, plane_req->src_y,
plane_req->src_w, plane_req->src_h);- if (!ret) {
plane->crtc = crtc;plane->fb = fb;
plane_req->src_w, plane_req->src_h,e);if (ret) {
if (plane_req->flags & DRM_MODE_PLANE_EVENT) {spin_lock_irqsave(&dev->event_lock, flags);file_priv->event_space += sizeof e->event;spin_unlock_irqrestore(&dev->event_lock, flags);kfree(e);}goto out;}
plane->crtc = crtc;
plane->fb = fb;
out: mutex_unlock(&dev->mode_config.mutex);
diff --git a/include/drm/drm.h b/include/drm/drm.h index 64ff02d..8e2d385 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -761,6 +761,7 @@ struct drm_event {
#define DRM_EVENT_VBLANK 0x01 #define DRM_EVENT_FLIP_COMPLETE 0x02 +#define DRM_EVENT_SET_PLANE_COMPLETE 0x03
struct drm_event_vblank { struct drm_event base; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e250eda..19fb9ea 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -602,7 +602,8 @@ struct drm_plane_funcs { int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y,
uint32_t src_w, uint32_t src_h);
uint32_t src_w, uint32_t src_h,struct drm_pending_vblank_event *event);}; diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h index 4a0aae3..5ebdbdd 100644 --- a/include/drm/drm_mode.h +++ b/include/drm/drm_mode.h @@ -124,6 +124,7 @@ struct drm_mode_crtc {
#define DRM_MODE_PRESENT_TOP_FIELD (1<<0) #define DRM_MODE_PRESENT_BOTTOM_FIELD (1<<1) +#define DRM_MODE_PLANE_EVENT (1<<2)
/* Planes blend with or override other bits on the CRTC */ struct drm_mode_set_plane { @@ -139,6 +140,8 @@ struct drm_mode_set_plane { /* Source values are 16.16 fixed point */ __u32 src_x, src_y; __u32 src_h, src_w;
- __u64 user_data;
};
struct drm_mode_get_plane {
1.7.5.4
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 04/18/2012 05:46 PM, Daniel Vetter wrote:
On Wed, Apr 18, 2012 at 01:31:59PM +0900, Joonyoung Shim wrote:
DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl can change the framebuffer of plane but user can't know completion of changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is added, we can also do pageflip of a plane.
Signed-off-by: Joonyoung Shimjy0922.shim@samsung.com Signed-off-by: Kyungmin Parkkyungmin.park@samsung.com
If I understand the current kms api correctly, set_plane is akin to set_base and should not generate an asynchronous flip completion event. To do that we need a new pageflip ioctl which changes a complete set of fb + planes + any crtc attributes that might be in an atomic fashion. At which point we can just reuse the existing page flip event mechanism.
It seems better way to add new pageflip ioctl for plane. I will try it.
Thanks for review.
On Wed, Apr 18, 2012 at 5:11 AM, Joonyoung Shim jy0922.shim@samsung.com wrote:
On 04/18/2012 05:46 PM, Daniel Vetter wrote:
On Wed, Apr 18, 2012 at 01:31:59PM +0900, Joonyoung Shim wrote:
DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl can change the framebuffer of plane but user can't know completion of changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is added, we can also do pageflip of a plane.
Signed-off-by: Joonyoung Shimjy0922.shim@samsung.com Signed-off-by: Kyungmin Parkkyungmin.park@samsung.com
If I understand the current kms api correctly, set_plane is akin to set_base and should not generate an asynchronous flip completion event. To do that we need a new pageflip ioctl which changes a complete set of fb + planes + any crtc attributes that might be in an atomic fashion. At which point we can just reuse the existing page flip event mechanism.
It seems better way to add new pageflip ioctl for plane. I will try it.
fwiw, an atomic mode set which can update crtc and zero or more plane layers is, I think, the way to go. Jesse Barnes had an RFC for this, although IIRC it was only the API and not the implementation. And combination w/ the plane/crtc properties patchset to allow setting properties as part of the update might not be a bad thing either.
BR, -R
Thanks for review.
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 04/18/2012 02:25 PM, Rob Clark wrote:
On Wed, Apr 18, 2012 at 5:11 AM, Joonyoung Shimjy0922.shim@samsung.com wrote:
On 04/18/2012 05:46 PM, Daniel Vetter wrote:
On Wed, Apr 18, 2012 at 01:31:59PM +0900, Joonyoung Shim wrote:
DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl can change the framebuffer of plane but user can't know completion of changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is added, we can also do pageflip of a plane.
Signed-off-by: Joonyoung Shimjy0922.shim@samsung.com Signed-off-by: Kyungmin Parkkyungmin.park@samsung.com
If I understand the current kms api correctly, set_plane is akin to set_base and should not generate an asynchronous flip completion event. To do that we need a new pageflip ioctl which changes a complete set of fb + planes + any crtc attributes that might be in an atomic fashion. At which point we can just reuse the existing page flip event mechanism.
It seems better way to add new pageflip ioctl for plane. I will try it.
fwiw, an atomic mode set which can update crtc and zero or more plane layers is, I think, the way to go. Jesse Barnes had an RFC for this, although IIRC it was only the API and not the implementation. And combination w/ the plane/crtc properties patchset to allow setting properties as part of the update might not be a bad thing either.
BR, -R
Before planes and rotation properties modeset was atomic (single ioctl). Would it not be possible to define atomic modeset for planes and properties like this?
SETPROPERTY and SETPLANE (maybe more) should not be commited when set, only on SETCRTC ioctl like before planes. All properties and plane changes before SETCRTC should be considered staged settings for the next SETCRTC. If this would break API backwards compatibility, maybe a new "standard" boolean property called EnableAtomicMode could be used to trigger this mode in the drivers. If a driver doesn't support it, things will just work as currently with modeset not being atomic across planes. Maybe this could be implemented in the generic parts of KMS, but it could be tried out first inside a driver.
/BR /Marcus
On Wed, Apr 18, 2012 at 04:04:56PM +0200, Marcus Lorentzon wrote:
On 04/18/2012 02:25 PM, Rob Clark wrote:
On Wed, Apr 18, 2012 at 5:11 AM, Joonyoung Shimjy0922.shim@samsung.com wrote:
On 04/18/2012 05:46 PM, Daniel Vetter wrote:
On Wed, Apr 18, 2012 at 01:31:59PM +0900, Joonyoung Shim wrote:
DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl can change the framebuffer of plane but user can't know completion of changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is added, we can also do pageflip of a plane.
Signed-off-by: Joonyoung Shimjy0922.shim@samsung.com Signed-off-by: Kyungmin Parkkyungmin.park@samsung.com
If I understand the current kms api correctly, set_plane is akin to set_base and should not generate an asynchronous flip completion event. To do that we need a new pageflip ioctl which changes a complete set of fb + planes + any crtc attributes that might be in an atomic fashion. At which point we can just reuse the existing page flip event mechanism.
It seems better way to add new pageflip ioctl for plane. I will try it.
fwiw, an atomic mode set which can update crtc and zero or more plane layers is, I think, the way to go. Jesse Barnes had an RFC for this, although IIRC it was only the API and not the implementation. And combination w/ the plane/crtc properties patchset to allow setting properties as part of the update might not be a bad thing either.
BR, -R
Before planes and rotation properties modeset was atomic (single ioctl). Would it not be possible to define atomic modeset for planes and properties like this?
SETPROPERTY and SETPLANE (maybe more) should not be commited when set, only on SETCRTC ioctl like before planes. All properties and plane changes before SETCRTC should be considered staged settings for the next SETCRTC. If this would break API backwards compatibility, maybe a new "standard" boolean property called EnableAtomicMode could be used to trigger this mode in the drivers. If a driver doesn't support it, things will just work as currently with modeset not being atomic across planes. Maybe this could be implemented in the generic parts of KMS, but it could be tried out first inside a driver.
Multi ioctl solution can have some issues since you can't hold any locks around the whole operation. Also there could be issues if the process performing the operation dies or hangs in mid operation. Error handling can also be difficult since the intermediate steps may violate some constraints in the system.
Also SETCRTC itself is a per-CRTC operation, but we actually want per-device atomic operations instead.
So I think a single ioctl solution is a better idea. I'm currently pondering on what the API would look like. Ideally I would like to go with the "everything is a property" approach, and I'd like to get rid of some of the other limitations in the current API at the same time.
On Wed, Apr 18, 2012 at 05:26:07PM +0300, Ville Syrjälä wrote:
On Wed, Apr 18, 2012 at 04:04:56PM +0200, Marcus Lorentzon wrote:
On 04/18/2012 02:25 PM, Rob Clark wrote:
On Wed, Apr 18, 2012 at 5:11 AM, Joonyoung Shimjy0922.shim@samsung.com wrote:
On 04/18/2012 05:46 PM, Daniel Vetter wrote:
On Wed, Apr 18, 2012 at 01:31:59PM +0900, Joonyoung Shim wrote:
DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl can change the framebuffer of plane but user can't know completion of changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is added, we can also do pageflip of a plane.
Signed-off-by: Joonyoung Shimjy0922.shim@samsung.com Signed-off-by: Kyungmin Parkkyungmin.park@samsung.com
If I understand the current kms api correctly, set_plane is akin to set_base and should not generate an asynchronous flip completion event. To do that we need a new pageflip ioctl which changes a complete set of fb + planes + any crtc attributes that might be in an atomic fashion. At which point we can just reuse the existing page flip event mechanism.
It seems better way to add new pageflip ioctl for plane. I will try it.
fwiw, an atomic mode set which can update crtc and zero or more plane layers is, I think, the way to go. Jesse Barnes had an RFC for this, although IIRC it was only the API and not the implementation. And combination w/ the plane/crtc properties patchset to allow setting properties as part of the update might not be a bad thing either.
BR, -R
Before planes and rotation properties modeset was atomic (single ioctl). Would it not be possible to define atomic modeset for planes and properties like this?
SETPROPERTY and SETPLANE (maybe more) should not be commited when set, only on SETCRTC ioctl like before planes. All properties and plane changes before SETCRTC should be considered staged settings for the next SETCRTC. If this would break API backwards compatibility, maybe a new "standard" boolean property called EnableAtomicMode could be used to trigger this mode in the drivers. If a driver doesn't support it, things will just work as currently with modeset not being atomic across planes. Maybe this could be implemented in the generic parts of KMS, but it could be tried out first inside a driver.
Multi ioctl solution can have some issues since you can't hold any locks around the whole operation. Also there could be issues if the process performing the operation dies or hangs in mid operation. Error handling can also be difficult since the intermediate steps may violate some constraints in the system.
Also SETCRTC itself is a per-CRTC operation, but we actually want per-device atomic operations instead.
So I think a single ioctl solution is a better idea. I'm currently pondering on what the API would look like. Ideally I would like to go with the "everything is a property" approach, and I'd like to get rid of some of the other limitations in the current API at the same time.
Last time around I've discussed with people we've ended up with 2 new ioctls:
- atomic modeset, to configure the output state on more than one crtc at the same time. This is useful to get pll assignement, memory bandwidth constrains and similar stuff right. This ioctl is synchronous. A testmode can be used to inquire the driver whether the proposed mode actually works. This could be used for gui interfaces to automatically grey out unsupportable configurations, e.g. if you have 3 crtc but on 2 pll and 2 modes need to have the same pixelclocks to drive all 3 crtcs.
- an atomic pageflip. This one would be like the current pageflip ioclt, i.e. run asynchronously and deliver a drm event upont completion. The idea is that compositors can use this to make flicker-free compositition with drm planes possible. I think we want drivers to be able to indicate which crtc properties they can switch with this ioctl, e.g. I expect some yuv->rbg color space properties might not work. All the changes should be applied on the same vblank, obviously.
But Jesse and Kristian have been more active in this discussion, cc'ed them.
Yours, Daniel
On 04/18/2012 04:36 PM, Daniel Vetter wrote:
Last time around I've discussed with people we've ended up with 2 new ioctls:
atomic modeset, to configure the output state on more than one crtc at the same time. This is useful to get pll assignement, memory bandwidth constrains and similar stuff right. This ioctl is synchronous. A testmode can be used to inquire the driver whether the proposed mode actually works. This could be used for gui interfaces to automatically grey out unsupportable configurations, e.g. if you have 3 crtc but on 2 pll and 2 modes need to have the same pixelclocks to drive all 3 crtcs.
an atomic pageflip. This one would be like the current pageflip ioclt, i.e. run asynchronously and deliver a drm event upont completion. The idea is that compositors can use this to make flicker-free compositition with drm planes possible. I think we want drivers to be able to indicate which crtc properties they can switch with this ioctl, e.g. I expect some yuv->rbg color space properties might not work. All the changes should be applied on the same vblank, obviously.
Why an atomic pagefilp? How is this different from an atomic modeset where only fbs change? Can't drm frmwrk "optimize" this like SETCRTC does today with base/full modeset helpers?
/BR /Marcus
On Wed, Apr 18, 2012 at 05:10:47PM +0200, Marcus Lorentzon wrote:
On 04/18/2012 04:36 PM, Daniel Vetter wrote:
Last time around I've discussed with people we've ended up with 2 new ioctls:
atomic modeset, to configure the output state on more than one crtc at the same time. This is useful to get pll assignement, memory bandwidth constrains and similar stuff right. This ioctl is synchronous. A testmode can be used to inquire the driver whether the proposed mode actually works. This could be used for gui interfaces to automatically grey out unsupportable configurations, e.g. if you have 3 crtc but on 2 pll and 2 modes need to have the same pixelclocks to drive all 3 crtcs.
an atomic pageflip. This one would be like the current pageflip ioclt, i.e. run asynchronously and deliver a drm event upont completion. The idea is that compositors can use this to make flicker-free compositition with drm planes possible. I think we want drivers to be able to indicate which crtc properties they can switch with this ioctl, e.g. I expect some yuv->rbg color space properties might not work. All the changes should be applied on the same vblank, obviously.
Why an atomic pagefilp? How is this different from an atomic modeset where only fbs change? Can't drm frmwrk "optimize" this like SETCRTC does today with base/full modeset helpers?
The important difference is that the pageflip should happen vsynced on one single crtc. So it can't do anything which takes longer than a vsync (usually you need to wait a frame for the clocks to stabilize before switching on the next stage in the output pipeline), so any output configuration changes are pretty much out of the window. We also want pageflip to run asynchronously and return immediately after emitting all relevant commands. That's not really important for modeset.
So yeah, you could smash all this into one giant ioctl, but I think the split is useful and would simplify things quite a bit on the implementation side. Otherwise you need to add funny queries so that userspace can figure out which modeset ops run asynchronous, which can be vblank synced. And it needs to tell the kernel for which it wants an event. Especially when you have multiple crtc and want to drive all of them with a compositor, this can get funny.
Also, I'm toying around with ideas to split up the big modeset lock such that operations that only touch the crtc (like pageflip, plane changes and cursor changes) do not take the big modeset lock but only a per-crtc mutex. This way a compositor running on crtc A could run without hiccups while we do a modeset operation on crtc B, or while a hotplug detection is reading back the edid from a unused connector (which can easily take longer than a few frames). We have tons of bug reports from users that complain that every 10s their cursor stalls (due to hotplug detect).
If you smash everything into one ioctl, I imagine you have plenty of fun implementing all this. Which is why I prefer to split this up. -Daniel
On 04/18/2012 05:27 PM, Daniel Vetter wrote:
On Wed, Apr 18, 2012 at 05:10:47PM +0200, Marcus Lorentzon wrote:
On 04/18/2012 04:36 PM, Daniel Vetter wrote:
Last time around I've discussed with people we've ended up with 2 new ioctls:
atomic modeset, to configure the output state on more than one crtc at the same time. This is useful to get pll assignement, memory bandwidth constrains and similar stuff right. This ioctl is synchronous. A testmode can be used to inquire the driver whether the proposed mode actually works. This could be used for gui interfaces to automatically grey out unsupportable configurations, e.g. if you have 3 crtc but on 2 pll and 2 modes need to have the same pixelclocks to drive all 3 crtcs.
an atomic pageflip. This one would be like the current pageflip ioclt, i.e. run asynchronously and deliver a drm event upont completion. The idea is that compositors can use this to make flicker-free compositition with drm planes possible. I think we want drivers to be able to indicate which crtc properties they can switch with this ioctl, e.g. I expect some yuv->rbg color space properties might not work. All the changes should be applied on the same vblank, obviously.
Why an atomic pagefilp? How is this different from an atomic modeset where only fbs change? Can't drm frmwrk "optimize" this like SETCRTC does today with base/full modeset helpers?
The important difference is that the pageflip should happen vsynced on one single crtc. So it can't do anything which takes longer than a vsync (usually you need to wait a frame for the clocks to stabilize before switching on the next stage in the output pipeline), so any output configuration changes are pretty much out of the window. We also want pageflip to run asynchronously and return immediately after emitting all relevant commands. That's not really important for modeset.
So yeah, you could smash all this into one giant ioctl, but I think the split is useful and would simplify things quite a bit on the implementation side. Otherwise you need to add funny queries so that userspace can figure out which modeset ops run asynchronous, which can be vblank synced. And it needs to tell the kernel for which it wants an event. Especially when you have multiple crtc and want to drive all of them with a compositor, this can get funny.
Also, I'm toying around with ideas to split up the big modeset lock such that operations that only touch the crtc (like pageflip, plane changes and cursor changes) do not take the big modeset lock but only a per-crtc mutex. This way a compositor running on crtc A could run without hiccups while we do a modeset operation on crtc B, or while a hotplug detection is reading back the edid from a unused connector (which can easily take longer than a few frames). We have tons of bug reports from users that complain that every 10s their cursor stalls (due to hotplug detect).
If you smash everything into one ioctl, I imagine you have plenty of fun implementing all this. Which is why I prefer to split this up. -Daniel
The async vs sync makes sense as reason for splitting them. My problem lies somewhere in between sync modeset and async flip - async crtc/plane/fbs modeset. In Wayland and Android HW composer I need to asynchronously flip and do crtc/plane modeset, but no connector/crtc modeset (so it is a fast operation). Because I don't consider enable/disable/move a plane to be a full synchronous modeset (same mode different fbs/planes). But I still want the same async events to tell me the new plane setup is activated at vsync. But as you say, maybe the biggest issue here is the "big drm lock". So maybe user space would be ok to do this crtc-modeset in sync mode, if it doesn't block other operations on other crtcs. But I would prefer to be able to do the crtc modeset async so I don't have to have a thread per crtc. Or am I missing the obvious solution to this?
/BR /Marcus
On Wed, 18 Apr 2012 17:55:15 +0200 Marcus Lorentzon marcus.xm.lorentzon@stericsson.com wrote:
The async vs sync makes sense as reason for splitting them. My problem lies somewhere in between sync modeset and async flip - async crtc/plane/fbs modeset. In Wayland and Android HW composer I need to asynchronously flip and do crtc/plane modeset, but no connector/crtc modeset (so it is a fast operation). Because I don't consider enable/disable/move a plane to be a full synchronous modeset (same mode different fbs/planes). But I still want the same async events to tell me the new plane setup is activated at vsync. But as you say, maybe the biggest issue here is the "big drm lock". So maybe user space would be ok to do this crtc-modeset in sync mode, if it doesn't block other operations on other crtcs. But I would prefer to be able to do the crtc modeset async so I don't have to have a thread per crtc. Or am I missing the obvious solution to this?
I don't think you're missing anything here; the obvious solution is the nuclear page flip. It's what I always envisioned would be needed once we had the basic sprite support in place. Basically we need a new page flip ioctl (which is async and gives you events) but that takes multiple planes as args, along with ancillary data.
Neither setcrtc nor setplane are the right place to put this. Neither take all the info we want, and historically setcrtc didn't emit an event, so I didn't add it to setplane (it would be of limited usefulness anyway since we really want to flip primary + sprite at the same time).
On Wed, Apr 18, 2012 at 05:55:15PM +0200, Marcus Lorentzon wrote:
On 04/18/2012 05:27 PM, Daniel Vetter wrote:
On Wed, Apr 18, 2012 at 05:10:47PM +0200, Marcus Lorentzon wrote:
On 04/18/2012 04:36 PM, Daniel Vetter wrote:
Last time around I've discussed with people we've ended up with 2 new ioctls:
atomic modeset, to configure the output state on more than one crtc at the same time. This is useful to get pll assignement, memory bandwidth constrains and similar stuff right. This ioctl is synchronous. A testmode can be used to inquire the driver whether the proposed mode actually works. This could be used for gui interfaces to automatically grey out unsupportable configurations, e.g. if you have 3 crtc but on 2 pll and 2 modes need to have the same pixelclocks to drive all 3 crtcs.
an atomic pageflip. This one would be like the current pageflip ioclt, i.e. run asynchronously and deliver a drm event upont completion. The idea is that compositors can use this to make flicker-free compositition with drm planes possible. I think we want drivers to be able to indicate which crtc properties they can switch with this ioctl, e.g. I expect some yuv->rbg color space properties might not work. All the changes should be applied on the same vblank, obviously.
Why an atomic pagefilp? How is this different from an atomic modeset where only fbs change? Can't drm frmwrk "optimize" this like SETCRTC does today with base/full modeset helpers?
The important difference is that the pageflip should happen vsynced on one single crtc. So it can't do anything which takes longer than a vsync (usually you need to wait a frame for the clocks to stabilize before switching on the next stage in the output pipeline), so any output configuration changes are pretty much out of the window. We also want pageflip to run asynchronously and return immediately after emitting all relevant commands. That's not really important for modeset.
So yeah, you could smash all this into one giant ioctl, but I think the split is useful and would simplify things quite a bit on the implementation side. Otherwise you need to add funny queries so that userspace can figure out which modeset ops run asynchronous, which can be vblank synced. And it needs to tell the kernel for which it wants an event. Especially when you have multiple crtc and want to drive all of them with a compositor, this can get funny.
Also, I'm toying around with ideas to split up the big modeset lock such that operations that only touch the crtc (like pageflip, plane changes and cursor changes) do not take the big modeset lock but only a per-crtc mutex. This way a compositor running on crtc A could run without hiccups while we do a modeset operation on crtc B, or while a hotplug detection is reading back the edid from a unused connector (which can easily take longer than a few frames). We have tons of bug reports from users that complain that every 10s their cursor stalls (due to hotplug detect).
If you smash everything into one ioctl, I imagine you have plenty of fun implementing all this. Which is why I prefer to split this up. -Daniel
The async vs sync makes sense as reason for splitting them. My problem lies somewhere in between sync modeset and async flip - async crtc/plane/fbs modeset. In Wayland and Android HW composer I need to asynchronously flip and do crtc/plane modeset, but no connector/crtc modeset (so it is a fast operation). Because I don't consider enable/disable/move a plane to be a full synchronous modeset (same mode different fbs/planes). But I still want the same async events to tell me the new plane setup is activated at vsync. But as you say, maybe the biggest issue here is the "big drm lock". So maybe user space would be ok to do this crtc-modeset in sync mode, if it doesn't block other operations on other crtcs. But I would prefer to be able to do the crtc modeset async so I don't have to have a thread per crtc. Or am I missing the obvious solution to this?
Changing moving planes would be part of the big new async monster pageflip. Generally everything that the hw can change vblank-synced and asynchronously up to the logical point where the pixeldata is generated. Ans yes, Wayland/SF is exactly the use-case for this, so that you can dynamically switch surfaces to be rendered with planes, all async and properly vblank-synced. -Daniel
On Wed, Apr 18, 2012 at 05:27:57PM +0200, Daniel Vetter wrote:
On Wed, Apr 18, 2012 at 05:10:47PM +0200, Marcus Lorentzon wrote:
On 04/18/2012 04:36 PM, Daniel Vetter wrote:
Last time around I've discussed with people we've ended up with 2 new ioctls:
atomic modeset, to configure the output state on more than one crtc at the same time. This is useful to get pll assignement, memory bandwidth constrains and similar stuff right. This ioctl is synchronous. A testmode can be used to inquire the driver whether the proposed mode actually works. This could be used for gui interfaces to automatically grey out unsupportable configurations, e.g. if you have 3 crtc but on 2 pll and 2 modes need to have the same pixelclocks to drive all 3 crtcs.
an atomic pageflip. This one would be like the current pageflip ioclt, i.e. run asynchronously and deliver a drm event upont completion. The idea is that compositors can use this to make flicker-free compositition with drm planes possible. I think we want drivers to be able to indicate which crtc properties they can switch with this ioctl, e.g. I expect some yuv->rbg color space properties might not work. All the changes should be applied on the same vblank, obviously.
Why an atomic pagefilp? How is this different from an atomic modeset where only fbs change? Can't drm frmwrk "optimize" this like SETCRTC does today with base/full modeset helpers?
The important difference is that the pageflip should happen vsynced on one single crtc. So it can't do anything which takes longer than a vsync (usually you need to wait a frame for the clocks to stabilize before switching on the next stage in the output pipeline), so any output configuration changes are pretty much out of the window. We also want pageflip to run asynchronously and return immediately after emitting all relevant commands. That's not really important for modeset.
Whether something can happen immediately or needs some multi frame sychronization steps may depend on the hardware. Also the split in this case is rather vague. You most likely want to change a lot of other state in addition to just flipping buffers.
So yeah, you could smash all this into one giant ioctl, but I think the split is useful and would simplify things quite a bit on the implementation side. Otherwise you need to add funny queries so that userspace can figure out which modeset ops run asynchronous, which can be vblank synced. And it needs to tell the kernel for which it wants an event. Especially when you have multiple crtc and want to drive all of them with a compositor, this can get funny.
Just ask for the event always, and if the driver decides to block during the ioctl, so be it.
That being said it would be nice to be able to pipeline all mode setting operations. The driver could implement some of that using a kernel thread if it has to perform blocking operations, or if you don't want to implement a state machine into the driver.
Also, I'm toying around with ideas to split up the big modeset lock such that operations that only touch the crtc (like pageflip, plane changes and cursor changes) do not take the big modeset lock but only a per-crtc mutex.
Plane operations might well involve multiple CRTCs (when moving the planes between pipes for example). You have to be careful with the locking order when doing stuff like that. One option would be to always take the per-CRTC locks in the same order (based on the object ID for example).
This way a compositor running on crtc A could run without hiccups while we do a modeset operation on crtc B, or while a hotplug detection is reading back the edid from a unused connector (which can easily take longer than a few frames). We have tons of bug reports from users that complain that every 10s their cursor stalls (due to hotplug detect).
I think the EDID stuff should be split completely from other state. I have one interesting bit of hardware at home where reading an EDID block took over 30 seconds. The device is structured like this: GPIO pins <-> bitbanged i2c <-> GPIO expander <-> bitbanged i2c <-> EDID
If I would like to use that device in any sane way, I'd really need to read the EDID from a kernel thread and possibly also come up with some way to avoid blocking access to encoders accessed through the same GPIO pins.
Obviously the 30 second wait for the EDID after plugging in a display would still be a major PITA.
If you smash everything into one ioctl, I imagine you have plenty of fun implementing all this. Which is why I prefer to split this up.
I don't think there's that much differnce. You build up the full device state, check it, and when you're ready to commit it you decide whether to go with the blocking approach or not.
On 04/18/2012 06:06 PM, Ville Syrjälä wrote:
If you smash everything into one ioctl, I imagine you have plenty of fun implementing all this. Which is why I prefer to split this up.
I don't think there's that much differnce. You build up the full device state, check it, and when you're ready to commit it you decide whether to go with the blocking approach or not.
Yes, this is exactly what I do in our driver today. It doesn't cost much CPU to do it. Just copying a few values to a device state struct and verifying it is ok on commit. Then just wait for vsync and apply. All "heavy" calculations are done before vsync. If modeset come late (just before vsync) it could just as easily have come just after, so it makes no difference. Plane/fb/crtc modeset should be expected to come before vsync since most rendering is async today. So user is probably spending most time waiting for vsync of previous modeset/flip and will issue the next as soon as the previous is complete. And the app rendering complexity probably outweighs the state tracking CPU load by far.
And I do like the idea of one single modeset ioctl that is async and atomic. I think this could make things simpler, not more complex. Having to support multiple paths depending on what ioctl is used by an app doesn't seem much easier (already 3 helper callbacks - base, full, flip). But I don't have the solid experience with drm frmwrk to make that decision.
/Marcus
On Wed, Apr 18, 2012 at 07:06:10PM +0300, Ville Syrjälä wrote:
On Wed, Apr 18, 2012 at 05:27:57PM +0200, Daniel Vetter wrote:
Also, I'm toying around with ideas to split up the big modeset lock such that operations that only touch the crtc (like pageflip, plane changes and cursor changes) do not take the big modeset lock but only a per-crtc mutex.
Plane operations might well involve multiple CRTCs (when moving the planes between pipes for example). You have to be careful with the locking order when doing stuff like that. One option would be to always take the per-CRTC locks in the same order (based on the object ID for example).
Multiple locks will also cause problems for the atomic mode set. The full device state may be needed to evaluate whether a certain change is allowed, which means any change happening in parallel can screw things up.
On Wed, Apr 18, 2012 at 09:19:42PM +0300, Ville Syrjälä wrote:
On Wed, Apr 18, 2012 at 07:06:10PM +0300, Ville Syrjälä wrote:
On Wed, Apr 18, 2012 at 05:27:57PM +0200, Daniel Vetter wrote:
Also, I'm toying around with ideas to split up the big modeset lock such that operations that only touch the crtc (like pageflip, plane changes and cursor changes) do not take the big modeset lock but only a per-crtc mutex.
Plane operations might well involve multiple CRTCs (when moving the planes between pipes for example). You have to be careful with the locking order when doing stuff like that. One option would be to always take the per-CRTC locks in the same order (based on the object ID for example).
Multiple locks will also cause problems for the atomic mode set. The full device state may be needed to evaluate whether a certain change is allowed, which means any change happening in parallel can screw things up.
Imo we can simply demand that any operation which touches more than 1 crtc needs the big modeset lock. So atomic modeset would still take this look, but modeset is a really rare operation, so no point to optimize for it. The only issue I see is with switching a sprite from one crtc to another, with that design this would also require the big modeset look. But Kristian just brought up the idea of a prepare_sprite ioctl, so we could do the crtc switch there, and userspace could run this ioctl asynchronous in a separate thread to avoid stalls (e.g. on the old gen2 intel overlay a crtc switch takes 2 full vblanks to disable and 1 full vblank to set up on the new crtc, then another vblank to show anything). The nuclear pageflip would then only touch one single crtc, so never get stalled by other modeset operations. -Daniel
On Wed, Apr 18, 2012 at 05:10:47PM +0200, Marcus Lorentzon wrote:
On 04/18/2012 04:36 PM, Daniel Vetter wrote:
Last time around I've discussed with people we've ended up with 2 new ioctls:
atomic modeset, to configure the output state on more than one crtc at the same time. This is useful to get pll assignement, memory bandwidth constrains and similar stuff right. This ioctl is synchronous. A testmode can be used to inquire the driver whether the proposed mode actually works. This could be used for gui interfaces to automatically grey out unsupportable configurations, e.g. if you have 3 crtc but on 2 pll and 2 modes need to have the same pixelclocks to drive all 3 crtcs.
an atomic pageflip. This one would be like the current pageflip ioclt, i.e. run asynchronously and deliver a drm event upont completion. The idea is that compositors can use this to make flicker-free compositition with drm planes possible. I think we want drivers to be able to indicate which crtc properties they can switch with this ioctl, e.g. I expect some yuv->rbg color space properties might not work. All the changes should be applied on the same vblank, obviously.
Why an atomic pagefilp? How is this different from an atomic modeset where only fbs change? Can't drm frmwrk "optimize" this like SETCRTC does today with base/full modeset helpers?
Buffering. When you are doing triple buffering and need to update addresses, which should be flipped at next vblank, you do not want to go through a massive amount of state handling.
I do not agree with SETCRTC though.
The CRTC is the entity that serializes, that has the actual modeline set, that might allow rotation and/or scaling all of the planes and cursors (to the extent that these are not planes) that are put into it.
Planes are the actual FB and possible overlays. It is the planes that do colour conversion and whatnot (pitch), might allow for z-ordering, individual scaling and rotation, and which might not need to be full screen. It is these elements that take in buffer addresses and should do page flipping. You might have to artificially split out the base plane and CRTC for your hardware, and have some code duplication or multiple uses of the same functions.
Now look at the relative probability of each of the actions: * The frequency of altering the mode (on the crtc) is very low. * The frequency of altering the planes is/should already a lot higher when you are using the likes of wayland or the android hw compositor. * The frequency of page flipping is very high.
This means that there should be (at least) three separate actions: * page flipping: buffers -> planes at next vblank, for a list of buffer(s) and plane tuples. * setplanes: colour format, position, scaling, ordering, rotation, color key, crtc adherance, _plus_ the above. * actual modeset, which handles the CRTC and out to the monitor and whatnot, which should also include the above.
With appropriate helper functions combining these events should not be hard as you move down the list.
As someone who has spent quite a lot of time with Xvideo in the past (and pretty much the only guy still around who wrote up ReputImage support for his graphics driver), i am glad that we have buffer management today, and we should use that to the max and cut down on duplicated and errorprone state tracking.
Luc Verhaegen.
On 04/18/2012 05:43 PM, Luc Verhaegen wrote:
This means that there should be (at least) three separate actions:
- page flipping: buffers -> planes at next vblank, for a list of
buffer(s) and plane tuples.
- setplanes: colour format, position, scaling, ordering, rotation, color
key, crtc adherance,_plus_ the above.
- actual modeset, which handles the CRTC and out to the monitor and
whatnot, which should also include the above.
The setplanes is exactly the piece I'm missing today ... and it should be async as flip.
/Marcus
On Wed, Apr 18, 2012 at 10:36 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Apr 18, 2012 at 05:26:07PM +0300, Ville Syrjälä wrote:
On Wed, Apr 18, 2012 at 04:04:56PM +0200, Marcus Lorentzon wrote:
On 04/18/2012 02:25 PM, Rob Clark wrote:
On Wed, Apr 18, 2012 at 5:11 AM, Joonyoung Shimjy0922.shim@samsung.com wrote:
On 04/18/2012 05:46 PM, Daniel Vetter wrote:
On Wed, Apr 18, 2012 at 01:31:59PM +0900, Joonyoung Shim wrote: > DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is > for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to > provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl > can change the framebuffer of plane but user can't know completion of > changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is > added, we can also do pageflip of a plane. > > Signed-off-by: Joonyoung Shimjy0922.shim@samsung.com > Signed-off-by: Kyungmin Parkkyungmin.park@samsung.com If I understand the current kms api correctly, set_plane is akin to set_base and should not generate an asynchronous flip completion event. To do that we need a new pageflip ioctl which changes a complete set of fb + planes + any crtc attributes that might be in an atomic fashion. At which point we can just reuse the existing page flip event mechanism.
It seems better way to add new pageflip ioctl for plane. I will try it.
fwiw, an atomic mode set which can update crtc and zero or more plane layers is, I think, the way to go. Jesse Barnes had an RFC for this, although IIRC it was only the API and not the implementation. And combination w/ the plane/crtc properties patchset to allow setting properties as part of the update might not be a bad thing either.
BR, -R
Before planes and rotation properties modeset was atomic (single ioctl). Would it not be possible to define atomic modeset for planes and properties like this?
SETPROPERTY and SETPLANE (maybe more) should not be commited when set, only on SETCRTC ioctl like before planes. All properties and plane changes before SETCRTC should be considered staged settings for the next SETCRTC. If this would break API backwards compatibility, maybe a new "standard" boolean property called EnableAtomicMode could be used to trigger this mode in the drivers. If a driver doesn't support it, things will just work as currently with modeset not being atomic across planes. Maybe this could be implemented in the generic parts of KMS, but it could be tried out first inside a driver.
Multi ioctl solution can have some issues since you can't hold any locks around the whole operation. Also there could be issues if the process performing the operation dies or hangs in mid operation. Error handling can also be difficult since the intermediate steps may violate some constraints in the system.
Also SETCRTC itself is a per-CRTC operation, but we actually want per-device atomic operations instead.
So I think a single ioctl solution is a better idea. I'm currently pondering on what the API would look like. Ideally I would like to go with the "everything is a property" approach, and I'd like to get rid of some of the other limitations in the current API at the same time.
Last time around I've discussed with people we've ended up with 2 new ioctls:
- atomic modeset, to configure the output state on more than one crtc at
the same time. This is useful to get pll assignement, memory bandwidth constrains and similar stuff right. This ioctl is synchronous. A testmode can be used to inquire the driver whether the proposed mode actually works. This could be used for gui interfaces to automatically grey out unsupportable configurations, e.g. if you have 3 crtc but on 2 pll and 2 modes need to have the same pixelclocks to drive all 3 crtcs.
- an atomic pageflip. This one would be like the current pageflip ioclt,
i.e. run asynchronously and deliver a drm event upont completion. The idea is that compositors can use this to make flicker-free compositition with drm planes possible. I think we want drivers to be able to indicate which crtc properties they can switch with this ioctl, e.g. I expect some yuv->rbg color space properties might not work. All the changes should be applied on the same vblank, obviously.
But Jesse and Kristian have been more active in this discussion, cc'ed them.
Yeah, that sums it up pretty well. The atomic modeset allows KMS to allocate resources (bandwidth, connectors, encoders etc) across all crtcs may take a while to complete. The nuclear pageflip only affects one crtc at a time, but ideally should let us change all properties that don't affect bandwidth or timing parameters: fb/overlay/sprite/hw cursor base addresses, overlay/sprite/hw cursor position, scaling properties.
One thing that's not clear to me is how we would enable a sprite without going through the atomic modeset again. If the atomic modeset is all about calculating bandwidth requirements etc, then enabling a sprite will affect that and it may or may not be possible based on the current configuration. However, enabling a sprite from one frame to another is something that we would want to do as part of the nuclear pageflip. So I'm not sure how this would work... maybe we could have a prepare_sprite_enable type ioctl that verifies that it's possible to add the sprite plane to the current configuration. Then if that succeeds, we can use the nuclear pageflip to enable the sprite plane.
Kristian
On Wed, Apr 18, 2012 at 09:24:58PM -0400, Kristian Høgsberg wrote:
One thing that's not clear to me is how we would enable a sprite without going through the atomic modeset again. If the atomic modeset is all about calculating bandwidth requirements etc, then enabling a sprite will affect that and it may or may not be possible based on the current configuration. However, enabling a sprite from one frame to another is something that we would want to do as part of the nuclear pageflip. So I'm not sure how this would work... maybe we could have a prepare_sprite_enable type ioctl that verifies that it's possible to add the sprite plane to the current configuration. Then if that succeeds, we can use the nuclear pageflip to enable the sprite plane.
Sprites also have an ugly issue wrt finer-grained locking: If we move to per-crtc locking, the nuclear pageflip would only need to take the crtc mutex it operates on. But the if we move a sprite from one crtc to another one, we'd need to lock both crtcs, and the easiest solution for the lock ordering problem this causes is to require that all code which needs more than one crtc look needs the big modeset lock. Which would be painful for pageflip.
Also, most sprites/overlays can't be switched in one step from one crtc to another, so these pageflips would run synchronous. Which is not the point of pageflipping. So I think a prepare_sprite ioctl which changes the crtc association (and checks memory bandwidth and other constrains) but does not display anything would be good to support the nuclear pageflip without jumping through too many hoops in the kernel. Nuclear pageflip would need to support pageflippping from/to NULL framebuffers on planes then to signal switching a plane on/off. Maybe we could even do that for the main crtc fb (and add a background color), e.g. for video watch to avoid scanning out black bars. -Daniel
On 04/18/2012 04:26 PM, Ville Syrjälä wrote:
On Wed, Apr 18, 2012 at 04:04:56PM +0200, Marcus Lorentzon wrote:
On 04/18/2012 02:25 PM, Rob Clark wrote:
On Wed, Apr 18, 2012 at 5:11 AM, Joonyoung Shimjy0922.shim@samsung.com wrote:
On 04/18/2012 05:46 PM, Daniel Vetter wrote:
On Wed, Apr 18, 2012 at 01:31:59PM +0900, Joonyoung Shim wrote:
DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl can change the framebuffer of plane but user can't know completion of changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is added, we can also do pageflip of a plane.
Signed-off-by: Joonyoung Shimjy0922.shim@samsung.com Signed-off-by: Kyungmin Parkkyungmin.park@samsung.com
If I understand the current kms api correctly, set_plane is akin to set_base and should not generate an asynchronous flip completion event. To do that we need a new pageflip ioctl which changes a complete set of fb + planes + any crtc attributes that might be in an atomic fashion. At which point we can just reuse the existing page flip event mechanism.
It seems better way to add new pageflip ioctl for plane. I will try it.
fwiw, an atomic mode set which can update crtc and zero or more plane layers is, I think, the way to go. Jesse Barnes had an RFC for this, although IIRC it was only the API and not the implementation. And combination w/ the plane/crtc properties patchset to allow setting properties as part of the update might not be a bad thing either.
BR, -R
Before planes and rotation properties modeset was atomic (single ioctl). Would it not be possible to define atomic modeset for planes and properties like this?
SETPROPERTY and SETPLANE (maybe more) should not be commited when set, only on SETCRTC ioctl like before planes. All properties and plane changes before SETCRTC should be considered staged settings for the next SETCRTC. If this would break API backwards compatibility, maybe a new "standard" boolean property called EnableAtomicMode could be used to trigger this mode in the drivers. If a driver doesn't support it, things will just work as currently with modeset not being atomic across planes. Maybe this could be implemented in the generic parts of KMS, but it could be tried out first inside a driver.
Multi ioctl solution can have some issues since you can't hold any locks around the whole operation. Also there could be issues if the process performing the operation dies or hangs in mid operation. Error handling can also be difficult since the intermediate steps may violate some constraints in the system.
Also SETCRTC itself is a per-CRTC operation, but we actually want per-device atomic operations instead.
So I think a single ioctl solution is a better idea. I'm currently pondering on what the API would look like. Ideally I would like to go with the "everything is a property" approach, and I'd like to get rid of some of the other limitations in the current API at the same time.
Yes, from previous emails I have seen that we are quite aligned on the single-atomic-modeset-with-properties version.
Do you have any actual proposal for this? Like the API at least and some comments on "the other limitations" you fix with it? I only recall seeing Jesse's API proposal, but not yours, only some ideas about a generic list of properties/values for modeset when I proposed something similar.
I'm about to implement atomic modeset now one way or the other, so the more proposals I have to choose from the better ;) I found that the per-crtc modeset above covers my requirements, so I might just take the easy route for now. But I welcome any work/proposal on generic support for atomic modeset.
/BR /Marcus
On Wed, 18 Apr 2012 16:58:36 +0200 Marcus Lorentzon marcus.xm.lorentzon@stericsson.com wrote:
On 04/18/2012 04:26 PM, Ville Syrjälä wrote:
Yes, from previous emails I have seen that we are quite aligned on the single-atomic-modeset-with-properties version.
Do you have any actual proposal for this? Like the API at least and some comments on "the other limitations" you fix with it? I only recall seeing Jesse's API proposal, but not yours, only some ideas about a generic list of properties/values for modeset when I proposed something similar.
I've been talking with Ville in private about this a little. Doing things well means a few additional APIs and properties, but I don't think he has anything concrete yet (I expect he's hacking on something now and probably has it working, just not to his satisfaction :p).
I'm about to implement atomic modeset now one way or the other, so the more proposals I have to choose from the better ;) I found that the per-crtc modeset above covers my requirements, so I might just take the easy route for now. But I welcome any work/proposal on generic support for atomic modeset.
I think Daniel summarized it well; it would be good to have an atomic mode set to change the whole device configuration atomically (including timings and other properties that involve global computation about memory bandwidth etc), and a separate ioctl for flipping new buffers onto one or more planes associated with a given pipe, along with ancillary data that may be needed (sprite position, z order, gamma, etc).
This could easily spiral out of control though, given how poorly the existing KMS API expresses the variety of display controllers out there; hopefully we can keep things incremental.
On Wed, Apr 18, 2012 at 01:31:59PM +0900, Joonyoung Shim wrote:
DRM_MODE_PLANE_EVENT is similar to DRM_MODE_PAGE_FLIP_EVENT but it is for a plane. The setplane ioctl (DRM_IOCTL_MODE_SETPLANE) needs to provide the event such as DRM_MODE_PAGE_FLIP_EVENT. The setplane ioctl can change the framebuffer of plane but user can't know completion of changing the framebuffer of plane via event. If DRM_MODE_PLANE_EVENT is added, we can also do pageflip of a plane.
This whole discussion brings up some related topics.
* Base planes, the actual main fb, should be treated as planes and fit in the same infrastructure. I see little point in treating these things seperately, just different capabilities (but not always).
* How to convey plane capabilities to userspace.
The current situation is not really satisfactory.
For FBs, which currently can be assigned to either a CRTC (base plane) or a plane, you have to crystal ball in userspace to guess what the layout for a given colourformat for a given plane should be, and the possible differences in layout between the different planes are not taken into account the current FB creation handlers. Apart from colour format for actual planes you get no information up front and, if you are lucky, you get treated with -EINVAL at the time of setting the plane.
In Xvideo, the Xv client got to ask the X server what the actual layout had to be for a given colour format at given dimensions. This made sure that not yet another copy, or further swizzling was needed before the hw could display it.
Then there is scaling, position and z-ordering, again, no information up front, sometimes you get -EINVAL, sometimes some values are silently altered, sometimes it just messes up.
This needs to be solved differently.
It is clear that not all information can be provided beforehand to suit all hardware and all situations, but most of what is listed above can be provided beforehand, part of it as plane resources, and part in a separate FB query which provides plane, colour format and dimensions, and which then returns sizes/offsets and pitches. That what cannot be standardized can then still be a silent alteration or -EINVAL.
Luc Verhaegen.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jesse Barnes -
Joonyoung Shim -
Kristian Høgsberg -
Luc Verhaegen -
Marcus Lorentzon -
Rob Clark -
Ville Syrjälä