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@xxxxxxxxxxx>
Cc: andrea.merello@xxxxxxxxx
Tested-and-Reported-by: andrea.merello@xxxxxxxxx
Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
---
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@xxxxxxxxxxx>
-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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
