[PATCH 02/27] drm/arc: Nuke event_list

[PATCH 02/27] drm/arc: Nuke event_list

[Daniel Vetter]

This is just used for cleanup in preclose, and with the reworked event
handling code this is now done properly by the core.

Nuke it!

But it also shows that arc totally fails at sending out drm events for
flips. Next patch will hack that up.

v2: Rebase it!

Cc: Carlos Palminha <palminha at synopsys.com>
Cc: Alexey Brodkin <abrodkin at synopsys.com>
Cc: linux-snps-arc at lists.infradead.org
Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
---
 drivers/gpu/drm/arc/arcpgu.h      |  1 -
 drivers/gpu/drm/arc/arcpgu_crtc.c |  4 ----
 drivers/gpu/drm/arc/arcpgu_drv.c  | 19 -------------------
 3 files changed, 24 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu.h b/drivers/gpu/drm/arc/arcpgu.h
index 86574b698a78..8c01a25d279a 100644
--- a/drivers/gpu/drm/arc/arcpgu.h
+++ b/drivers/gpu/drm/arc/arcpgu.h
@@ -22,7 +22,6 @@ struct arcpgu_drm_private {
 	struct clk		*clk;
 	struct drm_fbdev_cma	*fbdev;
 	struct drm_framebuffer	*fb;
-	struct list_head	event_list;
 	struct drm_crtc		crtc;
 	struct drm_plane	*plane;
 };
diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
index 92f8beff8e60..d5ca0c280e68 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -155,10 +155,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 		event->pipe = drm_crtc_index(crtc);
 
 		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
-
-		spin_lock_irqsave(&crtc->dev->event_lock, flags);
-		list_add_tail(&event->base.link, &arcpgu->event_list);
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 	}
 }
 
diff --git a/drivers/gpu/drm/arc/arcpgu_drv.c b/drivers/gpu/drm/arc/arcpgu_drv.c
index 7675bbc70133..d407fd79a400 100644
--- a/drivers/gpu/drm/arc/arcpgu_drv.c
+++ b/drivers/gpu/drm/arc/arcpgu_drv.c
@@ -81,22 +81,6 @@ static const struct file_operations arcpgu_drm_ops = {
 	.mmap = arcpgu_gem_mmap,
 };
 
-static void arcpgu_preclose(struct drm_device *drm, struct drm_file *file)
-{
-	struct arcpgu_drm_private *arcpgu = drm->dev_private;
-	struct drm_pending_vblank_event *e, *t;
-	unsigned long flags;
-
-	spin_lock_irqsave(&drm->event_lock, flags);
-	list_for_each_entry_safe(e, t, &arcpgu->event_list, base.link) {
-		if (e->base.file_priv != file)
-			continue;
-		list_del(&e->base.link);
-		kfree(&e->base);
-	}
-	spin_unlock_irqrestore(&drm->event_lock, flags);
-}
-
 static void arcpgu_lastclose(struct drm_device *drm)
 {
 	struct arcpgu_drm_private *arcpgu = drm->dev_private;
@@ -122,8 +106,6 @@ static int arcpgu_load(struct drm_device *drm)
 	if (IS_ERR(arcpgu->clk))
 		return PTR_ERR(arcpgu->clk);
 
-	INIT_LIST_HEAD(&arcpgu->event_list);
-
 	arcpgu_setup_mode_config(drm);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -192,7 +174,6 @@ int arcpgu_unload(struct drm_device *drm)
 static struct drm_driver arcpgu_drm_driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
 			   DRIVER_ATOMIC,
-	.preclose = arcpgu_preclose,
 	.lastclose = arcpgu_lastclose,
 	.name = "drm-arcpgu",
 	.desc = "ARC PGU Controller",
-- 
2.8.1

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Daniel Vetter]

The drm core has a nice ready-made helper for exactly the simple case
where it should fire on the next vblank.

Note that arming the vblank event in _begin is probably too early, and
might easily result in the vblank firing too early, before the new set
of planes are actually disabled. But that's kinda a minor issue
compared to just outright hanging userspace.

v2: Be more robust and either arm, when the CRTC is on, or just send
the event out right away.

Cc: Carlos Palminha <palminha at synopsys.com>
Cc: Alexey Brodkin <abrodkin at synopsys.com>
Cc: linux-snps-arc at lists.infradead.org
Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
---
 drivers/gpu/drm/arc/arcpgu_crtc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
index d5ca0c280e68..c9f183b11df9 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -145,16 +145,17 @@ static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
 static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 				      struct drm_crtc_state *state)
 {
-	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
-	unsigned long flags;
-
-	if (crtc->state->event) {
-		struct drm_pending_vblank_event *event = crtc->state->event;
+	struct drm_pending_vblank_event *event = crtc->state->event;
 
+	if (event) {
 		crtc->state->event = NULL;
-		event->pipe = drm_crtc_index(crtc);
 
-		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+		spin_lock_irq(&crtc->dev->event_lock);
+		if (drm_crtc_vblank_get(crtc) == 0)
+			drm_crtc_arm_vblank_event(crtc, event);
+		else
+			drm_crtc_send_vblank_event(crtc, event);
+		spin_unlock_irq(&crtc->dev->event_lock);
 	}
 }
 
-- 
2.8.1

[PATCH 11/27] drm/arc: Implement nonblocking commit correctly

[Daniel Vetter]

Committing with block it is not.

Thanks to the fixed up vblank event handling we can just use the
helper support for nonblocking commits now.

Cc: Carlos Palminha <palminha at synopsys.com>
Cc: Alexey Brodkin <abrodkin at synopsys.com>
Cc: linux-snps-arc at lists.infradead.org
Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
---
 drivers/gpu/drm/arc/arcpgu_drv.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_drv.c b/drivers/gpu/drm/arc/arcpgu_drv.c
index d407fd79a400..a92e533531c3 100644
--- a/drivers/gpu/drm/arc/arcpgu_drv.c
+++ b/drivers/gpu/drm/arc/arcpgu_drv.c
@@ -32,17 +32,11 @@ static void arcpgu_fb_output_poll_changed(struct drm_device *dev)
 		drm_fbdev_cma_hotplug_event(arcpgu->fbdev);
 }
 
-static int arcpgu_atomic_commit(struct drm_device *dev,
-				    struct drm_atomic_state *state, bool async)
-{
-	return drm_atomic_helper_commit(dev, state, false);
-}
-
 static struct drm_mode_config_funcs arcpgu_drm_modecfg_funcs = {
 	.fb_create  = drm_fb_cma_create,
 	.output_poll_changed = arcpgu_fb_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
-	.atomic_commit = arcpgu_atomic_commit,
+	.atomic_commit = drm_atomic_helper_commit,
 };
 
 static void arcpgu_setup_mode_config(struct drm_device *drm)
-- 
2.8.1

[PATCH 02/27] drm/arc: Nuke event_list

[Maarten Lankhorst]

Op 08-06-16 om 14:18 schreef Daniel Vetter:
> This is just used for cleanup in preclose, and with the reworked event
> handling code this is now done properly by the core.
>
> Nuke it!
>
> But it also shows that arc totally fails at sending out drm events for
> flips. Next patch will hack that up.
>
> v2: Rebase it!
Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Maarten Lankhorst]

Op 08-06-16 om 14:18 schreef Daniel Vetter:
> The drm core has a nice ready-made helper for exactly the simple case
> where it should fire on the next vblank.
>
> Note that arming the vblank event in _begin is probably too early, and
> might easily result in the vblank firing too early, before the new set
> of planes are actually disabled. But that's kinda a minor issue
> compared to just outright hanging userspace.
>
> v2: Be more robust and either arm, when the CRTC is on, or just send
> the event out right away.
>
> Cc: Carlos Palminha <palminha at synopsys.com>
> Cc: Alexey Brodkin <abrodkin at synopsys.com>
> Cc: linux-snps-arc at lists.infradead.org
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
Wouldn't it be better to do this in atomic_flush then?

[PATCH 11/27] drm/arc: Implement nonblocking commit correctly

[Maarten Lankhorst]

Op 08-06-16 om 14:19 schreef Daniel Vetter:
> Committing with block it is not.
>
> Thanks to the fixed up vblank event handling we can just use the
> helper support for nonblocking commits now.
>
> Cc: Carlos Palminha <palminha at synopsys.com>
> Cc: Alexey Brodkin <abrodkin at synopsys.com>
> Cc: linux-snps-arc at lists.infradead.org
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Daniel Vetter]

On Wed, Jun 08, 2016@04:14:38PM +0200, Maarten Lankhorst wrote:
> Op 08-06-16 om 14:18 schreef Daniel Vetter:
> > The drm core has a nice ready-made helper for exactly the simple case
> > where it should fire on the next vblank.
> >
> > Note that arming the vblank event in _begin is probably too early, and
> > might easily result in the vblank firing too early, before the new set
> > of planes are actually disabled. But that's kinda a minor issue
> > compared to just outright hanging userspace.
> >
> > v2: Be more robust and either arm, when the CRTC is on, or just send
> > the event out right away.
> >
> > Cc: Carlos Palminha <palminha at synopsys.com>
> > Cc: Alexey Brodkin <abrodkin at synopsys.com>
> > Cc: linux-snps-arc at lists.infradead.org
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> Wouldn't it be better to do this in atomic_flush then?

I'm not going to fix up other people's drivers completely, just enough to
hopefully not break them. If arc also blocks vblank interrupts with the go
bit, then doing this in _begin is correct. Either way it needs hw-specific
knowledge to asses whether it's correct, since doing the vblank event
stuff in _flush is also racy without some prevention.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Alexey Brodkin]

Hi Daniel,

On Wed, 2016-06-08@16:30 +0200, Daniel Vetter wrote:
> On Wed, Jun 08, 2016@04:14:38PM +0200, Maarten Lankhorst wrote:
> > 
> > Op 08-06-16 om 14:18 schreef Daniel Vetter:
> > > 
> > > The drm core has a nice ready-made helper for exactly the simple case
> > > where it should fire on the next vblank.
> > > 
> > > Note that arming the vblank event in _begin is probably too early, and
> > > might easily result in the vblank firing too early, before the new set
> > > of planes are actually disabled. But that's kinda a minor issue
> > > compared to just outright hanging userspace.
> > > 
> > > v2: Be more robust and either arm, when the CRTC is on, or just send
> > > the event out right away.
> > > 
> > > Cc: Carlos Palminha <palminha at synopsys.com>
> > > Cc: Alexey Brodkin <abrodkin at synopsys.com>
> > > Cc: linux-snps-arc at lists.infradead.org
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > Wouldn't it be better to do this in atomic_flush then?
> I'm not going to fix up other people's drivers completely, just enough to
> hopefully not break them. If arc also blocks vblank interrupts with the go
> bit, then doing this in _begin is correct. Either way it needs hw-specific
> knowledge to asses whether it's correct, since doing the vblank event
> stuff in _flush is also racy without some prevention.

Actually in ARC PGU driver that was one of many other copy-pastes from
other drivers. I.e. for me this is another boilerplate and if that's the
same for other drivers as well probably that's a good candidate for
generalization into something like?drm_helper_crtc_atomic_check().

-Alexey

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Daniel Vetter]

On Thu, Jun 09, 2016@10:54:45AM +0000, Alexey Brodkin wrote:
> Hi Daniel,
> 
> On Wed, 2016-06-08@16:30 +0200, Daniel Vetter wrote:
> > On Wed, Jun 08, 2016@04:14:38PM +0200, Maarten Lankhorst wrote:
> > > 
> > > Op 08-06-16 om 14:18 schreef Daniel Vetter:
> > > > 
> > > > The drm core has a nice ready-made helper for exactly the simple case
> > > > where it should fire on the next vblank.
> > > > 
> > > > Note that arming the vblank event in _begin is probably too early, and
> > > > might easily result in the vblank firing too early, before the new set
> > > > of planes are actually disabled. But that's kinda a minor issue
> > > > compared to just outright hanging userspace.
> > > > 
> > > > v2: Be more robust and either arm, when the CRTC is on, or just send
> > > > the event out right away.
> > > > 
> > > > Cc: Carlos Palminha <palminha at synopsys.com>
> > > > Cc: Alexey Brodkin <abrodkin at synopsys.com>
> > > > Cc: linux-snps-arc at lists.infradead.org
> > > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > Wouldn't it be better to do this in atomic_flush then?
> > I'm not going to fix up other people's drivers completely, just enough to
> > hopefully not break them. If arc also blocks vblank interrupts with the go
> > bit, then doing this in _begin is correct. Either way it needs hw-specific
> > knowledge to asses whether it's correct, since doing the vblank event
> > stuff in _flush is also racy without some prevention.
> 
> Actually in ARC PGU driver that was one of many other copy-pastes from
> other drivers. I.e. for me this is another boilerplate and if that's the
> same for other drivers as well probably that's a good candidate for
> generalization into something like?drm_helper_crtc_atomic_check().

I checked them all, you are special with your code here. And this can't be
generalized since you must send out vblank events in a race-free manner
against the actual hw update. This requires deep knowledge of the actual
hw, and it's not something the helpers can take care of you. It is very
much not boilerplate, but crucial for a correct implementation. And most
likely arcpgu is wrong, but since I don't have that hw knowledge I'm not
going to change it more than absolutely required.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Alexey Brodkin]

Hi Daniel,

On Thu, 2016-06-09@14:26 +0200, Daniel Vetter wrote:
> On Thu, Jun 09, 2016@10:54:45AM +0000, Alexey Brodkin wrote:
> > 
> > Hi Daniel,
> > 
> > On Wed, 2016-06-08@16:30 +0200, Daniel Vetter wrote:
> > > 
> > > On Wed, Jun 08, 2016@04:14:38PM +0200, Maarten Lankhorst wrote:
> > > > 
> > > > 
> > > > Op 08-06-16 om 14:18 schreef Daniel Vetter:
> > > > > 
> > > > > 
> > > > > The drm core has a nice ready-made helper for exactly the simple case
> > > > > where it should fire on the next vblank.
> > > > > 
> > > > > Note that arming the vblank event in _begin is probably too early, and
> > > > > might easily result in the vblank firing too early, before the new set
> > > > > of planes are actually disabled. But that's kinda a minor issue
> > > > > compared to just outright hanging userspace.
> > > > > 
> > > > > v2: Be more robust and either arm, when the CRTC is on, or just send
> > > > > the event out right away.
> > > > > 
> > > > > Cc: Carlos Palminha <palminha at synopsys.com>
> > > > > Cc: Alexey Brodkin <abrodkin at synopsys.com>
> > > > > Cc: linux-snps-arc at lists.infradead.org
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > > Wouldn't it be better to do this in atomic_flush then?
> > > I'm not going to fix up other people's drivers completely, just enough to
> > > hopefully not break them. If arc also blocks vblank interrupts with the go
> > > bit, then doing this in _begin is correct. Either way it needs hw-specific
> > > knowledge to asses whether it's correct, since doing the vblank event
> > > stuff in _flush is also racy without some prevention.
> > Actually in ARC PGU driver that was one of many other copy-pastes from
> > other drivers. I.e. for me this is another boilerplate and if that's the
> > same for other drivers as well probably that's a good candidate for
> > generalization into something like?drm_helper_crtc_atomic_check().
>
> I checked them all, you are special with your code here. And this can't be
> generalized since you must send out vblank events in a race-free manner
> against the actual hw update. This requires deep knowledge of the actual
> hw, and it's not something the helpers can take care of you. It is very
> much not boilerplate, but crucial for a correct implementation. And most
> likely arcpgu is wrong, but since I don't have that hw knowledge I'm not
> going to change it more than absolutely required.

Well I meant as of today we don't support vblank interrupts and so
arc_pgu_crtc_atomic_begin() barely makes any sense.

Still in the future we do plan to add support of interrupts.

-Alexey

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Daniel Vetter]

On Thu, Jun 09, 2016@12:48:31PM +0000, Alexey Brodkin wrote:
> Hi Daniel,
> 
> On Thu, 2016-06-09@14:26 +0200, Daniel Vetter wrote:
> > On Thu, Jun 09, 2016@10:54:45AM +0000, Alexey Brodkin wrote:
> > > 
> > > Hi Daniel,
> > > 
> > > On Wed, 2016-06-08@16:30 +0200, Daniel Vetter wrote:
> > > > 
> > > > On Wed, Jun 08, 2016@04:14:38PM +0200, Maarten Lankhorst wrote:
> > > > > 
> > > > > 
> > > > > Op 08-06-16 om 14:18 schreef Daniel Vetter:
> > > > > > 
> > > > > > 
> > > > > > The drm core has a nice ready-made helper for exactly the simple case
> > > > > > where it should fire on the next vblank.
> > > > > > 
> > > > > > Note that arming the vblank event in _begin is probably too early, and
> > > > > > might easily result in the vblank firing too early, before the new set
> > > > > > of planes are actually disabled. But that's kinda a minor issue
> > > > > > compared to just outright hanging userspace.
> > > > > > 
> > > > > > v2: Be more robust and either arm, when the CRTC is on, or just send
> > > > > > the event out right away.
> > > > > > 
> > > > > > Cc: Carlos Palminha <palminha at synopsys.com>
> > > > > > Cc: Alexey Brodkin <abrodkin at synopsys.com>
> > > > > > Cc: linux-snps-arc at lists.infradead.org
> > > > > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > > > Wouldn't it be better to do this in atomic_flush then?
> > > > I'm not going to fix up other people's drivers completely, just enough to
> > > > hopefully not break them. If arc also blocks vblank interrupts with the go
> > > > bit, then doing this in _begin is correct. Either way it needs hw-specific
> > > > knowledge to asses whether it's correct, since doing the vblank event
> > > > stuff in _flush is also racy without some prevention.
> > > Actually in ARC PGU driver that was one of many other copy-pastes from
> > > other drivers. I.e. for me this is another boilerplate and if that's the
> > > same for other drivers as well probably that's a good candidate for
> > > generalization into something like?drm_helper_crtc_atomic_check().
> >
> > I checked them all, you are special with your code here. And this can't be
> > generalized since you must send out vblank events in a race-free manner
> > against the actual hw update. This requires deep knowledge of the actual
> > hw, and it's not something the helpers can take care of you. It is very
> > much not boilerplate, but crucial for a correct implementation. And most
> > likely arcpgu is wrong, but since I don't have that hw knowledge I'm not
> > going to change it more than absolutely required.
> 
> Well I meant as of today we don't support vblank interrupts and so
> arc_pgu_crtc_atomic_begin() barely makes any sense.
> 
> Still in the future we do plan to add support of interrupts.

If you don't support vblank interrupts you're driver isn't compliant with
atomic. You need to at least fake them.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Alexey Brodkin]

Hi Daniel,

On Thu, 2016-06-09@15:23 +0200, Daniel Vetter wrote:
> On Thu, Jun 09, 2016@12:48:31PM +0000, Alexey Brodkin wrote:
> > 
> > Hi Daniel,
> > 
> > On Thu, 2016-06-09@14:26 +0200, Daniel Vetter wrote:
> > > 
> > > On Thu, Jun 09, 2016@10:54:45AM +0000, Alexey Brodkin wrote:
> > > > 
> > > > 
> > > > Hi Daniel,
> > > > 
> > > > On Wed, 2016-06-08@16:30 +0200, Daniel Vetter wrote:
> > > > > 
> > > > > 
> > > > > On Wed, Jun 08, 2016@04:14:38PM +0200, Maarten Lankhorst wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Op 08-06-16 om 14:18 schreef Daniel Vetter:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > The drm core has a nice ready-made helper for exactly the simple case
> > > > > > > where it should fire on the next vblank.
> > > > > > > 
> > > > > > > Note that arming the vblank event in _begin is probably too early, and
> > > > > > > might easily result in the vblank firing too early, before the new set
> > > > > > > of planes are actually disabled. But that's kinda a minor issue
> > > > > > > compared to just outright hanging userspace.
> > > > > > > 
> > > > > > > v2: Be more robust and either arm, when the CRTC is on, or just send
> > > > > > > the event out right away.
> > > > > > > 
> > > > > > > Cc: Carlos Palminha <palminha at synopsys.com>
> > > > > > > Cc: Alexey Brodkin <abrodkin at synopsys.com>
> > > > > > > Cc: linux-snps-arc at lists.infradead.org
> > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > > > > Wouldn't it be better to do this in atomic_flush then?
> > > > > I'm not going to fix up other people's drivers completely, just enough to
> > > > > hopefully not break them. If arc also blocks vblank interrupts with the go
> > > > > bit, then doing this in _begin is correct. Either way it needs hw-specific
> > > > > knowledge to asses whether it's correct, since doing the vblank event
> > > > > stuff in _flush is also racy without some prevention.
> > > > Actually in ARC PGU driver that was one of many other copy-pastes from
> > > > other drivers. I.e. for me this is another boilerplate and if that's the
> > > > same for other drivers as well probably that's a good candidate for
> > > > generalization into something like?drm_helper_crtc_atomic_check().
> > > I checked them all, you are special with your code here. And this can't be
> > > generalized since you must send out vblank events in a race-free manner
> > > against the actual hw update. This requires deep knowledge of the actual
> > > hw, and it's not something the helpers can take care of you. It is very
> > > much not boilerplate, but crucial for a correct implementation. And most
> > > likely arcpgu is wrong, but since I don't have that hw knowledge I'm not
> > > going to change it more than absolutely required.
> > Well I meant as of today we don't support vblank interrupts and so
> > arc_pgu_crtc_atomic_begin() barely makes any sense.
> > 
> > Still in the future we do plan to add support of interrupts.
> 
>
> If you don't support vblank interrupts you're driver isn't compliant with
> atomic. You need to at least fake them.

Indeed. So my assumption was there are (or could appear) other simple drivers
of the same kind and that fake implementation might be generic.

-Alexey

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Daniel Vetter]

On Thu, Jun 09, 2016@01:27:55PM +0000, Alexey Brodkin wrote:
> Hi Daniel,
> 
> On Thu, 2016-06-09@15:23 +0200, Daniel Vetter wrote:
> > On Thu, Jun 09, 2016@12:48:31PM +0000, Alexey Brodkin wrote:
> > > 
> > > Hi Daniel,
> > > 
> > > On Thu, 2016-06-09@14:26 +0200, Daniel Vetter wrote:
> > > > 
> > > > On Thu, Jun 09, 2016@10:54:45AM +0000, Alexey Brodkin wrote:
> > > > > 
> > > > > 
> > > > > Hi Daniel,
> > > > > 
> > > > > On Wed, 2016-06-08@16:30 +0200, Daniel Vetter wrote:
> > > > > > 
> > > > > > 
> > > > > > On Wed, Jun 08, 2016@04:14:38PM +0200, Maarten Lankhorst wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Op 08-06-16 om 14:18 schreef Daniel Vetter:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > The drm core has a nice ready-made helper for exactly the simple case
> > > > > > > > where it should fire on the next vblank.
> > > > > > > > 
> > > > > > > > Note that arming the vblank event in _begin is probably too early, and
> > > > > > > > might easily result in the vblank firing too early, before the new set
> > > > > > > > of planes are actually disabled. But that's kinda a minor issue
> > > > > > > > compared to just outright hanging userspace.
> > > > > > > > 
> > > > > > > > v2: Be more robust and either arm, when the CRTC is on, or just send
> > > > > > > > the event out right away.
> > > > > > > > 
> > > > > > > > Cc: Carlos Palminha <palminha at synopsys.com>
> > > > > > > > Cc: Alexey Brodkin <abrodkin at synopsys.com>
> > > > > > > > Cc: linux-snps-arc at lists.infradead.org
> > > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > > > > > Wouldn't it be better to do this in atomic_flush then?
> > > > > > I'm not going to fix up other people's drivers completely, just enough to
> > > > > > hopefully not break them. If arc also blocks vblank interrupts with the go
> > > > > > bit, then doing this in _begin is correct. Either way it needs hw-specific
> > > > > > knowledge to asses whether it's correct, since doing the vblank event
> > > > > > stuff in _flush is also racy without some prevention.
> > > > > Actually in ARC PGU driver that was one of many other copy-pastes from
> > > > > other drivers. I.e. for me this is another boilerplate and if that's the
> > > > > same for other drivers as well probably that's a good candidate for
> > > > > generalization into something like?drm_helper_crtc_atomic_check().
> > > > I checked them all, you are special with your code here. And this can't be
> > > > generalized since you must send out vblank events in a race-free manner
> > > > against the actual hw update. This requires deep knowledge of the actual
> > > > hw, and it's not something the helpers can take care of you. It is very
> > > > much not boilerplate, but crucial for a correct implementation. And most
> > > > likely arcpgu is wrong, but since I don't have that hw knowledge I'm not
> > > > going to change it more than absolutely required.
> > > Well I meant as of today we don't support vblank interrupts and so
> > > arc_pgu_crtc_atomic_begin() barely makes any sense.
> > > 
> > > Still in the future we do plan to add support of interrupts.
> > 
> >
> > If you don't support vblank interrupts you're driver isn't compliant with
> > atomic. You need to at least fake them.
> 
> Indeed. So my assumption was there are (or could appear) other simple drivers
> of the same kind and that fake implementation might be generic.

The fake implementation is fundamentally racy, and I don't want to write
helpers which can't be used correctly. Anyway I think without this patch
(or something similar) arcpgu will stall badly with the new nonblocking
helpers, because arcpgu didn't bother at all to implement nonblocking. Can
you pls ack this, or even better, test the entire patch series? The
helpers themselves should work, but in all 5 drivers tested thus far they
discovered some bugs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Alexey Brodkin]

Hi Daniel,

On Thu, 2016-06-09@15:52 +0200, Daniel Vetter wrote:
>?
> The fake implementation is fundamentally racy, and I don't want to write
> helpers which can't be used correctly. Anyway I think without this patch
> (or something similar) arcpgu will stall badly with the new nonblocking
> helpers, because arcpgu didn't bother at all to implement nonblocking. Can
> you pls ack this, or even better, test the entire patch series? The
> helpers themselves should work, but in all 5 drivers tested thus far they
> discovered some bugs.

Sure I will happily test this series.
The only question then is what should I use as a proper base?

-Alexey

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Daniel Vetter]

On Thu, Jun 9, 2016 at 4:29 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> Hi Daniel,
>
> On Thu, 2016-06-09@15:52 +0200, Daniel Vetter wrote:
>>
>> The fake implementation is fundamentally racy, and I don't want to write
>> helpers which can't be used correctly. Anyway I think without this patch
>> (or something similar) arcpgu will stall badly with the new nonblocking
>> helpers, because arcpgu didn't bother at all to implement nonblocking. Can
>> you pls ack this, or even better, test the entire patch series? The
>> helpers themselves should work, but in all 5 drivers tested thus far they
>> discovered some bugs.
>
> Sure I will happily test this series.
> The only question then is what should I use as a proper base?

It should apply on drm-next from Dave.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Daniel Vetter]

On Thu, Jun 9, 2016@4:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jun 9, 2016 at 4:29 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
>> Hi Daniel,
>>
>> On Thu, 2016-06-09@15:52 +0200, Daniel Vetter wrote:
>>>
>>> The fake implementation is fundamentally racy, and I don't want to write
>>> helpers which can't be used correctly. Anyway I think without this patch
>>> (or something similar) arcpgu will stall badly with the new nonblocking
>>> helpers, because arcpgu didn't bother at all to implement nonblocking. Can
>>> you pls ack this, or even better, test the entire patch series? The
>>> helpers themselves should work, but in all 5 drivers tested thus far they
>>> discovered some bugs.
>>
>> Sure I will happily test this series.
>> The only question then is what should I use as a proper base?
>
> It should apply on drm-next from Dave.

And indeed it won't work at all because arcpgu doesn't call
drm_crtc_handle_vblank anywhere. So you need to add your patch to
enable vblank interrupts somewhere. Note that as long as you leave
max_vblank_counter as 0, the only bits you need is drm_vblank_init and
drm_crtc_handle_vblanke() from the irq handler.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Alexey Brodkin]

Hi Daniel,

On Thu, 2016-06-09@16:37 +0200, Daniel Vetter wrote:
> On Thu, Jun 9, 2016@4:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > On Thu, Jun 9, 2016 at 4:29 PM, Alexey Brodkin
> > <Alexey.Brodkin@synopsys.com> wrote:
> > > 
> > > Hi Daniel,
> > > 
> > > On Thu, 2016-06-09@15:52 +0200, Daniel Vetter wrote:
> > > > 
> > > > 
> > > > The fake implementation is fundamentally racy, and I don't want to write
> > > > helpers which can't be used correctly. Anyway I think without this patch
> > > > (or something similar) arcpgu will stall badly with the new nonblocking
> > > > helpers, because arcpgu didn't bother at all to implement nonblocking. Can
> > > > you pls ack this, or even better, test the entire patch series? The
> > > > helpers themselves should work, but in all 5 drivers tested thus far they
> > > > discovered some bugs.
> > > Sure I will happily test this series.
> > > The only question then is what should I use as a proper base?
> > It should apply on drm-next from Dave.
>
> And indeed it won't work at all because arcpgu doesn't call
> drm_crtc_handle_vblank anywhere. So you need to add your patch to
> enable vblank interrupts somewhere. Note that as long as you leave
> max_vblank_counter as 0, the only bits you need is drm_vblank_init and
> drm_crtc_handle_vblanke() from the irq handler.

So is there any sense in testing that series if vblank interrupt is not yet
supported (I'm looking forward to implementing it sometime soon but definitely
I'm not there yet)?

-Alexey

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Daniel Vetter]

On Fri, Jun 10, 2016@01:23:22PM +0000, Alexey Brodkin wrote:
> Hi Daniel,
> 
> On Thu, 2016-06-09@16:37 +0200, Daniel Vetter wrote:
> > On Thu, Jun 9, 2016@4:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > 
> > > On Thu, Jun 9, 2016 at 4:29 PM, Alexey Brodkin
> > > <Alexey.Brodkin@synopsys.com> wrote:
> > > > 
> > > > Hi Daniel,
> > > > 
> > > > On Thu, 2016-06-09@15:52 +0200, Daniel Vetter wrote:
> > > > > 
> > > > > 
> > > > > The fake implementation is fundamentally racy, and I don't want to write
> > > > > helpers which can't be used correctly. Anyway I think without this patch
> > > > > (or something similar) arcpgu will stall badly with the new nonblocking
> > > > > helpers, because arcpgu didn't bother at all to implement nonblocking. Can
> > > > > you pls ack this, or even better, test the entire patch series? The
> > > > > helpers themselves should work, but in all 5 drivers tested thus far they
> > > > > discovered some bugs.
> > > > Sure I will happily test this series.
> > > > The only question then is what should I use as a proper base?
> > > It should apply on drm-next from Dave.
> >
> > And indeed it won't work at all because arcpgu doesn't call
> > drm_crtc_handle_vblank anywhere. So you need to add your patch to
> > enable vblank interrupts somewhere. Note that as long as you leave
> > max_vblank_counter as 0, the only bits you need is drm_vblank_init and
> > drm_crtc_handle_vblanke() from the irq handler.
> 
> So is there any sense in testing that series if vblank interrupt is not yet
> supported (I'm looking forward to implementing it sometime soon but definitely
> I'm not there yet)?

Well, it might break your driver, so yes. I'm ofc happy to help unbreak it,
but without someone who tests there's not much I can do, so will just go
ahead and apply and hope it works.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Daniel Vetter]

On Fri, Jun 10, 2016@04:19:27PM +0200, Daniel Vetter wrote:
> On Fri, Jun 10, 2016@01:23:22PM +0000, Alexey Brodkin wrote:
> > Hi Daniel,
> > 
> > On Thu, 2016-06-09@16:37 +0200, Daniel Vetter wrote:
> > > On Thu, Jun 9, 2016@4:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > 
> > > > On Thu, Jun 9, 2016 at 4:29 PM, Alexey Brodkin
> > > > <Alexey.Brodkin@synopsys.com> wrote:
> > > > > 
> > > > > Hi Daniel,
> > > > > 
> > > > > On Thu, 2016-06-09@15:52 +0200, Daniel Vetter wrote:
> > > > > > 
> > > > > > 
> > > > > > The fake implementation is fundamentally racy, and I don't want to write
> > > > > > helpers which can't be used correctly. Anyway I think without this patch
> > > > > > (or something similar) arcpgu will stall badly with the new nonblocking
> > > > > > helpers, because arcpgu didn't bother at all to implement nonblocking. Can
> > > > > > you pls ack this, or even better, test the entire patch series? The
> > > > > > helpers themselves should work, but in all 5 drivers tested thus far they
> > > > > > discovered some bugs.
> > > > > Sure I will happily test this series.
> > > > > The only question then is what should I use as a proper base?
> > > > It should apply on drm-next from Dave.
> > >
> > > And indeed it won't work at all because arcpgu doesn't call
> > > drm_crtc_handle_vblank anywhere. So you need to add your patch to
> > > enable vblank interrupts somewhere. Note that as long as you leave
> > > max_vblank_counter as 0, the only bits you need is drm_vblank_init and
> > > drm_crtc_handle_vblanke() from the irq handler.
> > 
> > So is there any sense in testing that series if vblank interrupt is not yet
> > supported (I'm looking forward to implementing it sometime soon but definitely
> > I'm not there yet)?
> 
> Well, it might break your driver, so yes. I'm ofc happy to help unbreak it,
> but without someone who tests there's not much I can do, so will just go
> ahead and apply and hope it works.

Ok I went ahead and pushed a slight revised version of that patch which
just unconditionally sends out the event. That's not correct, but at least
that way the nonblocking changes won't totally break arcpgu and I can move
ahead with those.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Alexey Brodkin]

Hi Daniel,

On Fri, 2016-06-10@16:54 +0200, Daniel Vetter wrote:
> On Fri, Jun 10, 2016@04:19:27PM +0200, Daniel Vetter wrote:
> > 
> > On Fri, Jun 10, 2016@01:23:22PM +0000, Alexey Brodkin wrote:
> > > 
> > > Hi Daniel,
> > > 
> > > On Thu, 2016-06-09@16:37 +0200, Daniel Vetter wrote:
> > > > 
> > > > On Thu, Jun 9, 2016@4:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > 
> > > > > 
> > > > > On Thu, Jun 9, 2016 at 4:29 PM, Alexey Brodkin
> > > > > <Alexey.Brodkin@synopsys.com> wrote:
> > > > > > 
> > > > > > 
> > > > > > Hi Daniel,
> > > > > > 
> > > > > > On Thu, 2016-06-09@15:52 +0200, Daniel Vetter wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > The fake implementation is fundamentally racy, and I don't want to write
> > > > > > > helpers which can't be used correctly. Anyway I think without this patch
> > > > > > > (or something similar) arcpgu will stall badly with the new nonblocking
> > > > > > > helpers, because arcpgu didn't bother at all to implement nonblocking. Can
> > > > > > > you pls ack this, or even better, test the entire patch series? The
> > > > > > > helpers themselves should work, but in all 5 drivers tested thus far they
> > > > > > > discovered some bugs.
> > > > > > Sure I will happily test this series.
> > > > > > The only question then is what should I use as a proper base?
> > > > > It should apply on drm-next from Dave.
> > > > And indeed it won't work at all because arcpgu doesn't call
> > > > drm_crtc_handle_vblank anywhere. So you need to add your patch to
> > > > enable vblank interrupts somewhere. Note that as long as you leave
> > > > max_vblank_counter as 0, the only bits you need is drm_vblank_init and
> > > > drm_crtc_handle_vblanke() from the irq handler.
> > > So is there any sense in testing that series if vblank interrupt is not yet
> > > supported (I'm looking forward to implementing it sometime soon but definitely
> > > I'm not there yet)?
> > Well, it might break your driver, so yes. I'm ofc happy to help unbreak it,
> > but without someone who tests there's not much I can do, so will just go
> > ahead and apply and hope it works.
>
> Ok I went ahead and pushed a slight revised version of that patch which
> just unconditionally sends out the event. That's not correct, but at least
> that way the nonblocking changes won't totally break arcpgu and I can move
> ahead with those.

Thanks for that.
In the meantime I tried previously sent patches:
--------------->8-------------
9267484 drm/arc: Actually bother with handling atomic events.
cf4a489 drm/arc: Nuke event_list
9c3152e drm/atomic-helper: Massage swap_state signature somewhat
--------------->8-------------
and on both boards (axs103 and nSIM OSCI) video works quite fine.

-Alexey

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Daniel Vetter]

On Fri, Jun 10, 2016@03:01:03PM +0000, Alexey Brodkin wrote:
> Hi Daniel,
> 
> On Fri, 2016-06-10@16:54 +0200, Daniel Vetter wrote:
> > On Fri, Jun 10, 2016@04:19:27PM +0200, Daniel Vetter wrote:
> > > 
> > > On Fri, Jun 10, 2016@01:23:22PM +0000, Alexey Brodkin wrote:
> > > > 
> > > > Hi Daniel,
> > > > 
> > > > On Thu, 2016-06-09@16:37 +0200, Daniel Vetter wrote:
> > > > > 
> > > > > On Thu, Jun 9, 2016@4:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > 
> > > > > > 
> > > > > > On Thu, Jun 9, 2016 at 4:29 PM, Alexey Brodkin
> > > > > > <Alexey.Brodkin@synopsys.com> wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > Hi Daniel,
> > > > > > > 
> > > > > > > On Thu, 2016-06-09@15:52 +0200, Daniel Vetter wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > The fake implementation is fundamentally racy, and I don't want to write
> > > > > > > > helpers which can't be used correctly. Anyway I think without this patch
> > > > > > > > (or something similar) arcpgu will stall badly with the new nonblocking
> > > > > > > > helpers, because arcpgu didn't bother at all to implement nonblocking. Can
> > > > > > > > you pls ack this, or even better, test the entire patch series? The
> > > > > > > > helpers themselves should work, but in all 5 drivers tested thus far they
> > > > > > > > discovered some bugs.
> > > > > > > Sure I will happily test this series.
> > > > > > > The only question then is what should I use as a proper base?
> > > > > > It should apply on drm-next from Dave.
> > > > > And indeed it won't work at all because arcpgu doesn't call
> > > > > drm_crtc_handle_vblank anywhere. So you need to add your patch to
> > > > > enable vblank interrupts somewhere. Note that as long as you leave
> > > > > max_vblank_counter as 0, the only bits you need is drm_vblank_init and
> > > > > drm_crtc_handle_vblanke() from the irq handler.
> > > > So is there any sense in testing that series if vblank interrupt is not yet
> > > > supported (I'm looking forward to implementing it sometime soon but definitely
> > > > I'm not there yet)?
> > > Well, it might break your driver, so yes. I'm ofc happy to help unbreak it,
> > > but without someone who tests there's not much I can do, so will just go
> > > ahead and apply and hope it works.
> >
> > Ok I went ahead and pushed a slight revised version of that patch which
> > just unconditionally sends out the event. That's not correct, but at least
> > that way the nonblocking changes won't totally break arcpgu and I can move
> > ahead with those.
> 
> Thanks for that.
> In the meantime I tried previously sent patches:
> --------------->8-------------
> 9267484 drm/arc: Actually bother with handling atomic events.
> cf4a489 drm/arc: Nuke event_list
> 9c3152e drm/atomic-helper: Massage swap_state signature somewhat
> --------------->8-------------
> and on both boards (axs103 and nSIM OSCI) video works quite fine.

The possible breakage only starts when you move further into the series,
up to patch 10. That implements generic nonblocking commit, but that
support relies upon crtc_state->event being signalled. Anyway I'm pulling
it all into drm-misc now, so you can just test that branch (or linux-next
when it's rebuild next week).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 03/27] drm/arc: Actually bother with handling atomic events.

[Alexey Brodkin]

Hi Daniel,

On Fri, 2016-06-10@17:09 +0200, Daniel Vetter wrote:
> On Fri, Jun 10, 2016@03:01:03PM +0000, Alexey Brodkin wrote:
> >?
> > > Ok I went ahead and pushed a slight revised version of that patch which
> > > just unconditionally sends out the event. That's not correct, but at least
> > > that way the nonblocking changes won't totally break arcpgu and I can move
> > > ahead with those.
> > Thanks for that.
> > In the meantime I tried previously sent patches:
> > --------------->8-------------
> > 9267484 drm/arc: Actually bother with handling atomic events.
> > cf4a489 drm/arc: Nuke event_list
> > 9c3152e drm/atomic-helper: Massage swap_state signature somewhat
> > --------------->8-------------
> > and on both boards (axs103 and nSIM OSCI) video works quite fine.
>
> The possible breakage only starts when you move further into the series,
> up to patch 10. That implements generic nonblocking commit, but that
> support relies upon crtc_state->event being signalled. Anyway I'm pulling
> it all into drm-misc now, so you can just test that branch (or linux-next
> when it's rebuild next week).

Ok thanks anyways.
I'll try linux-next once it gets updated with your changes.

-Alexey