| Message ID | 1471933540-31131-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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);
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); >
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);
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); >
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);
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(+)