All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/simple-helpers: Always add planes to the state update
@ 2016-08-23  6:25 Daniel Vetter
  2016-08-24 18:38 ` Noralf Trønnes
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2016-08-23  6:25 UTC (permalink / raw)
  To: DRI Development; +Cc: andrea.merello, Daniel Vetter, Daniel Vetter

Our update function is hooked to the single plane, which might not get
called for crtc-only updates. Which is surprising, so fix this by
always adding the plane.

While at it document how&when the event should be sent out better in
the kerneldoc.

Cc: Noralf Trønnes <noralf@tronnes.org>
Cc: andrea.merello@gmail.com
Tested-and-Reported-by: andrea.merello@gmail.com
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
I'm not entirely sure we really want to put the responsibility for
this onto driver. Plan B) would be to remove the kerneldoc I added
here and call the right function from drm_simple_kms_plane_atomic_update.
That way simple drivers don't need to deal with that detail, and in
general those drivers don't care that much about the miniscule
possible race a generic implementation would cause. What do you
suggest as the best approach?
-Daniel
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 7 +++++++
 include/drm/drm_simple_kms_helper.h     | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index bada17166512..447631018426 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -34,6 +34,12 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
+static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
+				     struct drm_crtc_state *state)
+{
+	return drm_atomic_add_affected_planes(state->state, crtc);
+}
+
 static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_simple_display_pipe *pipe;
@@ -57,6 +63,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
+	.atomic_check = drm_simple_kms_crtc_check,
 	.disable = drm_simple_kms_crtc_disable,
 	.enable = drm_simple_kms_crtc_enable,
 };
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index 269039722f91..826946ca2b82 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -60,6 +60,12 @@ struct drm_simple_display_pipe_funcs {
 	 *
 	 * This function is called when the underlying plane state is updated.
 	 * This hook is optional.
+	 *
+	 * This is the function drivers should submit the
+	 * &drm_pending_vblank_event from. Using either
+	 * drm_crtc_arm_vblank_event(), when the driver supports vblank
+	 * interrupt handling, or drm_crtc_send_vblank_event() directly in case
+	 * the hardware lacks vblank support entirely.
 	 */
 	void (*update)(struct drm_simple_display_pipe *pipe,
 		       struct drm_plane_state *plane_state);
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/simple-helpers: Always add planes to the state update
  2016-08-23  6:25 [PATCH] drm/simple-helpers: Always add planes to the state update Daniel Vetter
@ 2016-08-24 18:38 ` Noralf Trønnes
  2016-08-25  6:30   ` Daniel Vetter
  0 siblings, 1 reply; 5+ messages in thread
From: Noralf Trønnes @ 2016-08-24 18:38 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: andrea.merello, Daniel Vetter, DRI Development


Den 23.08.2016 08:25, skrev Daniel Vetter:
> Our update function is hooked to the single plane, which might not get
> called for crtc-only updates. Which is surprising, so fix this by
> always adding the plane.
>
> While at it document how&when the event should be sent out better in
> the kerneldoc.
>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: andrea.merello@gmail.com
> Tested-and-Reported-by: andrea.merello@gmail.com
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> I'm not entirely sure we really want to put the responsibility for
> this onto driver. Plan B) would be to remove the kerneldoc I added
> here and call the right function from drm_simple_kms_plane_atomic_update.
> That way simple drivers don't need to deal with that detail, and in
> general those drivers don't care that much about the miniscule
> possible race a generic implementation would cause. What do you
> suggest as the best approach?

If the driver is responsible, a helper like this would be nice:

void drm_simple_display_pipe_handle_vblank_event(struct 
drm_simple_display_pipe *pipe)
{
     struct drm_crtc *crtc = &pipe->crtc;

     if (crtc->state && crtc->state->event) {
         spin_lock_irq(&crtc->dev->event_lock);
         drm_crtc_send_vblank_event(crtc, crtc->state->event);
         spin_unlock_irq(&crtc->dev->event_lock);
         crtc->state->event = NULL;
     }
}

Then if the update() callback is not set, drm_simple_kms_plane_atomic_update
calls this, and if it is set, then the driver has to do it or something
similar.

It is difficult to predict what future drivers will need.
Unless there are drivers on the horizon that will need to handle the
event themselves, I suggest that we follow your plan B. If the need arises,
we just add a one-liner to all drivers that has the update() callback set.

Noralf.

Sidenote:
When I converted simpledrm I had problems with flip_done timeouts.
In the end I had to check for the event in disable/enable/update.
Trying it again now, it's enough to do it in update().
Not sure what has changed.

> -Daniel
> ---
>   drivers/gpu/drm/drm_simple_kms_helper.c | 7 +++++++
>   include/drm/drm_simple_kms_helper.h     | 6 ++++++
>   2 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index bada17166512..447631018426 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -34,6 +34,12 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
>   	.destroy = drm_encoder_cleanup,
>   };
>   
> +static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
> +				     struct drm_crtc_state *state)
> +{
> +	return drm_atomic_add_affected_planes(state->state, crtc);
> +}
> +
>   static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
>   {
>   	struct drm_simple_display_pipe *pipe;
> @@ -57,6 +63,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
>   }
>   
>   static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
> +	.atomic_check = drm_simple_kms_crtc_check,
>   	.disable = drm_simple_kms_crtc_disable,
>   	.enable = drm_simple_kms_crtc_enable,
>   };
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index 269039722f91..826946ca2b82 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -60,6 +60,12 @@ struct drm_simple_display_pipe_funcs {
>   	 *
>   	 * This function is called when the underlying plane state is updated.
>   	 * This hook is optional.
> +	 *
> +	 * This is the function drivers should submit the
> +	 * &drm_pending_vblank_event from. Using either
> +	 * drm_crtc_arm_vblank_event(), when the driver supports vblank
> +	 * interrupt handling, or drm_crtc_send_vblank_event() directly in case
> +	 * the hardware lacks vblank support entirely.
>   	 */
>   	void (*update)(struct drm_simple_display_pipe *pipe,
>   		       struct drm_plane_state *plane_state);

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/simple-helpers: Always add planes to the state update
  2016-08-24 18:38 ` Noralf Trønnes
@ 2016-08-25  6:30   ` Daniel Vetter
  2016-08-25 17:13     ` Noralf Trønnes
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2016-08-25  6:30 UTC (permalink / raw)
  To: Noralf Trønnes
  Cc: andrea.merello, Daniel Vetter, DRI Development, Daniel Vetter

On Wed, Aug 24, 2016 at 08:38:37PM +0200, Noralf Trønnes wrote:
> 
> Den 23.08.2016 08:25, skrev Daniel Vetter:
> > Our update function is hooked to the single plane, which might not get
> > called for crtc-only updates. Which is surprising, so fix this by
> > always adding the plane.
> > 
> > While at it document how&when the event should be sent out better in
> > the kerneldoc.
> > 
> > Cc: Noralf Trønnes <noralf@tronnes.org>
> > Cc: andrea.merello@gmail.com
> > Tested-and-Reported-by: andrea.merello@gmail.com
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > I'm not entirely sure we really want to put the responsibility for
> > this onto driver. Plan B) would be to remove the kerneldoc I added
> > here and call the right function from drm_simple_kms_plane_atomic_update.
> > That way simple drivers don't need to deal with that detail, and in
> > general those drivers don't care that much about the miniscule
> > possible race a generic implementation would cause. What do you
> > suggest as the best approach?
> 
> If the driver is responsible, a helper like this would be nice:
> 
> void drm_simple_display_pipe_handle_vblank_event(struct
> drm_simple_display_pipe *pipe)
> {
>     struct drm_crtc *crtc = &pipe->crtc;
> 
>     if (crtc->state && crtc->state->event) {
>         spin_lock_irq(&crtc->dev->event_lock);
>         drm_crtc_send_vblank_event(crtc, crtc->state->event);
>         spin_unlock_irq(&crtc->dev->event_lock);
>         crtc->state->event = NULL;

For a generic version we probably want to automatically switch to
drm_crtc_arm_vblank_event, if vblank support is available. That can be
checked by looking at drm_device->num_crtcs.

>     }
> }
> 
> Then if the update() callback is not set, drm_simple_kms_plane_atomic_update
> calls this, and if it is set, then the driver has to do it or something
> similar.
> 
> It is difficult to predict what future drivers will need.
> Unless there are drivers on the horizon that will need to handle the
> event themselves, I suggest that we follow your plan B. If the need arises,
> we just add a one-liner to all drivers that has the update() callback set.

Hm yeah. What about merging this patch here for now (would need your
formal reviewed-by), and then later on when we have 2-3 simple drivers
using this we can reconsider? Since I dont have hw for any of them I can't
do this myself.

> Noralf.
> 
> Sidenote:
> When I converted simpledrm I had problems with flip_done timeouts.
> In the end I had to check for the event in disable/enable/update.
> Trying it again now, it's enough to do it in update().
> Not sure what has changed.

Trying again with this patch, or without? Since this patch here is meant
to exactly the problem that you need to handle the event in too many
places. If it's fixed with just latest drm-misc, that would be surprising
...
-Daniel

> 
> > -Daniel
> > ---
> >   drivers/gpu/drm/drm_simple_kms_helper.c | 7 +++++++
> >   include/drm/drm_simple_kms_helper.h     | 6 ++++++
> >   2 files changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> > index bada17166512..447631018426 100644
> > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > @@ -34,6 +34,12 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> >   	.destroy = drm_encoder_cleanup,
> >   };
> > +static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
> > +				     struct drm_crtc_state *state)
> > +{
> > +	return drm_atomic_add_affected_planes(state->state, crtc);
> > +}
> > +
> >   static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
> >   {
> >   	struct drm_simple_display_pipe *pipe;
> > @@ -57,6 +63,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
> >   }
> >   static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
> > +	.atomic_check = drm_simple_kms_crtc_check,
> >   	.disable = drm_simple_kms_crtc_disable,
> >   	.enable = drm_simple_kms_crtc_enable,
> >   };
> > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> > index 269039722f91..826946ca2b82 100644
> > --- a/include/drm/drm_simple_kms_helper.h
> > +++ b/include/drm/drm_simple_kms_helper.h
> > @@ -60,6 +60,12 @@ struct drm_simple_display_pipe_funcs {
> >   	 *
> >   	 * This function is called when the underlying plane state is updated.
> >   	 * This hook is optional.
> > +	 *
> > +	 * This is the function drivers should submit the
> > +	 * &drm_pending_vblank_event from. Using either
> > +	 * drm_crtc_arm_vblank_event(), when the driver supports vblank
> > +	 * interrupt handling, or drm_crtc_send_vblank_event() directly in case
> > +	 * the hardware lacks vblank support entirely.
> >   	 */
> >   	void (*update)(struct drm_simple_display_pipe *pipe,
> >   		       struct drm_plane_state *plane_state);
> 

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/simple-helpers: Always add planes to the state update
  2016-08-25  6:30   ` Daniel Vetter
@ 2016-08-25 17:13     ` Noralf Trønnes
  2016-08-25 18:46       ` Daniel Vetter
  0 siblings, 1 reply; 5+ messages in thread
From: Noralf Trønnes @ 2016-08-25 17:13 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: andrea.merello, Daniel Vetter, DRI Development, Daniel Vetter


Den 25.08.2016 08:30, skrev Daniel Vetter:
> On Wed, Aug 24, 2016 at 08:38:37PM +0200, Noralf Trønnes wrote:
>> Den 23.08.2016 08:25, skrev Daniel Vetter:
>>> Our update function is hooked to the single plane, which might not get
>>> called for crtc-only updates. Which is surprising, so fix this by
>>> always adding the plane.
>>>
>>> While at it document how&when the event should be sent out better in
>>> the kerneldoc.
>>>
>>> Cc: Noralf Trønnes <noralf@tronnes.org>
>>> Cc: andrea.merello@gmail.com
>>> Tested-and-Reported-by: andrea.merello@gmail.com
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>> I'm not entirely sure we really want to put the responsibility for
>>> this onto driver. Plan B) would be to remove the kerneldoc I added
>>> here and call the right function from drm_simple_kms_plane_atomic_update.
>>> That way simple drivers don't need to deal with that detail, and in
>>> general those drivers don't care that much about the miniscule
>>> possible race a generic implementation would cause. What do you
>>> suggest as the best approach?
>> If the driver is responsible, a helper like this would be nice:
>>
>> void drm_simple_display_pipe_handle_vblank_event(struct
>> drm_simple_display_pipe *pipe)
>> {
>>      struct drm_crtc *crtc = &pipe->crtc;
>>
>>      if (crtc->state && crtc->state->event) {
>>          spin_lock_irq(&crtc->dev->event_lock);
>>          drm_crtc_send_vblank_event(crtc, crtc->state->event);
>>          spin_unlock_irq(&crtc->dev->event_lock);
>>          crtc->state->event = NULL;
> For a generic version we probably want to automatically switch to
> drm_crtc_arm_vblank_event, if vblank support is available. That can be
> checked by looking at drm_device->num_crtcs.
>
>>      }
>> }
>>
>> Then if the update() callback is not set, drm_simple_kms_plane_atomic_update
>> calls this, and if it is set, then the driver has to do it or something
>> similar.
>>
>> It is difficult to predict what future drivers will need.
>> Unless there are drivers on the horizon that will need to handle the
>> event themselves, I suggest that we follow your plan B. If the need arises,
>> we just add a one-liner to all drivers that has the update() callback set.
> Hm yeah. What about merging this patch here for now (would need your
> formal reviewed-by), and then later on when we have 2-3 simple drivers
> using this we can reconsider? Since I dont have hw for any of them I can't
> do this myself.
>
>> Noralf.
>>
>> Sidenote:
>> When I converted simpledrm I had problems with flip_done timeouts.
>> In the end I had to check for the event in disable/enable/update.
>> Trying it again now, it's enough to do it in update().
>> Not sure what has changed.
> Trying again with this patch, or without? Since this patch here is meant
> to exactly the problem that you need to handle the event in too many
> places. If it's fixed with just latest drm-misc, that would be surprising
> ...

I have updated Raspian so I could use xorg to test with, and indeed
without this patch and without sending vblank event in disable and enable,
the flip_done timeout showed up. modetest -v didn't trigger it.
With this patch I only have to send the vblank event in update().

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

> -Daniel
>
>>> -Daniel
>>> ---
>>>    drivers/gpu/drm/drm_simple_kms_helper.c | 7 +++++++
>>>    include/drm/drm_simple_kms_helper.h     | 6 ++++++
>>>    2 files changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
>>> index bada17166512..447631018426 100644
>>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>>> @@ -34,6 +34,12 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
>>>    	.destroy = drm_encoder_cleanup,
>>>    };
>>> +static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
>>> +				     struct drm_crtc_state *state)
>>> +{
>>> +	return drm_atomic_add_affected_planes(state->state, crtc);
>>> +}
>>> +
>>>    static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
>>>    {
>>>    	struct drm_simple_display_pipe *pipe;
>>> @@ -57,6 +63,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
>>>    }
>>>    static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
>>> +	.atomic_check = drm_simple_kms_crtc_check,
>>>    	.disable = drm_simple_kms_crtc_disable,
>>>    	.enable = drm_simple_kms_crtc_enable,
>>>    };
>>> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
>>> index 269039722f91..826946ca2b82 100644
>>> --- a/include/drm/drm_simple_kms_helper.h
>>> +++ b/include/drm/drm_simple_kms_helper.h
>>> @@ -60,6 +60,12 @@ struct drm_simple_display_pipe_funcs {
>>>    	 *
>>>    	 * This function is called when the underlying plane state is updated.
>>>    	 * This hook is optional.
>>> +	 *
>>> +	 * This is the function drivers should submit the
>>> +	 * &drm_pending_vblank_event from. Using either
>>> +	 * drm_crtc_arm_vblank_event(), when the driver supports vblank
>>> +	 * interrupt handling, or drm_crtc_send_vblank_event() directly in case
>>> +	 * the hardware lacks vblank support entirely.
>>>    	 */
>>>    	void (*update)(struct drm_simple_display_pipe *pipe,
>>>    		       struct drm_plane_state *plane_state);

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/simple-helpers: Always add planes to the state update
  2016-08-25 17:13     ` Noralf Trønnes
@ 2016-08-25 18:46       ` Daniel Vetter
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2016-08-25 18:46 UTC (permalink / raw)
  To: Noralf Trønnes
  Cc: andrea.merello, Daniel Vetter, DRI Development, Daniel Vetter

On Thu, Aug 25, 2016 at 07:13:07PM +0200, Noralf Trønnes wrote:
> 
> Den 25.08.2016 08:30, skrev Daniel Vetter:
> > On Wed, Aug 24, 2016 at 08:38:37PM +0200, Noralf Trønnes wrote:
> > > Den 23.08.2016 08:25, skrev Daniel Vetter:
> > > > Our update function is hooked to the single plane, which might not get
> > > > called for crtc-only updates. Which is surprising, so fix this by
> > > > always adding the plane.
> > > > 
> > > > While at it document how&when the event should be sent out better in
> > > > the kerneldoc.
> > > > 
> > > > Cc: Noralf Trønnes <noralf@tronnes.org>
> > > > Cc: andrea.merello@gmail.com
> > > > Tested-and-Reported-by: andrea.merello@gmail.com
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > ---
> > > > I'm not entirely sure we really want to put the responsibility for
> > > > this onto driver. Plan B) would be to remove the kerneldoc I added
> > > > here and call the right function from drm_simple_kms_plane_atomic_update.
> > > > That way simple drivers don't need to deal with that detail, and in
> > > > general those drivers don't care that much about the miniscule
> > > > possible race a generic implementation would cause. What do you
> > > > suggest as the best approach?
> > > If the driver is responsible, a helper like this would be nice:
> > > 
> > > void drm_simple_display_pipe_handle_vblank_event(struct
> > > drm_simple_display_pipe *pipe)
> > > {
> > >      struct drm_crtc *crtc = &pipe->crtc;
> > > 
> > >      if (crtc->state && crtc->state->event) {
> > >          spin_lock_irq(&crtc->dev->event_lock);
> > >          drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > >          spin_unlock_irq(&crtc->dev->event_lock);
> > >          crtc->state->event = NULL;
> > For a generic version we probably want to automatically switch to
> > drm_crtc_arm_vblank_event, if vblank support is available. That can be
> > checked by looking at drm_device->num_crtcs.
> > 
> > >      }
> > > }
> > > 
> > > Then if the update() callback is not set, drm_simple_kms_plane_atomic_update
> > > calls this, and if it is set, then the driver has to do it or something
> > > similar.
> > > 
> > > It is difficult to predict what future drivers will need.
> > > Unless there are drivers on the horizon that will need to handle the
> > > event themselves, I suggest that we follow your plan B. If the need arises,
> > > we just add a one-liner to all drivers that has the update() callback set.
> > Hm yeah. What about merging this patch here for now (would need your
> > formal reviewed-by), and then later on when we have 2-3 simple drivers
> > using this we can reconsider? Since I dont have hw for any of them I can't
> > do this myself.
> > 
> > > Noralf.
> > > 
> > > Sidenote:
> > > When I converted simpledrm I had problems with flip_done timeouts.
> > > In the end I had to check for the event in disable/enable/update.
> > > Trying it again now, it's enough to do it in update().
> > > Not sure what has changed.
> > Trying again with this patch, or without? Since this patch here is meant
> > to exactly the problem that you need to handle the event in too many
> > places. If it's fixed with just latest drm-misc, that would be surprising
> > ...
> 
> I have updated Raspian so I could use xorg to test with, and indeed
> without this patch and without sending vblank event in disable and enable,
> the flip_done timeout showed up. modetest -v didn't trigger it.
> With this patch I only have to send the vblank event in update().
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Thanks for the review, pushed onto -misc.
-Daniel

> 
> > -Daniel
> > 
> > > > -Daniel
> > > > ---
> > > >    drivers/gpu/drm/drm_simple_kms_helper.c | 7 +++++++
> > > >    include/drm/drm_simple_kms_helper.h     | 6 ++++++
> > > >    2 files changed, 13 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > index bada17166512..447631018426 100644
> > > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > @@ -34,6 +34,12 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> > > >    	.destroy = drm_encoder_cleanup,
> > > >    };
> > > > +static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
> > > > +				     struct drm_crtc_state *state)
> > > > +{
> > > > +	return drm_atomic_add_affected_planes(state->state, crtc);
> > > > +}
> > > > +
> > > >    static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
> > > >    {
> > > >    	struct drm_simple_display_pipe *pipe;
> > > > @@ -57,6 +63,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
> > > >    }
> > > >    static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
> > > > +	.atomic_check = drm_simple_kms_crtc_check,
> > > >    	.disable = drm_simple_kms_crtc_disable,
> > > >    	.enable = drm_simple_kms_crtc_enable,
> > > >    };
> > > > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> > > > index 269039722f91..826946ca2b82 100644
> > > > --- a/include/drm/drm_simple_kms_helper.h
> > > > +++ b/include/drm/drm_simple_kms_helper.h
> > > > @@ -60,6 +60,12 @@ struct drm_simple_display_pipe_funcs {
> > > >    	 *
> > > >    	 * This function is called when the underlying plane state is updated.
> > > >    	 * This hook is optional.
> > > > +	 *
> > > > +	 * This is the function drivers should submit the
> > > > +	 * &drm_pending_vblank_event from. Using either
> > > > +	 * drm_crtc_arm_vblank_event(), when the driver supports vblank
> > > > +	 * interrupt handling, or drm_crtc_send_vblank_event() directly in case
> > > > +	 * the hardware lacks vblank support entirely.
> > > >    	 */
> > > >    	void (*update)(struct drm_simple_display_pipe *pipe,
> > > >    		       struct drm_plane_state *plane_state);
> 

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-08-25 18:46 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-23  6:25 [PATCH] drm/simple-helpers: Always add planes to the state update Daniel Vetter
2016-08-24 18:38 ` Noralf Trønnes
2016-08-25  6:30   ` Daniel Vetter
2016-08-25 17:13     ` Noralf Trønnes
2016-08-25 18:46       ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.