[RFC PATCH] drm: Add plane event

[RFC PATCH] drm: Add plane event

[Joonyoung Shim]

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 {
-- 
1.7.5.4

Re: [RFC PATCH] drm: Add plane event

[Daniel Vetter]

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);
>  	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 {
> -- 
> 1.7.5.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [RFC PATCH] drm: Add plane event

[Joonyoung Shim]

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 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.

It seems better way to add new pageflip ioctl for plane. I will try it.

Thanks for review.

Re: [RFC PATCH] drm: Add plane event

[Rob Clark]

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 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.
>
>
> 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

Re: [RFC PATCH] drm: Add plane event

[Marcus Lorentzon]

On 04/18/2012 02:25 PM, Rob Clark wrote:
> 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 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.
>>
>> 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

Re: [RFC PATCH] drm: Add plane event

[Ville Syrjälä]

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 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 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.
> >>
> >> 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.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC PATCH] drm: Add plane event

[Daniel Vetter]

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 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 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.
> > >>
> > >> 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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [RFC PATCH] drm: Add plane event

[Marcus Lorentzon]

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 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 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.
>>>> 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

Re: [RFC PATCH] drm: Add plane event

[Marcus Lorentzon]

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

Re: [RFC PATCH] drm: Add plane event

[Daniel Vetter]

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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [RFC PATCH] drm: Add plane event

[Luc Verhaegen]

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.

Re: [RFC PATCH] drm: Add plane event

[Marcus Lorentzon]

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

Re: [RFC PATCH] drm: Add plane event

[Jesse Barnes]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] drm: Add plane event

[Marcus Lorentzon]

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

Re: [RFC PATCH] drm: Add plane event

[Jesse Barnes]

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).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [RFC PATCH] drm: Add plane event

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC PATCH] drm: Add plane event

[Marcus Lorentzon]

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

Re: [RFC PATCH] drm: Add plane event

[Luc Verhaegen]

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.

Re: [RFC PATCH] drm: Add plane event

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC PATCH] drm: Add plane event

[Daniel Vetter]

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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [RFC PATCH] drm: Add plane event

[Kristian Høgsberg]

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 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 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.
>> > >>
>> > >> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH] drm: Add plane event

[Daniel Vetter]

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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [RFC PATCH] drm: Add plane event

[Daniel Vetter]

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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48