Hello,
This is the second version of this series adding writeback support to the VC4 display engine.
This version is based on drm-misc-next and include a bunch of modifications to core that I had to add to make it work on VC4.
Feel free to comment on those modifications.
On the driver side, no much has changed except I modified a bit the implementation to adjust the latest revision of the writeback interface.
Regards,
Boris
Boris Brezillon (8): drm/atomic: Avoid connector to writeback_connector casts drm/connector: Pass a drm_connector_state to ->atomic_commit() drm/vc4: Use wait_for_flip_done() instead of wait_for_vblanks() drm/crtc: Add a generic infrastructure to fake VBLANK events drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic commit_tail() helpers drm/vc4: Call drm_atomic_helper_fake_vblank() in the commit path drm/vc4: Add support for the transposer block ARM: dts: bcm283x: Add Transposer block
.../devicetree/bindings/display/brcm,bcm-vc4.txt | 6 + arch/arm/boot/dts/bcm283x.dtsi | 6 + drivers/gpu/drm/drm_atomic.c | 4 +- drivers/gpu/drm/drm_atomic_helper.c | 46 +- drivers/gpu/drm/vc4/Makefile | 1 + drivers/gpu/drm/vc4/vc4_crtc.c | 139 +++++- drivers/gpu/drm/vc4/vc4_debugfs.c | 1 + drivers/gpu/drm/vc4/vc4_drv.c | 1 + drivers/gpu/drm/vc4/vc4_drv.h | 7 + drivers/gpu/drm/vc4/vc4_kms.c | 11 +- drivers/gpu/drm/vc4/vc4_regs.h | 1 + drivers/gpu/drm/vc4/vc4_txp.c | 487 +++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 + include/drm/drm_modeset_helper_vtables.h | 4 +- include/drm/drm_writeback.h | 6 + 16 files changed, 700 insertions(+), 36 deletions(-) create mode 100644 drivers/gpu/drm/vc4/vc4_txp.c
Use container_of() instead of type casting so that it keeps working even if base is moved inside the drm_writeback_connector struct.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- drivers/gpu/drm/drm_atomic.c | 4 +++- include/drm/drm_writeback.h | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 178842380f75..6ea20d5dee66 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2423,6 +2423,7 @@ static int prepare_signaling(struct drm_device *dev, }
for_each_new_connector_in_state(state, conn, conn_state, i) { + struct drm_writeback_connector *wb_conn; struct drm_writeback_job *job; struct drm_out_fence_state *f; struct dma_fence *fence; @@ -2446,7 +2447,8 @@ static int prepare_signaling(struct drm_device *dev, f[*num_fences].out_fence_ptr = fence_ptr; *fence_state = f;
- fence = drm_writeback_get_out_fence((struct drm_writeback_connector *)conn); + wb_conn = drm_connector_to_writeback(conn); + fence = drm_writeback_get_out_fence(wb_conn); if (!fence) return -ENOMEM;
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index a10fe556dfd4..23df9d463003 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -110,6 +110,12 @@ struct drm_writeback_job { struct dma_fence *out_fence; };
+static inline struct drm_writeback_connector * +drm_connector_to_writeback(struct drm_connector *connector) +{ + return container_of(connector, struct drm_writeback_connector, base); +} + int drm_writeback_connector_init(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs,
On Fri, Jun 29, 2018 at 01:17:14PM +0200, Boris Brezillon wrote:
Use container_of() instead of type casting so that it keeps working even if base is moved inside the drm_writeback_connector struct.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_atomic.c | 4 +++- include/drm/drm_writeback.h | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 178842380f75..6ea20d5dee66 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2423,6 +2423,7 @@ static int prepare_signaling(struct drm_device *dev, }
for_each_new_connector_in_state(state, conn, conn_state, i) {
struct drm_writeback_connector *wb_conn;@@ -2446,7 +2447,8 @@ static int prepare_signaling(struct drm_device *dev, f[*num_fences].out_fence_ptr = fence_ptr; *fence_state = f;
fence = drm_writeback_get_out_fence((struct drm_writeback_connector *)conn);
wb_conn = drm_connector_to_writeback(conn);fence = drm_writeback_get_out_fence(wb_conn);diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index a10fe556dfd4..23df9d463003 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -110,6 +110,12 @@ struct drm_writeback_job { struct dma_fence *out_fence; };
+static inline struct drm_writeback_connector * +drm_connector_to_writeback(struct drm_connector *connector) +{
- return container_of(connector, struct drm_writeback_connector, base);
+}
int drm_writeback_connector_init(struct drm_device *dev, struct drm_writeback_connector *wb_connector, const struct drm_connector_funcs *con_funcs, -- 2.14.1
Other atomic hooks are passed state objects, let's change this one to be consistent.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- drivers/gpu/drm/drm_atomic_helper.c | 2 +- include/drm/drm_modeset_helper_vtables.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 17baf5057132..69063bcf2334 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) { WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK); - funcs->atomic_commit(connector, new_conn_state->writeback_job); + funcs->atomic_commit(connector, new_conn_state); } } } diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 3b289773297c..fb841f44949c 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -980,11 +980,13 @@ struct drm_connector_helper_funcs { * * This hook is to be used by drivers implementing writeback connectors * that need a point when to commit the writeback job to the hardware. + * The writeback_job to commit is available in + * &drm_connector_state.writeback_job. * * This callback is used by the atomic modeset helpers. */ void (*atomic_commit)(struct drm_connector *connector, - struct drm_writeback_job *writeback_job); + struct drm_connector_state *state); };
/**
On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:
Other atomic hooks are passed state objects, let's change this one to be consistent.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
Acked-by: Liviu Dudau liviu.dudau@arm.com
drivers/gpu/drm/drm_atomic_helper.c | 2 +- include/drm/drm_modeset_helper_vtables.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 17baf5057132..69063bcf2334 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) { WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
funcs->atomic_commit(connector, new_conn_state->writeback_job);
funcs->atomic_commit(connector, new_conn_state);} diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 3b289773297c..fb841f44949c 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -980,11 +980,13 @@ struct drm_connector_helper_funcs { * * This hook is to be used by drivers implementing writeback connectors * that need a point when to commit the writeback job to the hardware.
* The writeback_job to commit is available in* &drm_connector_state.writeback_job.*/ void (*atomic_commit)(struct drm_connector *connector,
- This callback is used by the atomic modeset helpers.
struct drm_writeback_job *writeback_job);
struct drm_connector_state *state);};
/**
2.14.1
On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:
Other atomic hooks are passed state objects, let's change this one to be consistent.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/drm_atomic_helper.c | 2 +- include/drm/drm_modeset_helper_vtables.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 17baf5057132..69063bcf2334 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) { WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
funcs->atomic_commit(connector, new_conn_state->writeback_job);
funcs->atomic_commit(connector, new_conn_state);
Forgot to add: I think it is worth adding a check here that the hook has been implemented by the driver, AFAIK it is not a mandatory hook, even for writeback enabled drivers.
Let me know what you plan to do with this patch as I will have to update the malidp patches as well, otherwise linux-next is going to flag me.
Best regards, Liviu
}} } diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 3b289773297c..fb841f44949c 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -980,11 +980,13 @@ struct drm_connector_helper_funcs { * * This hook is to be used by drivers implementing writeback connectors * that need a point when to commit the writeback job to the hardware.
* The writeback_job to commit is available in* &drm_connector_state.writeback_job.*/ void (*atomic_commit)(struct drm_connector *connector,
- This callback is used by the atomic modeset helpers.
struct drm_writeback_job *writeback_job);
struct drm_connector_state *state);};
/**
2.14.1
On Fri, Jun 29, 2018 at 12:37:10PM +0100, Liviu Dudau wrote:
On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:
Other atomic hooks are passed state objects, let's change this one to be consistent.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/drm_atomic_helper.c | 2 +- include/drm/drm_modeset_helper_vtables.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 17baf5057132..69063bcf2334 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) { WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
funcs->atomic_commit(connector, new_conn_state->writeback_job);
funcs->atomic_commit(connector, new_conn_state);Forgot to add: I think it is worth adding a check here that the hook has been implemented by the driver, AFAIK it is not a mandatory hook, even for writeback enabled drivers.
Either way this should be documented in the hook (atm it says nothing about whether it's mandatory/optional and for whom).
Let me know what you plan to do with this patch as I will have to update the malidp patches as well, otherwise linux-next is going to flag me.
Probably simplest if you send a pull to Dave right away with the current malidp code, then we backmerge that into drm-misc-next, and Boris rebases. There's no point in hanging onto pull requests until right before the feature freeze, just makes things more complicated.
Patch itself lgtm.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Best regards, Liviu
}} } diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 3b289773297c..fb841f44949c 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -980,11 +980,13 @@ struct drm_connector_helper_funcs { * * This hook is to be used by drivers implementing writeback connectors * that need a point when to commit the writeback job to the hardware.
* The writeback_job to commit is available in* &drm_connector_state.writeback_job.*/ void (*atomic_commit)(struct drm_connector *connector,
- This callback is used by the atomic modeset helpers.
struct drm_writeback_job *writeback_job);
struct drm_connector_state *state);};
/**
2.14.1
--
| I would like to | | fix the world, | | but they're not | | giving me the | \ source code! /
¯\_(ツ)_/¯
On Mon, 2 Jul 2018 09:51:46 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jun 29, 2018 at 12:37:10PM +0100, Liviu Dudau wrote:
On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:
Other atomic hooks are passed state objects, let's change this one to be consistent.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/drm_atomic_helper.c | 2 +- include/drm/drm_modeset_helper_vtables.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 17baf5057132..69063bcf2334 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) { WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
funcs->atomic_commit(connector, new_conn_state->writeback_job);
funcs->atomic_commit(connector, new_conn_state);Forgot to add: I think it is worth adding a check here that the hook has been implemented by the driver, AFAIK it is not a mandatory hook, even for writeback enabled drivers.
I'm just curious, from where do you queue the writeback job if you don't have a connector->atomic_commit() hook implemented? AFAICT, the encoder->enable() method is only called when the encoder is being enabled, and not every time you update the FB_ID prop of the writeback connector. Am I missing something?
Either way this should be documented in the hook (atm it says nothing about whether it's mandatory/optional and for whom).
I'm fine making this hook optional. I'll update the code and the doc in a separate commit.
On Mon, Jul 02, 2018 at 11:49:11AM +0200, Boris Brezillon wrote:
On Mon, 2 Jul 2018 09:51:46 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jun 29, 2018 at 12:37:10PM +0100, Liviu Dudau wrote:
On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:
Other atomic hooks are passed state objects, let's change this one to be consistent.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/drm_atomic_helper.c | 2 +- include/drm/drm_modeset_helper_vtables.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 17baf5057132..69063bcf2334 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) { WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
funcs->atomic_commit(connector, new_conn_state->writeback_job);
funcs->atomic_commit(connector, new_conn_state);Forgot to add: I think it is worth adding a check here that the hook has been implemented by the driver, AFAIK it is not a mandatory hook, even for writeback enabled drivers.
I'm just curious, from where do you queue the writeback job if you don't have a connector->atomic_commit() hook implemented? AFAICT, the encoder->enable() method is only called when the encoder is being enabled, and not every time you update the FB_ID prop of the writeback connector. Am I missing something?
In malidp_drv.c:malidp_atomic_commit_tail() we call malidp_mw_atomic_commit() after drm_atomic_helper_commit_planes(drm, state, DRM_PLANE_COMMIT_ACTIVE_ONLY).
malidp_mw_atomic_commit() then checks the writeback job and if it has an fb associated with it then it calls drm_writeback_queue_job() before enabling the memwrite hardware bits.
You can see it all in action here:
https://github.com/ARM-software/linux/commit/f7a88ca52530958703bcfc30adc3028...
Best regards, Liviu
Either way this should be documented in the hook (atm it says nothing about whether it's mandatory/optional and for whom).
I'm fine making this hook optional. I'll update the code and the doc in a separate commit.
On Mon, 2 Jul 2018 12:14:20 +0100 Liviu Dudau Liviu.Dudau@arm.com wrote:
On Mon, Jul 02, 2018 at 11:49:11AM +0200, Boris Brezillon wrote:
On Mon, 2 Jul 2018 09:51:46 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jun 29, 2018 at 12:37:10PM +0100, Liviu Dudau wrote:
On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:
Other atomic hooks are passed state objects, let's change this one to be consistent.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/drm_atomic_helper.c | 2 +- include/drm/drm_modeset_helper_vtables.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 17baf5057132..69063bcf2334 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) { WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
funcs->atomic_commit(connector, new_conn_state->writeback_job);
funcs->atomic_commit(connector, new_conn_state);Forgot to add: I think it is worth adding a check here that the hook has been implemented by the driver, AFAIK it is not a mandatory hook, even for writeback enabled drivers.
I'm just curious, from where do you queue the writeback job if you don't have a connector->atomic_commit() hook implemented? AFAICT, the encoder->enable() method is only called when the encoder is being enabled, and not every time you update the FB_ID prop of the writeback connector. Am I missing something?
In malidp_drv.c:malidp_atomic_commit_tail() we call malidp_mw_atomic_commit() after drm_atomic_helper_commit_planes(drm, state, DRM_PLANE_COMMIT_ACTIVE_ONLY).
malidp_mw_atomic_commit() then checks the writeback job and if it has an fb associated with it then it calls drm_writeback_queue_job() before enabling the memwrite hardware bits.
Okay. That's a good reason to make it optional.
Thanks,
Boris
drm_atomic_helper_wait_for_vblanks() assumes the CRTC will continuously generate VBLANK events and the vblank counter will keep increasing. While this work for a regular pipeline, it doesn't when you have the CRTC is feeding the transposer block, because this block works in oneshot mode, and, by the time we reach drm_atomic_helper_wait_for_vblanks() the only VBLANK event might have already been sent and the VBLANK counter will stay unchanged, thus triggering a timeout.
Luckily, we can replace the drm_atomic_helper_wait_for_vblanks() call by drm_atomic_helper_wait_for_flip_done() because the only thing we want to check when calling drm_atomic_helper_wait_for_vblanks() from vc4_atomic_complete_commit() is that new FBs are in use and the old ones can be safely released.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- drivers/gpu/drm/vc4/vc4_kms.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 8a411e5f8776..91239b0a4fa0 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -153,18 +153,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
drm_atomic_helper_commit_modeset_enables(dev, state);
- /* Make sure that drm_atomic_helper_wait_for_vblanks() - * actually waits for vblank. If we're doing a full atomic - * modeset (as opposed to a vc4_update_plane() short circuit), - * then we need to wait for scanout to be done with our - * display lists before we free it and potentially reallocate - * and overwrite the dlist memory with a new modeset. - */ - state->legacy_cursor_update = false; - drm_atomic_helper_commit_hw_done(state);
- drm_atomic_helper_wait_for_vblanks(dev, state); + drm_atomic_helper_wait_for_flip_done(dev, state);
drm_atomic_helper_cleanup_planes(dev, state);
Boris Brezillon boris.brezillon@bootlin.com writes:
drm_atomic_helper_wait_for_vblanks() assumes the CRTC will continuously generate VBLANK events and the vblank counter will keep increasing. While this work for a regular pipeline, it doesn't when you have the CRTC is feeding the transposer block, because this block works in oneshot mode, and, by the time we reach drm_atomic_helper_wait_for_vblanks() the only VBLANK event might have already been sent and the VBLANK counter will stay unchanged, thus triggering a timeout.
Luckily, we can replace the drm_atomic_helper_wait_for_vblanks() call by drm_atomic_helper_wait_for_flip_done() because the only thing we want to check when calling drm_atomic_helper_wait_for_vblanks() from vc4_atomic_complete_commit() is that new FBs are in use and the old ones can be safely released.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
Reviewed-by: Eric Anholt eric@anholt.net
In some cases CRTCs are active but are not able to generating events, at least not at every frame at it's expected to. This is typically the case when the CRTC is feeding a writeback connector that has no job queued. In this situation the CRTC is usually stopped until a new job is queued, and this can lead to timeouts when part of the pipeline is updated but no new jobs are queued to the active writeback connector.
In order to solve that, we add a ->no_vblank flag to drm_crtc_state and ask the CRTC drivers to set it to true when they know they're not able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() helper can then be used to fake VBLANKs at commit time.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 ++++++++++++++ 3 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 69063bcf2334..ca586993c2a2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) } EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
+/** + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed + * @old_state: atomic state object with old state structures + * + * This function walks all CRTCs and fake VBLANK events on those with + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL. + * The primary use of this function is writeback connectors working in oneshot + * mode and faking VBLANK events. In this case they only fake the VBLANK event + * when a job is queued, and any change to the pipeline that does not touch the + * connector is leading to timeouts when calling + * drm_atomic_helper_wait_for_vblanks() or + * drm_atomic_helper_wait_for_flip_done(). + * + * This is part of the atomic helper support for nonblocking commits, see + * drm_atomic_helper_setup_commit() for an overview. + */ +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) +{ + struct drm_crtc_state *old_crtc_state, *new_crtc_state; + struct drm_crtc *crtc; + int i; + + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, + new_crtc_state, i) { + unsigned long flags; + + if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank) + continue; + + spin_lock_irqsave(&old_state->dev->event_lock, flags); + if (new_crtc_state->event) { + drm_crtc_send_vblank_event(crtc, + new_crtc_state->event); + new_crtc_state->event = NULL; + } + spin_unlock_irqrestore(&old_state->dev->event_lock, flags); + } +} +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank); + /** * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit * @old_state: atomic state object with old state structures diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 26aaba58d6ce..99e2a5297c69 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state, int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, bool nonblock); void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 23eddbccab10..7435dc66c08a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs; * @zpos_changed: zpos values of planes on this crtc have been updated * @color_mgmt_changed: color management properties have changed (degamma or * gamma LUT or CSC matrix) + * @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state + * usually depends on the pipeline configuration, and the main usuage is + * CRTCs feeding a writeback connector operating in oneshot mode. In this + * case the VBLANK event is only generated when a job is queued to the + * writeback connector, and we want the core to fake VBLANK events when + * this part of the pipeline hasn't changed but others had or when the + * CRTC and connectors are disabled. + * __drm_atomic_helper_crtc_duplicate_state() will the value from the + * current state, the CRTC driver is then responsible for updating this + * field when needed. + * Note that, even when no_blank is set to true, the CRTC driver can still + * steal the &drm_crtc_state.event object and send the event on its own. + * That's usually what happens when a job is queued to the writeback + * connector. * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders @@ -118,6 +132,7 @@ struct drm_crtc_state { bool connectors_changed : 1; bool zpos_changed : 1; bool color_mgmt_changed : 1; + bool no_vblank : 1;
/* attached planes bitmask: * WARNING: transitional helpers do not maintain plane_mask so
On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
In some cases CRTCs are active but are not able to generating events, at least not at every frame at it's expected to. This is typically the case when the CRTC is feeding a writeback connector that has no job queued. In this situation the CRTC is usually stopped until a new job is queued, and this can lead to timeouts when part of the pipeline is updated but no new jobs are queued to the active writeback connector.
In order to solve that, we add a ->no_vblank flag to drm_crtc_state and ask the CRTC drivers to set it to true when they know they're not able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() helper can then be used to fake VBLANKs at commit time.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
Reviewed-by: Liviu Dudau liviu.dudau@arm.com
drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 ++++++++++++++ 3 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 69063bcf2334..ca586993c2a2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) } EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
+/**
- drm_atomic_helper_fake_vblank - fake VBLANK events if needed
- @old_state: atomic state object with old state structures
- This function walks all CRTCs and fake VBLANK events on those with
- &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
- The primary use of this function is writeback connectors working in oneshot
- mode and faking VBLANK events. In this case they only fake the VBLANK event
- when a job is queued, and any change to the pipeline that does not touch the
- connector is leading to timeouts when calling
- drm_atomic_helper_wait_for_vblanks() or
- drm_atomic_helper_wait_for_flip_done().
- This is part of the atomic helper support for nonblocking commits, see
- drm_atomic_helper_setup_commit() for an overview.
- */
+void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) +{
- struct drm_crtc_state *old_crtc_state, *new_crtc_state;
- struct drm_crtc *crtc;
- int i;
- for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
new_crtc_state, i) {unsigned long flags;if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)continue;spin_lock_irqsave(&old_state->dev->event_lock, flags);if (new_crtc_state->event) {drm_crtc_send_vblank_event(crtc,new_crtc_state->event);new_crtc_state->event = NULL;}spin_unlock_irqrestore(&old_state->dev->event_lock, flags);- }
+} +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
/**
- drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
- @old_state: atomic state object with old state structures
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 26aaba58d6ce..99e2a5297c69 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state, int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, bool nonblock); void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 23eddbccab10..7435dc66c08a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
- @zpos_changed: zpos values of planes on this crtc have been updated
- @color_mgmt_changed: color management properties have changed (degamma or
- gamma LUT or CSC matrix)
- @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
- usually depends on the pipeline configuration, and the main usuage is
- CRTCs feeding a writeback connector operating in oneshot mode. In this
- case the VBLANK event is only generated when a job is queued to the
- writeback connector, and we want the core to fake VBLANK events when
- this part of the pipeline hasn't changed but others had or when the
- CRTC and connectors are disabled.
- __drm_atomic_helper_crtc_duplicate_state() will the value from the
- current state, the CRTC driver is then responsible for updating this
- field when needed.
- Note that, even when no_blank is set to true, the CRTC driver can still
- steal the &drm_crtc_state.event object and send the event on its own.
- That's usually what happens when a job is queued to the writeback
- connector.
- @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
- @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
- @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
@@ -118,6 +132,7 @@ struct drm_crtc_state { bool connectors_changed : 1; bool zpos_changed : 1; bool color_mgmt_changed : 1;
bool no_vblank : 1;
/* attached planes bitmask:
- WARNING: transitional helpers do not maintain plane_mask so
-- 2.14.1
On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
In some cases CRTCs are active but are not able to generating events, at least not at every frame at it's expected to. This is typically the case when the CRTC is feeding a writeback connector that has no job queued. In this situation the CRTC is usually stopped until a new job is queued, and this can lead to timeouts when part of the pipeline is updated but no new jobs are queued to the active writeback connector.
In order to solve that, we add a ->no_vblank flag to drm_crtc_state and ask the CRTC drivers to set it to true when they know they're not able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() helper can then be used to fake VBLANKs at commit time.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 ++++++++++++++ 3 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 69063bcf2334..ca586993c2a2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) } EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
+/**
- drm_atomic_helper_fake_vblank - fake VBLANK events if needed
- @old_state: atomic state object with old state structures
- This function walks all CRTCs and fake VBLANK events on those with
- &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
- The primary use of this function is writeback connectors working in oneshot
- mode and faking VBLANK events. In this case they only fake the VBLANK event
- when a job is queued, and any change to the pipeline that does not touch the
- connector is leading to timeouts when calling
- drm_atomic_helper_wait_for_vblanks() or
- drm_atomic_helper_wait_for_flip_done().
- This is part of the atomic helper support for nonblocking commits, see
- drm_atomic_helper_setup_commit() for an overview.
- */
+void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) +{
- struct drm_crtc_state *old_crtc_state, *new_crtc_state;
- struct drm_crtc *crtc;
- int i;
- for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
new_crtc_state, i) {unsigned long flags;if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)
Uh, this essentially makes it impossible to reset no_vblank. For control flow state bits we only check the new state for it (see e.g. the various *_changed or plane_bits or whatever).
continue;spin_lock_irqsave(&old_state->dev->event_lock, flags);if (new_crtc_state->event) {drm_crtc_send_vblank_event(crtc,new_crtc_state->event);new_crtc_state->event = NULL;}spin_unlock_irqrestore(&old_state->dev->event_lock, flags);- }
+} +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
/**
- drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
- @old_state: atomic state object with old state structures
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 26aaba58d6ce..99e2a5297c69 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state, int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, bool nonblock); void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 23eddbccab10..7435dc66c08a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
- @zpos_changed: zpos values of planes on this crtc have been updated
- @color_mgmt_changed: color management properties have changed (degamma or
- gamma LUT or CSC matrix)
- @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
- usually depends on the pipeline configuration, and the main usuage is
- CRTCs feeding a writeback connector operating in oneshot mode. In this
- case the VBLANK event is only generated when a job is queued to the
- writeback connector, and we want the core to fake VBLANK events when
- this part of the pipeline hasn't changed but others had or when the
- CRTC and connectors are disabled.
s/are disabled/are getting disabled/
You'll never get a request for an event when it's already off.
- __drm_atomic_helper_crtc_duplicate_state() will the value from the
- current state, the CRTC driver is then responsible for updating this
- field when needed.
Not parsing the above ... probably missing a "... will not reset ..."
- Note that, even when no_blank is set to true, the CRTC driver can still
- steal the &drm_crtc_state.event object and send the event on its own.
- That's usually what happens when a job is queued to the writeback
- connector.
The last sentence is confusing imo. Just drop it?
Please use the inline comment style for struct members, and then also polish the formatting a bit (e.g. paragraph breaks, which are only possible with the inline style).
With the nits addressed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
- @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
- @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
@@ -118,6 +132,7 @@ struct drm_crtc_state { bool connectors_changed : 1; bool zpos_changed : 1; bool color_mgmt_changed : 1;
bool no_vblank : 1;
/* attached planes bitmask:
- WARNING: transitional helpers do not maintain plane_mask so
-- 2.14.1
On Mon, 2 Jul 2018 10:02:52 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
In some cases CRTCs are active but are not able to generating events, at least not at every frame at it's expected to. This is typically the case when the CRTC is feeding a writeback connector that has no job queued. In this situation the CRTC is usually stopped until a new job is queued, and this can lead to timeouts when part of the pipeline is updated but no new jobs are queued to the active writeback connector.
In order to solve that, we add a ->no_vblank flag to drm_crtc_state and ask the CRTC drivers to set it to true when they know they're not able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() helper can then be used to fake VBLANKs at commit time.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 ++++++++++++++ 3 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 69063bcf2334..ca586993c2a2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) } EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
+/**
- drm_atomic_helper_fake_vblank - fake VBLANK events if needed
- @old_state: atomic state object with old state structures
- This function walks all CRTCs and fake VBLANK events on those with
- &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
- The primary use of this function is writeback connectors working in oneshot
- mode and faking VBLANK events. In this case they only fake the VBLANK event
- when a job is queued, and any change to the pipeline that does not touch the
- connector is leading to timeouts when calling
- drm_atomic_helper_wait_for_vblanks() or
- drm_atomic_helper_wait_for_flip_done().
- This is part of the atomic helper support for nonblocking commits, see
- drm_atomic_helper_setup_commit() for an overview.
- */
+void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) +{
- struct drm_crtc_state *old_crtc_state, *new_crtc_state;
- struct drm_crtc *crtc;
- int i;
- for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
new_crtc_state, i) {unsigned long flags;if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)Uh, this essentially makes it impossible to reset no_vblank.
I don't want ->no_vblank to be reset by the core. It's up to the CRTC driver to clear/set it when something changes in the pipeline.
For control flow state bits we only check the new state for it (see e.g. the various *_changed or plane_bits or whatever).
I tried with !new_crtc_state->no_vblank only, but then it does not handle the case where the CRTC and connector are being disabled, and I end up with a timeout.
continue;spin_lock_irqsave(&old_state->dev->event_lock, flags);if (new_crtc_state->event) {drm_crtc_send_vblank_event(crtc,new_crtc_state->event);new_crtc_state->event = NULL;}spin_unlock_irqrestore(&old_state->dev->event_lock, flags);- }
+} +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
/**
- drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
- @old_state: atomic state object with old state structures
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 26aaba58d6ce..99e2a5297c69 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state, int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, bool nonblock); void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 23eddbccab10..7435dc66c08a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
- @zpos_changed: zpos values of planes on this crtc have been updated
- @color_mgmt_changed: color management properties have changed (degamma or
- gamma LUT or CSC matrix)
- @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
- usually depends on the pipeline configuration, and the main usuage is
- CRTCs feeding a writeback connector operating in oneshot mode. In this
- case the VBLANK event is only generated when a job is queued to the
- writeback connector, and we want the core to fake VBLANK events when
- this part of the pipeline hasn't changed but others had or when the
- CRTC and connectors are disabled.
s/are disabled/are getting disabled/
You'll never get a request for an event when it's already off.
That's true.
- __drm_atomic_helper_crtc_duplicate_state() will the value from the
- current state, the CRTC driver is then responsible for updating this
- field when needed.
Not parsing the above ... probably missing a "... will not reset ..."
Exactly. Will add the missing words.
- Note that, even when no_blank is set to true, the CRTC driver can still
- steal the &drm_crtc_state.event object and send the event on its own.
- That's usually what happens when a job is queued to the writeback
- connector.
The last sentence is confusing imo. Just drop it?
Yes, I know, but it's also important to state that the ->no_blank + event == NULL is a valid combination, and just means that the driver decided to generate the event (that happens when a new WB job is queued).
Please use the inline comment style for struct members, and then also polish the formatting a bit (e.g. paragraph breaks, which are only possible with the inline style).
I considered that, but the other fields were already documented in the single block above the struct, so I thought keeping things consistent was better. Should I just add this field doc inline and keep the other ones where they are, or should I add a patch moving all docs inline?
With the nits addressed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
- @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
- @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
@@ -118,6 +132,7 @@ struct drm_crtc_state { bool connectors_changed : 1; bool zpos_changed : 1; bool color_mgmt_changed : 1;
bool no_vblank : 1;
/* attached planes bitmask:
- WARNING: transitional helpers do not maintain plane_mask so
-- 2.14.1
On Mon, Jul 02, 2018 at 10:14:51AM +0200, Boris Brezillon wrote:
On Mon, 2 Jul 2018 10:02:52 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
In some cases CRTCs are active but are not able to generating events, at least not at every frame at it's expected to. This is typically the case when the CRTC is feeding a writeback connector that has no job queued. In this situation the CRTC is usually stopped until a new job is queued, and this can lead to timeouts when part of the pipeline is updated but no new jobs are queued to the active writeback connector.
In order to solve that, we add a ->no_vblank flag to drm_crtc_state and ask the CRTC drivers to set it to true when they know they're not able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() helper can then be used to fake VBLANKs at commit time.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 ++++++++++++++ 3 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 69063bcf2334..ca586993c2a2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) } EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
+/**
- drm_atomic_helper_fake_vblank - fake VBLANK events if needed
- @old_state: atomic state object with old state structures
- This function walks all CRTCs and fake VBLANK events on those with
- &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
- The primary use of this function is writeback connectors working in oneshot
- mode and faking VBLANK events. In this case they only fake the VBLANK event
- when a job is queued, and any change to the pipeline that does not touch the
- connector is leading to timeouts when calling
- drm_atomic_helper_wait_for_vblanks() or
- drm_atomic_helper_wait_for_flip_done().
- This is part of the atomic helper support for nonblocking commits, see
- drm_atomic_helper_setup_commit() for an overview.
- */
+void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) +{
- struct drm_crtc_state *old_crtc_state, *new_crtc_state;
- struct drm_crtc *crtc;
- int i;
- for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
new_crtc_state, i) {unsigned long flags;if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)Uh, this essentially makes it impossible to reset no_vblank.
I don't want ->no_vblank to be reset by the core. It's up to the CRTC driver to clear/set it when something changes in the pipeline.
For control flow state bits we only check the new state for it (see e.g. the various *_changed or plane_bits or whatever).
I tried with !new_crtc_state->no_vblank only, but then it does not handle the case where the CRTC and connector are being disabled, and I end up with a timeout.
Why that? You should have a new_crtc_state even when you disable the crtc, and you can set the ->no_vblank on that one too. With your current code it's essentially impossible to reset ->no_vblank, since it will only have effect for the _next_ atomic update, not the one you're doing right now. That doesn't make sense.
E.g. try to do the same logic of checking both old&new state for e.g. needs_modeset and watch how all the logic totally falls to pieces. -Daniel
continue;spin_lock_irqsave(&old_state->dev->event_lock, flags);if (new_crtc_state->event) {drm_crtc_send_vblank_event(crtc,new_crtc_state->event);new_crtc_state->event = NULL;}spin_unlock_irqrestore(&old_state->dev->event_lock, flags);- }
+} +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
/**
- drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
- @old_state: atomic state object with old state structures
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 26aaba58d6ce..99e2a5297c69 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state, int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, bool nonblock); void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 23eddbccab10..7435dc66c08a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
- @zpos_changed: zpos values of planes on this crtc have been updated
- @color_mgmt_changed: color management properties have changed (degamma or
- gamma LUT or CSC matrix)
- @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
- usually depends on the pipeline configuration, and the main usuage is
- CRTCs feeding a writeback connector operating in oneshot mode. In this
- case the VBLANK event is only generated when a job is queued to the
- writeback connector, and we want the core to fake VBLANK events when
- this part of the pipeline hasn't changed but others had or when the
- CRTC and connectors are disabled.
s/are disabled/are getting disabled/
You'll never get a request for an event when it's already off.
That's true.
- __drm_atomic_helper_crtc_duplicate_state() will the value from the
- current state, the CRTC driver is then responsible for updating this
- field when needed.
Not parsing the above ... probably missing a "... will not reset ..."
Exactly. Will add the missing words.
- Note that, even when no_blank is set to true, the CRTC driver can still
- steal the &drm_crtc_state.event object and send the event on its own.
- That's usually what happens when a job is queued to the writeback
- connector.
The last sentence is confusing imo. Just drop it?
Yes, I know, but it's also important to state that the ->no_blank + event == NULL is a valid combination, and just means that the driver decided to generate the event (that happens when a new WB job is queued).
Please use the inline comment style for struct members, and then also polish the formatting a bit (e.g. paragraph breaks, which are only possible with the inline style).
I considered that, but the other fields were already documented in the single block above the struct, so I thought keeping things consistent was better. Should I just add this field doc inline and keep the other ones where they are, or should I add a patch moving all docs inline?
With the nits addressed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
- @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
- @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
@@ -118,6 +132,7 @@ struct drm_crtc_state { bool connectors_changed : 1; bool zpos_changed : 1; bool color_mgmt_changed : 1;
bool no_vblank : 1;
/* attached planes bitmask:
- WARNING: transitional helpers do not maintain plane_mask so
-- 2.14.1
On Mon, 2 Jul 2018 10:37:22 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Jul 02, 2018 at 10:14:51AM +0200, Boris Brezillon wrote:
On Mon, 2 Jul 2018 10:02:52 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
In some cases CRTCs are active but are not able to generating events, at least not at every frame at it's expected to. This is typically the case when the CRTC is feeding a writeback connector that has no job queued. In this situation the CRTC is usually stopped until a new job is queued, and this can lead to timeouts when part of the pipeline is updated but no new jobs are queued to the active writeback connector.
In order to solve that, we add a ->no_vblank flag to drm_crtc_state and ask the CRTC drivers to set it to true when they know they're not able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() helper can then be used to fake VBLANKs at commit time.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 ++++++++++++++ 3 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 69063bcf2334..ca586993c2a2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) } EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
+/**
- drm_atomic_helper_fake_vblank - fake VBLANK events if needed
- @old_state: atomic state object with old state structures
- This function walks all CRTCs and fake VBLANK events on those with
- &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
- The primary use of this function is writeback connectors working in oneshot
- mode and faking VBLANK events. In this case they only fake the VBLANK event
- when a job is queued, and any change to the pipeline that does not touch the
- connector is leading to timeouts when calling
- drm_atomic_helper_wait_for_vblanks() or
- drm_atomic_helper_wait_for_flip_done().
- This is part of the atomic helper support for nonblocking commits, see
- drm_atomic_helper_setup_commit() for an overview.
- */
+void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) +{
- struct drm_crtc_state *old_crtc_state, *new_crtc_state;
- struct drm_crtc *crtc;
- int i;
- for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
new_crtc_state, i) {unsigned long flags;if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)Uh, this essentially makes it impossible to reset no_vblank.
I don't want ->no_vblank to be reset by the core. It's up to the CRTC driver to clear/set it when something changes in the pipeline.
For control flow state bits we only check the new state for it (see e.g. the various *_changed or plane_bits or whatever).
I tried with !new_crtc_state->no_vblank only, but then it does not handle the case where the CRTC and connector are being disabled, and I end up with a timeout.
Why that? You should have a new_crtc_state even when you disable the crtc, and you can set the ->no_vblank on that one too.
Hm, right. I'll have to check why I didn't have ->no_blank set to true when I disable the CRTC. Probably a problem in vc4_crtc.c.
Forgot to address your other comments.
On Mon, Jul 02, 2018 at 10:14:51AM +0200, Boris Brezillon wrote:
On Mon, 2 Jul 2018 10:02:52 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
In some cases CRTCs are active but are not able to generating events, at least not at every frame at it's expected to. This is typically the case when the CRTC is feeding a writeback connector that has no job queued. In this situation the CRTC is usually stopped until a new job is queued, and this can lead to timeouts when part of the pipeline is updated but no new jobs are queued to the active writeback connector.
In order to solve that, we add a ->no_vblank flag to drm_crtc_state and ask the CRTC drivers to set it to true when they know they're not able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() helper can then be used to fake VBLANKs at commit time.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 ++++++++++++++ 3 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 69063bcf2334..ca586993c2a2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) } EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
+/**
- drm_atomic_helper_fake_vblank - fake VBLANK events if needed
- @old_state: atomic state object with old state structures
- This function walks all CRTCs and fake VBLANK events on those with
- &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
- The primary use of this function is writeback connectors working in oneshot
- mode and faking VBLANK events. In this case they only fake the VBLANK event
- when a job is queued, and any change to the pipeline that does not touch the
- connector is leading to timeouts when calling
- drm_atomic_helper_wait_for_vblanks() or
- drm_atomic_helper_wait_for_flip_done().
- This is part of the atomic helper support for nonblocking commits, see
- drm_atomic_helper_setup_commit() for an overview.
- */
+void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) +{
- struct drm_crtc_state *old_crtc_state, *new_crtc_state;
- struct drm_crtc *crtc;
- int i;
- for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
new_crtc_state, i) {unsigned long flags;if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)Uh, this essentially makes it impossible to reset no_vblank.
I don't want ->no_vblank to be reset by the core. It's up to the CRTC driver to clear/set it when something changes in the pipeline.
For control flow state bits we only check the new state for it (see e.g. the various *_changed or plane_bits or whatever).
I tried with !new_crtc_state->no_vblank only, but then it does not handle the case where the CRTC and connector are being disabled, and I end up with a timeout.
continue;spin_lock_irqsave(&old_state->dev->event_lock, flags);if (new_crtc_state->event) {drm_crtc_send_vblank_event(crtc,new_crtc_state->event);new_crtc_state->event = NULL;}spin_unlock_irqrestore(&old_state->dev->event_lock, flags);- }
+} +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
/**
- drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
- @old_state: atomic state object with old state structures
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 26aaba58d6ce..99e2a5297c69 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state, int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, bool nonblock); void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 23eddbccab10..7435dc66c08a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
- @zpos_changed: zpos values of planes on this crtc have been updated
- @color_mgmt_changed: color management properties have changed (degamma or
- gamma LUT or CSC matrix)
- @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
- usually depends on the pipeline configuration, and the main usuage is
- CRTCs feeding a writeback connector operating in oneshot mode. In this
- case the VBLANK event is only generated when a job is queued to the
- writeback connector, and we want the core to fake VBLANK events when
- this part of the pipeline hasn't changed but others had or when the
- CRTC and connectors are disabled.
s/are disabled/are getting disabled/
You'll never get a request for an event when it's already off.
That's true.
- __drm_atomic_helper_crtc_duplicate_state() will the value from the
- current state, the CRTC driver is then responsible for updating this
- field when needed.
Not parsing the above ... probably missing a "... will not reset ..."
Exactly. Will add the missing words.
- Note that, even when no_blank is set to true, the CRTC driver can still
- steal the &drm_crtc_state.event object and send the event on its own.
- That's usually what happens when a job is queued to the writeback
- connector.
The last sentence is confusing imo. Just drop it?
Yes, I know, but it's also important to state that the ->no_blank + event == NULL is a valid combination, and just means that the driver decided to generate the event (that happens when a new WB job is queued).
Then make it more explicit, as-is I had no idea what you meant exactly. What about
Please use the inline comment style for struct members, and then also polish the formatting a bit (e.g. paragraph breaks, which are only possible with the inline style).
I considered that, but the other fields were already documented in the single block above the struct, so I thought keeping things consistent was better. Should I just add this field doc inline and keep the other ones where they are, or should I add a patch moving all docs inline?
There's _lots_ of inline comments already, and all new ones should be inline. The only reason I haven't done the conversion to all of them is that it would be a nice opportunity to update/clean up the comments (often there's a lot more to say than what's captured in the single line), which is why it didn't happen yet. Just moving the comments without updating them seems less useful imo.
I'd just do the inline comment for this, that's what everyone else is doing too. -Daniel
With the nits addressed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
- @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
- @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
@@ -118,6 +132,7 @@ struct drm_crtc_state { bool connectors_changed : 1; bool zpos_changed : 1; bool color_mgmt_changed : 1;
bool no_vblank : 1;
/* attached planes bitmask:
- WARNING: transitional helpers do not maintain plane_mask so
-- 2.14.1
On Mon, 2 Jul 2018 10:40:54 +0200 Daniel Vetter daniel@ffwll.ch wrote:
- Note that, even when no_blank is set to true, the CRTC driver can still
- steal the &drm_crtc_state.event object and send the event on its own.
- That's usually what happens when a job is queued to the writeback
- connector.
The last sentence is confusing imo. Just drop it?
Yes, I know, but it's also important to state that the ->no_blank + event == NULL is a valid combination, and just means that the driver decided to generate the event (that happens when a new WB job is queued).
Then make it more explicit, as-is I had no idea what you meant exactly. What about
Your suggestion is missing :P.
Please use the inline comment style for struct members, and then also polish the formatting a bit (e.g. paragraph breaks, which are only possible with the inline style).
I considered that, but the other fields were already documented in the single block above the struct, so I thought keeping things consistent was better. Should I just add this field doc inline and keep the other ones where they are, or should I add a patch moving all docs inline?
There's _lots_ of inline comments already, and all new ones should be inline. The only reason I haven't done the conversion to all of them is that it would be a nice opportunity to update/clean up the comments (often there's a lot more to say than what's captured in the single line), which is why it didn't happen yet. Just moving the comments without updating them seems less useful imo.
I'd just do the inline comment for this, that's what everyone else is doing too.
Okay, will do.
Now that we have a way to fake VBLANK events when requested by the CRTC hook it up to the generic commit_tail() helpers.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ca586993c2a2..1a088462bc42 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
drm_atomic_helper_commit_modeset_enables(dev, old_state);
+ drm_atomic_helper_fake_vblank(old_state); + drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state); @@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) drm_atomic_helper_commit_planes(dev, old_state, DRM_PLANE_COMMIT_ACTIVE_ONLY);
+ drm_atomic_helper_fake_vblank(old_state); + drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
On Fri, Jun 29, 2018 at 01:17:18PM +0200, Boris Brezillon wrote:
Now that we have a way to fake VBLANK events when requested by the CRTC hook it up to the generic commit_tail() helpers.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
Reviewed-by: Liviu Dudau liviu.dudau@arm.com
drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ca586993c2a2..1a088462bc42 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
drm_atomic_helper_commit_modeset_enables(dev, old_state);
drm_atomic_helper_fake_vblank(old_state);
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
@@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) drm_atomic_helper_commit_planes(dev, old_state, DRM_PLANE_COMMIT_ACTIVE_ONLY);
drm_atomic_helper_fake_vblank(old_state);
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
-- 2.14.1
On Fri, Jun 29, 2018 at 01:17:18PM +0200, Boris Brezillon wrote:
Now that we have a way to fake VBLANK events when requested by the CRTC hook it up to the generic commit_tail() helpers.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
I really don't like this. We've had epic amounts of bugs with atomic drivers failing to send out vblank events when they should, and I added a bunch of debug checks to make sure that doesn't happen anymore.
Now there's no way anymore for drivers to spot this until they have misrenderings on wayland and no idea why. Imo this should only be used by specific drivers, with a comment why exactly they need it. -Daniel
drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ca586993c2a2..1a088462bc42 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
drm_atomic_helper_commit_modeset_enables(dev, old_state);
drm_atomic_helper_fake_vblank(old_state);
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
@@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) drm_atomic_helper_commit_planes(dev, old_state, DRM_PLANE_COMMIT_ACTIVE_ONLY);
drm_atomic_helper_fake_vblank(old_state);
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
-- 2.14.1
On Mon, Jul 02, 2018 at 09:54:32AM +0200, Daniel Vetter wrote:
On Fri, Jun 29, 2018 at 01:17:18PM +0200, Boris Brezillon wrote:
Now that we have a way to fake VBLANK events when requested by the CRTC hook it up to the generic commit_tail() helpers.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
I really don't like this. We've had epic amounts of bugs with atomic drivers failing to send out vblank events when they should, and I added a bunch of debug checks to make sure that doesn't happen anymore.
Now there's no way anymore for drivers to spot this until they have misrenderings on wayland and no idea why. Imo this should only be used by specific drivers, with a comment why exactly they need it.
Meh I retract, you have the special no_vblank state flag to control this. Looks all good to me.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
-Daniel
drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ca586993c2a2..1a088462bc42 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
drm_atomic_helper_commit_modeset_enables(dev, old_state);
drm_atomic_helper_fake_vblank(old_state);
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
@@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) drm_atomic_helper_commit_planes(dev, old_state, DRM_PLANE_COMMIT_ACTIVE_ONLY);
drm_atomic_helper_fake_vblank(old_state);
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
-- 2.14.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Mon, 2 Jul 2018 09:57:30 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Jul 02, 2018 at 09:54:32AM +0200, Daniel Vetter wrote:
On Fri, Jun 29, 2018 at 01:17:18PM +0200, Boris Brezillon wrote:
Now that we have a way to fake VBLANK events when requested by the CRTC hook it up to the generic commit_tail() helpers.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
I really don't like this. We've had epic amounts of bugs with atomic drivers failing to send out vblank events when they should, and I added a bunch of debug checks to make sure that doesn't happen anymore.
Now there's no way anymore for drivers to spot this until they have misrenderings on wayland and no idea why. Imo this should only be used by specific drivers, with a comment why exactly they need it.
Meh I retract, you have the special no_vblank state flag to control this. Looks all good to me.
Hehe, just replied to your other email :-).
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
-Daniel
drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ca586993c2a2..1a088462bc42 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
drm_atomic_helper_commit_modeset_enables(dev, old_state);
drm_atomic_helper_fake_vblank(old_state);
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
@@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) drm_atomic_helper_commit_planes(dev, old_state, DRM_PLANE_COMMIT_ACTIVE_ONLY);
drm_atomic_helper_fake_vblank(old_state);
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
-- 2.14.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Mon, 2 Jul 2018 09:54:32 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jun 29, 2018 at 01:17:18PM +0200, Boris Brezillon wrote:
Now that we have a way to fake VBLANK events when requested by the CRTC hook it up to the generic commit_tail() helpers.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
I really don't like this. We've had epic amounts of bugs with atomic drivers failing to send out vblank events when they should, and I added a bunch of debug checks to make sure that doesn't happen anymore.
Now there's no way anymore for drivers to spot this until they have misrenderings on wayland and no idea why. Imo this should only be used by specific drivers, with a comment why exactly they need it.
Hm, that's not entirely true. Drivers have to explicitly set ->no_vblank to true for drm_atomic_helper_fake_vblank() to actually do something, and I seriously hope drivers won't set that field randomly. So, on all existing drivers but VC4 after the TXP patch, the behavior will be unchanged, and the core will spot misbehaving drivers just like before.
-Daniel
drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ca586993c2a2..1a088462bc42 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
drm_atomic_helper_commit_modeset_enables(dev, old_state);
drm_atomic_helper_fake_vblank(old_state);
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
@@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) drm_atomic_helper_commit_planes(dev, old_state, DRM_PLANE_COMMIT_ACTIVE_ONLY);
drm_atomic_helper_fake_vblank(old_state);
drm_atomic_helper_commit_hw_done(old_state);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
-- 2.14.1
Mimic what is done in drm_atomic_commit_tail() and call drm_atomic_helper_fake_vblank() so that VBLANK events are faked when the drm_crtc_state.no_vblank is true. Will be needed when we'll add support for the transposer block.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- drivers/gpu/drm/vc4/vc4_kms.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 91239b0a4fa0..ca5aa7fba769 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -153,6 +153,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
drm_atomic_helper_commit_modeset_enables(dev, state);
+ drm_atomic_helper_fake_vblank(state); + drm_atomic_helper_commit_hw_done(state);
drm_atomic_helper_wait_for_flip_done(dev, state);
Boris Brezillon boris.brezillon@bootlin.com writes:
Mimic what is done in drm_atomic_commit_tail() and call drm_atomic_helper_fake_vblank() so that VBLANK events are faked when the drm_crtc_state.no_vblank is true. Will be needed when we'll add support for the transposer block.
Reviewed-by: Eric Anholt eric@anholt.net
From: Boris Brezillon boris.brezillon@free-electrons.com
The transposer block is providing support for mem-to-mem composition, which is exposed as a drm_writeback connector in DRM.
Add a driver to support this feature.
Signed-off-by: Boris Brezillon boris.brezillon@free-electrons.com --- .../devicetree/bindings/display/brcm,bcm-vc4.txt | 6 + drivers/gpu/drm/vc4/Makefile | 1 + drivers/gpu/drm/vc4/vc4_crtc.c | 139 +++++- drivers/gpu/drm/vc4/vc4_debugfs.c | 1 + drivers/gpu/drm/vc4/vc4_drv.c | 1 + drivers/gpu/drm/vc4/vc4_drv.h | 7 + drivers/gpu/drm/vc4/vc4_regs.h | 1 + drivers/gpu/drm/vc4/vc4_txp.c | 487 +++++++++++++++++++++ 8 files changed, 619 insertions(+), 24 deletions(-) create mode 100644 drivers/gpu/drm/vc4/vc4_txp.c
diff --git a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt index 284e2b14cfbe..26649b4c4dd8 100644 --- a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt +++ b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt @@ -74,6 +74,12 @@ Required properties for DSI: The 3 clocks output from the DSI analog PHY: dsi[01]_byte, dsi[01]_ddr2, and dsi[01]_ddr
+Required properties for the TXP (writeback) block: +- compatible: Should be "brcm,bcm2835-txp" +- reg: Physical base address and length of the TXP block's registers +- interrupts: The interrupt number + See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt + [1] Documentation/devicetree/bindings/media/video-interfaces.txt
Example: diff --git a/drivers/gpu/drm/vc4/Makefile b/drivers/gpu/drm/vc4/Makefile index 4a3a868235f8..b303703bc7f3 100644 --- a/drivers/gpu/drm/vc4/Makefile +++ b/drivers/gpu/drm/vc4/Makefile @@ -19,6 +19,7 @@ vc4-y := \ vc4_plane.o \ vc4_render_cl.o \ vc4_trace_points.o \ + vc4_txp.o \ vc4_v3d.o \ vc4_validate.o \ vc4_validate_shaders.o diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index dcadf793ee80..477dadc8d6db 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -46,6 +46,8 @@ struct vc4_crtc_state { struct drm_crtc_state base; /* Dlist area for this CRTC configuration. */ struct drm_mm_node mm; + bool feed_txp; + bool txp_armed; };
static inline struct vc4_crtc_state * @@ -324,10 +326,8 @@ static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc) return NULL; }
-static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) +static void vc4_crtc_config_pv(struct drm_crtc *crtc) { - struct drm_device *dev = crtc->dev; - struct vc4_dev *vc4 = to_vc4_dev(dev); struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc); struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); @@ -338,12 +338,6 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) bool is_dsi = (vc4_encoder->type == VC4_ENCODER_TYPE_DSI0 || vc4_encoder->type == VC4_ENCODER_TYPE_DSI1); u32 format = is_dsi ? PV_CONTROL_FORMAT_DSIV_24 : PV_CONTROL_FORMAT_24; - bool debug_dump_regs = false; - - if (debug_dump_regs) { - DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc)); - vc4_crtc_dump_regs(vc4_crtc); - }
/* Reset the PV fifo. */ CRTC_WRITE(PV_CONTROL, 0); @@ -419,6 +413,52 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) PV_CONTROL_CLK_SELECT) | PV_CONTROL_FIFO_CLR | PV_CONTROL_EN); +} + +static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) +{ + struct drm_device *dev = crtc->dev; + struct vc4_dev *vc4 = to_vc4_dev(dev); + struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); + struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); + struct drm_display_mode *mode = &crtc->state->adjusted_mode; + bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE; + bool debug_dump_regs = false; + + if (debug_dump_regs) { + DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc)); + vc4_crtc_dump_regs(vc4_crtc); + } + + if (vc4_crtc->channel == 2) { + u32 dispctrl; + u32 dsp3_mux; + + /* + * SCALER_DISPCTRL_DSP3 = X, where X < 2 means 'connect DSP3 to + * FIFO X'. + * SCALER_DISPCTRL_DSP3 = 3 means 'disable DSP 3'. + * + * DSP3 is connected to FIFO2 unless the transposer is + * enabled. In this case, FIFO 2 is directly accessed by the + * TXP IP, and we need to prevent disable the + * FIFO2 -> pixelvalve1 route. + */ + if (vc4_state->feed_txp) + dsp3_mux = VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX); + else + dsp3_mux = VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX); + + /* Reconfigure the DSP3 mux if required. */ + dispctrl = HVS_READ(SCALER_DISPCTRL); + if ((dispctrl & SCALER_DISPCTRL_DSP3_MUX_MASK) != dsp3_mux) { + dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK; + HVS_WRITE(SCALER_DISPCTRL, dispctrl | dsp3_mux); + } + } + + if (!vc4_state->feed_txp) + vc4_crtc_config_pv(crtc);
HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel), SCALER_DISPBKGND_AUTOHS | @@ -499,6 +539,13 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc, } }
+void vc4_crtc_txp_armed(struct drm_crtc_state *state) +{ + struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state); + + vc4_state->txp_armed = true; +} + static void vc4_crtc_update_dlist(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; @@ -514,8 +561,11 @@ static void vc4_crtc_update_dlist(struct drm_crtc *crtc) WARN_ON(drm_crtc_vblank_get(crtc) != 0);
spin_lock_irqsave(&dev->event_lock, flags); - vc4_crtc->event = crtc->state->event; - crtc->state->event = NULL; + + if (!vc4_state->feed_txp || vc4_state->txp_armed) { + vc4_crtc->event = crtc->state->event; + crtc->state->event = NULL; + }
HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel), vc4_state->mm.start); @@ -533,8 +583,8 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); - struct drm_crtc_state *state = crtc->state; - struct drm_display_mode *mode = &state->adjusted_mode; + struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); + struct drm_display_mode *mode = &crtc->state->adjusted_mode;
require_hvs_enabled(dev);
@@ -546,15 +596,21 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
/* Turn on the scaler, which will wait for vstart to start * compositing. + * When feeding the transposer, we should operate in oneshot + * mode. */ HVS_WRITE(SCALER_DISPCTRLX(vc4_crtc->channel), VC4_SET_FIELD(mode->hdisplay, SCALER_DISPCTRLX_WIDTH) | VC4_SET_FIELD(mode->vdisplay, SCALER_DISPCTRLX_HEIGHT) | - SCALER_DISPCTRLX_ENABLE); + SCALER_DISPCTRLX_ENABLE | + (vc4_state->feed_txp ? SCALER_DISPCTRLX_ONESHOT : 0));
- /* Turn on the pixel valve, which will emit the vstart signal. */ - CRTC_WRITE(PV_V_CONTROL, - CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN); + /* When feeding the composer block the pixelvalve is unneeded and + * should not be enabled. + */ + if (!vc4_state->feed_txp) + CRTC_WRITE(PV_V_CONTROL, + CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN); }
static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc, @@ -579,8 +635,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, struct drm_plane *plane; unsigned long flags; const struct drm_plane_state *plane_state; + struct drm_connector *conn; + struct drm_connector_state *conn_state; u32 dlist_count = 0; - int ret; + int ret, i;
/* The pixelvalve can only feed one encoder (and encoders are * 1:1 with connectors.) @@ -600,6 +658,22 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, if (ret) return ret;
+ state->no_vblank = false; + for_each_new_connector_in_state(state->state, conn, conn_state, i) { + if (conn_state->crtc != crtc) + continue; + + /* The writeback connector is implemented using the transposer + * block which is directly taking its data from the HVS FIFO. + */ + if (conn->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) { + state->no_vblank = true; + vc4_state->feed_txp = true; + } + + break; + } + return 0; }
@@ -713,7 +787,8 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc)
spin_lock_irqsave(&dev->event_lock, flags); if (vc4_crtc->event && - (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)))) { + (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)) || + vc4_state->feed_txp)) { drm_crtc_send_vblank_event(crtc, vc4_crtc->event); vc4_crtc->event = NULL; drm_crtc_vblank_put(crtc); @@ -721,6 +796,13 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc) spin_unlock_irqrestore(&dev->event_lock, flags); }
+void vc4_crtc_handle_vblank(struct vc4_crtc *crtc) +{ + crtc->t_vblank = ktime_get(); + drm_crtc_handle_vblank(&crtc->base); + vc4_crtc_handle_page_flip(crtc); +} + static irqreturn_t vc4_crtc_irq_handler(int irq, void *data) { struct vc4_crtc *vc4_crtc = data; @@ -728,10 +810,8 @@ static irqreturn_t vc4_crtc_irq_handler(int irq, void *data) irqreturn_t ret = IRQ_NONE;
if (stat & PV_INT_VFP_START) { - vc4_crtc->t_vblank = ktime_get(); CRTC_WRITE(PV_INTSTAT, PV_INT_VFP_START); - drm_crtc_handle_vblank(&vc4_crtc->base); - vc4_crtc_handle_page_flip(vc4_crtc); + vc4_crtc_handle_vblank(vc4_crtc); ret = IRQ_HANDLED; }
@@ -884,12 +964,15 @@ static int vc4_page_flip(struct drm_crtc *crtc,
static struct drm_crtc_state *vc4_crtc_duplicate_state(struct drm_crtc *crtc) { - struct vc4_crtc_state *vc4_state; + struct vc4_crtc_state *vc4_state, *old_vc4_state;
vc4_state = kzalloc(sizeof(*vc4_state), GFP_KERNEL); if (!vc4_state) return NULL;
+ old_vc4_state = to_vc4_crtc_state(crtc->state); + vc4_state->feed_txp = old_vc4_state->feed_txp; + __drm_atomic_helper_crtc_duplicate_state(crtc, &vc4_state->base); return &vc4_state->base; } @@ -987,9 +1070,17 @@ static void vc4_set_crtc_possible_masks(struct drm_device *drm, struct drm_encoder *encoder;
drm_for_each_encoder(encoder, drm) { - struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder); + struct vc4_encoder *vc4_encoder; int i;
+ /* HVS FIFO2 can feed the TXP IP. */ + if (crtc_data->hvs_channel == 2 && + encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL) { + encoder->possible_crtcs |= drm_crtc_mask(crtc); + continue; + } + + vc4_encoder = to_vc4_encoder(encoder); for (i = 0; i < ARRAY_SIZE(crtc_data->encoder_types); i++) { if (vc4_encoder->type == encoder_types[i]) { vc4_encoder->clock_select = i; diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c index 5db06bdb5f27..7a0003de71ab 100644 --- a/drivers/gpu/drm/vc4/vc4_debugfs.c +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c @@ -21,6 +21,7 @@ static const struct drm_info_list vc4_debugfs_list[] = { {"dsi1_regs", vc4_dsi_debugfs_regs, 0, (void *)(uintptr_t)1}, {"hdmi_regs", vc4_hdmi_debugfs_regs, 0}, {"vec_regs", vc4_vec_debugfs_regs, 0}, + {"txp_regs", vc4_txp_debugfs_regs, 0}, {"hvs_regs", vc4_hvs_debugfs_regs, 0}, {"crtc0_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)0}, {"crtc1_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)1}, diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 466d0a27b415..e42fd5ec41cc 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -344,6 +344,7 @@ static struct platform_driver *const component_drivers[] = { &vc4_vec_driver, &vc4_dpi_driver, &vc4_dsi_driver, + &vc4_txp_driver, &vc4_hvs_driver, &vc4_crtc_driver, &vc4_v3d_driver, diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index eace76c621a1..bd6ef1f31822 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -73,6 +73,7 @@ struct vc4_dev { struct vc4_dpi *dpi; struct vc4_dsi *dsi1; struct vc4_vec *vec; + struct vc4_txp *txp;
struct vc4_hang_state *hang_state;
@@ -698,6 +699,8 @@ bool vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id, bool in_vblank_irq, int *vpos, int *hpos, ktime_t *stime, ktime_t *etime, const struct drm_display_mode *mode); +void vc4_crtc_handle_vblank(struct vc4_crtc *crtc); +void vc4_crtc_txp_armed(struct drm_crtc_state *state);
/* vc4_debugfs.c */ int vc4_debugfs_init(struct drm_minor *minor); @@ -745,6 +748,10 @@ int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused); extern struct platform_driver vc4_vec_driver; int vc4_vec_debugfs_regs(struct seq_file *m, void *unused);
+/* vc4_txp.c */ +extern struct platform_driver vc4_txp_driver; +int vc4_txp_debugfs_regs(struct seq_file *m, void *unused); + /* vc4_irq.c */ irqreturn_t vc4_irq(int irq, void *arg); void vc4_irq_preinstall(struct drm_device *dev); diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h index d6864fa4bd14..744a689751f0 100644 --- a/drivers/gpu/drm/vc4/vc4_regs.h +++ b/drivers/gpu/drm/vc4/vc4_regs.h @@ -330,6 +330,7 @@ #define SCALER_DISPCTRL0 0x00000040 # define SCALER_DISPCTRLX_ENABLE BIT(31) # define SCALER_DISPCTRLX_RESET BIT(30) + /* Generates a single frame when VSTART is seen and stops at the last * pixel read from the FIFO. */ diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c new file mode 100644 index 000000000000..e352d1916fc4 --- /dev/null +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -0,0 +1,487 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright © 2018 Broadcom + * + * Authors: + * Eric Anholt eric@anholt.net + * Boris Brezillon boris.brezillon@bootlin.com + */ + +#include <drm/drm_atomic_helper.h> +#include <drm/drm_fb_cma_helper.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_edid.h> +#include <drm/drm_panel.h> +#include <drm/drm_writeback.h> +#include <linux/clk.h> +#include <linux/component.h> +#include <linux/of_graph.h> +#include <linux/of_platform.h> +#include <linux/pm_runtime.h> + +#include "vc4_drv.h" +#include "vc4_regs.h" + +/* Base address of the output. Raster formats must be 4-byte aligned, + * T and LT must be 16-byte aligned or maybe utile-aligned (docs are + * inconsistent, but probably utile). + */ +#define TXP_DST_PTR 0x00 + +/* Pitch in bytes for raster images, 16-byte aligned. For tiled, it's + * the width in tiles. + */ +#define TXP_DST_PITCH 0x04 +/* For T-tiled imgaes, DST_PITCH should be the number of tiles wide, + * shifted up. + */ +# define TXP_T_TILE_WIDTH_SHIFT 7 +/* For LT-tiled images, DST_PITCH should be the number of utiles wide, + * shifted up. + */ +# define TXP_LT_TILE_WIDTH_SHIFT 4 + +/* Pre-rotation width/height of the image. Must match HVS config. + * + * If TFORMAT and 32-bit, limit is 1920 for 32-bit and 3840 to 16-bit + * and width/height must be tile or utile-aligned as appropriate. If + * transposing (rotating), width is limited to 1920. + * + * Height is limited to various numbers between 4088 and 4095. I'd + * just use 4088 to be safe. + */ +#define TXP_DIM 0x08 +# define TXP_HEIGHT_SHIFT 16 +# define TXP_HEIGHT_MASK GENMASK(31, 16) +# define TXP_WIDTH_SHIFT 0 +# define TXP_WIDTH_MASK GENMASK(15, 0) + +#define TXP_DST_CTRL 0x0c +/* These bits are set to 0x54 */ +#define TXP_PILOT_SHIFT 24 +#define TXP_PILOT_MASK GENMASK(31, 24) +/* Bits 22-23 are set to 0x01 */ +#define TXP_VERSION_SHIFT 22 +#define TXP_VERSION_MASK GENMASK(23, 22) + +/* Powers down the internal memory. */ +# define TXP_POWERDOWN BIT(21) + +/* Enables storing the alpha component in 8888/4444, instead of + * filling with ~ALPHA_INVERT. + */ +# define TXP_ALPHA_ENABLE BIT(20) + +/* 4 bits, each enables stores for a channel in each set of 4 bytes. + * Set to 0xf for normal operation. + */ +# define TXP_BYTE_ENABLE_SHIFT 16 +# define TXP_BYTE_ENABLE_MASK GENMASK(19, 16) + +/* Debug: Generate VSTART again at EOF. */ +# define TXP_VSTART_AT_EOF BIT(15) + +/* Debug: Terminate the current frame immediately. Stops AXI + * writes. + */ +# define TXP_ABORT BIT(14) + +# define TXP_DITHER BIT(13) + +/* Inverts alpha if TXP_ALPHA_ENABLE, chooses fill value for + * !TXP_ALPHA_ENABLE. + */ +# define TXP_ALPHA_INVERT BIT(12) + +/* Note: I've listed the channels here in high bit (in byte 3/2/1) to + * low bit (in byte 0) order. + */ +# define TXP_FORMAT_SHIFT 8 +# define TXP_FORMAT_MASK GENMASK(11, 8) +# define TXP_FORMAT_ABGR4444 0 +# define TXP_FORMAT_ARGB4444 1 +# define TXP_FORMAT_BGRA4444 2 +# define TXP_FORMAT_RGBA4444 3 +# define TXP_FORMAT_BGR565 6 +# define TXP_FORMAT_RGB565 7 +/* 888s are non-rotated, raster-only */ +# define TXP_FORMAT_BGR888 8 +# define TXP_FORMAT_RGB888 9 +# define TXP_FORMAT_ABGR8888 12 +# define TXP_FORMAT_ARGB8888 13 +# define TXP_FORMAT_BGRA8888 14 +# define TXP_FORMAT_RGBA8888 15 + +/* If TFORMAT is set, generates LT instead of T format. */ +# define TXP_LINEAR_UTILE BIT(7) + +/* Rotate output by 90 degrees. */ +# define TXP_TRANSPOSE BIT(6) + +/* Generate a tiled format for V3D. */ +# define TXP_TFORMAT BIT(5) + +/* Generates some undefined test mode output. */ +# define TXP_TEST_MODE BIT(4) + +/* Request odd field from HVS. */ +# define TXP_FIELD BIT(3) + +/* Raise interrupt when idle. */ +# define TXP_EI BIT(2) + +/* Set when generating a frame, clears when idle. */ +# define TXP_BUSY BIT(1) + +/* Starts a frame. Self-clearing. */ +# define TXP_GO BIT(0) + +/* Number of lines received and committed to memory. */ +#define TXP_PROGRESS 0x10 + +#define TXP_READ(offset) readl(txp->regs + (offset)) +#define TXP_WRITE(offset, val) writel(val, txp->regs + (offset)) + +struct vc4_txp { + struct platform_device *pdev; + + struct drm_writeback_connector connector; + + void __iomem *regs; +}; + +static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder) +{ + return container_of(encoder, struct vc4_txp, connector.encoder); +} + +static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn) +{ + return container_of(conn, struct vc4_txp, connector.base); +} + +#define TXP_REG(reg) { reg, #reg } +static const struct { + u32 reg; + const char *name; +} txp_regs[] = { + TXP_REG(TXP_DST_PTR), + TXP_REG(TXP_DST_PITCH), + TXP_REG(TXP_DIM), + TXP_REG(TXP_DST_CTRL), + TXP_REG(TXP_PROGRESS), +}; + +#ifdef CONFIG_DEBUG_FS +int vc4_txp_debugfs_regs(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = (struct drm_info_node *)m->private; + struct drm_device *dev = node->minor->dev; + struct vc4_dev *vc4 = to_vc4_dev(dev); + struct vc4_txp *txp = vc4->txp; + int i; + + if (!txp) + return 0; + + for (i = 0; i < ARRAY_SIZE(txp_regs); i++) { + seq_printf(m, "%s (0x%04x): 0x%08x\n", + txp_regs[i].name, txp_regs[i].reg, + TXP_READ(txp_regs[i].reg)); + } + + return 0; +} +#endif + +static int vc4_txp_connector_get_modes(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + + return drm_add_modes_noedid(connector, dev->mode_config.max_width, + dev->mode_config.max_height); +} + +static enum drm_mode_status +vc4_txp_connector_mode_valid(struct drm_connector *connector, + struct drm_display_mode *mode) +{ + struct drm_device *dev = connector->dev; + struct drm_mode_config *mode_config = &dev->mode_config; + int w = mode->hdisplay, h = mode->vdisplay; + + if (w < mode_config->min_width || w > mode_config->max_width) + return MODE_BAD_HVALUE; + + if (h < mode_config->min_height || h > mode_config->max_height) + return MODE_BAD_VVALUE; + + return MODE_OK; +} + +static const u32 vc4_txp_formats[] = { + DRM_FORMAT_RGB888, + DRM_FORMAT_BGR888, + DRM_FORMAT_XRGB8888, + DRM_FORMAT_XBGR8888, + DRM_FORMAT_ARGB8888, + DRM_FORMAT_ABGR8888, + DRM_FORMAT_RGBX8888, + DRM_FORMAT_BGRX8888, + DRM_FORMAT_RGBA8888, + DRM_FORMAT_BGRA8888, +}; + +static int vc4_txp_connector_atomic_check(struct drm_connector *conn, + struct drm_connector_state *conn_state) +{ + struct drm_crtc_state *crtc_state; + struct drm_gem_cma_object *gem; + struct drm_framebuffer *fb; + int i; + + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) + return 0; + + crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, + conn_state->crtc); + + fb = conn_state->writeback_job->fb; + if (fb->width != crtc_state->mode.hdisplay || + fb->height != crtc_state->mode.vdisplay) { + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", + fb->width, fb->height); + return -EINVAL; + } + + for (i = 0; i < ARRAY_SIZE(vc4_txp_formats); i++) { + if (fb->format->format == vc4_txp_formats[i]) + break; + } + + if (i == ARRAY_SIZE(vc4_txp_formats)) + return -EINVAL; + + gem = drm_fb_cma_get_gem_obj(fb, 0); + + /* Pitch must be aligned on 16 bytes. */ + if (fb->pitches[0] & GENMASK(3, 0)) + return -EINVAL; + + vc4_crtc_txp_armed(crtc_state); + + return 0; +} + +static void vc4_txp_connector_atomic_commit(struct drm_connector *conn, + struct drm_connector_state *conn_state) +{ + struct vc4_txp *txp = connector_to_vc4_txp(conn); + struct drm_gem_cma_object *gem; + struct drm_display_mode *mode; + struct drm_framebuffer *fb; + u32 ctrl = TXP_GO | TXP_VSTART_AT_EOF | TXP_EI; + + if (WARN_ON(!conn_state->writeback_job || + !conn_state->writeback_job->fb)) + return; + + mode = &conn_state->crtc->state->adjusted_mode; + fb = conn_state->writeback_job->fb; + + switch (fb->format->format) { + case DRM_FORMAT_ARGB8888: + ctrl |= TXP_ALPHA_ENABLE; + case DRM_FORMAT_XRGB8888: + ctrl |= VC4_SET_FIELD(TXP_FORMAT_ARGB8888, TXP_FORMAT) | + VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE); + break; + + case DRM_FORMAT_ABGR8888: + ctrl |= TXP_ALPHA_ENABLE; + case DRM_FORMAT_XBGR8888: + ctrl |= VC4_SET_FIELD(TXP_FORMAT_ABGR8888, TXP_FORMAT) | + VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE); + break; + + case DRM_FORMAT_RGBA8888: + ctrl |= TXP_ALPHA_ENABLE; + case DRM_FORMAT_RGBX8888: + ctrl |= VC4_SET_FIELD(TXP_FORMAT_RGBA8888, TXP_FORMAT) | + VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE); + break; + + case DRM_FORMAT_BGRA8888: + ctrl |= TXP_ALPHA_ENABLE; + case DRM_FORMAT_BGRX8888: + ctrl |= VC4_SET_FIELD(TXP_FORMAT_BGRA8888, TXP_FORMAT) | + VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE); + break; + + case DRM_FORMAT_BGR888: + ctrl |= VC4_SET_FIELD(TXP_FORMAT_BGR888, TXP_FORMAT) | + VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE); + break; + + case DRM_FORMAT_RGB888: + ctrl |= VC4_SET_FIELD(TXP_FORMAT_RGB888, TXP_FORMAT) | + VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE); + break; + default: + WARN_ON(1); + return; + } + + gem = drm_fb_cma_get_gem_obj(fb, 0); + TXP_WRITE(TXP_DST_PTR, gem->paddr + fb->offsets[0]); + TXP_WRITE(TXP_DST_PITCH, fb->pitches[0]); + TXP_WRITE(TXP_DIM, + VC4_SET_FIELD(mode->hdisplay, TXP_WIDTH) | + VC4_SET_FIELD(mode->vdisplay, TXP_HEIGHT)); + + TXP_WRITE(TXP_DST_CTRL, ctrl); + + drm_writeback_queue_job(&txp->connector, conn_state->writeback_job); +} + +static const struct drm_connector_helper_funcs vc4_txp_connector_helper_funcs = { + .get_modes = vc4_txp_connector_get_modes, + .mode_valid = vc4_txp_connector_mode_valid, + .atomic_check = vc4_txp_connector_atomic_check, + .atomic_commit = vc4_txp_connector_atomic_commit, +}; + +static enum drm_connector_status +vc4_txp_connector_detect(struct drm_connector *connector, bool force) +{ + return connector_status_connected; +} + +static void vc4_txp_connector_destroy(struct drm_connector *connector) +{ + drm_connector_unregister(connector); + drm_connector_cleanup(connector); +} + +static const struct drm_connector_funcs vc4_txp_connector_funcs = { + .detect = vc4_txp_connector_detect, + .fill_modes = drm_helper_probe_single_connector_modes, + .destroy = vc4_txp_connector_destroy, + .reset = drm_atomic_helper_connector_reset, + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, +}; + +static void vc4_txp_encoder_disable(struct drm_encoder *encoder) +{ + struct vc4_txp *txp = encoder_to_vc4_txp(encoder); + + if (TXP_READ(TXP_DST_CTRL) & TXP_BUSY) { + TXP_WRITE(TXP_DST_CTRL, TXP_ABORT); + + while (TXP_READ(TXP_DST_CTRL) & TXP_BUSY) + ; + } + + TXP_WRITE(TXP_DST_CTRL, TXP_POWERDOWN); +} + +static const struct drm_encoder_helper_funcs vc4_txp_encoder_helper_funcs = { + .disable = vc4_txp_encoder_disable, +}; + +static irqreturn_t vc4_txp_interrupt(int irq, void *data) +{ + struct vc4_txp *txp = data; + + TXP_WRITE(TXP_DST_CTRL, TXP_READ(TXP_DST_CTRL) & ~TXP_EI); + vc4_crtc_handle_vblank(to_vc4_crtc(txp->connector.base.state->crtc)); + drm_writeback_signal_completion(&txp->connector, 0); + + return IRQ_HANDLED; +} + +static int vc4_txp_bind(struct device *dev, struct device *master, void *data) +{ + struct platform_device *pdev = to_platform_device(dev); + struct drm_device *drm = dev_get_drvdata(master); + struct vc4_dev *vc4 = to_vc4_dev(drm); + struct vc4_txp *txp; + int ret, irq; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + txp = devm_kzalloc(dev, sizeof(*txp), GFP_KERNEL); + if (!txp) + return -ENOMEM; + + txp->pdev = pdev; + + txp->regs = vc4_ioremap_regs(pdev, 0); + if (IS_ERR(txp->regs)) + return PTR_ERR(txp->regs); + + drm_connector_helper_add(&txp->connector.base, + &vc4_txp_connector_helper_funcs); + ret = drm_writeback_connector_init(drm, &txp->connector, + &vc4_txp_connector_funcs, + &vc4_txp_encoder_helper_funcs, + vc4_txp_formats, + ARRAY_SIZE(vc4_txp_formats)); + if (ret) + return ret; + + ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0, + dev_name(dev), txp); + if (ret) + return ret; + + dev_set_drvdata(dev, txp); + vc4->txp = txp; + + return 0; +} + +static void vc4_txp_unbind(struct device *dev, struct device *master, + void *data) +{ + struct drm_device *drm = dev_get_drvdata(master); + struct vc4_dev *vc4 = to_vc4_dev(drm); + struct vc4_txp *txp = dev_get_drvdata(dev); + + vc4_txp_connector_destroy(&txp->connector.base); + + vc4->txp = NULL; +} + +static const struct component_ops vc4_txp_ops = { + .bind = vc4_txp_bind, + .unbind = vc4_txp_unbind, +}; + +static int vc4_txp_probe(struct platform_device *pdev) +{ + return component_add(&pdev->dev, &vc4_txp_ops); +} + +static int vc4_txp_remove(struct platform_device *pdev) +{ + component_del(&pdev->dev, &vc4_txp_ops); + return 0; +} + +static const struct of_device_id vc4_txp_dt_match[] = { + { .compatible = "brcm,bcm2835-txp" }, + { /* sentinel */ }, +}; + +struct platform_driver vc4_txp_driver = { + .probe = vc4_txp_probe, + .remove = vc4_txp_remove, + .driver = { + .name = "vc4_txp", + .of_match_table = vc4_txp_dt_match, + }, +};
Boris Brezillon boris.brezillon@bootlin.com writes:
From: Boris Brezillon boris.brezillon@free-electrons.com
The transposer block is providing support for mem-to-mem composition, which is exposed as a drm_writeback connector in DRM.
Add a driver to support this feature.
Signed-off-by: Boris Brezillon boris.brezillon@free-electrons.com
+static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) +{
- struct drm_device *dev = crtc->dev;
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
- struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
- struct drm_display_mode *mode = &crtc->state->adjusted_mode;
- bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE;
- bool debug_dump_regs = false;
- if (debug_dump_regs) {
DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc));vc4_crtc_dump_regs(vc4_crtc);- }
- if (vc4_crtc->channel == 2) {
u32 dispctrl;u32 dsp3_mux;/** SCALER_DISPCTRL_DSP3 = X, where X < 2 means 'connect DSP3 to* FIFO X'.* SCALER_DISPCTRL_DSP3 = 3 means 'disable DSP 3'.** DSP3 is connected to FIFO2 unless the transposer is* enabled. In this case, FIFO 2 is directly accessed by the* TXP IP, and we need to prevent disable the
s/prevent //
* FIFO2 -> pixelvalve1 route.*/if (vc4_state->feed_txp)dsp3_mux = VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX);elsedsp3_mux = VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);/* Reconfigure the DSP3 mux if required. */dispctrl = HVS_READ(SCALER_DISPCTRL);if ((dispctrl & SCALER_DISPCTRL_DSP3_MUX_MASK) != dsp3_mux) {dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK;HVS_WRITE(SCALER_DISPCTRL, dispctrl | dsp3_mux);}
This is fine, but you could also skip the matching mux check here -- the read is the expensive part.
}
if (!vc4_state->feed_txp)
vc4_crtc_config_pv(crtc);HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel), SCALER_DISPBKGND_AUTOHS |
@@ -499,6 +539,13 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc, } }
+void vc4_crtc_txp_armed(struct drm_crtc_state *state) +{
- struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
- vc4_state->txp_armed = true;
+}
static void vc4_crtc_update_dlist(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; @@ -514,8 +561,11 @@ static void vc4_crtc_update_dlist(struct drm_crtc *crtc) WARN_ON(drm_crtc_vblank_get(crtc) != 0);
spin_lock_irqsave(&dev->event_lock, flags);
vc4_crtc->event = crtc->state->event;crtc->state->event = NULL;
if (!vc4_state->feed_txp || vc4_state->txp_armed) {vc4_crtc->event = crtc->state->event;crtc->state->event = NULL;}HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel), vc4_state->mm.start);
@@ -533,8 +583,8 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
- struct drm_crtc_state *state = crtc->state;
- struct drm_display_mode *mode = &state->adjusted_mode;
struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
struct drm_display_mode *mode = &crtc->state->adjusted_mode;
require_hvs_enabled(dev);
@@ -546,15 +596,21 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
/* Turn on the scaler, which will wait for vstart to start * compositing.
* When feeding the transposer, we should operate in oneshot* mode.
SCALER_DISPCTRLX_ENABLE);
SCALER_DISPCTRLX_ENABLE |(vc4_state->feed_txp ? SCALER_DISPCTRLX_ONESHOT : 0));
- /* Turn on the pixel valve, which will emit the vstart signal. */
- CRTC_WRITE(PV_V_CONTROL,
CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);
- /* When feeding the composer block the pixelvalve is unneeded and
"transposer block"? composer block made me think HVS.
* should not be enabled.*/- if (!vc4_state->feed_txp)
CRTC_WRITE(PV_V_CONTROL,CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);}
static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc, @@ -579,8 +635,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, struct drm_plane *plane; unsigned long flags; const struct drm_plane_state *plane_state;
- struct drm_connector *conn;
- struct drm_connector_state *conn_state; u32 dlist_count = 0;
- int ret;
int ret, i;
/* The pixelvalve can only feed one encoder (and encoders are
- 1:1 with connectors.)
@@ -600,6 +658,22 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, if (ret) return ret;
- state->no_vblank = false;
- for_each_new_connector_in_state(state->state, conn, conn_state, i) {
if (conn_state->crtc != crtc)continue;/* The writeback connector is implemented using the transposer* block which is directly taking its data from the HVS FIFO.*/if (conn->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) {state->no_vblank = true;vc4_state->feed_txp = true;}break;- }
- return 0;
}
@@ -713,7 +787,8 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc)
spin_lock_irqsave(&dev->event_lock, flags); if (vc4_crtc->event &&
(vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)))) {
(vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)) ||vc4_state->feed_txp)) {
Can vc4_crtc->event even be set if vc4_state->feed_txp?
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h index d6864fa4bd14..744a689751f0 100644 --- a/drivers/gpu/drm/vc4/vc4_regs.h +++ b/drivers/gpu/drm/vc4/vc4_regs.h @@ -330,6 +330,7 @@ #define SCALER_DISPCTRL0 0x00000040 # define SCALER_DISPCTRLX_ENABLE BIT(31) # define SCALER_DISPCTRLX_RESET BIT(30)
/* Generates a single frame when VSTART is seen and stops at the last
- pixel read from the FIFO.
*/
stray hunk?
+static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
struct drm_connector_state *conn_state)+{
- struct vc4_txp *txp = connector_to_vc4_txp(conn);
- struct drm_gem_cma_object *gem;
- struct drm_display_mode *mode;
- struct drm_framebuffer *fb;
- u32 ctrl = TXP_GO | TXP_VSTART_AT_EOF | TXP_EI;
- if (WARN_ON(!conn_state->writeback_job ||
!conn_state->writeback_job->fb))return;- mode = &conn_state->crtc->state->adjusted_mode;
- fb = conn_state->writeback_job->fb;
- switch (fb->format->format) {
- case DRM_FORMAT_ARGB8888:
ctrl |= TXP_ALPHA_ENABLE;
Optional suggestion: Have the txp_formats[] table be a struct with these register values in it.
All my feedback seems really minor, so with whatever components you like integrated, feel free to add:
Reviewed-by: Eric Anholt eric@anholt.net
Hi Eric,
On Fri, 29 Jun 2018 13:35:04 -0700 Eric Anholt eric@anholt.net wrote:
Boris Brezillon boris.brezillon@bootlin.com writes:
From: Boris Brezillon boris.brezillon@free-electrons.com
The transposer block is providing support for mem-to-mem composition, which is exposed as a drm_writeback connector in DRM.
Add a driver to support this feature.
Signed-off-by: Boris Brezillon boris.brezillon@free-electrons.com
+static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc) +{
- struct drm_device *dev = crtc->dev;
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
- struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
- struct drm_display_mode *mode = &crtc->state->adjusted_mode;
- bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE;
- bool debug_dump_regs = false;
- if (debug_dump_regs) {
DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc));vc4_crtc_dump_regs(vc4_crtc);- }
- if (vc4_crtc->channel == 2) {
u32 dispctrl;u32 dsp3_mux;/** SCALER_DISPCTRL_DSP3 = X, where X < 2 means 'connect DSP3 to* FIFO X'.* SCALER_DISPCTRL_DSP3 = 3 means 'disable DSP 3'.** DSP3 is connected to FIFO2 unless the transposer is* enabled. In this case, FIFO 2 is directly accessed by the* TXP IP, and we need to prevent disable thes/prevent //
* FIFO2 -> pixelvalve1 route.*/if (vc4_state->feed_txp)dsp3_mux = VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX);elsedsp3_mux = VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);/* Reconfigure the DSP3 mux if required. */dispctrl = HVS_READ(SCALER_DISPCTRL);if ((dispctrl & SCALER_DISPCTRL_DSP3_MUX_MASK) != dsp3_mux) {dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK;HVS_WRITE(SCALER_DISPCTRL, dispctrl | dsp3_mux);}This is fine, but you could also skip the matching mux check here -- the read is the expensive part.
}
if (!vc4_state->feed_txp)
vc4_crtc_config_pv(crtc);HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel), SCALER_DISPBKGND_AUTOHS |
@@ -499,6 +539,13 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc, } }
+void vc4_crtc_txp_armed(struct drm_crtc_state *state) +{
- struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
- vc4_state->txp_armed = true;
+}
static void vc4_crtc_update_dlist(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; @@ -514,8 +561,11 @@ static void vc4_crtc_update_dlist(struct drm_crtc *crtc) WARN_ON(drm_crtc_vblank_get(crtc) != 0);
spin_lock_irqsave(&dev->event_lock, flags);
vc4_crtc->event = crtc->state->event;crtc->state->event = NULL;
if (!vc4_state->feed_txp || vc4_state->txp_armed) {vc4_crtc->event = crtc->state->event;crtc->state->event = NULL;}HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel), vc4_state->mm.start);
@@ -533,8 +583,8 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
- struct drm_crtc_state *state = crtc->state;
- struct drm_display_mode *mode = &state->adjusted_mode;
struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
struct drm_display_mode *mode = &crtc->state->adjusted_mode;
require_hvs_enabled(dev);
@@ -546,15 +596,21 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
/* Turn on the scaler, which will wait for vstart to start * compositing.
* When feeding the transposer, we should operate in oneshot* mode.
SCALER_DISPCTRLX_ENABLE);
SCALER_DISPCTRLX_ENABLE |(vc4_state->feed_txp ? SCALER_DISPCTRLX_ONESHOT : 0));
- /* Turn on the pixel valve, which will emit the vstart signal. */
- CRTC_WRITE(PV_V_CONTROL,
CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);
- /* When feeding the composer block the pixelvalve is unneeded and
"transposer block"? composer block made me think HVS.
* should not be enabled.*/- if (!vc4_state->feed_txp)
CRTC_WRITE(PV_V_CONTROL,CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);}
static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc, @@ -579,8 +635,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, struct drm_plane *plane; unsigned long flags; const struct drm_plane_state *plane_state;
- struct drm_connector *conn;
- struct drm_connector_state *conn_state; u32 dlist_count = 0;
- int ret;
int ret, i;
/* The pixelvalve can only feed one encoder (and encoders are
- 1:1 with connectors.)
@@ -600,6 +658,22 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, if (ret) return ret;
- state->no_vblank = false;
- for_each_new_connector_in_state(state->state, conn, conn_state, i) {
if (conn_state->crtc != crtc)continue;/* The writeback connector is implemented using the transposer* block which is directly taking its data from the HVS FIFO.*/if (conn->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) {state->no_vblank = true;vc4_state->feed_txp = true;}break;- }
- return 0;
}
@@ -713,7 +787,8 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc)
spin_lock_irqsave(&dev->event_lock, flags); if (vc4_crtc->event &&
(vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)))) {
(vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)) ||vc4_state->feed_txp)) {Can vc4_crtc->event even be set if vc4_state->feed_txp?
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h index d6864fa4bd14..744a689751f0 100644 --- a/drivers/gpu/drm/vc4/vc4_regs.h +++ b/drivers/gpu/drm/vc4/vc4_regs.h @@ -330,6 +330,7 @@ #define SCALER_DISPCTRL0 0x00000040 # define SCALER_DISPCTRLX_ENABLE BIT(31) # define SCALER_DISPCTRLX_RESET BIT(30)
/* Generates a single frame when VSTART is seen and stops at the last
- pixel read from the FIFO.
*/
stray hunk?
+static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
struct drm_connector_state *conn_state)+{
- struct vc4_txp *txp = connector_to_vc4_txp(conn);
- struct drm_gem_cma_object *gem;
- struct drm_display_mode *mode;
- struct drm_framebuffer *fb;
- u32 ctrl = TXP_GO | TXP_VSTART_AT_EOF | TXP_EI;
- if (WARN_ON(!conn_state->writeback_job ||
!conn_state->writeback_job->fb))return;- mode = &conn_state->crtc->state->adjusted_mode;
- fb = conn_state->writeback_job->fb;
- switch (fb->format->format) {
- case DRM_FORMAT_ARGB8888:
ctrl |= TXP_ALPHA_ENABLE;Optional suggestion: Have the txp_formats[] table be a struct with these register values in it.
All my feedback seems really minor, so with whatever components you like integrated, feel free to add:
Will fix all the things you pointed in your review.
Thanks,
Boris
From: Boris Brezillon boris.brezillon@free-electrons.com
The transposer block is allowing one to write the result of the VC4 composition back to memory instead of displaying it on a screen.
Signed-off-by: Boris Brezillon boris.brezillon@free-electrons.com --- arch/arm/boot/dts/bcm283x.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi index ac00e730f898..740870898b1e 100644 --- a/arch/arm/boot/dts/bcm283x.dtsi +++ b/arch/arm/boot/dts/bcm283x.dtsi @@ -66,6 +66,12 @@ clock-frequency = <1000000>; };
+ txp@7e004000 { + compatible = "brcm,bcm2835-txp"; + reg = <0x7e004000 0x20>; + interrupts = <1 11>; + }; + dma: dma@7e007000 { compatible = "brcm,bcm2835-dma"; reg = <0x7e007000 0xf00>;
Boris Brezillon boris.brezillon@bootlin.com writes:
From: Boris Brezillon boris.brezillon@free-electrons.com
The transposer block is allowing one to write the result of the VC4 composition back to memory instead of displaying it on a screen.
Signed-off-by: Boris Brezillon boris.brezillon@free-electrons.com
Reviewed-by: Eric Anholt eric@anholt.net
Hello,
This is the second version of this series adding writeback support to the VC4 display engine.
This version is based on drm-misc-next and include a bunch of modifications to core that I had to add to make it work on VC4.
Feel free to comment on those modifications.
On the driver side, no much has changed except I modified a bit the implementation to adjust the latest revision of the writeback interface.
Regards,
Boris
Boris Brezillon (8): drm/atomic: Avoid connector to writeback_connector casts drm/connector: Pass a drm_connector_state to ->atomic_commit() drm/vc4: Use wait_for_flip_done() instead of wait_for_vblanks() drm/crtc: Add a generic infrastructure to fake VBLANK events drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic commit_tail() helpers drm/vc4: Call drm_atomic_helper_fake_vblank() in the commit path drm/vc4: Add support for the transposer block ARM: dts: bcm283x: Add Transposer block
.../devicetree/bindings/display/brcm,bcm-vc4.txt | 6 + arch/arm/boot/dts/bcm283x.dtsi | 6 + drivers/gpu/drm/drm_atomic.c | 4 +- drivers/gpu/drm/drm_atomic_helper.c | 46 +- drivers/gpu/drm/vc4/Makefile | 1 + drivers/gpu/drm/vc4/vc4_crtc.c | 139 +++++- drivers/gpu/drm/vc4/vc4_debugfs.c | 1 + drivers/gpu/drm/vc4/vc4_drv.c | 1 + drivers/gpu/drm/vc4/vc4_drv.h | 7 + drivers/gpu/drm/vc4/vc4_kms.c | 11 +- drivers/gpu/drm/vc4/vc4_regs.h | 1 + drivers/gpu/drm/vc4/vc4_txp.c | 487 +++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 + include/drm/drm_modeset_helper_vtables.h | 4 +- include/drm/drm_writeback.h | 6 + 16 files changed, 700 insertions(+), 36 deletions(-) create mode 100644 drivers/gpu/drm/vc4/vc4_txp.c
On Fri, Jun 29, 2018 at 01:17:22PM +0200, Boris Brezillon wrote:
Hello,
This is the second version of this series adding writeback support to the VC4 display engine.
This version is based on drm-misc-next and include a bunch of modifications to core that I had to add to make it work on VC4.
Feel free to comment on those modifications.
On the driver side, no much has changed except I modified a bit the implementation to adjust the latest revision of the writeback interface.
This is a duplicate of the cover letter.
I've reviewed / checked patches 1-5 and 8, so you can add my Reviewed-by tag from me if you want.
Best regards, Liviu
Regards,
Boris
Boris Brezillon (8): drm/atomic: Avoid connector to writeback_connector casts drm/connector: Pass a drm_connector_state to ->atomic_commit() drm/vc4: Use wait_for_flip_done() instead of wait_for_vblanks() drm/crtc: Add a generic infrastructure to fake VBLANK events drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic commit_tail() helpers drm/vc4: Call drm_atomic_helper_fake_vblank() in the commit path drm/vc4: Add support for the transposer block ARM: dts: bcm283x: Add Transposer block
.../devicetree/bindings/display/brcm,bcm-vc4.txt | 6 + arch/arm/boot/dts/bcm283x.dtsi | 6 + drivers/gpu/drm/drm_atomic.c | 4 +- drivers/gpu/drm/drm_atomic_helper.c | 46 +- drivers/gpu/drm/vc4/Makefile | 1 + drivers/gpu/drm/vc4/vc4_crtc.c | 139 +++++- drivers/gpu/drm/vc4/vc4_debugfs.c | 1 + drivers/gpu/drm/vc4/vc4_drv.c | 1 + drivers/gpu/drm/vc4/vc4_drv.h | 7 + drivers/gpu/drm/vc4/vc4_kms.c | 11 +- drivers/gpu/drm/vc4/vc4_regs.h | 1 + drivers/gpu/drm/vc4/vc4_txp.c | 487 +++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 + include/drm/drm_modeset_helper_vtables.h | 4 +- include/drm/drm_writeback.h | 6 + 16 files changed, 700 insertions(+), 36 deletions(-) create mode 100644 drivers/gpu/drm/vc4/vc4_txp.c
-- 2.14.1
On Fri, 29 Jun 2018 12:40:45 +0100 Liviu Dudau Liviu.Dudau@arm.com wrote:
On Fri, Jun 29, 2018 at 01:17:22PM +0200, Boris Brezillon wrote:
Hello,
This is the second version of this series adding writeback support to the VC4 display engine.
This version is based on drm-misc-next and include a bunch of modifications to core that I had to add to make it work on VC4.
Feel free to comment on those modifications.
On the driver side, no much has changed except I modified a bit the implementation to adjust the latest revision of the writeback interface.
This is a duplicate of the cover letter.
Yep, I forgot to clean the directory I used to generate my patches :-/
I've reviewed / checked patches 1-5 and 8, so you can add my Reviewed-by tag from me if you want.
Thanks for your review.
Boris
dri-devel@lists.freedesktop.org
-
Boris Brezillon -
Daniel Vetter -
Eric Anholt -
Liviu Dudau