* [PATCH v6 01/18] Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 02/18] media: vsp1: wpf: Fix partition configuration for display pipelines Laurent Pinchart
` (16 subsequent siblings)
17 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
This reverts commit 3299ba5c0b21 ("[media] v4l: vsp1: Supply frames to
the DU continuously")
The DU output mode does not rely on frames being supplied on the WPF as
its pipeline is supplied from DRM. For the upcoming WPF writeback
functionality, we will choose to enable writeback mode if there is an
output buffer, or disable it (leaving the existing display pipeline
unharmed) otherwise.
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/media/platform/vsp1/vsp1_video.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 7ceaf3222145..328d686189be 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -307,11 +307,6 @@ static int vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
* This function completes the current buffer by filling its sequence number,
* time stamp and payload size, and hands it back to the videobuf core.
*
- * When operating in DU output mode (deep pipeline to the DU through the LIF),
- * the VSP1 needs to constantly supply frames to the display. In that case, if
- * no other buffer is queued, reuse the one that has just been processed instead
- * of handing it back to the videobuf core.
- *
* Return the next queued buffer or NULL if the queue is empty.
*/
static struct vsp1_vb2_buffer *
@@ -333,12 +328,6 @@ vsp1_video_complete_buffer(struct vsp1_video *video)
done = list_first_entry(&video->irqqueue,
struct vsp1_vb2_buffer, queue);
- /* In DU output mode reuse the buffer if the list is singular. */
- if (pipe->lif && list_is_singular(&video->irqqueue)) {
- spin_unlock_irqrestore(&video->irqlock, flags);
- return done;
- }
-
list_del(&done->queue);
if (!list_empty(&video->irqqueue))
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 02/18] media: vsp1: wpf: Fix partition configuration for display pipelines
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 01/18] Revert "[media] v4l: vsp1: Supply frames to the DU continuously" Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 03/18] media: vsp1: Replace leftover occurrence of fragment with body Laurent Pinchart
` (15 subsequent siblings)
17 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
When configuring partitions for memory-to-memory pipelines, the WPF
accesses data of the current partition through pipe->partition.
Writeback support will require full configuration of the WPF while not
providing a valid pipe->partition. Rework the configuration code to fall
back to the full image width in that case, as is already done for the
part of the configuration currently relevant for display pipelines.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
drivers/media/platform/vsp1/vsp1_wpf.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index 32bb207b2007..a07c5944b598 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -362,6 +362,7 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
unsigned int width;
unsigned int height;
+ unsigned int left;
unsigned int offset;
unsigned int flip;
unsigned int i;
@@ -371,13 +372,16 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
RWPF_PAD_SINK);
width = sink_format->width;
height = sink_format->height;
+ left = 0;
/*
* Cropping. The partition algorithm can split the image into
* multiple slices.
*/
- if (pipe->partitions > 1)
+ if (pipe->partitions > 1) {
width = pipe->partition->wpf.width;
+ left = pipe->partition->wpf.left;
+ }
vsp1_wpf_write(wpf, dlb, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN |
(0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
@@ -408,13 +412,11 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
flip = wpf->flip.active;
if (flip & BIT(WPF_CTRL_HFLIP) && !wpf->flip.rotate)
- offset = format->width - pipe->partition->wpf.left
- - pipe->partition->wpf.width;
+ offset = format->width - left - width;
else if (flip & BIT(WPF_CTRL_VFLIP) && wpf->flip.rotate)
- offset = format->height - pipe->partition->wpf.left
- - pipe->partition->wpf.width;
+ offset = format->height - left - width;
else
- offset = pipe->partition->wpf.left;
+ offset = left;
for (i = 0; i < format->num_planes; ++i) {
unsigned int hsub = i > 0 ? fmtinfo->hsub : 1;
@@ -436,7 +438,7 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
* image height.
*/
if (wpf->flip.rotate)
- height = pipe->partition->wpf.width;
+ height = width;
else
height = format->height;
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 03/18] media: vsp1: Replace leftover occurrence of fragment with body
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 01/18] Revert "[media] v4l: vsp1: Supply frames to the DU continuously" Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 02/18] media: vsp1: wpf: Fix partition configuration for display pipelines Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 04/18] media: vsp1: Fix addresses of display-related registers for VSP-DL Laurent Pinchart
` (14 subsequent siblings)
17 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
Display list fragments have been renamed to bodies. Replace one last
occurrence of the word fragment in the documentation.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
drivers/media/platform/vsp1/vsp1_dl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
index 26289adaf658..64af449791b0 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -699,8 +699,8 @@ struct vsp1_dl_body *vsp1_dl_list_get_body0(struct vsp1_dl_list *dl)
* which bodies are added.
*
* Adding a body to a display list passes ownership of the body to the list. The
- * caller retains its reference to the fragment when adding it to the display
- * list, but is not allowed to add new entries to the body.
+ * caller retains its reference to the body when adding it to the display list,
+ * but is not allowed to add new entries to the body.
*
* The reference must be explicitly released by a call to vsp1_dl_body_put()
* when the body isn't needed anymore.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 04/18] media: vsp1: Fix addresses of display-related registers for VSP-DL
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (2 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 03/18] media: vsp1: Replace leftover occurrence of fragment with body Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 05/18] media: vsp1: Replace the display list internal flag with a flags field Laurent Pinchart
` (13 subsequent siblings)
17 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
The VSP-DL instances have two LIFs, and thus two copies of the
VI6_DISP_IRQ_ENB, VI6_DISP_IRQ_STA and VI6_WPF_WRBCK_CTRL registers. Fix
the corresponding macros accordingly.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
drivers/media/platform/vsp1/vsp1_regs.h | 6 +++---
drivers/media/platform/vsp1/vsp1_wpf.c | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 8d86f618ec77..048190fd3a2d 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -700,8 +700,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
drm_pipe->du_private = cfg->callback_data;
/* Disable the display interrupts. */
- vsp1_write(vsp1, VI6_DISP_IRQ_STA, 0);
- vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);
+ vsp1_write(vsp1, VI6_DISP_IRQ_STA(pipe_index), 0);
+ vsp1_write(vsp1, VI6_DISP_IRQ_ENB(pipe_index), 0);
/* Configure all entities in the pipeline. */
vsp1_du_pipeline_configure(pipe);
diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
index f6e4157095cc..1bb1d39c60d9 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -39,12 +39,12 @@
#define VI6_WFP_IRQ_STA_DFE (1 << 1)
#define VI6_WFP_IRQ_STA_FRE (1 << 0)
-#define VI6_DISP_IRQ_ENB 0x0078
+#define VI6_DISP_IRQ_ENB(n) (0x0078 + (n) * 60)
#define VI6_DISP_IRQ_ENB_DSTE (1 << 8)
#define VI6_DISP_IRQ_ENB_MAEE (1 << 5)
#define VI6_DISP_IRQ_ENB_LNEE(n) (1 << (n))
-#define VI6_DISP_IRQ_STA 0x007c
+#define VI6_DISP_IRQ_STA(n) (0x007c + (n) * 60)
#define VI6_DISP_IRQ_STA_DST (1 << 8)
#define VI6_DISP_IRQ_STA_MAE (1 << 5)
#define VI6_DISP_IRQ_STA_LNE(n) (1 << (n))
@@ -307,7 +307,7 @@
#define VI6_WPF_DSTM_ADDR_C0 0x1028
#define VI6_WPF_DSTM_ADDR_C1 0x102c
-#define VI6_WPF_WRBCK_CTRL 0x1034
+#define VI6_WPF_WRBCK_CTRL(n) (0x1034 + (n) * 0x100)
#define VI6_WPF_WRBCK_CTRL_WBMD (1 << 0)
/* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index a07c5944b598..18c49e3a7875 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -291,7 +291,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(wpf->entity.index),
VI6_DPR_WPF_FPORCH_FP_WPFN);
- vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL, 0);
+ vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(wpf->entity.index), 0);
/*
* Sources. If the pipeline has a single input and BRx is not used,
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 05/18] media: vsp1: Replace the display list internal flag with a flags field
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (3 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 04/18] media: vsp1: Fix addresses of display-related registers for VSP-DL Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 06/18] media: vsp1: Add vsp1_dl_list argument to .configure_stream() operation Laurent Pinchart
` (12 subsequent siblings)
17 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
To prepare for addition of more flags to the display list, replace the
'internal' flag field by a bitmask 'flags' field.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
Changes since v4:
- Fix check for the completed flag in vsp1_du_pipeline_frame_end()
---
drivers/media/platform/vsp1/vsp1_dl.c | 31 +++++++++++++-----------
drivers/media/platform/vsp1/vsp1_dl.h | 2 +-
drivers/media/platform/vsp1/vsp1_drm.c | 8 ++++--
drivers/media/platform/vsp1/vsp1_video.c | 2 +-
4 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
index 64af449791b0..886b3a69d329 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -178,7 +178,7 @@ struct vsp1_dl_cmd_pool {
* @post_cmd: post command to be issued through extended dl header
* @has_chain: if true, indicates that there's a partition chain
* @chain: entry in the display list partition chain
- * @internal: whether the display list is used for internal purpose
+ * @flags: display list flags, a combination of VSP1_DL_FRAME_END_*
*/
struct vsp1_dl_list {
struct list_head list;
@@ -197,7 +197,7 @@ struct vsp1_dl_list {
bool has_chain;
struct list_head chain;
- bool internal;
+ unsigned int flags;
};
/**
@@ -861,13 +861,15 @@ static void vsp1_dl_list_commit_continuous(struct vsp1_dl_list *dl)
*
* If a display list is already pending we simply drop it as the new
* display list is assumed to contain a more recent configuration. It is
- * an error if the already pending list has the internal flag set, as
- * there is then a process waiting for that list to complete. This
- * shouldn't happen as the waiting process should perform proper
- * locking, but warn just in case.
+ * an error if the already pending list has the
+ * VSP1_DL_FRAME_END_INTERNAL flag set, as there is then a process
+ * waiting for that list to complete. This shouldn't happen as the
+ * waiting process should perform proper locking, but warn just in
+ * case.
*/
if (vsp1_dl_list_hw_update_pending(dlm)) {
- WARN_ON(dlm->pending && dlm->pending->internal);
+ WARN_ON(dlm->pending &&
+ (dlm->pending->flags & VSP1_DL_FRAME_END_INTERNAL));
__vsp1_dl_list_put(dlm->pending);
dlm->pending = dl;
return;
@@ -897,7 +899,7 @@ static void vsp1_dl_list_commit_singleshot(struct vsp1_dl_list *dl)
dlm->active = dl;
}
-void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
+void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
{
struct vsp1_dl_manager *dlm = dl->dlm;
struct vsp1_dl_list *dl_next;
@@ -912,7 +914,7 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
vsp1_dl_list_fill_header(dl_next, last);
}
- dl->internal = internal;
+ dl->flags = dl_flags & ~VSP1_DL_FRAME_END_COMPLETED;
spin_lock_irqsave(&dlm->lock, flags);
@@ -941,9 +943,10 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
* set in single-shot mode as display list processing is then not continuous and
* races never occur.
*
- * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the previous display list
- * has completed and had been queued with the internal notification flag.
- * Internal notification is only supported for continuous mode.
+ * The following flags are only supported for continuous mode.
+ *
+ * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
+ * became active had been queued with the internal notification flag.
*/
unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
{
@@ -986,9 +989,9 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
* frame end interrupt. The display list thus becomes active.
*/
if (dlm->queued) {
- if (dlm->queued->internal)
+ if (dlm->queued->flags & VSP1_DL_FRAME_END_INTERNAL)
flags |= VSP1_DL_FRAME_END_INTERNAL;
- dlm->queued->internal = false;
+ dlm->queued->flags &= ~VSP1_DL_FRAME_END_INTERNAL;
__vsp1_dl_list_put(dlm->active);
dlm->active = dlm->queued;
diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
index 125750dc8b5c..e0fdb145e6ed 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.h
+++ b/drivers/media/platform/vsp1/vsp1_dl.h
@@ -61,7 +61,7 @@ struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
void vsp1_dl_list_put(struct vsp1_dl_list *dl);
struct vsp1_dl_body *vsp1_dl_list_get_body0(struct vsp1_dl_list *dl);
struct vsp1_dl_ext_cmd *vsp1_dl_get_pre_cmd(struct vsp1_dl_list *dl);
-void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal);
+void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags);
struct vsp1_dl_body_pool *
vsp1_dl_body_pool_create(struct vsp1_device *vsp1, unsigned int num_bodies,
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 048190fd3a2d..89773d3a916c 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -34,7 +34,7 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
unsigned int completion)
{
struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
- bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
+ bool complete = completion & VSP1_DL_FRAME_END_COMPLETED;
if (drm_pipe->du_complete) {
struct vsp1_entity *uif = drm_pipe->uif;
@@ -537,6 +537,10 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
struct vsp1_entity *next;
struct vsp1_dl_list *dl;
struct vsp1_dl_body *dlb;
+ unsigned int dl_flags = 0;
+
+ if (drm_pipe->force_brx_release)
+ dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
dl = vsp1_dl_list_get(pipe->output->dlm);
dlb = vsp1_dl_list_get_body0(dl);
@@ -559,7 +563,7 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
vsp1_entity_configure_partition(entity, pipe, dl, dlb);
}
- vsp1_dl_list_commit(dl, drm_pipe->force_brx_release);
+ vsp1_dl_list_commit(dl, dl_flags);
}
/* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 328d686189be..9ae20982604a 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -421,7 +421,7 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline *pipe)
}
/* Complete, and commit the head display list. */
- vsp1_dl_list_commit(dl, false);
+ vsp1_dl_list_commit(dl, 0);
pipe->configured = true;
vsp1_pipeline_run(pipe);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 06/18] media: vsp1: Add vsp1_dl_list argument to .configure_stream() operation
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (4 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 05/18] media: vsp1: Replace the display list internal flag with a flags field Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 10:36 ` Kieran Bingham
2019-03-13 0:05 ` [PATCH v6 07/18] media: vsp1: dl: Allow chained display lists for display pipelines Laurent Pinchart
` (11 subsequent siblings)
17 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
The WPF needs access to the current display list to configure writeback.
Add a display list pointer to the VSP1 entity .configure_stream()
operation.
Only display pipelines can make use of the display list there as
mem-to-mem pipelines don't have access to a display list at stream
configuration time. This is not an issue as writeback is only used for
display pipelines.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/media/platform/vsp1/vsp1_brx.c | 1 +
drivers/media/platform/vsp1/vsp1_clu.c | 1 +
drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
drivers/media/platform/vsp1/vsp1_entity.c | 3 ++-
drivers/media/platform/vsp1/vsp1_entity.h | 7 +++++--
drivers/media/platform/vsp1/vsp1_hgo.c | 1 +
drivers/media/platform/vsp1/vsp1_hgt.c | 1 +
drivers/media/platform/vsp1/vsp1_hsit.c | 1 +
drivers/media/platform/vsp1/vsp1_lif.c | 1 +
drivers/media/platform/vsp1/vsp1_lut.c | 1 +
drivers/media/platform/vsp1/vsp1_rpf.c | 1 +
drivers/media/platform/vsp1/vsp1_sru.c | 1 +
drivers/media/platform/vsp1/vsp1_uds.c | 1 +
drivers/media/platform/vsp1/vsp1_uif.c | 1 +
drivers/media/platform/vsp1/vsp1_video.c | 3 ++-
drivers/media/platform/vsp1/vsp1_wpf.c | 1 +
16 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_brx.c b/drivers/media/platform/vsp1/vsp1_brx.c
index 5e50178b057d..43468bc8fb56 100644
--- a/drivers/media/platform/vsp1/vsp1_brx.c
+++ b/drivers/media/platform/vsp1/vsp1_brx.c
@@ -283,6 +283,7 @@ static const struct v4l2_subdev_ops brx_ops = {
static void brx_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
struct vsp1_brx *brx = to_brx(&entity->subdev);
diff --git a/drivers/media/platform/vsp1/vsp1_clu.c b/drivers/media/platform/vsp1/vsp1_clu.c
index 942fc14c19d1..a47b23bf5abf 100644
--- a/drivers/media/platform/vsp1/vsp1_clu.c
+++ b/drivers/media/platform/vsp1/vsp1_clu.c
@@ -171,6 +171,7 @@ static const struct v4l2_subdev_ops clu_ops = {
static void clu_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
struct vsp1_clu *clu = to_clu(&entity->subdev);
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 89773d3a916c..4f1bc51d1ef4 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -558,7 +558,7 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
}
vsp1_entity_route_setup(entity, pipe, dlb);
- vsp1_entity_configure_stream(entity, pipe, dlb);
+ vsp1_entity_configure_stream(entity, pipe, dl, dlb);
vsp1_entity_configure_frame(entity, pipe, dl, dlb);
vsp1_entity_configure_partition(entity, pipe, dl, dlb);
}
diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
index a54ab528b060..aa9d2286056e 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.c
+++ b/drivers/media/platform/vsp1/vsp1_entity.c
@@ -71,10 +71,11 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity,
void vsp1_entity_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
if (entity->ops->configure_stream)
- entity->ops->configure_stream(entity, pipe, dlb);
+ entity->ops->configure_stream(entity, pipe, dl, dlb);
}
void vsp1_entity_configure_frame(struct vsp1_entity *entity,
diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h
index 97acb7795cf1..a1ceb37bb837 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.h
+++ b/drivers/media/platform/vsp1/vsp1_entity.h
@@ -67,7 +67,9 @@ struct vsp1_route {
* struct vsp1_entity_operations - Entity operations
* @destroy: Destroy the entity.
* @configure_stream: Setup the hardware parameters for the stream which do
- * not vary between frames (pipeline, formats).
+ * not vary between frames (pipeline, formats). Note that
+ * the vsp1_dl_list argument is only valid for display
+ * pipeline and will be NULL for mem-to-mem pipelines.
* @configure_frame: Configure the runtime parameters for each frame.
* @configure_partition: Configure partition specific parameters.
* @max_width: Return the max supported width of data that the entity can
@@ -78,7 +80,7 @@ struct vsp1_route {
struct vsp1_entity_operations {
void (*destroy)(struct vsp1_entity *);
void (*configure_stream)(struct vsp1_entity *, struct vsp1_pipeline *,
- struct vsp1_dl_body *);
+ struct vsp1_dl_list *, struct vsp1_dl_body *);
void (*configure_frame)(struct vsp1_entity *, struct vsp1_pipeline *,
struct vsp1_dl_list *, struct vsp1_dl_body *);
void (*configure_partition)(struct vsp1_entity *,
@@ -155,6 +157,7 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity,
void vsp1_entity_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb);
void vsp1_entity_configure_frame(struct vsp1_entity *entity,
diff --git a/drivers/media/platform/vsp1/vsp1_hgo.c b/drivers/media/platform/vsp1/vsp1_hgo.c
index 827373c25351..bf3f981f93a1 100644
--- a/drivers/media/platform/vsp1/vsp1_hgo.c
+++ b/drivers/media/platform/vsp1/vsp1_hgo.c
@@ -131,6 +131,7 @@ static const struct v4l2_ctrl_config hgo_num_bins_control = {
static void hgo_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
struct vsp1_hgo *hgo = to_hgo(&entity->subdev);
diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c b/drivers/media/platform/vsp1/vsp1_hgt.c
index bb6ce6fdd5f4..aa1c718e0453 100644
--- a/drivers/media/platform/vsp1/vsp1_hgt.c
+++ b/drivers/media/platform/vsp1/vsp1_hgt.c
@@ -127,6 +127,7 @@ static const struct v4l2_ctrl_config hgt_hue_areas = {
static void hgt_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
struct vsp1_hgt *hgt = to_hgt(&entity->subdev);
diff --git a/drivers/media/platform/vsp1/vsp1_hsit.c b/drivers/media/platform/vsp1/vsp1_hsit.c
index 39ab2e0c7c18..d5ebd9d08c8a 100644
--- a/drivers/media/platform/vsp1/vsp1_hsit.c
+++ b/drivers/media/platform/vsp1/vsp1_hsit.c
@@ -129,6 +129,7 @@ static const struct v4l2_subdev_ops hsit_ops = {
static void hsit_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
struct vsp1_hsit *hsit = to_hsit(&entity->subdev);
diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c
index 8b0a26335d70..14ed5d7bd061 100644
--- a/drivers/media/platform/vsp1/vsp1_lif.c
+++ b/drivers/media/platform/vsp1/vsp1_lif.c
@@ -84,6 +84,7 @@ static const struct v4l2_subdev_ops lif_ops = {
static void lif_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
const struct v4l2_mbus_framefmt *format;
diff --git a/drivers/media/platform/vsp1/vsp1_lut.c b/drivers/media/platform/vsp1/vsp1_lut.c
index 64c48d9459b0..9f88842d7048 100644
--- a/drivers/media/platform/vsp1/vsp1_lut.c
+++ b/drivers/media/platform/vsp1/vsp1_lut.c
@@ -147,6 +147,7 @@ static const struct v4l2_subdev_ops lut_ops = {
static void lut_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
struct vsp1_lut *lut = to_lut(&entity->subdev);
diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
index 616afa7e165f..85587c1b6a37 100644
--- a/drivers/media/platform/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rpf.c
@@ -57,6 +57,7 @@ static const struct v4l2_subdev_ops rpf_ops = {
static void rpf_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
struct vsp1_rwpf *rpf = to_rwpf(&entity->subdev);
diff --git a/drivers/media/platform/vsp1/vsp1_sru.c b/drivers/media/platform/vsp1/vsp1_sru.c
index b1617cb1f2b9..2b65457ee12f 100644
--- a/drivers/media/platform/vsp1/vsp1_sru.c
+++ b/drivers/media/platform/vsp1/vsp1_sru.c
@@ -269,6 +269,7 @@ static const struct v4l2_subdev_ops sru_ops = {
static void sru_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
const struct vsp1_sru_param *param;
diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
index 27012af973b2..5fc04c082d1a 100644
--- a/drivers/media/platform/vsp1/vsp1_uds.c
+++ b/drivers/media/platform/vsp1/vsp1_uds.c
@@ -257,6 +257,7 @@ static const struct v4l2_subdev_ops uds_ops = {
static void uds_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
struct vsp1_uds *uds = to_uds(&entity->subdev);
diff --git a/drivers/media/platform/vsp1/vsp1_uif.c b/drivers/media/platform/vsp1/vsp1_uif.c
index 4b58d51df231..467d1072577b 100644
--- a/drivers/media/platform/vsp1/vsp1_uif.c
+++ b/drivers/media/platform/vsp1/vsp1_uif.c
@@ -192,6 +192,7 @@ static const struct v4l2_subdev_ops uif_ops = {
static void uif_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
struct vsp1_uif *uif = to_uif(&entity->subdev);
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 9ae20982604a..fd98e483b2f4 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -825,7 +825,8 @@ static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
list_for_each_entry(entity, &pipe->entities, list_pipe) {
vsp1_entity_route_setup(entity, pipe, pipe->stream_config);
- vsp1_entity_configure_stream(entity, pipe, pipe->stream_config);
+ vsp1_entity_configure_stream(entity, pipe, NULL,
+ pipe->stream_config);
}
return 0;
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index 18c49e3a7875..fc5c1b0f6633 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -234,6 +234,7 @@ static void vsp1_wpf_destroy(struct vsp1_entity *entity)
static void wpf_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
+ struct vsp1_dl_list *dl,
struct vsp1_dl_body *dlb)
{
struct vsp1_rwpf *wpf = to_rwpf(&entity->subdev);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 06/18] media: vsp1: Add vsp1_dl_list argument to .configure_stream() operation
2019-03-13 0:05 ` [PATCH v6 06/18] media: vsp1: Add vsp1_dl_list argument to .configure_stream() operation Laurent Pinchart
@ 2019-03-13 10:36 ` Kieran Bingham
0 siblings, 0 replies; 36+ messages in thread
From: Kieran Bingham @ 2019-03-13 10:36 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc, Liviu Dudau, linux-media
Hi Laurent,
On 13/03/2019 00:05, Laurent Pinchart wrote:
> The WPF needs access to the current display list to configure writeback.
> Add a display list pointer to the VSP1 entity .configure_stream()
> operation.
>
> Only display pipelines can make use of the display list there as
> mem-to-mem pipelines don't have access to a display list at stream
> configuration time. This is not an issue as writeback is only used for
> display pipelines.
As we purposefully inject NULL DL variables, Do we need to add NULL
checks in to these functions
I think I'm happy leaving the null-checks out - because as you state
above - the only time this variable is used, it will not be NULL, and it
is documented as such in the code.
So,
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> drivers/media/platform/vsp1/vsp1_brx.c | 1 +
> drivers/media/platform/vsp1/vsp1_clu.c | 1 +
> drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> drivers/media/platform/vsp1/vsp1_entity.c | 3 ++-
> drivers/media/platform/vsp1/vsp1_entity.h | 7 +++++--
> drivers/media/platform/vsp1/vsp1_hgo.c | 1 +
> drivers/media/platform/vsp1/vsp1_hgt.c | 1 +
> drivers/media/platform/vsp1/vsp1_hsit.c | 1 +
> drivers/media/platform/vsp1/vsp1_lif.c | 1 +
> drivers/media/platform/vsp1/vsp1_lut.c | 1 +
> drivers/media/platform/vsp1/vsp1_rpf.c | 1 +
> drivers/media/platform/vsp1/vsp1_sru.c | 1 +
> drivers/media/platform/vsp1/vsp1_uds.c | 1 +
> drivers/media/platform/vsp1/vsp1_uif.c | 1 +
> drivers/media/platform/vsp1/vsp1_video.c | 3 ++-
> drivers/media/platform/vsp1/vsp1_wpf.c | 1 +
> 16 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_brx.c b/drivers/media/platform/vsp1/vsp1_brx.c
> index 5e50178b057d..43468bc8fb56 100644
> --- a/drivers/media/platform/vsp1/vsp1_brx.c
> +++ b/drivers/media/platform/vsp1/vsp1_brx.c
> @@ -283,6 +283,7 @@ static const struct v4l2_subdev_ops brx_ops = {
>
> static void brx_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> struct vsp1_brx *brx = to_brx(&entity->subdev);
> diff --git a/drivers/media/platform/vsp1/vsp1_clu.c b/drivers/media/platform/vsp1/vsp1_clu.c
> index 942fc14c19d1..a47b23bf5abf 100644
> --- a/drivers/media/platform/vsp1/vsp1_clu.c
> +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> @@ -171,6 +171,7 @@ static const struct v4l2_subdev_ops clu_ops = {
>
> static void clu_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> struct vsp1_clu *clu = to_clu(&entity->subdev);
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 89773d3a916c..4f1bc51d1ef4 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -558,7 +558,7 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
> }
>
> vsp1_entity_route_setup(entity, pipe, dlb);
> - vsp1_entity_configure_stream(entity, pipe, dlb);
> + vsp1_entity_configure_stream(entity, pipe, dl, dlb);
> vsp1_entity_configure_frame(entity, pipe, dl, dlb);
> vsp1_entity_configure_partition(entity, pipe, dl, dlb);
> }
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
> index a54ab528b060..aa9d2286056e 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -71,10 +71,11 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity,
>
> void vsp1_entity_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> if (entity->ops->configure_stream)
> - entity->ops->configure_stream(entity, pipe, dlb);
> + entity->ops->configure_stream(entity, pipe, dl, dlb);
> }
>
> void vsp1_entity_configure_frame(struct vsp1_entity *entity,
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h
> index 97acb7795cf1..a1ceb37bb837 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.h
> +++ b/drivers/media/platform/vsp1/vsp1_entity.h
> @@ -67,7 +67,9 @@ struct vsp1_route {
> * struct vsp1_entity_operations - Entity operations
> * @destroy: Destroy the entity.
> * @configure_stream: Setup the hardware parameters for the stream which do
> - * not vary between frames (pipeline, formats).
> + * not vary between frames (pipeline, formats). Note that
> + * the vsp1_dl_list argument is only valid for display
> + * pipeline and will be NULL for mem-to-mem pipelines.
> * @configure_frame: Configure the runtime parameters for each frame.
> * @configure_partition: Configure partition specific parameters.
> * @max_width: Return the max supported width of data that the entity can
> @@ -78,7 +80,7 @@ struct vsp1_route {
> struct vsp1_entity_operations {
> void (*destroy)(struct vsp1_entity *);
> void (*configure_stream)(struct vsp1_entity *, struct vsp1_pipeline *,
> - struct vsp1_dl_body *);
> + struct vsp1_dl_list *, struct vsp1_dl_body *);
> void (*configure_frame)(struct vsp1_entity *, struct vsp1_pipeline *,
> struct vsp1_dl_list *, struct vsp1_dl_body *);
> void (*configure_partition)(struct vsp1_entity *,
> @@ -155,6 +157,7 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity,
>
> void vsp1_entity_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb);
>
> void vsp1_entity_configure_frame(struct vsp1_entity *entity,
> diff --git a/drivers/media/platform/vsp1/vsp1_hgo.c b/drivers/media/platform/vsp1/vsp1_hgo.c
> index 827373c25351..bf3f981f93a1 100644
> --- a/drivers/media/platform/vsp1/vsp1_hgo.c
> +++ b/drivers/media/platform/vsp1/vsp1_hgo.c
> @@ -131,6 +131,7 @@ static const struct v4l2_ctrl_config hgo_num_bins_control = {
>
> static void hgo_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> struct vsp1_hgo *hgo = to_hgo(&entity->subdev);
> diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c b/drivers/media/platform/vsp1/vsp1_hgt.c
> index bb6ce6fdd5f4..aa1c718e0453 100644
> --- a/drivers/media/platform/vsp1/vsp1_hgt.c
> +++ b/drivers/media/platform/vsp1/vsp1_hgt.c
> @@ -127,6 +127,7 @@ static const struct v4l2_ctrl_config hgt_hue_areas = {
>
> static void hgt_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> struct vsp1_hgt *hgt = to_hgt(&entity->subdev);
> diff --git a/drivers/media/platform/vsp1/vsp1_hsit.c b/drivers/media/platform/vsp1/vsp1_hsit.c
> index 39ab2e0c7c18..d5ebd9d08c8a 100644
> --- a/drivers/media/platform/vsp1/vsp1_hsit.c
> +++ b/drivers/media/platform/vsp1/vsp1_hsit.c
> @@ -129,6 +129,7 @@ static const struct v4l2_subdev_ops hsit_ops = {
>
> static void hsit_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> struct vsp1_hsit *hsit = to_hsit(&entity->subdev);
> diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c
> index 8b0a26335d70..14ed5d7bd061 100644
> --- a/drivers/media/platform/vsp1/vsp1_lif.c
> +++ b/drivers/media/platform/vsp1/vsp1_lif.c
> @@ -84,6 +84,7 @@ static const struct v4l2_subdev_ops lif_ops = {
>
> static void lif_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> const struct v4l2_mbus_framefmt *format;
> diff --git a/drivers/media/platform/vsp1/vsp1_lut.c b/drivers/media/platform/vsp1/vsp1_lut.c
> index 64c48d9459b0..9f88842d7048 100644
> --- a/drivers/media/platform/vsp1/vsp1_lut.c
> +++ b/drivers/media/platform/vsp1/vsp1_lut.c
> @@ -147,6 +147,7 @@ static const struct v4l2_subdev_ops lut_ops = {
>
> static void lut_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> struct vsp1_lut *lut = to_lut(&entity->subdev);
> diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
> index 616afa7e165f..85587c1b6a37 100644
> --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rpf.c
> @@ -57,6 +57,7 @@ static const struct v4l2_subdev_ops rpf_ops = {
>
> static void rpf_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> struct vsp1_rwpf *rpf = to_rwpf(&entity->subdev);
> diff --git a/drivers/media/platform/vsp1/vsp1_sru.c b/drivers/media/platform/vsp1/vsp1_sru.c
> index b1617cb1f2b9..2b65457ee12f 100644
> --- a/drivers/media/platform/vsp1/vsp1_sru.c
> +++ b/drivers/media/platform/vsp1/vsp1_sru.c
> @@ -269,6 +269,7 @@ static const struct v4l2_subdev_ops sru_ops = {
>
> static void sru_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> const struct vsp1_sru_param *param;
> diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
> index 27012af973b2..5fc04c082d1a 100644
> --- a/drivers/media/platform/vsp1/vsp1_uds.c
> +++ b/drivers/media/platform/vsp1/vsp1_uds.c
> @@ -257,6 +257,7 @@ static const struct v4l2_subdev_ops uds_ops = {
>
> static void uds_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> struct vsp1_uds *uds = to_uds(&entity->subdev);
> diff --git a/drivers/media/platform/vsp1/vsp1_uif.c b/drivers/media/platform/vsp1/vsp1_uif.c
> index 4b58d51df231..467d1072577b 100644
> --- a/drivers/media/platform/vsp1/vsp1_uif.c
> +++ b/drivers/media/platform/vsp1/vsp1_uif.c
> @@ -192,6 +192,7 @@ static const struct v4l2_subdev_ops uif_ops = {
>
> static void uif_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> struct vsp1_uif *uif = to_uif(&entity->subdev);
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> index 9ae20982604a..fd98e483b2f4 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -825,7 +825,8 @@ static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
>
> list_for_each_entry(entity, &pipe->entities, list_pipe) {
> vsp1_entity_route_setup(entity, pipe, pipe->stream_config);
> - vsp1_entity_configure_stream(entity, pipe, pipe->stream_config);
> + vsp1_entity_configure_stream(entity, pipe, NULL,
> + pipe->stream_config);
> }
>
> return 0;
> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
> index 18c49e3a7875..fc5c1b0f6633 100644
> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> @@ -234,6 +234,7 @@ static void vsp1_wpf_destroy(struct vsp1_entity *entity)
>
> static void wpf_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> + struct vsp1_dl_list *dl,
> struct vsp1_dl_body *dlb)
> {
> struct vsp1_rwpf *wpf = to_rwpf(&entity->subdev);
>
--
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 07/18] media: vsp1: dl: Allow chained display lists for display pipelines
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (5 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 06/18] media: vsp1: Add vsp1_dl_list argument to .configure_stream() operation Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 11:07 ` Kieran Bingham
2019-03-13 0:05 ` [PATCH v6 08/18] media: vsp1: wpf: Add writeback support Laurent Pinchart
` (10 subsequent siblings)
17 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
Refactor the display list header setup to allow chained display lists
with display pipelines. Chain the display lists as for mem-to-mem
pipelines, but enable the frame end interrupt for every list as display
pipelines have a single list per frame.
This feature will be used to disable writeback exactly one frame after
enabling it by chaining a writeback disable display list.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/media/platform/vsp1/vsp1_dl.c | 35 ++++++++++++++++++---------
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
index 886b3a69d329..ed7cda4130f2 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -770,17 +770,35 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last)
}
dl->header->num_lists = num_lists;
+ dl->header->flags = 0;
- if (!list_empty(&dl->chain) && !is_last) {
+ /*
+ * Enable the interrupt for the end of each frame. In continuous mode
+ * chained lists are used with one list per frame, so enable the
+ * interrupt for each list. In singleshot mode chained lists are used
+ * to partition a single frame, so enable the interrupt for the last
+ * list only.
+ */
+ if (!dlm->singleshot || is_last)
+ dl->header->flags |= VSP1_DLH_INT_ENABLE;
+
+ /*
+ * In continuous mode enable auto-start for all lists, as the VSP must
+ * loop on the same list until a new one is queued. In singleshot mode
+ * enable auto-start for all lists but the last to chain processing of
+ * partitions without software intervention.
+ */
+ if (!dlm->singleshot || !is_last)
+ dl->header->flags |= VSP1_DLH_AUTO_START;
+
+ if (!is_last) {
/*
- * If this display list's chain is not empty, we are on a list,
- * and the next item is the display list that we must queue for
- * automatic processing by the hardware.
+ * If this is not the last display list in the chain, queue the
+ * next item for automatic processing by the hardware.
*/
struct vsp1_dl_list *next = list_next_entry(dl, chain);
dl->header->next_header = next->dma;
- dl->header->flags = VSP1_DLH_AUTO_START;
} else if (!dlm->singleshot) {
/*
* if the display list manager works in continuous mode, the VSP
@@ -788,13 +806,6 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last)
* instructed to do otherwise.
*/
dl->header->next_header = dl->dma;
- dl->header->flags = VSP1_DLH_INT_ENABLE | VSP1_DLH_AUTO_START;
- } else {
- /*
- * Otherwise, in mem-to-mem mode, we work in single-shot mode
- * and the next display list must not be started automatically.
- */
- dl->header->flags = VSP1_DLH_INT_ENABLE;
}
if (!dl->extension)
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 07/18] media: vsp1: dl: Allow chained display lists for display pipelines
2019-03-13 0:05 ` [PATCH v6 07/18] media: vsp1: dl: Allow chained display lists for display pipelines Laurent Pinchart
@ 2019-03-13 11:07 ` Kieran Bingham
0 siblings, 0 replies; 36+ messages in thread
From: Kieran Bingham @ 2019-03-13 11:07 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc, Liviu Dudau, linux-media
Hi Laurent,
On 13/03/2019 00:05, Laurent Pinchart wrote:
> Refactor the display list header setup to allow chained display lists
> with display pipelines. Chain the display lists as for mem-to-mem
> pipelines, but enable the frame end interrupt for every list as display
> pipelines have a single list per frame.
>
> This feature will be used to disable writeback exactly one frame after
> enabling it by chaining a writeback disable display list.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
This is a bit hard to grok, but I can see that the key change is now
continuous display pipelines can go into the code path which sets the
vsp1_dl_list *next. The rest of the refactoring looks good too.
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> drivers/media/platform/vsp1/vsp1_dl.c | 35 ++++++++++++++++++---------
> 1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> index 886b3a69d329..ed7cda4130f2 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -770,17 +770,35 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last)
> }
>
> dl->header->num_lists = num_lists;
> + dl->header->flags = 0;
>
> - if (!list_empty(&dl->chain) && !is_last) {
> + /*
> + * Enable the interrupt for the end of each frame. In continuous mode
> + * chained lists are used with one list per frame, so enable the
> + * interrupt for each list. In singleshot mode chained lists are used
> + * to partition a single frame, so enable the interrupt for the last
> + * list only.
> + */
> + if (!dlm->singleshot || is_last)
> + dl->header->flags |= VSP1_DLH_INT_ENABLE;
> +
> + /*
> + * In continuous mode enable auto-start for all lists, as the VSP must
> + * loop on the same list until a new one is queued. In singleshot mode
> + * enable auto-start for all lists but the last to chain processing of
> + * partitions without software intervention.
> + */
> + if (!dlm->singleshot || !is_last)
> + dl->header->flags |= VSP1_DLH_AUTO_START;
> +
> + if (!is_last) {
> /*
> - * If this display list's chain is not empty, we are on a list,
> - * and the next item is the display list that we must queue for
> - * automatic processing by the hardware.
> + * If this is not the last display list in the chain, queue the
> + * next item for automatic processing by the hardware.
> */
> struct vsp1_dl_list *next = list_next_entry(dl, chain);
>
> dl->header->next_header = next->dma;
> - dl->header->flags = VSP1_DLH_AUTO_START;
> } else if (!dlm->singleshot) {
> /*
> * if the display list manager works in continuous mode, the VSP
> @@ -788,13 +806,6 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last)
> * instructed to do otherwise.
> */
> dl->header->next_header = dl->dma;
> - dl->header->flags = VSP1_DLH_INT_ENABLE | VSP1_DLH_AUTO_START;
> - } else {
> - /*
> - * Otherwise, in mem-to-mem mode, we work in single-shot mode
> - * and the next display list must not be started automatically.
> - */
> - dl->header->flags = VSP1_DLH_INT_ENABLE;
> }
>
> if (!dl->extension)
>
--
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 08/18] media: vsp1: wpf: Add writeback support
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (6 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 07/18] media: vsp1: dl: Allow chained display lists for display pipelines Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 10:59 ` Kieran Bingham
2019-03-13 0:05 ` [PATCH v6 09/18] media: vsp1: drm: Split RPF format setting to separate function Laurent Pinchart
` (9 subsequent siblings)
17 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
Add support for the writeback feature of the WPF, to enable capturing
frames at the WPF output for display pipelines. To enable writeback the
vsp1_rwpf structure mem field must be set to the address of the
writeback buffer and the writeback field set to true before the WPF
.configure_stream() and .configure_partition() are called. The WPF will
enable writeback in the display list for a single frame, and writeback
will then be automatically disabled.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/media/platform/vsp1/vsp1_rwpf.h | 2 +
drivers/media/platform/vsp1/vsp1_wpf.c | 73 ++++++++++++++++++++++---
2 files changed, 66 insertions(+), 9 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h b/drivers/media/platform/vsp1/vsp1_rwpf.h
index 70742ecf766f..910990b27617 100644
--- a/drivers/media/platform/vsp1/vsp1_rwpf.h
+++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
@@ -35,6 +35,7 @@ struct vsp1_rwpf {
struct v4l2_ctrl_handler ctrls;
struct vsp1_video *video;
+ bool has_writeback;
unsigned int max_width;
unsigned int max_height;
@@ -61,6 +62,7 @@ struct vsp1_rwpf {
} flip;
struct vsp1_rwpf_memory mem;
+ bool writeback;
struct vsp1_dl_manager *dlm;
};
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index fc5c1b0f6633..390ac478336d 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -232,6 +232,27 @@ static void vsp1_wpf_destroy(struct vsp1_entity *entity)
vsp1_dlm_destroy(wpf->dlm);
}
+static int wpf_configure_writeback_chain(struct vsp1_rwpf *wpf,
+ struct vsp1_dl_list *dl)
+{
+ unsigned int index = wpf->entity.index;
+ struct vsp1_dl_list *dl_next;
+ struct vsp1_dl_body *dlb;
+
+ dl_next = vsp1_dl_list_get(wpf->dlm);
+ if (!dl_next) {
+ dev_err(wpf->entity.vsp1->dev,
+ "Failed to obtain a dl list, disabling writeback\n");
+ return -ENOMEM;
+ }
+
+ dlb = vsp1_dl_list_get_body0(dl_next);
+ vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(index), 0);
+ vsp1_dl_list_add_chain(dl, dl_next);
+
+ return 0;
+}
+
static void wpf_configure_stream(struct vsp1_entity *entity,
struct vsp1_pipeline *pipe,
struct vsp1_dl_list *dl,
@@ -241,9 +262,11 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
struct vsp1_device *vsp1 = wpf->entity.vsp1;
const struct v4l2_mbus_framefmt *source_format;
const struct v4l2_mbus_framefmt *sink_format;
+ unsigned int index = wpf->entity.index;
unsigned int i;
u32 outfmt = 0;
u32 srcrpf = 0;
+ int ret;
sink_format = vsp1_entity_get_pad_format(&wpf->entity,
wpf->entity.config,
@@ -251,8 +274,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
source_format = vsp1_entity_get_pad_format(&wpf->entity,
wpf->entity.config,
RWPF_PAD_SOURCE);
+
/* Format */
- if (!pipe->lif) {
+ if (!pipe->lif || wpf->writeback) {
const struct v4l2_pix_format_mplane *format = &wpf->format;
const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
@@ -277,8 +301,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
vsp1_wpf_write(wpf, dlb, VI6_WPF_DSWAP, fmtinfo->swap);
- if (vsp1_feature(vsp1, VSP1_HAS_WPF_HFLIP) &&
- wpf->entity.index == 0)
+ if (vsp1_feature(vsp1, VSP1_HAS_WPF_HFLIP) && index == 0)
vsp1_wpf_write(wpf, dlb, VI6_WPF_ROT_CTRL,
VI6_WPF_ROT_CTRL_LN16 |
(256 << VI6_WPF_ROT_CTRL_LMEM_WD_SHIFT));
@@ -289,11 +312,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
wpf->outfmt = outfmt;
- vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(wpf->entity.index),
+ vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(index),
VI6_DPR_WPF_FPORCH_FP_WPFN);
- vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(wpf->entity.index), 0);
-
/*
* Sources. If the pipeline has a single input and BRx is not used,
* configure it as the master layer. Otherwise configure all
@@ -319,9 +340,26 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
/* Enable interrupts. */
- vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(wpf->entity.index), 0);
- vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(wpf->entity.index),
+ vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
+ vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index),
VI6_WFP_IRQ_ENB_DFEE);
+
+ /*
+ * Configure writeback for display pipelines (the wpf writeback flag is
+ * never set for memory-to-memory pipelines). Start by adding a chained
+ * display list to disable writeback after a single frame, and process
+ * to enable writeback. If the display list allocation fails don't
+ * enable writeback as we wouldn't be able to safely disable it,
+ * resulting in possible memory corruption.
+ */
+ if (wpf->writeback) {
+ ret = wpf_configure_writeback_chain(wpf, dl);
+ if (ret < 0)
+ wpf->writeback = false;
+ }
+
+ vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(index),
+ wpf->writeback ? VI6_WPF_WRBCK_CTRL_WBMD : 0);
}
static void wpf_configure_frame(struct vsp1_entity *entity,
@@ -391,7 +429,11 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
(0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
(height << VI6_WPF_SZCLIP_SIZE_SHIFT));
- if (pipe->lif)
+ /*
+ * For display pipelines without writeback enabled there's no memory
+ * address to configure, return now.
+ */
+ if (pipe->lif && !wpf->writeback)
return;
/*
@@ -480,6 +522,12 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_Y, mem.addr[0]);
vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_C0, mem.addr[1]);
vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_C1, mem.addr[2]);
+
+ /*
+ * Writeback operates in single-shot mode and lasts for a single frame,
+ * reset the writeback flag to false for the next frame.
+ */
+ wpf->writeback = false;
}
static unsigned int wpf_max_width(struct vsp1_entity *entity,
@@ -530,6 +578,13 @@ struct vsp1_rwpf *vsp1_wpf_create(struct vsp1_device *vsp1, unsigned int index)
wpf->max_height = WPF_GEN3_MAX_HEIGHT;
}
+ /*
+ * On Gen3 WPFs with a LIF output can also write to memory for display
+ * writeback.
+ */
+ if (vsp1->info->gen > 2 && index < vsp1->info->lif_count)
+ wpf->has_writeback = true;
+
wpf->entity.ops = &wpf_entity_ops;
wpf->entity.type = VSP1_ENTITY_WPF;
wpf->entity.index = index;
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 08/18] media: vsp1: wpf: Add writeback support
2019-03-13 0:05 ` [PATCH v6 08/18] media: vsp1: wpf: Add writeback support Laurent Pinchart
@ 2019-03-13 10:59 ` Kieran Bingham
2019-03-13 11:15 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Kieran Bingham @ 2019-03-13 10:59 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc, Liviu Dudau, linux-media
Hi Laurent,
On 13/03/2019 00:05, Laurent Pinchart wrote:
> Add support for the writeback feature of the WPF, to enable capturing
> frames at the WPF output for display pipelines. To enable writeback the
> vsp1_rwpf structure mem field must be set to the address of the
> writeback buffer and the writeback field set to true before the WPF
> .configure_stream() and .configure_partition() are called. The WPF will
> enable writeback in the display list for a single frame, and writeback
> will then be automatically disabled.
>
This looks good.
Took some time to go through it while I argue with myself, but I think I
reached an agreement with me in the end :)
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> drivers/media/platform/vsp1/vsp1_rwpf.h | 2 +
> drivers/media/platform/vsp1/vsp1_wpf.c | 73 ++++++++++++++++++++++---
> 2 files changed, 66 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h b/drivers/media/platform/vsp1/vsp1_rwpf.h
> index 70742ecf766f..910990b27617 100644
> --- a/drivers/media/platform/vsp1/vsp1_rwpf.h
> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
> @@ -35,6 +35,7 @@ struct vsp1_rwpf {
> struct v4l2_ctrl_handler ctrls;
>
> struct vsp1_video *video;
> + bool has_writeback;
>
> unsigned int max_width;
> unsigned int max_height;
> @@ -61,6 +62,7 @@ struct vsp1_rwpf {
> } flip;
>
> struct vsp1_rwpf_memory mem;
> + bool writeback;
Does this need to be initialised to false somewhere?
answering my own question;
No - because we allocate the "struct vsp1_rwpf *wpf" with devm_kzalloc.
>
> struct vsp1_dl_manager *dlm;
> };
> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
> index fc5c1b0f6633..390ac478336d 100644
> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> @@ -232,6 +232,27 @@ static void vsp1_wpf_destroy(struct vsp1_entity *entity)
> vsp1_dlm_destroy(wpf->dlm);
> }
>
> +static int wpf_configure_writeback_chain(struct vsp1_rwpf *wpf,
> + struct vsp1_dl_list *dl)
> +{
> + unsigned int index = wpf->entity.index;
> + struct vsp1_dl_list *dl_next;
> + struct vsp1_dl_body *dlb;
> +
> + dl_next = vsp1_dl_list_get(wpf->dlm);> + if (!dl_next) {
> + dev_err(wpf->entity.vsp1->dev,
> + "Failed to obtain a dl list, disabling writeback\n");
> + return -ENOMEM;
> + }
> +
> + dlb = vsp1_dl_list_get_body0(dl_next);
> + vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(index), 0);
> + vsp1_dl_list_add_chain(dl, dl_next);
Two thoughts for future consideration.
1) There was a patch I had floated to reduce the allocations of the pool
sizes. This would need to be checked if it's ever reconsidered, as we
now use an extra DL.
This is currently allocated as 64 lists in vsp1_wpf_create() with:
wpf->dlm = vsp1_dlm_create(vsp1, index, 64);
which is actually 65 lists because there's a + 1 in vsp1_dlm_create(),
so I think we have more than we'll ever need for a display pipeline
currently.
2) I did think we could pre-allocate this write back display list and
re-use it, by always attaching the same "writeback disable display list"
to the chain.
If we do that - we'll have to be careful about the refcounting of the
chained list as it will automatically be put back on to the dlm->free
list currently when the frame completes.
I think it's probably only a small optimisation to re-use the list
anyway, so just getting a new one and chaining it is certainly adequate
for this solution.
> +
> + return 0;
> +}
> +
> static void wpf_configure_stream(struct vsp1_entity *entity,
> struct vsp1_pipeline *pipe,
> struct vsp1_dl_list *dl,
> @@ -241,9 +262,11 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> struct vsp1_device *vsp1 = wpf->entity.vsp1;
> const struct v4l2_mbus_framefmt *source_format;
> const struct v4l2_mbus_framefmt *sink_format;
> + unsigned int index = wpf->entity.index;
> unsigned int i;
> u32 outfmt = 0;
> u32 srcrpf = 0;
> + int ret;
>
> sink_format = vsp1_entity_get_pad_format(&wpf->entity,
> wpf->entity.config,
> @@ -251,8 +274,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> source_format = vsp1_entity_get_pad_format(&wpf->entity,
> wpf->entity.config,
> RWPF_PAD_SOURCE);
> +
> /* Format */
> - if (!pipe->lif) {
> + if (!pipe->lif || wpf->writeback) {
> const struct v4l2_pix_format_mplane *format = &wpf->format;
> const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
>
> @@ -277,8 +301,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>
> vsp1_wpf_write(wpf, dlb, VI6_WPF_DSWAP, fmtinfo->swap);
>
> - if (vsp1_feature(vsp1, VSP1_HAS_WPF_HFLIP) &&
> - wpf->entity.index == 0)
> + if (vsp1_feature(vsp1, VSP1_HAS_WPF_HFLIP) && index == 0)
> vsp1_wpf_write(wpf, dlb, VI6_WPF_ROT_CTRL,
> VI6_WPF_ROT_CTRL_LN16 |
> (256 << VI6_WPF_ROT_CTRL_LMEM_WD_SHIFT));
> @@ -289,11 +312,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>
> wpf->outfmt = outfmt;
>
> - vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(wpf->entity.index),
> + vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(index),
> VI6_DPR_WPF_FPORCH_FP_WPFN);
>
> - vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(wpf->entity.index), 0);
> -
> /*
> * Sources. If the pipeline has a single input and BRx is not used,
> * configure it as the master layer. Otherwise configure all
> @@ -319,9 +340,26 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
>
> /* Enable interrupts. */
> - vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(wpf->entity.index), 0);
> - vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(wpf->entity.index),
> + vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
> + vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index),
> VI6_WFP_IRQ_ENB_DFEE);
> +
> + /*
> + * Configure writeback for display pipelines (the wpf writeback flag is
> + * never set for memory-to-memory pipelines). Start by adding a chained
> + * display list to disable writeback after a single frame, and process
> + * to enable writeback. If the display list allocation fails don't
> + * enable writeback as we wouldn't be able to safely disable it,
> + * resulting in possible memory corruption.
> + */
> + if (wpf->writeback) {
> + ret = wpf_configure_writeback_chain(wpf, dl);
> + if (ret < 0)
> + wpf->writeback = false;
> + }
> +
> + vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(index),
> + wpf->writeback ? VI6_WPF_WRBCK_CTRL_WBMD : 0);
> }
>
> static void wpf_configure_frame(struct vsp1_entity *entity,
> @@ -391,7 +429,11 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
> (height << VI6_WPF_SZCLIP_SIZE_SHIFT));
>
> - if (pipe->lif)
> + /*
> + * For display pipelines without writeback enabled there's no memory
> + * address to configure, return now.
> + */
> + if (pipe->lif && !wpf->writeback)
> return;
>
> /*
> @@ -480,6 +522,12 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_Y, mem.addr[0]);
> vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_C0, mem.addr[1]);
> vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_C1, mem.addr[2]);
> +
> + /*
> + * Writeback operates in single-shot mode and lasts for a single frame,
> + * reset the writeback flag to false for the next frame.
> + */
> + wpf->writeback = false;
> }
>
> static unsigned int wpf_max_width(struct vsp1_entity *entity,
> @@ -530,6 +578,13 @@ struct vsp1_rwpf *vsp1_wpf_create(struct vsp1_device *vsp1, unsigned int index)
> wpf->max_height = WPF_GEN3_MAX_HEIGHT;
> }
>
> + /*
> + * On Gen3 WPFs with a LIF output can also write to memory for display
> + * writeback.
> + */
> + if (vsp1->info->gen > 2 && index < vsp1->info->lif_count)
> + wpf->has_writeback = true;
> +
> wpf->entity.ops = &wpf_entity_ops;
> wpf->entity.type = VSP1_ENTITY_WPF;
> wpf->entity.index = index;
>
--
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 08/18] media: vsp1: wpf: Add writeback support
2019-03-13 10:59 ` Kieran Bingham
@ 2019-03-13 11:15 ` Laurent Pinchart
0 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 11:15 UTC (permalink / raw)
To: Kieran Bingham
Cc: Laurent Pinchart, Liviu Dudau, dri-devel, linux-renesas-soc, linux-media
Hi Kieran,
On Wed, Mar 13, 2019 at 10:59:18AM +0000, Kieran Bingham wrote:
> On 13/03/2019 00:05, Laurent Pinchart wrote:
> > Add support for the writeback feature of the WPF, to enable capturing
> > frames at the WPF output for display pipelines. To enable writeback the
> > vsp1_rwpf structure mem field must be set to the address of the
> > writeback buffer and the writeback field set to true before the WPF
> > .configure_stream() and .configure_partition() are called. The WPF will
> > enable writeback in the display list for a single frame, and writeback
> > will then be automatically disabled.
>
> This looks good.
>
> Took some time to go through it while I argue with myself, but I think I
> reached an agreement with me in the end :)
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > drivers/media/platform/vsp1/vsp1_rwpf.h | 2 +
> > drivers/media/platform/vsp1/vsp1_wpf.c | 73 ++++++++++++++++++++++---
> > 2 files changed, 66 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h b/drivers/media/platform/vsp1/vsp1_rwpf.h
> > index 70742ecf766f..910990b27617 100644
> > --- a/drivers/media/platform/vsp1/vsp1_rwpf.h
> > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
> > @@ -35,6 +35,7 @@ struct vsp1_rwpf {
> > struct v4l2_ctrl_handler ctrls;
> >
> > struct vsp1_video *video;
> > + bool has_writeback;
> >
> > unsigned int max_width;
> > unsigned int max_height;
> > @@ -61,6 +62,7 @@ struct vsp1_rwpf {
> > } flip;
> >
> > struct vsp1_rwpf_memory mem;
> > + bool writeback;
>
> Does this need to be initialised to false somewhere?
>
> answering my own question;
> No - because we allocate the "struct vsp1_rwpf *wpf" with devm_kzalloc.
>
> > struct vsp1_dl_manager *dlm;
> > };
> > diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
> > index fc5c1b0f6633..390ac478336d 100644
> > --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> > +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> > @@ -232,6 +232,27 @@ static void vsp1_wpf_destroy(struct vsp1_entity *entity)
> > vsp1_dlm_destroy(wpf->dlm);
> > }
> >
> > +static int wpf_configure_writeback_chain(struct vsp1_rwpf *wpf,
> > + struct vsp1_dl_list *dl)
> > +{
> > + unsigned int index = wpf->entity.index;
> > + struct vsp1_dl_list *dl_next;
> > + struct vsp1_dl_body *dlb;
> > +
> > + dl_next = vsp1_dl_list_get(wpf->dlm);> + if (!dl_next) {
> > + dev_err(wpf->entity.vsp1->dev,
> > + "Failed to obtain a dl list, disabling writeback\n");
> > + return -ENOMEM;
> > + }
> > +
> > + dlb = vsp1_dl_list_get_body0(dl_next);
> > + vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(index), 0);
> > + vsp1_dl_list_add_chain(dl, dl_next);
>
> Two thoughts for future consideration.
>
> 1) There was a patch I had floated to reduce the allocations of the pool
> sizes. This would need to be checked if it's ever reconsidered, as we
> now use an extra DL.
>
> This is currently allocated as 64 lists in vsp1_wpf_create() with:
>
> wpf->dlm = vsp1_dlm_create(vsp1, index, 64);
>
> which is actually 65 lists because there's a + 1 in vsp1_dlm_create(),
> so I think we have more than we'll ever need for a display pipeline
> currently.
>
>
> 2) I did think we could pre-allocate this write back display list and
> re-use it, by always attaching the same "writeback disable display list"
> to the chain.
>
> If we do that - we'll have to be careful about the refcounting of the
> chained list as it will automatically be put back on to the dlm->free
> list currently when the frame completes.
Yes, and that's why I haven't done so yet. I think it can be implemented
on top of this series as it's an optimization.
> I think it's probably only a small optimisation to re-use the list
> anyway, so just getting a new one and chaining it is certainly adequate
> for this solution.
>
> > +
> > + return 0;
> > +}
> > +
> > static void wpf_configure_stream(struct vsp1_entity *entity,
> > struct vsp1_pipeline *pipe,
> > struct vsp1_dl_list *dl,
> > @@ -241,9 +262,11 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> > struct vsp1_device *vsp1 = wpf->entity.vsp1;
> > const struct v4l2_mbus_framefmt *source_format;
> > const struct v4l2_mbus_framefmt *sink_format;
> > + unsigned int index = wpf->entity.index;
> > unsigned int i;
> > u32 outfmt = 0;
> > u32 srcrpf = 0;
> > + int ret;
> >
> > sink_format = vsp1_entity_get_pad_format(&wpf->entity,
> > wpf->entity.config,
> > @@ -251,8 +274,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> > source_format = vsp1_entity_get_pad_format(&wpf->entity,
> > wpf->entity.config,
> > RWPF_PAD_SOURCE);
> > +
> > /* Format */
> > - if (!pipe->lif) {
> > + if (!pipe->lif || wpf->writeback) {
> > const struct v4l2_pix_format_mplane *format = &wpf->format;
> > const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
> >
> > @@ -277,8 +301,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_DSWAP, fmtinfo->swap);
> >
> > - if (vsp1_feature(vsp1, VSP1_HAS_WPF_HFLIP) &&
> > - wpf->entity.index == 0)
> > + if (vsp1_feature(vsp1, VSP1_HAS_WPF_HFLIP) && index == 0)
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_ROT_CTRL,
> > VI6_WPF_ROT_CTRL_LN16 |
> > (256 << VI6_WPF_ROT_CTRL_LMEM_WD_SHIFT));
> > @@ -289,11 +312,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >
> > wpf->outfmt = outfmt;
> >
> > - vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(wpf->entity.index),
> > + vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(index),
> > VI6_DPR_WPF_FPORCH_FP_WPFN);
> >
> > - vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(wpf->entity.index), 0);
> > -
> > /*
> > * Sources. If the pipeline has a single input and BRx is not used,
> > * configure it as the master layer. Otherwise configure all
> > @@ -319,9 +340,26 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
> >
> > /* Enable interrupts. */
> > - vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(wpf->entity.index), 0);
> > - vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(wpf->entity.index),
> > + vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
> > + vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index),
> > VI6_WFP_IRQ_ENB_DFEE);
> > +
> > + /*
> > + * Configure writeback for display pipelines (the wpf writeback flag is
> > + * never set for memory-to-memory pipelines). Start by adding a chained
> > + * display list to disable writeback after a single frame, and process
> > + * to enable writeback. If the display list allocation fails don't
> > + * enable writeback as we wouldn't be able to safely disable it,
> > + * resulting in possible memory corruption.
> > + */
> > + if (wpf->writeback) {
> > + ret = wpf_configure_writeback_chain(wpf, dl);
> > + if (ret < 0)
> > + wpf->writeback = false;
> > + }
> > +
> > + vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(index),
> > + wpf->writeback ? VI6_WPF_WRBCK_CTRL_WBMD : 0);
> > }
> >
> > static void wpf_configure_frame(struct vsp1_entity *entity,
> > @@ -391,7 +429,11 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> > (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
> > (height << VI6_WPF_SZCLIP_SIZE_SHIFT));
> >
> > - if (pipe->lif)
> > + /*
> > + * For display pipelines without writeback enabled there's no memory
> > + * address to configure, return now.
> > + */
> > + if (pipe->lif && !wpf->writeback)
> > return;
> >
> > /*
> > @@ -480,6 +522,12 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_Y, mem.addr[0]);
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_C0, mem.addr[1]);
> > vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_C1, mem.addr[2]);
> > +
> > + /*
> > + * Writeback operates in single-shot mode and lasts for a single frame,
> > + * reset the writeback flag to false for the next frame.
> > + */
> > + wpf->writeback = false;
> > }
> >
> > static unsigned int wpf_max_width(struct vsp1_entity *entity,
> > @@ -530,6 +578,13 @@ struct vsp1_rwpf *vsp1_wpf_create(struct vsp1_device *vsp1, unsigned int index)
> > wpf->max_height = WPF_GEN3_MAX_HEIGHT;
> > }
> >
> > + /*
> > + * On Gen3 WPFs with a LIF output can also write to memory for display
> > + * writeback.
> > + */
> > + if (vsp1->info->gen > 2 && index < vsp1->info->lif_count)
> > + wpf->has_writeback = true;
> > +
> > wpf->entity.ops = &wpf_entity_ops;
> > wpf->entity.type = VSP1_ENTITY_WPF;
> > wpf->entity.index = index;
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 09/18] media: vsp1: drm: Split RPF format setting to separate function
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (7 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 08/18] media: vsp1: wpf: Add writeback support Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 11:12 ` Kieran Bingham
2019-03-13 0:05 ` [PATCH v6 10/18] media: vsp1: drm: Extend frame completion API to the DU driver Laurent Pinchart
` (8 subsequent siblings)
17 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
The code that initializes the RPF format-related fields for display
pipelines will also be useful for the WPF to implement writeback
support. Split it from vsp1_du_atomic_update() to a new
vsp1_du_pipeline_set_rwpf_format() function.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/media/platform/vsp1/vsp1_drm.c | 55 ++++++++++++++++----------
1 file changed, 35 insertions(+), 20 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 4f1bc51d1ef4..5601a787688b 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -566,6 +566,36 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
vsp1_dl_list_commit(dl, dl_flags);
}
+static int vsp1_du_pipeline_set_rwpf_format(struct vsp1_device *vsp1,
+ struct vsp1_rwpf *rwpf,
+ u32 pixelformat, unsigned int pitch)
+{
+ const struct vsp1_format_info *fmtinfo;
+ unsigned int chroma_hsub;
+
+ fmtinfo = vsp1_get_format_info(vsp1, pixelformat);
+ if (!fmtinfo) {
+ dev_dbg(vsp1->dev, "Unsupported pixel format %08x for RPF\n",
+ pixelformat);
+ return -EINVAL;
+ }
+
+ /*
+ * Only formats with three planes can affect the chroma planes pitch.
+ * All formats with two planes have a horizontal subsampling value of 2,
+ * but combine U and V in a single chroma plane, which thus results in
+ * the luma plane and chroma plane having the same pitch.
+ */
+ chroma_hsub = (fmtinfo->planes == 3) ? fmtinfo->hsub : 1;
+
+ rwpf->fmtinfo = fmtinfo;
+ rwpf->format.num_planes = fmtinfo->planes;
+ rwpf->format.plane_fmt[0].bytesperline = pitch;
+ rwpf->format.plane_fmt[1].bytesperline = pitch / chroma_hsub;
+
+ return 0;
+}
+
/* -----------------------------------------------------------------------------
* DU Driver API
*/
@@ -773,9 +803,8 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
{
struct vsp1_device *vsp1 = dev_get_drvdata(dev);
struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
- const struct vsp1_format_info *fmtinfo;
- unsigned int chroma_hsub;
struct vsp1_rwpf *rpf;
+ int ret;
if (rpf_index >= vsp1->info->rpf_count)
return -EINVAL;
@@ -808,25 +837,11 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
* Store the format, stride, memory buffer address, crop and compose
* rectangles and Z-order position and for the input.
*/
- fmtinfo = vsp1_get_format_info(vsp1, cfg->pixelformat);
- if (!fmtinfo) {
- dev_dbg(vsp1->dev, "Unsupported pixel format %08x for RPF\n",
- cfg->pixelformat);
- return -EINVAL;
- }
+ ret = vsp1_du_pipeline_set_rwpf_format(vsp1, rpf, cfg->pixelformat,
+ cfg->pitch);
+ if (ret < 0)
+ return ret;
- /*
- * Only formats with three planes can affect the chroma planes pitch.
- * All formats with two planes have a horizontal subsampling value of 2,
- * but combine U and V in a single chroma plane, which thus results in
- * the luma plane and chroma plane having the same pitch.
- */
- chroma_hsub = (fmtinfo->planes == 3) ? fmtinfo->hsub : 1;
-
- rpf->fmtinfo = fmtinfo;
- rpf->format.num_planes = fmtinfo->planes;
- rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
- rpf->format.plane_fmt[1].bytesperline = cfg->pitch / chroma_hsub;
rpf->alpha = cfg->alpha;
rpf->mem.addr[0] = cfg->mem[0];
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 09/18] media: vsp1: drm: Split RPF format setting to separate function
2019-03-13 0:05 ` [PATCH v6 09/18] media: vsp1: drm: Split RPF format setting to separate function Laurent Pinchart
@ 2019-03-13 11:12 ` Kieran Bingham
2019-03-13 11:17 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Kieran Bingham @ 2019-03-13 11:12 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc, Liviu Dudau, linux-media
Hi Laurent,
On 13/03/2019 00:05, Laurent Pinchart wrote:
> The code that initializes the RPF format-related fields for display
> pipelines will also be useful for the WPF to implement writeback
> support. Split it from vsp1_du_atomic_update() to a new
> vsp1_du_pipeline_set_rwpf_format() function.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> drivers/media/platform/vsp1/vsp1_drm.c | 55 ++++++++++++++++----------
> 1 file changed, 35 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 4f1bc51d1ef4..5601a787688b 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -566,6 +566,36 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
> vsp1_dl_list_commit(dl, dl_flags);
> }
>
> +static int vsp1_du_pipeline_set_rwpf_format(struct vsp1_device *vsp1,
> + struct vsp1_rwpf *rwpf,
> + u32 pixelformat, unsigned int pitch)
> +{
> + const struct vsp1_format_info *fmtinfo;
> + unsigned int chroma_hsub;
> +
> + fmtinfo = vsp1_get_format_info(vsp1, pixelformat);
> + if (!fmtinfo) {
> + dev_dbg(vsp1->dev, "Unsupported pixel format %08x for RPF\n",
Isn't this now a RWPF ?
Other than that
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> + pixelformat);
> + return -EINVAL;
> + }
> +
> + /*
> + * Only formats with three planes can affect the chroma planes pitch.
> + * All formats with two planes have a horizontal subsampling value of 2,
> + * but combine U and V in a single chroma plane, which thus results in
> + * the luma plane and chroma plane having the same pitch.
> + */
> + chroma_hsub = (fmtinfo->planes == 3) ? fmtinfo->hsub : 1;
> +
> + rwpf->fmtinfo = fmtinfo;
> + rwpf->format.num_planes = fmtinfo->planes;
> + rwpf->format.plane_fmt[0].bytesperline = pitch;
> + rwpf->format.plane_fmt[1].bytesperline = pitch / chroma_hsub;
> +
> + return 0;
> +}
> +
> /* -----------------------------------------------------------------------------
> * DU Driver API
> */
> @@ -773,9 +803,8 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
> {
> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> - const struct vsp1_format_info *fmtinfo;
> - unsigned int chroma_hsub;
> struct vsp1_rwpf *rpf;
> + int ret;
>
> if (rpf_index >= vsp1->info->rpf_count)
> return -EINVAL;
> @@ -808,25 +837,11 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
> * Store the format, stride, memory buffer address, crop and compose
> * rectangles and Z-order position and for the input.
> */
> - fmtinfo = vsp1_get_format_info(vsp1, cfg->pixelformat);
> - if (!fmtinfo) {
> - dev_dbg(vsp1->dev, "Unsupported pixel format %08x for RPF\n",
> - cfg->pixelformat);
> - return -EINVAL;
> - }
> + ret = vsp1_du_pipeline_set_rwpf_format(vsp1, rpf, cfg->pixelformat,
> + cfg->pitch);
> + if (ret < 0)
> + return ret;
>
> - /*
> - * Only formats with three planes can affect the chroma planes pitch.
> - * All formats with two planes have a horizontal subsampling value of 2,
> - * but combine U and V in a single chroma plane, which thus results in
> - * the luma plane and chroma plane having the same pitch.
> - */
> - chroma_hsub = (fmtinfo->planes == 3) ? fmtinfo->hsub : 1;
> -
> - rpf->fmtinfo = fmtinfo;
> - rpf->format.num_planes = fmtinfo->planes;
> - rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
> - rpf->format.plane_fmt[1].bytesperline = cfg->pitch / chroma_hsub;
> rpf->alpha = cfg->alpha;
>
> rpf->mem.addr[0] = cfg->mem[0];
>
--
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 09/18] media: vsp1: drm: Split RPF format setting to separate function
2019-03-13 11:12 ` Kieran Bingham
@ 2019-03-13 11:17 ` Laurent Pinchart
0 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 11:17 UTC (permalink / raw)
To: Kieran Bingham
Cc: Laurent Pinchart, Liviu Dudau, dri-devel, linux-renesas-soc, linux-media
Hi Kieran,
On Wed, Mar 13, 2019 at 11:12:57AM +0000, Kieran Bingham wrote:
> On 13/03/2019 00:05, Laurent Pinchart wrote:
> > The code that initializes the RPF format-related fields for display
> > pipelines will also be useful for the WPF to implement writeback
> > support. Split it from vsp1_du_atomic_update() to a new
> > vsp1_du_pipeline_set_rwpf_format() function.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > drivers/media/platform/vsp1/vsp1_drm.c | 55 ++++++++++++++++----------
> > 1 file changed, 35 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> > index 4f1bc51d1ef4..5601a787688b 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -566,6 +566,36 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
> > vsp1_dl_list_commit(dl, dl_flags);
> > }
> >
> > +static int vsp1_du_pipeline_set_rwpf_format(struct vsp1_device *vsp1,
> > + struct vsp1_rwpf *rwpf,
> > + u32 pixelformat, unsigned int pitch)
> > +{
> > + const struct vsp1_format_info *fmtinfo;
> > + unsigned int chroma_hsub;
> > +
> > + fmtinfo = vsp1_get_format_info(vsp1, pixelformat);
> > + if (!fmtinfo) {
> > + dev_dbg(vsp1->dev, "Unsupported pixel format %08x for RPF\n",
>
> Isn't this now a RWPF ?
It is. I'll drop the "for RPF" part.
> Other than that
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> > + pixelformat);
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * Only formats with three planes can affect the chroma planes pitch.
> > + * All formats with two planes have a horizontal subsampling value of 2,
> > + * but combine U and V in a single chroma plane, which thus results in
> > + * the luma plane and chroma plane having the same pitch.
> > + */
> > + chroma_hsub = (fmtinfo->planes == 3) ? fmtinfo->hsub : 1;
> > +
> > + rwpf->fmtinfo = fmtinfo;
> > + rwpf->format.num_planes = fmtinfo->planes;
> > + rwpf->format.plane_fmt[0].bytesperline = pitch;
> > + rwpf->format.plane_fmt[1].bytesperline = pitch / chroma_hsub;
> > +
> > + return 0;
> > +}
> > +
> > /* -----------------------------------------------------------------------------
> > * DU Driver API
> > */
> > @@ -773,9 +803,8 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
> > {
> > struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> > struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> > - const struct vsp1_format_info *fmtinfo;
> > - unsigned int chroma_hsub;
> > struct vsp1_rwpf *rpf;
> > + int ret;
> >
> > if (rpf_index >= vsp1->info->rpf_count)
> > return -EINVAL;
> > @@ -808,25 +837,11 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
> > * Store the format, stride, memory buffer address, crop and compose
> > * rectangles and Z-order position and for the input.
> > */
> > - fmtinfo = vsp1_get_format_info(vsp1, cfg->pixelformat);
> > - if (!fmtinfo) {
> > - dev_dbg(vsp1->dev, "Unsupported pixel format %08x for RPF\n",
> > - cfg->pixelformat);
> > - return -EINVAL;
> > - }
> > + ret = vsp1_du_pipeline_set_rwpf_format(vsp1, rpf, cfg->pixelformat,
> > + cfg->pitch);
> > + if (ret < 0)
> > + return ret;
> >
> > - /*
> > - * Only formats with three planes can affect the chroma planes pitch.
> > - * All formats with two planes have a horizontal subsampling value of 2,
> > - * but combine U and V in a single chroma plane, which thus results in
> > - * the luma plane and chroma plane having the same pitch.
> > - */
> > - chroma_hsub = (fmtinfo->planes == 3) ? fmtinfo->hsub : 1;
> > -
> > - rpf->fmtinfo = fmtinfo;
> > - rpf->format.num_planes = fmtinfo->planes;
> > - rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
> > - rpf->format.plane_fmt[1].bytesperline = cfg->pitch / chroma_hsub;
> > rpf->alpha = cfg->alpha;
> >
> > rpf->mem.addr[0] = cfg->mem[0];
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 10/18] media: vsp1: drm: Extend frame completion API to the DU driver
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (8 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 09/18] media: vsp1: drm: Split RPF format setting to separate function Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 11:26 ` Kieran Bingham
2019-03-13 0:05 ` [PATCH v6 11/18] media: vsp1: drm: Implement writeback support Laurent Pinchart
` (7 subsequent siblings)
17 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
The VSP1 driver will need to pass extra flags to the DU through the
frame completion API. Replace the completed bool flag by a bitmask to
support this.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 ++--
drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
drivers/media/platform/vsp1/vsp1_drm.h | 2 +-
include/media/vsp1.h | 4 +++-
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 76a39eee7c9c..28bfeb8c24fb 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -26,14 +26,14 @@
#include "rcar_du_kms.h"
#include "rcar_du_vsp.h"
-static void rcar_du_vsp_complete(void *private, bool completed, u32 crc)
+static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
{
struct rcar_du_crtc *crtc = private;
if (crtc->vblank_enable)
drm_crtc_handle_vblank(&crtc->crtc);
- if (completed)
+ if (status & VSP1_DU_STATUS_COMPLETE)
rcar_du_crtc_finish_page_flip(crtc);
drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 5601a787688b..0367f88135bf 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -34,14 +34,14 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
unsigned int completion)
{
struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
- bool complete = completion & VSP1_DL_FRAME_END_COMPLETED;
if (drm_pipe->du_complete) {
struct vsp1_entity *uif = drm_pipe->uif;
+ unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
u32 crc;
crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
- drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
+ drm_pipe->du_complete(drm_pipe->du_private, status, crc);
}
if (completion & VSP1_DL_FRAME_END_INTERNAL) {
diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h
index 8dfd274a59e2..e85ad4366fbb 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.h
+++ b/drivers/media/platform/vsp1/vsp1_drm.h
@@ -42,7 +42,7 @@ struct vsp1_drm_pipeline {
struct vsp1_du_crc_config crc;
/* Frame synchronisation */
- void (*du_complete)(void *data, bool completed, u32 crc);
+ void (*du_complete)(void *data, unsigned int status, u32 crc);
void *du_private;
};
diff --git a/include/media/vsp1.h b/include/media/vsp1.h
index 1cf868360701..877496936487 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -17,6 +17,8 @@ struct device;
int vsp1_du_init(struct device *dev);
+#define VSP1_DU_STATUS_COMPLETE BIT(0)
+
/**
* struct vsp1_du_lif_config - VSP LIF configuration
* @width: output frame width
@@ -32,7 +34,7 @@ struct vsp1_du_lif_config {
unsigned int height;
bool interlaced;
- void (*callback)(void *data, bool completed, u32 crc);
+ void (*callback)(void *data, unsigned int status, u32 crc);
void *callback_data;
};
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 10/18] media: vsp1: drm: Extend frame completion API to the DU driver
2019-03-13 0:05 ` [PATCH v6 10/18] media: vsp1: drm: Extend frame completion API to the DU driver Laurent Pinchart
@ 2019-03-13 11:26 ` Kieran Bingham
2019-03-13 15:50 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Kieran Bingham @ 2019-03-13 11:26 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc, Liviu Dudau, linux-media
Hi Laurent,
On 13/03/2019 00:05, Laurent Pinchart wrote:
> The VSP1 driver will need to pass extra flags to the DU through the
> frame completion API. Replace the completed bool flag by a bitmask to
> support this.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 ++--
> drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
> drivers/media/platform/vsp1/vsp1_drm.h | 2 +-
> include/media/vsp1.h | 4 +++-
> 4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 76a39eee7c9c..28bfeb8c24fb 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -26,14 +26,14 @@
> #include "rcar_du_kms.h"
> #include "rcar_du_vsp.h"
>
> -static void rcar_du_vsp_complete(void *private, bool completed, u32 crc)
> +static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
> {
> struct rcar_du_crtc *crtc = private;
>
> if (crtc->vblank_enable)
> drm_crtc_handle_vblank(&crtc->crtc);
>
> - if (completed)
> + if (status & VSP1_DU_STATUS_COMPLETE)
> rcar_du_crtc_finish_page_flip(crtc);
>
> drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 5601a787688b..0367f88135bf 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -34,14 +34,14 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
> unsigned int completion)
> {
> struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
> - bool complete = completion & VSP1_DL_FRAME_END_COMPLETED;
>
> if (drm_pipe->du_complete) {
> struct vsp1_entity *uif = drm_pipe->uif;
> + unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
Why do you mask the bit(s) here?
Can't we just pass completion into
drm_pipe->du_complete(p, completion, c); ?
[(edit: no, because completion contains things such as
VSP1_DL_FRAME_END_INTERNAL)]
Or are you relying on the fact that,
VSP1_DL_FRAME_END_COMPLETED == VSP1_DU_STATUS_COMPLETE
in which case perhaps we should be more explicit somehow, either in code
or with a comment that this section is converting between the two
bitfield types.
However you resolve, the rest looks fine:
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> u32 crc;
>
> crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> - drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
> + drm_pipe->du_complete(drm_pipe->du_private, status, crc);
> }
>
> if (completion & VSP1_DL_FRAME_END_INTERNAL) {
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h
> index 8dfd274a59e2..e85ad4366fbb 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -42,7 +42,7 @@ struct vsp1_drm_pipeline {
> struct vsp1_du_crc_config crc;
>
> /* Frame synchronisation */
> - void (*du_complete)(void *data, bool completed, u32 crc);
> + void (*du_complete)(void *data, unsigned int status, u32 crc);
> void *du_private;
> };
>
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index 1cf868360701..877496936487 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -17,6 +17,8 @@ struct device;
>
> int vsp1_du_init(struct device *dev);
>
> +#define VSP1_DU_STATUS_COMPLETE BIT(0)
> +
> /**
> * struct vsp1_du_lif_config - VSP LIF configuration
> * @width: output frame width
> @@ -32,7 +34,7 @@ struct vsp1_du_lif_config {
> unsigned int height;
> bool interlaced;
>
> - void (*callback)(void *data, bool completed, u32 crc);
> + void (*callback)(void *data, unsigned int status, u32 crc);
> void *callback_data;
> };
>
>
--
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 10/18] media: vsp1: drm: Extend frame completion API to the DU driver
2019-03-13 11:26 ` Kieran Bingham
@ 2019-03-13 15:50 ` Laurent Pinchart
0 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 15:50 UTC (permalink / raw)
To: Kieran Bingham
Cc: Laurent Pinchart, Liviu Dudau, dri-devel, linux-renesas-soc, linux-media
Hi Kieran,
On Wed, Mar 13, 2019 at 11:26:14AM +0000, Kieran Bingham wrote:
> On 13/03/2019 00:05, Laurent Pinchart wrote:
> > The VSP1 driver will need to pass extra flags to the DU through the
> > frame completion API. Replace the completed bool flag by a bitmask to
> > support this.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 4 ++--
> > drivers/media/platform/vsp1/vsp1_drm.c | 4 ++--
> > drivers/media/platform/vsp1/vsp1_drm.h | 2 +-
> > include/media/vsp1.h | 4 +++-
> > 4 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > index 76a39eee7c9c..28bfeb8c24fb 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > @@ -26,14 +26,14 @@
> > #include "rcar_du_kms.h"
> > #include "rcar_du_vsp.h"
> >
> > -static void rcar_du_vsp_complete(void *private, bool completed, u32 crc)
> > +static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
> > {
> > struct rcar_du_crtc *crtc = private;
> >
> > if (crtc->vblank_enable)
> > drm_crtc_handle_vblank(&crtc->crtc);
> >
> > - if (completed)
> > + if (status & VSP1_DU_STATUS_COMPLETE)
> > rcar_du_crtc_finish_page_flip(crtc);
> >
> > drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> > index 5601a787688b..0367f88135bf 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -34,14 +34,14 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
> > unsigned int completion)
> > {
> > struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
> > - bool complete = completion & VSP1_DL_FRAME_END_COMPLETED;
> >
> > if (drm_pipe->du_complete) {
> > struct vsp1_entity *uif = drm_pipe->uif;
> > + unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
>
>
> Why do you mask the bit(s) here?
>
> Can't we just pass completion into
> drm_pipe->du_complete(p, completion, c); ?
>
> [(edit: no, because completion contains things such as
> VSP1_DL_FRAME_END_INTERNAL)]
>
> Or are you relying on the fact that,
> VSP1_DL_FRAME_END_COMPLETED == VSP1_DU_STATUS_COMPLETE
>
> in which case perhaps we should be more explicit somehow, either in code
> or with a comment that this section is converting between the two
> bitfield types.
Good point. I'll add the following comment above the definition of
VSP1_DL_FRAME_END_COMPLETED:
/* Keep these flags in sync with VSP1_DU_STATUS_* in include/media/vsp1.h. */
> However you resolve, the rest looks fine:
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> > u32 crc;
> >
> > crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> > - drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
> > + drm_pipe->du_complete(drm_pipe->du_private, status, crc);
> > }
> >
> > if (completion & VSP1_DL_FRAME_END_INTERNAL) {
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h
> > index 8dfd274a59e2..e85ad4366fbb 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.h
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> > @@ -42,7 +42,7 @@ struct vsp1_drm_pipeline {
> > struct vsp1_du_crc_config crc;
> >
> > /* Frame synchronisation */
> > - void (*du_complete)(void *data, bool completed, u32 crc);
> > + void (*du_complete)(void *data, unsigned int status, u32 crc);
> > void *du_private;
> > };
> >
> > diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> > index 1cf868360701..877496936487 100644
> > --- a/include/media/vsp1.h
> > +++ b/include/media/vsp1.h
> > @@ -17,6 +17,8 @@ struct device;
> >
> > int vsp1_du_init(struct device *dev);
> >
> > +#define VSP1_DU_STATUS_COMPLETE BIT(0)
> > +
> > /**
> > * struct vsp1_du_lif_config - VSP LIF configuration
> > * @width: output frame width
> > @@ -32,7 +34,7 @@ struct vsp1_du_lif_config {
> > unsigned int height;
> > bool interlaced;
> >
> > - void (*callback)(void *data, bool completed, u32 crc);
> > + void (*callback)(void *data, unsigned int status, u32 crc);
> > void *callback_data;
> > };
> >
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 11/18] media: vsp1: drm: Implement writeback support
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (9 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 10/18] media: vsp1: drm: Extend frame completion API to the DU driver Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 11:42 ` Kieran Bingham
2019-03-13 0:05 ` [PATCH v6 12/18] drm: writeback: Cleanup job ownership handling when queuing job Laurent Pinchart
` (6 subsequent siblings)
17 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
Extend the vsp1_du_atomic_flush() API with writeback support by adding
format, pitch and memory addresses of the writeback framebuffer.
Writeback completion is reported through the existing frame completion
callback with a new VSP1_DU_STATUS_WRITEBACK status flag.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/media/platform/vsp1/vsp1_dl.c | 14 ++++++++++++++
drivers/media/platform/vsp1/vsp1_dl.h | 3 ++-
drivers/media/platform/vsp1/vsp1_drm.c | 25 ++++++++++++++++++++++++-
include/media/vsp1.h | 15 +++++++++++++++
4 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
index ed7cda4130f2..104b6f514536 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -958,6 +958,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
*
* The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
* became active had been queued with the internal notification flag.
+ *
+ * The VSP1_DL_FRAME_END_WRITEBACK flag indicates that the previously active
+ * display list had been queued with the writeback flag.
*/
unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
{
@@ -995,6 +998,17 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
if (status & VI6_STATUS_FLD_STD(dlm->index))
goto done;
+ /*
+ * If the active display list has the writeback flag set, the frame
+ * completion marks the end of the writeback capture. Return the
+ * VSP1_DL_FRAME_END_WRITEBACK flag and reset the display list's
+ * writeback flag.
+ */
+ if (dlm->active && (dlm->active->flags & VSP1_DL_FRAME_END_WRITEBACK)) {
+ flags |= VSP1_DL_FRAME_END_WRITEBACK;
+ dlm->active->flags &= ~VSP1_DL_FRAME_END_WRITEBACK;
+ }
+
/*
* The device starts processing the queued display list right after the
* frame end interrupt. The display list thus becomes active.
diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
index e0fdb145e6ed..4d7bcfdc9bd9 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.h
+++ b/drivers/media/platform/vsp1/vsp1_dl.h
@@ -18,7 +18,8 @@ struct vsp1_dl_list;
struct vsp1_dl_manager;
#define VSP1_DL_FRAME_END_COMPLETED BIT(0)
-#define VSP1_DL_FRAME_END_INTERNAL BIT(1)
+#define VSP1_DL_FRAME_END_WRITEBACK BIT(1)
+#define VSP1_DL_FRAME_END_INTERNAL BIT(2)
/**
* struct vsp1_dl_ext_cmd - Extended Display command
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 0367f88135bf..16826bf184c7 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -37,7 +37,9 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
if (drm_pipe->du_complete) {
struct vsp1_entity *uif = drm_pipe->uif;
- unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
+ unsigned int status = completion
+ & (VSP1_DU_STATUS_COMPLETE |
+ VSP1_DU_STATUS_WRITEBACK);
u32 crc;
crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
@@ -541,6 +543,8 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
if (drm_pipe->force_brx_release)
dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
+ if (pipe->output->writeback)
+ dl_flags |= VSP1_DL_FRAME_END_WRITEBACK;
dl = vsp1_dl_list_get(pipe->output->dlm);
dlb = vsp1_dl_list_get_body0(dl);
@@ -870,12 +874,31 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
struct vsp1_device *vsp1 = dev_get_drvdata(dev);
struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
struct vsp1_pipeline *pipe = &drm_pipe->pipe;
+ int ret;
drm_pipe->crc = cfg->crc;
mutex_lock(&vsp1->drm->lock);
+
+ if (pipe->output->has_writeback && cfg->writeback.pixelformat) {
+ const struct vsp1_du_writeback_config *wb_cfg = &cfg->writeback;
+
+ ret = vsp1_du_pipeline_set_rwpf_format(vsp1, pipe->output,
+ wb_cfg->pixelformat,
+ wb_cfg->pitch);
+ if (WARN_ON(ret < 0))
+ goto done;
+
+ pipe->output->mem.addr[0] = wb_cfg->mem[0];
+ pipe->output->mem.addr[1] = wb_cfg->mem[1];
+ pipe->output->mem.addr[2] = wb_cfg->mem[2];
+ pipe->output->writeback = true;
+ }
+
vsp1_du_pipeline_setup_inputs(vsp1, pipe);
vsp1_du_pipeline_configure(pipe);
+
+done:
mutex_unlock(&vsp1->drm->lock);
}
EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
diff --git a/include/media/vsp1.h b/include/media/vsp1.h
index 877496936487..cc1b0d42ce95 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -18,6 +18,7 @@ struct device;
int vsp1_du_init(struct device *dev);
#define VSP1_DU_STATUS_COMPLETE BIT(0)
+#define VSP1_DU_STATUS_WRITEBACK BIT(1)
/**
* struct vsp1_du_lif_config - VSP LIF configuration
@@ -83,12 +84,26 @@ struct vsp1_du_crc_config {
unsigned int index;
};
+/**
+ * struct vsp1_du_writeback_config - VSP writeback configuration parameters
+ * @pixelformat: plane pixel format (V4L2 4CC)
+ * @pitch: line pitch in bytes for the first plane
+ * @mem: DMA memory address for each plane of the frame buffer
+ */
+struct vsp1_du_writeback_config {
+ u32 pixelformat;
+ unsigned int pitch;
+ dma_addr_t mem[3];
+};
+
/**
* struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
* @crc: CRC computation configuration
+ * @writeback: writeback configuration
*/
struct vsp1_du_atomic_pipe_config {
struct vsp1_du_crc_config crc;
+ struct vsp1_du_writeback_config writeback;
};
void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 11/18] media: vsp1: drm: Implement writeback support
2019-03-13 0:05 ` [PATCH v6 11/18] media: vsp1: drm: Implement writeback support Laurent Pinchart
@ 2019-03-13 11:42 ` Kieran Bingham
2019-03-13 15:56 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Kieran Bingham @ 2019-03-13 11:42 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc, Liviu Dudau, linux-media
Hi Laurent,
On 13/03/2019 00:05, Laurent Pinchart wrote:
> Extend the vsp1_du_atomic_flush() API with writeback support by adding
> format, pitch and memory addresses of the writeback framebuffer.
> Writeback completion is reported through the existing frame completion
> callback with a new VSP1_DU_STATUS_WRITEBACK status flag.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> drivers/media/platform/vsp1/vsp1_dl.c | 14 ++++++++++++++
> drivers/media/platform/vsp1/vsp1_dl.h | 3 ++-
> drivers/media/platform/vsp1/vsp1_drm.c | 25 ++++++++++++++++++++++++-
> include/media/vsp1.h | 15 +++++++++++++++
> 4 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> index ed7cda4130f2..104b6f514536 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -958,6 +958,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
> *
> * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
> * became active had been queued with the internal notification flag.
> + *
> + * The VSP1_DL_FRAME_END_WRITEBACK flag indicates that the previously active
> + * display list had been queued with the writeback flag.
How does this interact with the possibility of the writeback being
disabled by the WPF in the event of it failing to get a DL.
It's only a small corner case, but will the 'writeback' report back as
though it succeeded? (without writing to memory, and thus giving an
unmodified buffer back?)
> */
> unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> {
> @@ -995,6 +998,17 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> if (status & VI6_STATUS_FLD_STD(dlm->index))
> goto done;
>
> + /*
> + * If the active display list has the writeback flag set, the frame
> + * completion marks the end of the writeback capture. Return the
> + * VSP1_DL_FRAME_END_WRITEBACK flag and reset the display list's
> + * writeback flag.
> + */
> + if (dlm->active && (dlm->active->flags & VSP1_DL_FRAME_END_WRITEBACK)) {
> + flags |= VSP1_DL_FRAME_END_WRITEBACK;
> + dlm->active->flags &= ~VSP1_DL_FRAME_END_WRITEBACK;
> + }
> +
> /*
> * The device starts processing the queued display list right after the
> * frame end interrupt. The display list thus becomes active.
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
> index e0fdb145e6ed..4d7bcfdc9bd9 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -18,7 +18,8 @@ struct vsp1_dl_list;
> struct vsp1_dl_manager;
>
> #define VSP1_DL_FRAME_END_COMPLETED BIT(0)
> -#define VSP1_DL_FRAME_END_INTERNAL BIT(1)
> +#define VSP1_DL_FRAME_END_WRITEBACK BIT(1)
So below BIT(2) (code above) the flags match the externally exposed
bitfield for the VSP1_DU_STATUS_
While above (code below), are 'private' bitfields.
Should this requirement be documented here somehow? especially the
mapping of FRAME_END_{COMPLETED,WRITEBACK} to
DU_STATUS_{COMPLETED,WRITEBACK}.
> +#define VSP1_DL_FRAME_END_INTERNAL BIT(2)
>
> /**
> * struct vsp1_dl_ext_cmd - Extended Display command
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 0367f88135bf..16826bf184c7 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -37,7 +37,9 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
>
> if (drm_pipe->du_complete) {
> struct vsp1_entity *uif = drm_pipe->uif;
> - unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
> + unsigned int status = completion
> + & (VSP1_DU_STATUS_COMPLETE |
> + VSP1_DU_STATUS_WRITEBACK);
> u32 crc;
>
> crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> @@ -541,6 +543,8 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
>
> if (drm_pipe->force_brx_release)
> dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
> + if (pipe->output->writeback)
> + dl_flags |= VSP1_DL_FRAME_END_WRITEBACK;
>
> dl = vsp1_dl_list_get(pipe->output->dlm);
> dlb = vsp1_dl_list_get_body0(dl);
> @@ -870,12 +874,31 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> struct vsp1_pipeline *pipe = &drm_pipe->pipe;
> + int ret;
>
> drm_pipe->crc = cfg->crc;
>
> mutex_lock(&vsp1->drm->lock);
> +
> + if (pipe->output->has_writeback && cfg->writeback.pixelformat) {
Is pipe->output->has_writeback necessary here? Can
cfg->writeback.pixelformat be set if pipe->output->has_writeback is false?
Hrm ... actually - Perhaps it is useful. It validates both sides of the
system.
pipe->output->has_writeback is a capability of the VSP, where as
cfg->writeback.pixelformat is a 'request' from the DU.
> + const struct vsp1_du_writeback_config *wb_cfg = &cfg->writeback;
> +
> + ret = vsp1_du_pipeline_set_rwpf_format(vsp1, pipe->output,
> + wb_cfg->pixelformat,
> + wb_cfg->pitch);
> + if (WARN_ON(ret < 0))
> + goto done;
> +
> + pipe->output->mem.addr[0] = wb_cfg->mem[0];
> + pipe->output->mem.addr[1] = wb_cfg->mem[1];
> + pipe->output->mem.addr[2] = wb_cfg->mem[2];
> + pipe->output->writeback = true;
> + }
> +
> vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> vsp1_du_pipeline_configure(pipe);
> +
> +done:
> mutex_unlock(&vsp1->drm->lock);
> }
> EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index 877496936487..cc1b0d42ce95 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -18,6 +18,7 @@ struct device;
> int vsp1_du_init(struct device *dev);
>
> #define VSP1_DU_STATUS_COMPLETE BIT(0)
> +#define VSP1_DU_STATUS_WRITEBACK BIT(1)
>
> /**
> * struct vsp1_du_lif_config - VSP LIF configuration
> @@ -83,12 +84,26 @@ struct vsp1_du_crc_config {
> unsigned int index;
> };
>
> +/**
> + * struct vsp1_du_writeback_config - VSP writeback configuration parameters
> + * @pixelformat: plane pixel format (V4L2 4CC)
> + * @pitch: line pitch in bytes for the first plane
> + * @mem: DMA memory address for each plane of the frame buffer
> + */
> +struct vsp1_du_writeback_config {
> + u32 pixelformat;
> + unsigned int pitch;
> + dma_addr_t mem[3];
> +};
> +
> /**
> * struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
> * @crc: CRC computation configuration
> + * @writeback: writeback configuration
> */
> struct vsp1_du_atomic_pipe_config {
> struct vsp1_du_crc_config crc;
> + struct vsp1_du_writeback_config writeback;
> };
>
> void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
>
--
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 11/18] media: vsp1: drm: Implement writeback support
2019-03-13 11:42 ` Kieran Bingham
@ 2019-03-13 15:56 ` Laurent Pinchart
2019-03-14 8:28 ` Kieran Bingham
0 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 15:56 UTC (permalink / raw)
To: Kieran Bingham
Cc: Laurent Pinchart, Liviu Dudau, dri-devel, linux-renesas-soc, linux-media
Hi Kieran,
On Wed, Mar 13, 2019 at 11:42:34AM +0000, Kieran Bingham wrote:
> On 13/03/2019 00:05, Laurent Pinchart wrote:
> > Extend the vsp1_du_atomic_flush() API with writeback support by adding
> > format, pitch and memory addresses of the writeback framebuffer.
> > Writeback completion is reported through the existing frame completion
> > callback with a new VSP1_DU_STATUS_WRITEBACK status flag.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > drivers/media/platform/vsp1/vsp1_dl.c | 14 ++++++++++++++
> > drivers/media/platform/vsp1/vsp1_dl.h | 3 ++-
> > drivers/media/platform/vsp1/vsp1_drm.c | 25 ++++++++++++++++++++++++-
> > include/media/vsp1.h | 15 +++++++++++++++
> > 4 files changed, 55 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> > index ed7cda4130f2..104b6f514536 100644
> > --- a/drivers/media/platform/vsp1/vsp1_dl.c
> > +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> > @@ -958,6 +958,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
> > *
> > * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
> > * became active had been queued with the internal notification flag.
> > + *
> > + * The VSP1_DL_FRAME_END_WRITEBACK flag indicates that the previously active
> > + * display list had been queued with the writeback flag.
>
> How does this interact with the possibility of the writeback being
> disabled by the WPF in the event of it failing to get a DL.
>
> It's only a small corner case, but will the 'writeback' report back as
> though it succeeded? (without writing to memory, and thus giving an
> unmodified buffer back?)
Wrteback completion will never be reported in that case. This shouldn't
happen as we should never fail to get a display list. Do you think it
would be better to fake completion ?
> > */
> > unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> > {
> > @@ -995,6 +998,17 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> > if (status & VI6_STATUS_FLD_STD(dlm->index))
> > goto done;
> >
> > + /*
> > + * If the active display list has the writeback flag set, the frame
> > + * completion marks the end of the writeback capture. Return the
> > + * VSP1_DL_FRAME_END_WRITEBACK flag and reset the display list's
> > + * writeback flag.
> > + */
> > + if (dlm->active && (dlm->active->flags & VSP1_DL_FRAME_END_WRITEBACK)) {
> > + flags |= VSP1_DL_FRAME_END_WRITEBACK;
> > + dlm->active->flags &= ~VSP1_DL_FRAME_END_WRITEBACK;
> > + }
> > +
> > /*
> > * The device starts processing the queued display list right after the
> > * frame end interrupt. The display list thus becomes active.
> > diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
> > index e0fdb145e6ed..4d7bcfdc9bd9 100644
> > --- a/drivers/media/platform/vsp1/vsp1_dl.h
> > +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> > @@ -18,7 +18,8 @@ struct vsp1_dl_list;
> > struct vsp1_dl_manager;
> >
> > #define VSP1_DL_FRAME_END_COMPLETED BIT(0)
> > -#define VSP1_DL_FRAME_END_INTERNAL BIT(1)
> > +#define VSP1_DL_FRAME_END_WRITEBACK BIT(1)
>
> So below BIT(2) (code above) the flags match the externally exposed
> bitfield for the VSP1_DU_STATUS_
>
> While above (code below), are 'private' bitfields.
>
> Should this requirement be documented here somehow? especially the
> mapping of FRAME_END_{COMPLETED,WRITEBACK} to
> DU_STATUS_{COMPLETED,WRITEBACK}.
I've added a comment here, as explained in my reply to your review of
10/18, to document this.
> > +#define VSP1_DL_FRAME_END_INTERNAL BIT(2)
> >
> > /**
> > * struct vsp1_dl_ext_cmd - Extended Display command
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> > index 0367f88135bf..16826bf184c7 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -37,7 +37,9 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
> >
> > if (drm_pipe->du_complete) {
> > struct vsp1_entity *uif = drm_pipe->uif;
> > - unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
> > + unsigned int status = completion
> > + & (VSP1_DU_STATUS_COMPLETE |
> > + VSP1_DU_STATUS_WRITEBACK);
> > u32 crc;
> >
> > crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> > @@ -541,6 +543,8 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
> >
> > if (drm_pipe->force_brx_release)
> > dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
> > + if (pipe->output->writeback)
> > + dl_flags |= VSP1_DL_FRAME_END_WRITEBACK;
> >
> > dl = vsp1_dl_list_get(pipe->output->dlm);
> > dlb = vsp1_dl_list_get_body0(dl);
> > @@ -870,12 +874,31 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> > struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> > struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> > struct vsp1_pipeline *pipe = &drm_pipe->pipe;
> > + int ret;
> >
> > drm_pipe->crc = cfg->crc;
> >
> > mutex_lock(&vsp1->drm->lock);
> > +
> > + if (pipe->output->has_writeback && cfg->writeback.pixelformat) {
>
> Is pipe->output->has_writeback necessary here? Can
> cfg->writeback.pixelformat be set if pipe->output->has_writeback is false?
>
> Hrm ... actually - Perhaps it is useful. It validates both sides of the
> system.
>
> pipe->output->has_writeback is a capability of the VSP, where as
> cfg->writeback.pixelformat is a 'request' from the DU.
Correct, I think it's best to check both, to ensure we don't try to
queue a writeback request on a system that doesn't support writeback. On
the other hand this shouldn't happen as the DU driver shouldn't expose
writeback to userspace in that case, so if you don't think the check is
worth it I can remove the has_writeback field completely.
> > + const struct vsp1_du_writeback_config *wb_cfg = &cfg->writeback;
> > +
> > + ret = vsp1_du_pipeline_set_rwpf_format(vsp1, pipe->output,
> > + wb_cfg->pixelformat,
> > + wb_cfg->pitch);
> > + if (WARN_ON(ret < 0))
> > + goto done;
> > +
> > + pipe->output->mem.addr[0] = wb_cfg->mem[0];
> > + pipe->output->mem.addr[1] = wb_cfg->mem[1];
> > + pipe->output->mem.addr[2] = wb_cfg->mem[2];
> > + pipe->output->writeback = true;
> > + }
> > +
> > vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> > vsp1_du_pipeline_configure(pipe);
> > +
> > +done:
> > mutex_unlock(&vsp1->drm->lock);
> > }
> > EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
> > diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> > index 877496936487..cc1b0d42ce95 100644
> > --- a/include/media/vsp1.h
> > +++ b/include/media/vsp1.h
> > @@ -18,6 +18,7 @@ struct device;
> > int vsp1_du_init(struct device *dev);
> >
> > #define VSP1_DU_STATUS_COMPLETE BIT(0)
> > +#define VSP1_DU_STATUS_WRITEBACK BIT(1)
> >
> > /**
> > * struct vsp1_du_lif_config - VSP LIF configuration
> > @@ -83,12 +84,26 @@ struct vsp1_du_crc_config {
> > unsigned int index;
> > };
> >
> > +/**
> > + * struct vsp1_du_writeback_config - VSP writeback configuration parameters
> > + * @pixelformat: plane pixel format (V4L2 4CC)
> > + * @pitch: line pitch in bytes for the first plane
> > + * @mem: DMA memory address for each plane of the frame buffer
> > + */
> > +struct vsp1_du_writeback_config {
> > + u32 pixelformat;
> > + unsigned int pitch;
> > + dma_addr_t mem[3];
> > +};
> > +
> > /**
> > * struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
> > * @crc: CRC computation configuration
> > + * @writeback: writeback configuration
> > */
> > struct vsp1_du_atomic_pipe_config {
> > struct vsp1_du_crc_config crc;
> > + struct vsp1_du_writeback_config writeback;
> > };
> >
> > void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 11/18] media: vsp1: drm: Implement writeback support
2019-03-13 15:56 ` Laurent Pinchart
@ 2019-03-14 8:28 ` Kieran Bingham
2019-03-14 12:09 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Kieran Bingham @ 2019-03-14 8:28 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Laurent Pinchart, Liviu Dudau, dri-devel, linux-renesas-soc, linux-media
Hi Laurent,
On 13/03/2019 15:56, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Wed, Mar 13, 2019 at 11:42:34AM +0000, Kieran Bingham wrote:
>> On 13/03/2019 00:05, Laurent Pinchart wrote:
>>> Extend the vsp1_du_atomic_flush() API with writeback support by adding
>>> format, pitch and memory addresses of the writeback framebuffer.
>>> Writeback completion is reported through the existing frame completion
>>> callback with a new VSP1_DU_STATUS_WRITEBACK status flag.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
My concerns have been addressed here:
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>> ---
>>> drivers/media/platform/vsp1/vsp1_dl.c | 14 ++++++++++++++
>>> drivers/media/platform/vsp1/vsp1_dl.h | 3 ++-
>>> drivers/media/platform/vsp1/vsp1_drm.c | 25 ++++++++++++++++++++++++-
>>> include/media/vsp1.h | 15 +++++++++++++++
>>> 4 files changed, 55 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
>>> index ed7cda4130f2..104b6f514536 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_dl.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
>>> @@ -958,6 +958,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
>>> *
>>> * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
>>> * became active had been queued with the internal notification flag.
>>> + *
>>> + * The VSP1_DL_FRAME_END_WRITEBACK flag indicates that the previously active
>>> + * display list had been queued with the writeback flag.
>>
>> How does this interact with the possibility of the writeback being
>> disabled by the WPF in the event of it failing to get a DL.
>>
>> It's only a small corner case, but will the 'writeback' report back as
>> though it succeeded? (without writing to memory, and thus giving an
>> unmodified buffer back?)
>
> Wrteback completion will never be reported in that case. This shouldn't
> happen as we should never fail to get a display list. Do you think it
> would be better to fake completion ?
Would this lack of completion cause a hang while DRM waits for the
completion to occur? I guess this would timeout after some period.
I'm not sure what's worse. To hang / block for a timeout - or just
return a 'bad buffer'. We would know in the VSP that the completion has
failed, so we could signal a failure, but I think as the rest of the DRM
model goes with a timeout if the flip_done fails to complete for
example, we should follow that.
So leave this as is, and perhaps lets make sure the core writeback
framework will report a warning if it hits a time out.
If we ever fail to get a display list - we will have bigger issues which
will propogate elsewhere :)
>>> */
>>> unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>>> {
>>> @@ -995,6 +998,17 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>>> if (status & VI6_STATUS_FLD_STD(dlm->index))
>>> goto done;
>>>
>>> + /*
>>> + * If the active display list has the writeback flag set, the frame
>>> + * completion marks the end of the writeback capture. Return the
>>> + * VSP1_DL_FRAME_END_WRITEBACK flag and reset the display list's
>>> + * writeback flag.
>>> + */
>>> + if (dlm->active && (dlm->active->flags & VSP1_DL_FRAME_END_WRITEBACK)) {
>>> + flags |= VSP1_DL_FRAME_END_WRITEBACK;
>>> + dlm->active->flags &= ~VSP1_DL_FRAME_END_WRITEBACK;
>>> + }
>>> +
>>> /*
>>> * The device starts processing the queued display list right after the
>>> * frame end interrupt. The display list thus becomes active.
>>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
>>> index e0fdb145e6ed..4d7bcfdc9bd9 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_dl.h
>>> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
>>> @@ -18,7 +18,8 @@ struct vsp1_dl_list;
>>> struct vsp1_dl_manager;
>>>
>>> #define VSP1_DL_FRAME_END_COMPLETED BIT(0)
>>> -#define VSP1_DL_FRAME_END_INTERNAL BIT(1)
>>> +#define VSP1_DL_FRAME_END_WRITEBACK BIT(1)
>>
>> So below BIT(2) (code above) the flags match the externally exposed
>> bitfield for the VSP1_DU_STATUS_
>>
>> While above (code below), are 'private' bitfields.
>>
>> Should this requirement be documented here somehow? especially the
>> mapping of FRAME_END_{COMPLETED,WRITEBACK} to
>> DU_STATUS_{COMPLETED,WRITEBACK}.
>
> I've added a comment here, as explained in my reply to your review of
> 10/18, to document this.
Great.
>>> +#define VSP1_DL_FRAME_END_INTERNAL BIT(2)
>>>
>>> /**
>>> * struct vsp1_dl_ext_cmd - Extended Display command
>>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
>>> index 0367f88135bf..16826bf184c7 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
>>> @@ -37,7 +37,9 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
>>>
>>> if (drm_pipe->du_complete) {
>>> struct vsp1_entity *uif = drm_pipe->uif;
>>> - unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
>>> + unsigned int status = completion
>>> + & (VSP1_DU_STATUS_COMPLETE |
>>> + VSP1_DU_STATUS_WRITEBACK);
>>> u32 crc;
>>>
>>> crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
>>> @@ -541,6 +543,8 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
>>>
>>> if (drm_pipe->force_brx_release)
>>> dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
>>> + if (pipe->output->writeback)
>>> + dl_flags |= VSP1_DL_FRAME_END_WRITEBACK;
>>>
>>> dl = vsp1_dl_list_get(pipe->output->dlm);
>>> dlb = vsp1_dl_list_get_body0(dl);
>>> @@ -870,12 +874,31 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
>>> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>>> struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
>>> struct vsp1_pipeline *pipe = &drm_pipe->pipe;
>>> + int ret;
>>>
>>> drm_pipe->crc = cfg->crc;
>>>
>>> mutex_lock(&vsp1->drm->lock);
>>> +
>>> + if (pipe->output->has_writeback && cfg->writeback.pixelformat) {
>>
>> Is pipe->output->has_writeback necessary here? Can
>> cfg->writeback.pixelformat be set if pipe->output->has_writeback is false?
>>
>> Hrm ... actually - Perhaps it is useful. It validates both sides of the
>> system.
>>
>> pipe->output->has_writeback is a capability of the VSP, where as
>> cfg->writeback.pixelformat is a 'request' from the DU.
>
> Correct, I think it's best to check both, to ensure we don't try to
> queue a writeback request on a system that doesn't support writeback. On
> the other hand this shouldn't happen as the DU driver shouldn't expose
> writeback to userspace in that case, so if you don't think the check is
> worth it I can remove the has_writeback field completely.
It's a cheap check, I don't think it is too much of an issue - but I
agree (if we don't already) then we should make sure userspace does not
see a writeback functionality if it is not supported through the whole
pipeline (i.e. including the capability in the VSP1).
That would make me lean towards removing this check here - *iff* we
guarantee that the VSP will only be asked to do write back when it's
possible.
>>> + const struct vsp1_du_writeback_config *wb_cfg = &cfg->writeback;
>>> +
>>> + ret = vsp1_du_pipeline_set_rwpf_format(vsp1, pipe->output,
>>> + wb_cfg->pixelformat,
>>> + wb_cfg->pitch);
>>> + if (WARN_ON(ret < 0))
>>> + goto done;
>>> +
>>> + pipe->output->mem.addr[0] = wb_cfg->mem[0];
>>> + pipe->output->mem.addr[1] = wb_cfg->mem[1];
>>> + pipe->output->mem.addr[2] = wb_cfg->mem[2];
>>> + pipe->output->writeback = true;
>>> + }
>>> +
>>> vsp1_du_pipeline_setup_inputs(vsp1, pipe);
>>> vsp1_du_pipeline_configure(pipe);
>>> +
>>> +done:
>>> mutex_unlock(&vsp1->drm->lock);
>>> }
>>> EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
>>> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
>>> index 877496936487..cc1b0d42ce95 100644
>>> --- a/include/media/vsp1.h
>>> +++ b/include/media/vsp1.h
>>> @@ -18,6 +18,7 @@ struct device;
>>> int vsp1_du_init(struct device *dev);
>>>
>>> #define VSP1_DU_STATUS_COMPLETE BIT(0)
>>> +#define VSP1_DU_STATUS_WRITEBACK BIT(1)
>>>
>>> /**
>>> * struct vsp1_du_lif_config - VSP LIF configuration
>>> @@ -83,12 +84,26 @@ struct vsp1_du_crc_config {
>>> unsigned int index;
>>> };
>>>
>>> +/**
>>> + * struct vsp1_du_writeback_config - VSP writeback configuration parameters
>>> + * @pixelformat: plane pixel format (V4L2 4CC)
>>> + * @pitch: line pitch in bytes for the first plane
>>> + * @mem: DMA memory address for each plane of the frame buffer
>>> + */
>>> +struct vsp1_du_writeback_config {
>>> + u32 pixelformat;
>>> + unsigned int pitch;
>>> + dma_addr_t mem[3];
>>> +};
>>> +
>>> /**
>>> * struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
>>> * @crc: CRC computation configuration
>>> + * @writeback: writeback configuration
>>> */
>>> struct vsp1_du_atomic_pipe_config {
>>> struct vsp1_du_crc_config crc;
>>> + struct vsp1_du_writeback_config writeback;
>>> };
>>>
>>> void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
>
--
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 11/18] media: vsp1: drm: Implement writeback support
2019-03-14 8:28 ` Kieran Bingham
@ 2019-03-14 12:09 ` Laurent Pinchart
0 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-14 12:09 UTC (permalink / raw)
To: Kieran Bingham
Cc: Laurent Pinchart, Liviu Dudau, dri-devel, linux-renesas-soc, linux-media
Hi Kieran,
On Thu, Mar 14, 2019 at 08:28:27AM +0000, Kieran Bingham wrote:
> On 13/03/2019 15:56, Laurent Pinchart wrote:
> > On Wed, Mar 13, 2019 at 11:42:34AM +0000, Kieran Bingham wrote:
> >> On 13/03/2019 00:05, Laurent Pinchart wrote:
> >>> Extend the vsp1_du_atomic_flush() API with writeback support by adding
> >>> format, pitch and memory addresses of the writeback framebuffer.
> >>> Writeback completion is reported through the existing frame completion
> >>> callback with a new VSP1_DU_STATUS_WRITEBACK status flag.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> My concerns have been addressed here:
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> >>> ---
> >>> drivers/media/platform/vsp1/vsp1_dl.c | 14 ++++++++++++++
> >>> drivers/media/platform/vsp1/vsp1_dl.h | 3 ++-
> >>> drivers/media/platform/vsp1/vsp1_drm.c | 25 ++++++++++++++++++++++++-
> >>> include/media/vsp1.h | 15 +++++++++++++++
> >>> 4 files changed, 55 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> >>> index ed7cda4130f2..104b6f514536 100644
> >>> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> >>> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> >>> @@ -958,6 +958,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
> >>> *
> >>> * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
> >>> * became active had been queued with the internal notification flag.
> >>> + *
> >>> + * The VSP1_DL_FRAME_END_WRITEBACK flag indicates that the previously active
> >>> + * display list had been queued with the writeback flag.
> >>
> >> How does this interact with the possibility of the writeback being
> >> disabled by the WPF in the event of it failing to get a DL.
> >>
> >> It's only a small corner case, but will the 'writeback' report back as
> >> though it succeeded? (without writing to memory, and thus giving an
> >> unmodified buffer back?)
> >
> > Wrteback completion will never be reported in that case. This shouldn't
> > happen as we should never fail to get a display list. Do you think it
> > would be better to fake completion ?
>
> Would this lack of completion cause a hang while DRM waits for the
> completion to occur? I guess this would timeout after some period.
Not in the kernel as far as I can tell, but userspace could then wait
forever.
> I'm not sure what's worse. To hang / block for a timeout - or just
> return a 'bad buffer'. We would know in the VSP that the completion has
> failed, so we could signal a failure, but I think as the rest of the DRM
> model goes with a timeout if the flip_done fails to complete for
> example, we should follow that.
>
> So leave this as is, and perhaps lets make sure the core writeback
> framework will report a warning if it hits a time out.
There's no timeout handling for writeback in the core.
> If we ever fail to get a display list - we will have bigger issues which
> will propogate elsewhere :)
Yes, I think so too. This should really never happen.
> >>> */
> >>> unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> >>> {
> >>> @@ -995,6 +998,17 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> >>> if (status & VI6_STATUS_FLD_STD(dlm->index))
> >>> goto done;
> >>>
> >>> + /*
> >>> + * If the active display list has the writeback flag set, the frame
> >>> + * completion marks the end of the writeback capture. Return the
> >>> + * VSP1_DL_FRAME_END_WRITEBACK flag and reset the display list's
> >>> + * writeback flag.
> >>> + */
> >>> + if (dlm->active && (dlm->active->flags & VSP1_DL_FRAME_END_WRITEBACK)) {
> >>> + flags |= VSP1_DL_FRAME_END_WRITEBACK;
> >>> + dlm->active->flags &= ~VSP1_DL_FRAME_END_WRITEBACK;
> >>> + }
> >>> +
> >>> /*
> >>> * The device starts processing the queued display list right after the
> >>> * frame end interrupt. The display list thus becomes active.
> >>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
> >>> index e0fdb145e6ed..4d7bcfdc9bd9 100644
> >>> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> >>> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> >>> @@ -18,7 +18,8 @@ struct vsp1_dl_list;
> >>> struct vsp1_dl_manager;
> >>>
> >>> #define VSP1_DL_FRAME_END_COMPLETED BIT(0)
> >>> -#define VSP1_DL_FRAME_END_INTERNAL BIT(1)
> >>> +#define VSP1_DL_FRAME_END_WRITEBACK BIT(1)
> >>
> >> So below BIT(2) (code above) the flags match the externally exposed
> >> bitfield for the VSP1_DU_STATUS_
> >>
> >> While above (code below), are 'private' bitfields.
> >>
> >> Should this requirement be documented here somehow? especially the
> >> mapping of FRAME_END_{COMPLETED,WRITEBACK} to
> >> DU_STATUS_{COMPLETED,WRITEBACK}.
> >
> > I've added a comment here, as explained in my reply to your review of
> > 10/18, to document this.
>
> Great.
>
> >>> +#define VSP1_DL_FRAME_END_INTERNAL BIT(2)
> >>>
> >>> /**
> >>> * struct vsp1_dl_ext_cmd - Extended Display command
> >>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> >>> index 0367f88135bf..16826bf184c7 100644
> >>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> >>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> >>> @@ -37,7 +37,9 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
> >>>
> >>> if (drm_pipe->du_complete) {
> >>> struct vsp1_entity *uif = drm_pipe->uif;
> >>> - unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
> >>> + unsigned int status = completion
> >>> + & (VSP1_DU_STATUS_COMPLETE |
> >>> + VSP1_DU_STATUS_WRITEBACK);
> >>> u32 crc;
> >>>
> >>> crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> >>> @@ -541,6 +543,8 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
> >>>
> >>> if (drm_pipe->force_brx_release)
> >>> dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
> >>> + if (pipe->output->writeback)
> >>> + dl_flags |= VSP1_DL_FRAME_END_WRITEBACK;
> >>>
> >>> dl = vsp1_dl_list_get(pipe->output->dlm);
> >>> dlb = vsp1_dl_list_get_body0(dl);
> >>> @@ -870,12 +874,31 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> >>> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> >>> struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> >>> struct vsp1_pipeline *pipe = &drm_pipe->pipe;
> >>> + int ret;
> >>>
> >>> drm_pipe->crc = cfg->crc;
> >>>
> >>> mutex_lock(&vsp1->drm->lock);
> >>> +
> >>> + if (pipe->output->has_writeback && cfg->writeback.pixelformat) {
> >>
> >> Is pipe->output->has_writeback necessary here? Can
> >> cfg->writeback.pixelformat be set if pipe->output->has_writeback is false?
> >>
> >> Hrm ... actually - Perhaps it is useful. It validates both sides of the
> >> system.
> >>
> >> pipe->output->has_writeback is a capability of the VSP, where as
> >> cfg->writeback.pixelformat is a 'request' from the DU.
> >
> > Correct, I think it's best to check both, to ensure we don't try to
> > queue a writeback request on a system that doesn't support writeback. On
> > the other hand this shouldn't happen as the DU driver shouldn't expose
> > writeback to userspace in that case, so if you don't think the check is
> > worth it I can remove the has_writeback field completely.
>
> It's a cheap check, I don't think it is too much of an issue - but I
> agree (if we don't already) then we should make sure userspace does not
> see a writeback functionality if it is not supported through the whole
> pipeline (i.e. including the capability in the VSP1).
>
> That would make me lean towards removing this check here - *iff* we
> guarantee that the VSP will only be asked to do write back when it's
> possible.
Unless there's a bug in the DU side, we have such a guarantee. I'll
remove the check.
> >>> + const struct vsp1_du_writeback_config *wb_cfg = &cfg->writeback;
> >>> +
> >>> + ret = vsp1_du_pipeline_set_rwpf_format(vsp1, pipe->output,
> >>> + wb_cfg->pixelformat,
> >>> + wb_cfg->pitch);
> >>> + if (WARN_ON(ret < 0))
> >>> + goto done;
> >>> +
> >>> + pipe->output->mem.addr[0] = wb_cfg->mem[0];
> >>> + pipe->output->mem.addr[1] = wb_cfg->mem[1];
> >>> + pipe->output->mem.addr[2] = wb_cfg->mem[2];
> >>> + pipe->output->writeback = true;
> >>> + }
> >>> +
> >>> vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> >>> vsp1_du_pipeline_configure(pipe);
> >>> +
> >>> +done:
> >>> mutex_unlock(&vsp1->drm->lock);
> >>> }
> >>> EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
> >>> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> >>> index 877496936487..cc1b0d42ce95 100644
> >>> --- a/include/media/vsp1.h
> >>> +++ b/include/media/vsp1.h
> >>> @@ -18,6 +18,7 @@ struct device;
> >>> int vsp1_du_init(struct device *dev);
> >>>
> >>> #define VSP1_DU_STATUS_COMPLETE BIT(0)
> >>> +#define VSP1_DU_STATUS_WRITEBACK BIT(1)
> >>>
> >>> /**
> >>> * struct vsp1_du_lif_config - VSP LIF configuration
> >>> @@ -83,12 +84,26 @@ struct vsp1_du_crc_config {
> >>> unsigned int index;
> >>> };
> >>>
> >>> +/**
> >>> + * struct vsp1_du_writeback_config - VSP writeback configuration parameters
> >>> + * @pixelformat: plane pixel format (V4L2 4CC)
> >>> + * @pitch: line pitch in bytes for the first plane
> >>> + * @mem: DMA memory address for each plane of the frame buffer
> >>> + */
> >>> +struct vsp1_du_writeback_config {
> >>> + u32 pixelformat;
> >>> + unsigned int pitch;
> >>> + dma_addr_t mem[3];
> >>> +};
> >>> +
> >>> /**
> >>> * struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
> >>> * @crc: CRC computation configuration
> >>> + * @writeback: writeback configuration
> >>> */
> >>> struct vsp1_du_atomic_pipe_config {
> >>> struct vsp1_du_crc_config crc;
> >>> + struct vsp1_du_writeback_config writeback;
> >>> };
> >>>
> >>> void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
> >
>
> --
> Regards
> --
> Kieran
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 12/18] drm: writeback: Cleanup job ownership handling when queuing job
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (10 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 11/18] media: vsp1: drm: Implement writeback support Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 11:45 ` Kieran Bingham
2019-03-13 0:05 ` [PATCH v6 13/18] drm: writeback: Fix leak of writeback job Laurent Pinchart
` (5 subsequent siblings)
17 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
The drm_writeback_queue_job() function takes ownership of the passed job
and requires the caller to manually set the connector state
writeback_job pointer to NULL. To simplify drivers and avoid errors
(such as the missing NULL set in the vc4 driver), pass the connector
state pointer to the function instead of the job pointer, and set the
writeback_job pointer to NULL internally.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Brian Starkey <brian.starkey@arm.com>
Acked-by: Eric Anholt <eric@anholt.net>
Acked-by: Liviu Dudau <liviu.dudau@arm.com>
---
drivers/gpu/drm/arm/malidp_mw.c | 3 +--
drivers/gpu/drm/drm_writeback.c | 15 ++++++++++-----
drivers/gpu/drm/vc4/vc4_txp.c | 2 +-
include/drm/drm_writeback.h | 2 +-
4 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index 041a64dc7167..87627219ce3b 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -252,8 +252,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm,
&mw_state->addrs[0],
mw_state->format);
- drm_writeback_queue_job(mw_conn, conn_state->writeback_job);
- conn_state->writeback_job = NULL;
+ drm_writeback_queue_job(mw_conn, conn_state);
hwdev->hw->enable_memwrite(hwdev, mw_state->addrs,
mw_state->pitches, mw_state->n_planes,
fb->width, fb->height, mw_state->format,
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index c20e6fe00cb3..338b993d7c9f 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -242,11 +242,12 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
/**
* drm_writeback_queue_job - Queue a writeback job for later signalling
* @wb_connector: The writeback connector to queue a job on
- * @job: The job to queue
+ * @conn_state: The connector state containing the job to queue
*
- * This function adds a job to the job_queue for a writeback connector. It
- * should be considered to take ownership of the writeback job, and so any other
- * references to the job must be cleared after calling this function.
+ * This function adds the job contained in @conn_state to the job_queue for a
+ * writeback connector. It takes ownership of the writeback job and sets the
+ * @conn_state->writeback_job to NULL, and so no access to the job may be
+ * performed by the caller after this function returns.
*
* Drivers must ensure that for a given writeback connector, jobs are queued in
* exactly the same order as they will be completed by the hardware (and
@@ -258,10 +259,14 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
* See also: drm_writeback_signal_completion()
*/
void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
- struct drm_writeback_job *job)
+ struct drm_connector_state *conn_state)
{
+ struct drm_writeback_job *job;
unsigned long flags;
+ job = conn_state->writeback_job;
+ conn_state->writeback_job = NULL;
+
spin_lock_irqsave(&wb_connector->job_lock, flags);
list_add_tail(&job->list_entry, &wb_connector->job_queue);
spin_unlock_irqrestore(&wb_connector->job_lock, flags);
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index aa279b5b0de7..5dabd91f2d7e 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -327,7 +327,7 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
TXP_WRITE(TXP_DST_CTRL, ctrl);
- drm_writeback_queue_job(&txp->connector, conn_state->writeback_job);
+ drm_writeback_queue_job(&txp->connector, conn_state);
}
static const struct drm_connector_helper_funcs vc4_txp_connector_helper_funcs = {
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 23df9d463003..47662c362743 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -123,7 +123,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
const u32 *formats, int n_formats);
void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
- struct drm_writeback_job *job);
+ struct drm_connector_state *conn_state);
void drm_writeback_cleanup_job(struct drm_writeback_job *job);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 12/18] drm: writeback: Cleanup job ownership handling when queuing job
2019-03-13 0:05 ` [PATCH v6 12/18] drm: writeback: Cleanup job ownership handling when queuing job Laurent Pinchart
@ 2019-03-13 11:45 ` Kieran Bingham
0 siblings, 0 replies; 36+ messages in thread
From: Kieran Bingham @ 2019-03-13 11:45 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc, Liviu Dudau, linux-media
Hi Laurent,
On 13/03/2019 00:05, Laurent Pinchart wrote:
> The drm_writeback_queue_job() function takes ownership of the passed job
> and requires the caller to manually set the connector state
> writeback_job pointer to NULL. To simplify drivers and avoid errors
> (such as the missing NULL set in the vc4 driver), pass the connector
> state pointer to the function instead of the job pointer, and set the
> writeback_job pointer to NULL internally.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> Acked-by: Eric Anholt <eric@anholt.net>
> Acked-by: Liviu Dudau <liviu.dudau@arm.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> drivers/gpu/drm/arm/malidp_mw.c | 3 +--
> drivers/gpu/drm/drm_writeback.c | 15 ++++++++++-----
> drivers/gpu/drm/vc4/vc4_txp.c | 2 +-
> include/drm/drm_writeback.h | 2 +-
> 4 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
> index 041a64dc7167..87627219ce3b 100644
> --- a/drivers/gpu/drm/arm/malidp_mw.c
> +++ b/drivers/gpu/drm/arm/malidp_mw.c
> @@ -252,8 +252,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm,
> &mw_state->addrs[0],
> mw_state->format);
>
> - drm_writeback_queue_job(mw_conn, conn_state->writeback_job);
> - conn_state->writeback_job = NULL;
> + drm_writeback_queue_job(mw_conn, conn_state);
> hwdev->hw->enable_memwrite(hwdev, mw_state->addrs,
> mw_state->pitches, mw_state->n_planes,
> fb->width, fb->height, mw_state->format,
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index c20e6fe00cb3..338b993d7c9f 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -242,11 +242,12 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
> /**
> * drm_writeback_queue_job - Queue a writeback job for later signalling
> * @wb_connector: The writeback connector to queue a job on
> - * @job: The job to queue
> + * @conn_state: The connector state containing the job to queue
> *
> - * This function adds a job to the job_queue for a writeback connector. It
> - * should be considered to take ownership of the writeback job, and so any other
> - * references to the job must be cleared after calling this function.
> + * This function adds the job contained in @conn_state to the job_queue for a
> + * writeback connector. It takes ownership of the writeback job and sets the
> + * @conn_state->writeback_job to NULL, and so no access to the job may be
> + * performed by the caller after this function returns.
> *
> * Drivers must ensure that for a given writeback connector, jobs are queued in
> * exactly the same order as they will be completed by the hardware (and
> @@ -258,10 +259,14 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
> * See also: drm_writeback_signal_completion()
> */
> void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> - struct drm_writeback_job *job)
> + struct drm_connector_state *conn_state)
> {
> + struct drm_writeback_job *job;
> unsigned long flags;
>
> + job = conn_state->writeback_job;
> + conn_state->writeback_job = NULL;
> +
> spin_lock_irqsave(&wb_connector->job_lock, flags);
> list_add_tail(&job->list_entry, &wb_connector->job_queue);
> spin_unlock_irqrestore(&wb_connector->job_lock, flags);
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index aa279b5b0de7..5dabd91f2d7e 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -327,7 +327,7 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
>
> TXP_WRITE(TXP_DST_CTRL, ctrl);
>
> - drm_writeback_queue_job(&txp->connector, conn_state->writeback_job);
> + drm_writeback_queue_job(&txp->connector, conn_state);
> }
>
> static const struct drm_connector_helper_funcs vc4_txp_connector_helper_funcs = {
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 23df9d463003..47662c362743 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -123,7 +123,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> const u32 *formats, int n_formats);
>
> void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> - struct drm_writeback_job *job);
> + struct drm_connector_state *conn_state);
>
> void drm_writeback_cleanup_job(struct drm_writeback_job *job);
>
>
--
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 13/18] drm: writeback: Fix leak of writeback job
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (11 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 12/18] drm: writeback: Cleanup job ownership handling when queuing job Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 14/18] drm: writeback: Add job prepare and cleanup operations Laurent Pinchart
` (4 subsequent siblings)
17 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
Writeback jobs are allocated when the WRITEBACK_FB_ID is set, and
deleted when the jobs complete. This results in both a memory leak of
the job and a leak of the framebuffer if the atomic commit returns
before the job is queued for processing, for instance if the atomic
check fails or if the commit runs in test-only mode.
Fix this by implementing the drm_writeback_cleanup_job() function and
calling it from __drm_atomic_helper_connector_destroy_state(). As
writeback jobs are removed from the state when they're queued for
processing, any job left in the state when the state gets destroyed
needs to be cleaned up.
The existing declaration of the drm_writeback_cleanup_job() function
without an implementation hints that this problem was considered, but
never addressed.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Brian Starkey <brian.starkey@arm.com>
Acked-by: Liviu Dudau <liviu.dudau@arm.com>
---
Changes since v5:
- Export drm_writeback_cleanup_job()
---
drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++
drivers/gpu/drm/drm_writeback.c | 14 +++++++++++---
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 4985384e51f6..59ffb6b9c745 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -30,6 +30,7 @@
#include <drm/drm_connector.h>
#include <drm/drm_atomic.h>
#include <drm/drm_device.h>
+#include <drm/drm_writeback.h>
#include <linux/slab.h>
#include <linux/dma-fence.h>
@@ -412,6 +413,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
if (state->commit)
drm_crtc_commit_put(state->commit);
+
+ if (state->writeback_job)
+ drm_writeback_cleanup_job(state->writeback_job);
}
EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 338b993d7c9f..1b497d3530b5 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -273,6 +273,15 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
}
EXPORT_SYMBOL(drm_writeback_queue_job);
+void drm_writeback_cleanup_job(struct drm_writeback_job *job)
+{
+ if (job->fb)
+ drm_framebuffer_put(job->fb);
+
+ kfree(job);
+}
+EXPORT_SYMBOL(drm_writeback_cleanup_job);
+
/*
* @cleanup_work: deferred cleanup of a writeback job
*
@@ -285,10 +294,9 @@ static void cleanup_work(struct work_struct *work)
struct drm_writeback_job *job = container_of(work,
struct drm_writeback_job,
cleanup_work);
- drm_framebuffer_put(job->fb);
- kfree(job);
-}
+ drm_writeback_cleanup_job(job);
+}
/**
* drm_writeback_signal_completion - Signal the completion of a writeback job
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 14/18] drm: writeback: Add job prepare and cleanup operations
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (12 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 13/18] drm: writeback: Fix leak of writeback job Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-15 17:54 ` Liviu Dudau
2019-03-13 0:05 ` [PATCH v6 15/18] drm: rcar-du: Fix rcar_du_crtc structure documentation Laurent Pinchart
` (3 subsequent siblings)
17 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
As writeback jobs contain a framebuffer, drivers may need to prepare and
cleanup them the same way they can prepare and cleanup framebuffers for
planes. Add two new optional connector helper operations,
.prepare_writeback_job() and .cleanup_writeback_job() to support this.
The job prepare operation is called from
drm_atomic_helper_prepare_planes() to avoid a new atomic commit helper
that would need to be called by all drivers not using
drm_atomic_helper_commit(). The job cleanup operation is called from the
existing drm_writeback_cleanup_job() function, invoked both when
destroying the job as part of a aborted commit, or when the job
completes.
The drm_writeback_job structure is extended with a priv field to let
drivers store per-job data, such as mappings related to the writeback
framebuffer.
For internal plumbing reasons the drm_writeback_job structure needs to
store a back-pointer to the drm_writeback_connector. To avoid pushing
too much writeback-specific knowledge to drm_atomic_uapi.c, create a
drm_writeback_set_fb() function, move the writeback job setup code
there, and set the connector backpointer. The prepare_signaling()
function doesn't need to allocate writeback jobs and can ignore
connectors without a job, as it is called after the writeback jobs are
allocated to store framebuffers, and a writeback fence with a
framebuffer is an invalid configuration that gets rejected by the commit
check.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v5:
- Export drm_writeback_prepare_job()
- Check for .prepare_writeback_job() in drm_writeback_prepare_job()
---
drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++
drivers/gpu/drm/drm_atomic_uapi.c | 31 +++++------------
drivers/gpu/drm/drm_writeback.c | 44 ++++++++++++++++++++++++
include/drm/drm_modeset_helper_vtables.h | 7 ++++
include/drm/drm_writeback.h | 28 ++++++++++++++-
5 files changed, 97 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 6fe2303fccd9..70a4886c6e65 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2245,10 +2245,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
int drm_atomic_helper_prepare_planes(struct drm_device *dev,
struct drm_atomic_state *state)
{
+ struct drm_connector *connector;
+ struct drm_connector_state *new_conn_state;
struct drm_plane *plane;
struct drm_plane_state *new_plane_state;
int ret, i, j;
+ for_each_new_connector_in_state(state, connector, new_conn_state, i) {
+ if (!new_conn_state->writeback_job)
+ continue;
+
+ ret = drm_writeback_prepare_job(new_conn_state->writeback_job);
+ if (ret < 0)
+ return ret;
+ }
+
for_each_new_plane_in_state(state, plane, new_plane_state, i) {
const struct drm_plane_helper_funcs *funcs;
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index c40889888a16..e802152a01ad 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -647,28 +647,15 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
return 0;
}
-static struct drm_writeback_job *
-drm_atomic_get_writeback_job(struct drm_connector_state *conn_state)
-{
- WARN_ON(conn_state->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
-
- if (!conn_state->writeback_job)
- conn_state->writeback_job =
- kzalloc(sizeof(*conn_state->writeback_job), GFP_KERNEL);
-
- return conn_state->writeback_job;
-}
-
static int drm_atomic_set_writeback_fb_for_connector(
struct drm_connector_state *conn_state,
struct drm_framebuffer *fb)
{
- struct drm_writeback_job *job =
- drm_atomic_get_writeback_job(conn_state);
- if (!job)
- return -ENOMEM;
+ int ret;
- drm_framebuffer_assign(&job->fb, fb);
+ ret = drm_writeback_set_fb(conn_state, fb);
+ if (ret < 0)
+ return ret;
if (fb)
DRM_DEBUG_ATOMIC("Set [FB:%d] for connector state %p\n",
@@ -1158,19 +1145,17 @@ 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;
s32 __user *fence_ptr;
+ if (!conn_state->writeback_job)
+ continue;
+
fence_ptr = get_out_fence_for_connector(state, conn);
if (!fence_ptr)
continue;
- job = drm_atomic_get_writeback_job(conn_state);
- if (!job)
- return -ENOMEM;
-
f = krealloc(*fence_state, sizeof(**fence_state) *
(*num_fences + 1), GFP_KERNEL);
if (!f)
@@ -1192,7 +1177,7 @@ static int prepare_signaling(struct drm_device *dev,
return ret;
}
- job->out_fence = fence;
+ conn_state->writeback_job->out_fence = fence;
}
/*
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 1b497d3530b5..79ac014701c8 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -239,6 +239,43 @@ int drm_writeback_connector_init(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_writeback_connector_init);
+int drm_writeback_set_fb(struct drm_connector_state *conn_state,
+ struct drm_framebuffer *fb)
+{
+ WARN_ON(conn_state->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
+
+ if (!conn_state->writeback_job) {
+ conn_state->writeback_job =
+ kzalloc(sizeof(*conn_state->writeback_job), GFP_KERNEL);
+ if (!conn_state->writeback_job)
+ return -ENOMEM;
+
+ conn_state->writeback_job->connector =
+ drm_connector_to_writeback(conn_state->connector);
+ }
+
+ drm_framebuffer_assign(&conn_state->writeback_job->fb, fb);
+ return 0;
+}
+
+int drm_writeback_prepare_job(struct drm_writeback_job *job)
+{
+ struct drm_writeback_connector *connector = job->connector;
+ const struct drm_connector_helper_funcs *funcs =
+ connector->base.helper_private;
+ int ret;
+
+ if (funcs->prepare_writeback_job) {
+ ret = funcs->prepare_writeback_job(connector, job);
+ if (ret < 0)
+ return ret;
+ }
+
+ job->prepared = true;
+ return 0;
+}
+EXPORT_SYMBOL(drm_writeback_prepare_job);
+
/**
* drm_writeback_queue_job - Queue a writeback job for later signalling
* @wb_connector: The writeback connector to queue a job on
@@ -275,6 +312,13 @@ EXPORT_SYMBOL(drm_writeback_queue_job);
void drm_writeback_cleanup_job(struct drm_writeback_job *job)
{
+ struct drm_writeback_connector *connector = job->connector;
+ const struct drm_connector_helper_funcs *funcs =
+ connector->base.helper_private;
+
+ if (job->prepared && funcs->cleanup_writeback_job)
+ funcs->cleanup_writeback_job(connector, job);
+
if (job->fb)
drm_framebuffer_put(job->fb);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 61142aa0ab23..73d03fe66799 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -49,6 +49,8 @@
*/
enum mode_set_atomic;
+struct drm_writeback_connector;
+struct drm_writeback_job;
/**
* struct drm_crtc_helper_funcs - helper operations for CRTCs
@@ -989,6 +991,11 @@ struct drm_connector_helper_funcs {
*/
void (*atomic_commit)(struct drm_connector *connector,
struct drm_connector_state *state);
+
+ int (*prepare_writeback_job)(struct drm_writeback_connector *connector,
+ struct drm_writeback_job *job);
+ void (*cleanup_writeback_job)(struct drm_writeback_connector *connector,
+ struct drm_writeback_job *job);
};
/**
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 47662c362743..777c14c847f0 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -79,6 +79,20 @@ struct drm_writeback_connector {
};
struct drm_writeback_job {
+ /**
+ * @connector:
+ *
+ * Back-pointer to the writeback connector associated with the job
+ */
+ struct drm_writeback_connector *connector;
+
+ /**
+ * @prepared:
+ *
+ * Set when the job has been prepared with drm_writeback_prepare_job()
+ */
+ bool prepared;
+
/**
* @cleanup_work:
*
@@ -98,7 +112,7 @@ struct drm_writeback_job {
* @fb:
*
* Framebuffer to be written to by the writeback connector. Do not set
- * directly, use drm_atomic_set_writeback_fb_for_connector()
+ * directly, use drm_writeback_set_fb()
*/
struct drm_framebuffer *fb;
@@ -108,6 +122,13 @@ struct drm_writeback_job {
* Fence which will signal once the writeback has completed
*/
struct dma_fence *out_fence;
+
+ /**
+ * @priv:
+ *
+ * Driver-private data
+ */
+ void *priv;
};
static inline struct drm_writeback_connector *
@@ -122,6 +143,11 @@ int drm_writeback_connector_init(struct drm_device *dev,
const struct drm_encoder_helper_funcs *enc_helper_funcs,
const u32 *formats, int n_formats);
+int drm_writeback_set_fb(struct drm_connector_state *conn_state,
+ struct drm_framebuffer *fb);
+
+int drm_writeback_prepare_job(struct drm_writeback_job *job);
+
void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
struct drm_connector_state *conn_state);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 14/18] drm: writeback: Add job prepare and cleanup operations
2019-03-13 0:05 ` [PATCH v6 14/18] drm: writeback: Add job prepare and cleanup operations Laurent Pinchart
@ 2019-03-15 17:54 ` Liviu Dudau
0 siblings, 0 replies; 36+ messages in thread
From: Liviu Dudau @ 2019-03-15 17:54 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-renesas-soc, Kieran Bingham, dri-devel, linux-media
On Wed, Mar 13, 2019 at 02:05:28AM +0200, Laurent Pinchart wrote:
> As writeback jobs contain a framebuffer, drivers may need to prepare and
> cleanup them the same way they can prepare and cleanup framebuffers for
> planes. Add two new optional connector helper operations,
> .prepare_writeback_job() and .cleanup_writeback_job() to support this.
>
> The job prepare operation is called from
> drm_atomic_helper_prepare_planes() to avoid a new atomic commit helper
> that would need to be called by all drivers not using
> drm_atomic_helper_commit(). The job cleanup operation is called from the
> existing drm_writeback_cleanup_job() function, invoked both when
> destroying the job as part of a aborted commit, or when the job
> completes.
>
> The drm_writeback_job structure is extended with a priv field to let
> drivers store per-job data, such as mappings related to the writeback
> framebuffer.
>
> For internal plumbing reasons the drm_writeback_job structure needs to
> store a back-pointer to the drm_writeback_connector. To avoid pushing
> too much writeback-specific knowledge to drm_atomic_uapi.c, create a
> drm_writeback_set_fb() function, move the writeback job setup code
> there, and set the connector backpointer. The prepare_signaling()
> function doesn't need to allocate writeback jobs and can ignore
> connectors without a job, as it is called after the writeback jobs are
> allocated to store framebuffers, and a writeback fence with a
> framebuffer is an invalid configuration that gets rejected by the commit
> check.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
Best regards,
Liviu
> ---
> Changes since v5:
>
> - Export drm_writeback_prepare_job()
> - Check for .prepare_writeback_job() in drm_writeback_prepare_job()
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++
> drivers/gpu/drm/drm_atomic_uapi.c | 31 +++++------------
> drivers/gpu/drm/drm_writeback.c | 44 ++++++++++++++++++++++++
> include/drm/drm_modeset_helper_vtables.h | 7 ++++
> include/drm/drm_writeback.h | 28 ++++++++++++++-
> 5 files changed, 97 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 6fe2303fccd9..70a4886c6e65 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2245,10 +2245,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
> int drm_atomic_helper_prepare_planes(struct drm_device *dev,
> struct drm_atomic_state *state)
> {
> + struct drm_connector *connector;
> + struct drm_connector_state *new_conn_state;
> struct drm_plane *plane;
> struct drm_plane_state *new_plane_state;
> int ret, i, j;
>
> + for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> + if (!new_conn_state->writeback_job)
> + continue;
> +
> + ret = drm_writeback_prepare_job(new_conn_state->writeback_job);
> + if (ret < 0)
> + return ret;
> + }
> +
> for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> const struct drm_plane_helper_funcs *funcs;
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c40889888a16..e802152a01ad 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -647,28 +647,15 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> return 0;
> }
>
> -static struct drm_writeback_job *
> -drm_atomic_get_writeback_job(struct drm_connector_state *conn_state)
> -{
> - WARN_ON(conn_state->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
> -
> - if (!conn_state->writeback_job)
> - conn_state->writeback_job =
> - kzalloc(sizeof(*conn_state->writeback_job), GFP_KERNEL);
> -
> - return conn_state->writeback_job;
> -}
> -
> static int drm_atomic_set_writeback_fb_for_connector(
> struct drm_connector_state *conn_state,
> struct drm_framebuffer *fb)
> {
> - struct drm_writeback_job *job =
> - drm_atomic_get_writeback_job(conn_state);
> - if (!job)
> - return -ENOMEM;
> + int ret;
>
> - drm_framebuffer_assign(&job->fb, fb);
> + ret = drm_writeback_set_fb(conn_state, fb);
> + if (ret < 0)
> + return ret;
>
> if (fb)
> DRM_DEBUG_ATOMIC("Set [FB:%d] for connector state %p\n",
> @@ -1158,19 +1145,17 @@ 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;
> s32 __user *fence_ptr;
>
> + if (!conn_state->writeback_job)
> + continue;
> +
> fence_ptr = get_out_fence_for_connector(state, conn);
> if (!fence_ptr)
> continue;
>
> - job = drm_atomic_get_writeback_job(conn_state);
> - if (!job)
> - return -ENOMEM;
> -
> f = krealloc(*fence_state, sizeof(**fence_state) *
> (*num_fences + 1), GFP_KERNEL);
> if (!f)
> @@ -1192,7 +1177,7 @@ static int prepare_signaling(struct drm_device *dev,
> return ret;
> }
>
> - job->out_fence = fence;
> + conn_state->writeback_job->out_fence = fence;
> }
>
> /*
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 1b497d3530b5..79ac014701c8 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -239,6 +239,43 @@ int drm_writeback_connector_init(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_writeback_connector_init);
>
> +int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> + struct drm_framebuffer *fb)
> +{
> + WARN_ON(conn_state->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
> +
> + if (!conn_state->writeback_job) {
> + conn_state->writeback_job =
> + kzalloc(sizeof(*conn_state->writeback_job), GFP_KERNEL);
> + if (!conn_state->writeback_job)
> + return -ENOMEM;
> +
> + conn_state->writeback_job->connector =
> + drm_connector_to_writeback(conn_state->connector);
> + }
> +
> + drm_framebuffer_assign(&conn_state->writeback_job->fb, fb);
> + return 0;
> +}
> +
> +int drm_writeback_prepare_job(struct drm_writeback_job *job)
> +{
> + struct drm_writeback_connector *connector = job->connector;
> + const struct drm_connector_helper_funcs *funcs =
> + connector->base.helper_private;
> + int ret;
> +
> + if (funcs->prepare_writeback_job) {
> + ret = funcs->prepare_writeback_job(connector, job);
> + if (ret < 0)
> + return ret;
> + }
> +
> + job->prepared = true;
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_writeback_prepare_job);
> +
> /**
> * drm_writeback_queue_job - Queue a writeback job for later signalling
> * @wb_connector: The writeback connector to queue a job on
> @@ -275,6 +312,13 @@ EXPORT_SYMBOL(drm_writeback_queue_job);
>
> void drm_writeback_cleanup_job(struct drm_writeback_job *job)
> {
> + struct drm_writeback_connector *connector = job->connector;
> + const struct drm_connector_helper_funcs *funcs =
> + connector->base.helper_private;
> +
> + if (job->prepared && funcs->cleanup_writeback_job)
> + funcs->cleanup_writeback_job(connector, job);
> +
> if (job->fb)
> drm_framebuffer_put(job->fb);
>
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 61142aa0ab23..73d03fe66799 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -49,6 +49,8 @@
> */
>
> enum mode_set_atomic;
> +struct drm_writeback_connector;
> +struct drm_writeback_job;
>
> /**
> * struct drm_crtc_helper_funcs - helper operations for CRTCs
> @@ -989,6 +991,11 @@ struct drm_connector_helper_funcs {
> */
> void (*atomic_commit)(struct drm_connector *connector,
> struct drm_connector_state *state);
> +
> + int (*prepare_writeback_job)(struct drm_writeback_connector *connector,
> + struct drm_writeback_job *job);
> + void (*cleanup_writeback_job)(struct drm_writeback_connector *connector,
> + struct drm_writeback_job *job);
> };
>
> /**
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 47662c362743..777c14c847f0 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -79,6 +79,20 @@ struct drm_writeback_connector {
> };
>
> struct drm_writeback_job {
> + /**
> + * @connector:
> + *
> + * Back-pointer to the writeback connector associated with the job
> + */
> + struct drm_writeback_connector *connector;
> +
> + /**
> + * @prepared:
> + *
> + * Set when the job has been prepared with drm_writeback_prepare_job()
> + */
> + bool prepared;
> +
> /**
> * @cleanup_work:
> *
> @@ -98,7 +112,7 @@ struct drm_writeback_job {
> * @fb:
> *
> * Framebuffer to be written to by the writeback connector. Do not set
> - * directly, use drm_atomic_set_writeback_fb_for_connector()
> + * directly, use drm_writeback_set_fb()
> */
> struct drm_framebuffer *fb;
>
> @@ -108,6 +122,13 @@ struct drm_writeback_job {
> * Fence which will signal once the writeback has completed
> */
> struct dma_fence *out_fence;
> +
> + /**
> + * @priv:
> + *
> + * Driver-private data
> + */
> + void *priv;
> };
>
> static inline struct drm_writeback_connector *
> @@ -122,6 +143,11 @@ int drm_writeback_connector_init(struct drm_device *dev,
> const struct drm_encoder_helper_funcs *enc_helper_funcs,
> const u32 *formats, int n_formats);
>
> +int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> + struct drm_framebuffer *fb);
> +
> +int drm_writeback_prepare_job(struct drm_writeback_job *job);
> +
> void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> struct drm_connector_state *conn_state);
>
> --
> Regards,
>
> Laurent Pinchart
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 15/18] drm: rcar-du: Fix rcar_du_crtc structure documentation
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (13 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 14/18] drm: writeback: Add job prepare and cleanup operations Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 16/18] drm: rcar-du: Store V4L2 fourcc in rcar_du_format_info structure Laurent Pinchart
` (2 subsequent siblings)
17 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
The rcar_du_crtc structure index field contains the CRTC hardware index,
not the hardware and software index. Update the documentation
accordingly.
Fixes: 5361cc7f8e91 ("drm: rcar-du: Split CRTC handling to support hardware indexing")
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index bcb35b0b7612..c478953be092 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -27,7 +27,7 @@ struct rcar_du_vsp;
* @clock: the CRTC functional clock
* @extclock: external pixel dot clock (optional)
* @mmio_offset: offset of the CRTC registers in the DU MMIO block
- * @index: CRTC software and hardware index
+ * @index: CRTC hardware index
* @initialized: whether the CRTC has been initialized and clocks enabled
* @dsysr: cached value of the DSYSR register
* @vblank_enable: whether vblank events are enabled on this CRTC
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 16/18] drm: rcar-du: Store V4L2 fourcc in rcar_du_format_info structure
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (14 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 15/18] drm: rcar-du: Fix rcar_du_crtc structure documentation Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 17/18] drm: rcar-du: vsp: Extract framebuffer (un)mapping to separate functions Laurent Pinchart
2019-03-13 0:05 ` [PATCH v6 18/18] drm: rcar-du: Add writeback support for R-Car Gen3 Laurent Pinchart
17 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
The mapping between DRM and V4L2 fourcc's is stored in two separate
tables in rcar_du_vsp.c. In order to make it reusable to implement
writeback support, move it to the rcar_du_format_info structure.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 25 +++++++++++++++
drivers/gpu/drm/rcar-du/rcar_du_kms.h | 1 +
drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 44 ++++-----------------------
3 files changed, 32 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index b0c80dffd8b8..999440c7b258 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -32,60 +32,70 @@
static const struct rcar_du_format_info rcar_du_format_infos[] = {
{
.fourcc = DRM_FORMAT_RGB565,
+ .v4l2 = V4L2_PIX_FMT_RGB565,
.bpp = 16,
.planes = 1,
.pnmr = PnMR_SPIM_TP | PnMR_DDDF_16BPP,
.edf = PnDDCR4_EDF_NONE,
}, {
.fourcc = DRM_FORMAT_ARGB1555,
+ .v4l2 = V4L2_PIX_FMT_ARGB555,
.bpp = 16,
.planes = 1,
.pnmr = PnMR_SPIM_ALP | PnMR_DDDF_ARGB,
.edf = PnDDCR4_EDF_NONE,
}, {
.fourcc = DRM_FORMAT_XRGB1555,
+ .v4l2 = V4L2_PIX_FMT_XRGB555,
.bpp = 16,
.planes = 1,
.pnmr = PnMR_SPIM_ALP | PnMR_DDDF_ARGB,
.edf = PnDDCR4_EDF_NONE,
}, {
.fourcc = DRM_FORMAT_XRGB8888,
+ .v4l2 = V4L2_PIX_FMT_XBGR32,
.bpp = 32,
.planes = 1,
.pnmr = PnMR_SPIM_TP | PnMR_DDDF_16BPP,
.edf = PnDDCR4_EDF_RGB888,
}, {
.fourcc = DRM_FORMAT_ARGB8888,
+ .v4l2 = V4L2_PIX_FMT_ABGR32,
.bpp = 32,
.planes = 1,
.pnmr = PnMR_SPIM_ALP | PnMR_DDDF_16BPP,
.edf = PnDDCR4_EDF_ARGB8888,
}, {
.fourcc = DRM_FORMAT_UYVY,
+ .v4l2 = V4L2_PIX_FMT_UYVY,
.bpp = 16,
.planes = 1,
.pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
.edf = PnDDCR4_EDF_NONE,
}, {
.fourcc = DRM_FORMAT_YUYV,
+ .v4l2 = V4L2_PIX_FMT_YUYV,
.bpp = 16,
.planes = 1,
.pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
.edf = PnDDCR4_EDF_NONE,
}, {
.fourcc = DRM_FORMAT_NV12,
+ .v4l2 = V4L2_PIX_FMT_NV12M,
.bpp = 12,
.planes = 2,
.pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
.edf = PnDDCR4_EDF_NONE,
}, {
.fourcc = DRM_FORMAT_NV21,
+ .v4l2 = V4L2_PIX_FMT_NV21M,
.bpp = 12,
.planes = 2,
.pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
.edf = PnDDCR4_EDF_NONE,
}, {
.fourcc = DRM_FORMAT_NV16,
+ .v4l2 = V4L2_PIX_FMT_NV16M,
.bpp = 16,
.planes = 2,
.pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
@@ -97,62 +107,77 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
*/
{
.fourcc = DRM_FORMAT_RGB332,
+ .v4l2 = V4L2_PIX_FMT_RGB332,
.bpp = 8,
.planes = 1,
}, {
.fourcc = DRM_FORMAT_ARGB4444,
+ .v4l2 = V4L2_PIX_FMT_ARGB444,
.bpp = 16,
.planes = 1,
}, {
.fourcc = DRM_FORMAT_XRGB4444,
+ .v4l2 = V4L2_PIX_FMT_XRGB444,
.bpp = 16,
.planes = 1,
}, {
.fourcc = DRM_FORMAT_BGR888,
+ .v4l2 = V4L2_PIX_FMT_RGB24,
.bpp = 24,
.planes = 1,
}, {
.fourcc = DRM_FORMAT_RGB888,
+ .v4l2 = V4L2_PIX_FMT_BGR24,
.bpp = 24,
.planes = 1,
}, {
.fourcc = DRM_FORMAT_BGRA8888,
+ .v4l2 = V4L2_PIX_FMT_ARGB32,
.bpp = 32,
.planes = 1,
}, {
.fourcc = DRM_FORMAT_BGRX8888,
+ .v4l2 = V4L2_PIX_FMT_XRGB32,
.bpp = 32,
.planes = 1,
}, {
.fourcc = DRM_FORMAT_YVYU,
+ .v4l2 = V4L2_PIX_FMT_YVYU,
.bpp = 16,
.planes = 1,
}, {
.fourcc = DRM_FORMAT_NV61,
+ .v4l2 = V4L2_PIX_FMT_NV61M,
.bpp = 16,
.planes = 2,
}, {
.fourcc = DRM_FORMAT_YUV420,
+ .v4l2 = V4L2_PIX_FMT_YUV420M,
.bpp = 12,
.planes = 3,
}, {
.fourcc = DRM_FORMAT_YVU420,
+ .v4l2 = V4L2_PIX_FMT_YVU420M,
.bpp = 12,
.planes = 3,
}, {
.fourcc = DRM_FORMAT_YUV422,
+ .v4l2 = V4L2_PIX_FMT_YUV422M,
.bpp = 16,
.planes = 3,
}, {
.fourcc = DRM_FORMAT_YVU422,
+ .v4l2 = V4L2_PIX_FMT_YVU422M,
.bpp = 16,
.planes = 3,
}, {
.fourcc = DRM_FORMAT_YUV444,
+ .v4l2 = V4L2_PIX_FMT_YUV444M,
.bpp = 24,
.planes = 3,
}, {
.fourcc = DRM_FORMAT_YVU444,
+ .v4l2 = V4L2_PIX_FMT_YVU444M,
.bpp = 24,
.planes = 3,
},
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.h b/drivers/gpu/drm/rcar-du/rcar_du_kms.h
index e171527abdaa..0346504d8c59 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.h
@@ -19,6 +19,7 @@ struct rcar_du_device;
struct rcar_du_format_info {
u32 fourcc;
+ u32 v4l2;
unsigned int bpp;
unsigned int planes;
unsigned int pnmr;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 28bfeb8c24fb..29a08f7b0761 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -109,8 +109,7 @@ void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
}
-/* Keep the two tables in sync. */
-static const u32 formats_kms[] = {
+static const u32 rcar_du_vsp_formats[] = {
DRM_FORMAT_RGB332,
DRM_FORMAT_ARGB4444,
DRM_FORMAT_XRGB4444,
@@ -138,40 +137,13 @@ static const u32 formats_kms[] = {
DRM_FORMAT_YVU444,
};
-static const u32 formats_v4l2[] = {
- V4L2_PIX_FMT_RGB332,
- V4L2_PIX_FMT_ARGB444,
- V4L2_PIX_FMT_XRGB444,
- V4L2_PIX_FMT_ARGB555,
- V4L2_PIX_FMT_XRGB555,
- V4L2_PIX_FMT_RGB565,
- V4L2_PIX_FMT_RGB24,
- V4L2_PIX_FMT_BGR24,
- V4L2_PIX_FMT_ARGB32,
- V4L2_PIX_FMT_XRGB32,
- V4L2_PIX_FMT_ABGR32,
- V4L2_PIX_FMT_XBGR32,
- V4L2_PIX_FMT_UYVY,
- V4L2_PIX_FMT_YUYV,
- V4L2_PIX_FMT_YVYU,
- V4L2_PIX_FMT_NV12M,
- V4L2_PIX_FMT_NV21M,
- V4L2_PIX_FMT_NV16M,
- V4L2_PIX_FMT_NV61M,
- V4L2_PIX_FMT_YUV420M,
- V4L2_PIX_FMT_YVU420M,
- V4L2_PIX_FMT_YUV422M,
- V4L2_PIX_FMT_YVU422M,
- V4L2_PIX_FMT_YUV444M,
- V4L2_PIX_FMT_YVU444M,
-};
-
static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
{
struct rcar_du_vsp_plane_state *state =
to_rcar_vsp_plane_state(plane->plane.state);
struct rcar_du_crtc *crtc = to_rcar_crtc(state->state.crtc);
struct drm_framebuffer *fb = plane->plane.state->fb;
+ const struct rcar_du_format_info *format;
struct vsp1_du_atomic_config cfg = {
.pixelformat = 0,
.pitch = fb->pitches[0],
@@ -194,12 +166,8 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
cfg.mem[i] = sg_dma_address(state->sg_tables[i].sgl)
+ fb->offsets[i];
- for (i = 0; i < ARRAY_SIZE(formats_kms); ++i) {
- if (formats_kms[i] == state->format->fourcc) {
- cfg.pixelformat = formats_v4l2[i];
- break;
- }
- }
+ format = rcar_du_format_info(state->format->fourcc);
+ cfg.pixelformat = format->v4l2;
vsp1_du_atomic_update(plane->vsp->vsp, crtc->vsp_pipe,
plane->index, &cfg);
@@ -394,8 +362,8 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs,
&rcar_du_vsp_plane_funcs,
- formats_kms,
- ARRAY_SIZE(formats_kms),
+ rcar_du_vsp_formats,
+ ARRAY_SIZE(rcar_du_vsp_formats),
NULL, type, NULL);
if (ret < 0)
return ret;
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 17/18] drm: rcar-du: vsp: Extract framebuffer (un)mapping to separate functions
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (15 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 16/18] drm: rcar-du: Store V4L2 fourcc in rcar_du_format_info structure Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 11:54 ` Kieran Bingham
2019-03-13 0:05 ` [PATCH v6 18/18] drm: rcar-du: Add writeback support for R-Car Gen3 Laurent Pinchart
17 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
The rcar_du_vsp_plane_prepare_fb() and rcar_du_vsp_plane_cleanup_fb()
functions implement the DRM plane .prepare_fb() and .cleanup_fb()
operations. They map and unmap the framebuffer to/from the VSP
internally, which will be useful to implement writeback support. Split
the mapping and unmapping out to separate functions.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 68 ++++++++++++++++-----------
drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 17 +++++++
2 files changed, 58 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 29a08f7b0761..0806a69c4679 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -173,26 +173,16 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
plane->index, &cfg);
}
-static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
- struct drm_plane_state *state)
+int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
+ struct sg_table sg_tables[3])
{
- struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
- struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
struct rcar_du_device *rcdu = vsp->dev;
unsigned int i;
int ret;
- /*
- * There's no need to prepare (and unprepare) the framebuffer when the
- * plane is not visible, as it will not be displayed.
- */
- if (!state->visible)
- return 0;
-
- for (i = 0; i < rstate->format->planes; ++i) {
- struct drm_gem_cma_object *gem =
- drm_fb_cma_get_gem_obj(state->fb, i);
- struct sg_table *sgt = &rstate->sg_tables[i];
+ for (i = 0; i < fb->format->num_planes; ++i) {
+ struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(fb, i);
+ struct sg_table *sgt = &sg_tables[i];
ret = dma_get_sgtable(rcdu->dev, sgt, gem->vaddr, gem->paddr,
gem->base.size);
@@ -207,15 +197,11 @@ static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
}
}
- ret = drm_gem_fb_prepare_fb(plane, state);
- if (ret)
- goto fail;
-
return 0;
fail:
while (i--) {
- struct sg_table *sgt = &rstate->sg_tables[i];
+ struct sg_table *sgt = &sg_tables[i];
vsp1_du_unmap_sg(vsp->vsp, sgt);
sg_free_table(sgt);
@@ -224,22 +210,50 @@ static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
return ret;
}
+static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
+ struct drm_plane_state *state)
+{
+ struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
+ struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
+ int ret;
+
+ /*
+ * There's no need to prepare (and unprepare) the framebuffer when the
+ * plane is not visible, as it will not be displayed.
+ */
+ if (!state->visible)
+ return 0;
+
+ ret = rcar_du_vsp_map_fb(vsp, state->fb, rstate->sg_tables);
+ if (ret < 0)
+ return ret;
+
+ return drm_gem_fb_prepare_fb(plane, state);
+}
+
+void rcar_du_vsp_unmap_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
+ struct sg_table sg_tables[3])
+{
+ unsigned int i;
+
+ for (i = 0; i < fb->format->num_planes; ++i) {
+ struct sg_table *sgt = &sg_tables[i];
+
+ vsp1_du_unmap_sg(vsp->vsp, sgt);
+ sg_free_table(sgt);
+ }
+}
+
static void rcar_du_vsp_plane_cleanup_fb(struct drm_plane *plane,
struct drm_plane_state *state)
{
struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
- unsigned int i;
if (!state->visible)
return;
- for (i = 0; i < rstate->format->planes; ++i) {
- struct sg_table *sgt = &rstate->sg_tables[i];
-
- vsp1_du_unmap_sg(vsp->vsp, sgt);
- sg_free_table(sgt);
- }
+ rcar_du_vsp_unmap_fb(vsp, state->fb, rstate->sg_tables);
}
static int rcar_du_vsp_plane_atomic_check(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
index db232037f24a..9b4724159378 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
@@ -12,8 +12,10 @@
#include <drm/drm_plane.h>
+struct drm_framebuffer;
struct rcar_du_format_info;
struct rcar_du_vsp;
+struct sg_table;
struct rcar_du_vsp_plane {
struct drm_plane plane;
@@ -60,6 +62,10 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc);
void rcar_du_vsp_disable(struct rcar_du_crtc *crtc);
void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc);
+int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
+ struct sg_table sg_tables[3]);
+void rcar_du_vsp_unmap_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
+ struct sg_table sg_tables[3]);
#else
static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp,
struct device_node *np,
@@ -71,6 +77,17 @@ static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };
static inline void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc) { };
+static inline int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp,
+ struct drm_framebuffer *fb,
+ struct sg_table sg_tables[3])
+{
+ return -ENXIO;
+}
+static inline void rcar_du_vsp_unmap_fb(struct rcar_du_vsp *vsp,
+ struct drm_framebuffer *fb,
+ struct sg_table sg_tables[3])
+{
+}
#endif
#endif /* __RCAR_DU_VSP_H__ */
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 17/18] drm: rcar-du: vsp: Extract framebuffer (un)mapping to separate functions
2019-03-13 0:05 ` [PATCH v6 17/18] drm: rcar-du: vsp: Extract framebuffer (un)mapping to separate functions Laurent Pinchart
@ 2019-03-13 11:54 ` Kieran Bingham
0 siblings, 0 replies; 36+ messages in thread
From: Kieran Bingham @ 2019-03-13 11:54 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc, Liviu Dudau, linux-media
Hi Laurent,
On 13/03/2019 00:05, Laurent Pinchart wrote:
> The rcar_du_vsp_plane_prepare_fb() and rcar_du_vsp_plane_cleanup_fb()
> functions implement the DRM plane .prepare_fb() and .cleanup_fb()
> operations. They map and unmap the framebuffer to/from the VSP
> internally, which will be useful to implement writeback support. Split
> the mapping and unmapping out to separate functions.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Looks good to me, Just a refactor and nothing controversial.
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 68 ++++++++++++++++-----------
> drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 17 +++++++
> 2 files changed, 58 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 29a08f7b0761..0806a69c4679 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -173,26 +173,16 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
> plane->index, &cfg);
> }
>
> -static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> - struct drm_plane_state *state)
> +int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
> + struct sg_table sg_tables[3])
> {
> - struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
> - struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> struct rcar_du_device *rcdu = vsp->dev;
> unsigned int i;
> int ret;
>
> - /*
> - * There's no need to prepare (and unprepare) the framebuffer when the
> - * plane is not visible, as it will not be displayed.
> - */
> - if (!state->visible)
> - return 0;
> -
> - for (i = 0; i < rstate->format->planes; ++i) {
> - struct drm_gem_cma_object *gem =
> - drm_fb_cma_get_gem_obj(state->fb, i);
> - struct sg_table *sgt = &rstate->sg_tables[i];
> + for (i = 0; i < fb->format->num_planes; ++i) {
> + struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(fb, i);
> + struct sg_table *sgt = &sg_tables[i];
>
> ret = dma_get_sgtable(rcdu->dev, sgt, gem->vaddr, gem->paddr,
> gem->base.size);
> @@ -207,15 +197,11 @@ static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> }
> }
>
> - ret = drm_gem_fb_prepare_fb(plane, state);
> - if (ret)
> - goto fail;
> -
> return 0;
>
> fail:
> while (i--) {
> - struct sg_table *sgt = &rstate->sg_tables[i];
> + struct sg_table *sgt = &sg_tables[i];
>
> vsp1_du_unmap_sg(vsp->vsp, sgt);
> sg_free_table(sgt);
> @@ -224,22 +210,50 @@ static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> return ret;
> }
>
> +static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> + struct drm_plane_state *state)
> +{
> + struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
> + struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> + int ret;
> +
> + /*
> + * There's no need to prepare (and unprepare) the framebuffer when the
> + * plane is not visible, as it will not be displayed.
> + */
> + if (!state->visible)
> + return 0;
What about writeback?
- Never mind - that doesn't call through here.
> +
> + ret = rcar_du_vsp_map_fb(vsp, state->fb, rstate->sg_tables);
> + if (ret < 0)
> + return ret;
> +
> + return drm_gem_fb_prepare_fb(plane, state);
> +}
> +
> +void rcar_du_vsp_unmap_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
> + struct sg_table sg_tables[3])
> +{
> + unsigned int i;
> +
> + for (i = 0; i < fb->format->num_planes; ++i) {
> + struct sg_table *sgt = &sg_tables[i];
> +
> + vsp1_du_unmap_sg(vsp->vsp, sgt);
> + sg_free_table(sgt);
> + }
> +}
> +
> static void rcar_du_vsp_plane_cleanup_fb(struct drm_plane *plane,
> struct drm_plane_state *state)
> {
> struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
> struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> - unsigned int i;
>
> if (!state->visible)
> return;
>
> - for (i = 0; i < rstate->format->planes; ++i) {
> - struct sg_table *sgt = &rstate->sg_tables[i];
> -
> - vsp1_du_unmap_sg(vsp->vsp, sgt);
> - sg_free_table(sgt);
> - }
> + rcar_du_vsp_unmap_fb(vsp, state->fb, rstate->sg_tables);
> }
>
> static int rcar_du_vsp_plane_atomic_check(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> index db232037f24a..9b4724159378 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> @@ -12,8 +12,10 @@
>
> #include <drm/drm_plane.h>
>
> +struct drm_framebuffer;
> struct rcar_du_format_info;
> struct rcar_du_vsp;
> +struct sg_table;
>
> struct rcar_du_vsp_plane {
> struct drm_plane plane;
> @@ -60,6 +62,10 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc);
> void rcar_du_vsp_disable(struct rcar_du_crtc *crtc);
> void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
> void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc);
> +int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
> + struct sg_table sg_tables[3]);
> +void rcar_du_vsp_unmap_fb(struct rcar_du_vsp *vsp, struct drm_framebuffer *fb,
> + struct sg_table sg_tables[3]);
> #else
> static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp,
> struct device_node *np,
> @@ -71,6 +77,17 @@ static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
> static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
> static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };
> static inline void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc) { };
> +static inline int rcar_du_vsp_map_fb(struct rcar_du_vsp *vsp,
> + struct drm_framebuffer *fb,
> + struct sg_table sg_tables[3])
> +{
> + return -ENXIO;
> +}
> +static inline void rcar_du_vsp_unmap_fb(struct rcar_du_vsp *vsp,
> + struct drm_framebuffer *fb,
> + struct sg_table sg_tables[3])
> +{
> +}
> #endif
>
> #endif /* __RCAR_DU_VSP_H__ */
>
--
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 18/18] drm: rcar-du: Add writeback support for R-Car Gen3
2019-03-13 0:05 [PATCH v6 00/18] R-Car DU display writeback support Laurent Pinchart
` (16 preceding siblings ...)
2019-03-13 0:05 ` [PATCH v6 17/18] drm: rcar-du: vsp: Extract framebuffer (un)mapping to separate functions Laurent Pinchart
@ 2019-03-13 0:05 ` Laurent Pinchart
2019-03-13 12:06 ` Kieran Bingham
17 siblings, 1 reply; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 0:05 UTC (permalink / raw)
To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham, Liviu Dudau, linux-media
Implement writeback support for R-Car Gen3 by exposing writeback
connectors. Behind the scene the calls are forwarded to the VSP
backend.
Using writeback connectors will allow implemented writeback support for
R-Car Gen2 with a consistent API if desired.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v5:
- Skip writeback connector when configuring output routing
- Implement writeback connector atomic state operations
---
drivers/gpu/drm/rcar-du/Kconfig | 4 +
drivers/gpu/drm/rcar-du/Makefile | 3 +-
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 7 +-
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 7 +-
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 12 +
drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 5 +
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 243 ++++++++++++++++++++
drivers/gpu/drm/rcar-du/rcar_du_writeback.h | 39 ++++
8 files changed, 317 insertions(+), 3 deletions(-)
create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_writeback.c
create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_writeback.h
diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 7c36e2777a15..1529849e217e 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -36,3 +36,7 @@ config DRM_RCAR_VSP
depends on VIDEO_RENESAS_VSP1=y || (VIDEO_RENESAS_VSP1 && DRM_RCAR_DU=m)
help
Enable support to expose the R-Car VSP Compositor as KMS planes.
+
+config DRM_RCAR_WRITEBACK
+ bool
+ default y if ARM64
diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
index 2a3b8d7972b5..6c2ed9c46467 100644
--- a/drivers/gpu/drm/rcar-du/Makefile
+++ b/drivers/gpu/drm/rcar-du/Makefile
@@ -4,7 +4,7 @@ rcar-du-drm-y := rcar_du_crtc.o \
rcar_du_encoder.o \
rcar_du_group.o \
rcar_du_kms.o \
- rcar_du_plane.o
+ rcar_du_plane.o \
rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
rcar_du_of_lvds_r8a7790.dtb.o \
@@ -13,6 +13,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
rcar_du_of_lvds_r8a7795.dtb.o \
rcar_du_of_lvds_r8a7796.dtb.o
rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
+rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o
obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 96175d48a902..a95cf6bab4e0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -645,8 +645,13 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc,
rstate->outputs = 0;
drm_for_each_encoder_mask(encoder, crtc->dev, state->encoder_mask) {
- struct rcar_du_encoder *renc = to_rcar_encoder(encoder);
+ struct rcar_du_encoder *renc;
+ /* Skip the writeback encoder. */
+ if (encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL)
+ continue;
+
+ renc = to_rcar_encoder(encoder);
rstate->outputs |= BIT(renc->output);
}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index c478953be092..92f7d5f3ff80 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -15,6 +15,7 @@
#include <linux/wait.h>
#include <drm/drm_crtc.h>
+#include <drm/drm_writeback.h>
#include <media/vsp1.h>
@@ -39,6 +40,7 @@ struct rcar_du_vsp;
* @group: CRTC group this CRTC belongs to
* @vsp: VSP feeding video to this CRTC
* @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
+ * @writeback: the writeback connector
*/
struct rcar_du_crtc {
struct drm_crtc crtc;
@@ -65,9 +67,12 @@ struct rcar_du_crtc {
const char *const *sources;
unsigned int sources_count;
+
+ struct drm_writeback_connector writeback;
};
-#define to_rcar_crtc(c) container_of(c, struct rcar_du_crtc, crtc)
+#define to_rcar_crtc(c) container_of(c, struct rcar_du_crtc, crtc)
+#define wb_to_rcar_crtc(c) container_of(c, struct rcar_du_crtc, writeback)
/**
* struct rcar_du_crtc_state - Driver-specific CRTC state
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 999440c7b258..c729f048626e 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -24,6 +24,7 @@
#include "rcar_du_kms.h"
#include "rcar_du_regs.h"
#include "rcar_du_vsp.h"
+#include "rcar_du_writeback.h"
/* -----------------------------------------------------------------------------
* Format helpers
@@ -662,6 +663,17 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
encoder->possible_clones = (1 << num_encoders) - 1;
}
+ /* Create the writeback connectors. */
+ if (rcdu->info->gen >= 3) {
+ for (i = 0; i < rcdu->num_crtcs; ++i) {
+ struct rcar_du_crtc *rcrtc = &rcdu->crtcs[i];
+
+ ret = rcar_du_writeback_init(rcdu, rcrtc);
+ if (ret < 0)
+ return ret;
+ }
+ }
+
/*
* Initialize the default DPAD0 source to the index of the first DU
* channel that can be connected to DPAD0. The exact value doesn't
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 0806a69c4679..99ae03a1713a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -25,6 +25,7 @@
#include "rcar_du_drv.h"
#include "rcar_du_kms.h"
#include "rcar_du_vsp.h"
+#include "rcar_du_writeback.h"
static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
{
@@ -35,6 +36,8 @@ static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
if (status & VSP1_DU_STATUS_COMPLETE)
rcar_du_crtc_finish_page_flip(crtc);
+ if (status & VSP1_DU_STATUS_WRITEBACK)
+ rcar_du_writeback_complete(crtc);
drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
}
@@ -106,6 +109,8 @@ void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
state = to_rcar_crtc_state(crtc->crtc.state);
cfg.crc = state->crc;
+ rcar_du_writeback_atomic_flush(crtc, &cfg.writeback);
+
vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
new file mode 100644
index 000000000000..cfe18e69c1e7
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
@@ -0,0 +1,243 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * rcar_du_writeback.c -- R-Car Display Unit Writeback Support
+ *
+ * Copyright (C) 2019 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ */
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_device.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_writeback.h>
+
+#include "rcar_du_crtc.h"
+#include "rcar_du_drv.h"
+#include "rcar_du_kms.h"
+
+/**
+ * struct rcar_du_wb_conn_state - Driver-specific writeback connector state
+ * @state: base DRM connector state
+ * @format: format of the writeback framebuffer
+ */
+struct rcar_du_wb_conn_state {
+ struct drm_connector_state state;
+ const struct rcar_du_format_info *format;
+};
+
+
+#define to_rcar_wb_conn_state(s) \
+ container_of(s, struct rcar_du_wb_conn_state, state)
+
+/**
+ * struct rcar_du_wb_job - Driver-private data for writeback jobs
+ * @sg_tables: scatter-gather tables for the framebuffer memory
+ */
+struct rcar_du_wb_job {
+ struct sg_table sg_tables[3];
+};
+
+static int rcar_du_wb_conn_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 int rcar_du_wb_prepare_job(struct drm_writeback_connector *connector,
+ struct drm_writeback_job *job)
+{
+ struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
+ struct rcar_du_wb_job *rjob;
+ int ret;
+
+ if (!job->fb)
+ return 0;
+
+ rjob = kzalloc(sizeof(*rjob), GFP_KERNEL);
+ if (!rjob)
+ return -ENOMEM;
+
+ /* Map the framebuffer to the VSP. */
+ ret = rcar_du_vsp_map_fb(rcrtc->vsp, job->fb, rjob->sg_tables);
+ if (ret < 0) {
+ kfree(rjob);
+ return ret;
+ }
+
+ job->priv = rjob;
+ return 0;
+}
+
+static void rcar_du_wb_cleanup_job(struct drm_writeback_connector *connector,
+ struct drm_writeback_job *job)
+{
+ struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
+ struct rcar_du_wb_job *rjob = job->priv;
+
+ if (!job->fb)
+ return;
+
+ rcar_du_vsp_unmap_fb(rcrtc->vsp, job->fb, rjob->sg_tables);
+ kfree(rjob);
+}
+
+static const struct drm_connector_helper_funcs rcar_du_wb_conn_helper_funcs = {
+ .get_modes = rcar_du_wb_conn_get_modes,
+ .prepare_writeback_job = rcar_du_wb_prepare_job,
+ .cleanup_writeback_job = rcar_du_wb_cleanup_job,
+};
+
+static struct drm_connector_state *
+rcar_du_wb_conn_duplicate_state(struct drm_connector *connector)
+{
+ struct rcar_du_wb_conn_state *copy;
+
+ if (WARN_ON(!connector->state))
+ return NULL;
+
+ copy = kzalloc(sizeof(*copy), GFP_KERNEL);
+ if (!copy)
+ return NULL;
+
+ __drm_atomic_helper_connector_duplicate_state(connector, ©->state);
+
+ return ©->state;
+}
+
+static void rcar_du_wb_conn_destroy_state(struct drm_connector *connector,
+ struct drm_connector_state *state)
+{
+ __drm_atomic_helper_connector_destroy_state(state);
+ kfree(to_rcar_wb_conn_state(state));
+}
+
+static void rcar_du_wb_conn_reset(struct drm_connector *connector)
+{
+ struct rcar_du_wb_conn_state *state;
+
+ if (connector->state) {
+ rcar_du_wb_conn_destroy_state(connector, connector->state);
+ connector->state = NULL;
+ }
+
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
+ if (state == NULL)
+ return;
+
+ __drm_atomic_helper_connector_reset(connector, &state->state);
+}
+
+static const struct drm_connector_funcs rcar_du_wb_conn_funcs = {
+ .reset = rcar_du_wb_conn_reset,
+ .fill_modes = drm_helper_probe_single_connector_modes,
+ .destroy = drm_connector_cleanup,
+ .atomic_duplicate_state = rcar_du_wb_conn_duplicate_state,
+ .atomic_destroy_state = rcar_du_wb_conn_destroy_state,
+};
+
+static int rcar_du_wb_enc_atomic_check(struct drm_encoder *encoder,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ struct rcar_du_wb_conn_state *wb_state =
+ to_rcar_wb_conn_state(conn_state);
+ const struct drm_display_mode *mode = &crtc_state->mode;
+ struct drm_device *dev = encoder->dev;
+ struct drm_framebuffer *fb;
+
+ if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
+ return 0;
+
+ fb = conn_state->writeback_job->fb;
+
+ /*
+ * Verify that the framebuffer format is supported and that its size
+ * matches the current mode.
+ */
+ if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
+ dev_dbg(dev->dev, "%s: invalid framebuffer size %ux%u\n",
+ __func__, fb->width, fb->height);
+ return -EINVAL;
+ }
+
+ wb_state->format = rcar_du_format_info(fb->format->format);
+ if (wb_state->format == NULL) {
+ dev_dbg(dev->dev, "%s: unsupported format %08x\n", __func__,
+ fb->format->format);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static const struct drm_encoder_helper_funcs rcar_du_wb_enc_helper_funcs = {
+ .atomic_check = rcar_du_wb_enc_atomic_check,
+};
+
+/*
+ * Only RGB formats are currently supported as the VSP outputs RGB to the DU
+ * and can't convert to YUV separately for writeback.
+ */
+static const u32 writeback_formats[] = {
+ DRM_FORMAT_RGB332,
+ DRM_FORMAT_ARGB4444,
+ DRM_FORMAT_XRGB4444,
+ DRM_FORMAT_ARGB1555,
+ DRM_FORMAT_XRGB1555,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_BGRA8888,
+ DRM_FORMAT_BGRX8888,
+ DRM_FORMAT_ARGB8888,
+ DRM_FORMAT_XRGB8888,
+};
+
+int rcar_du_writeback_init(struct rcar_du_device *rcdu,
+ struct rcar_du_crtc *rcrtc)
+{
+ struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
+
+ wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
+ drm_connector_helper_add(&wb_conn->base,
+ &rcar_du_wb_conn_helper_funcs);
+
+ return drm_writeback_connector_init(rcdu->ddev, wb_conn,
+ &rcar_du_wb_conn_funcs,
+ &rcar_du_wb_enc_helper_funcs,
+ writeback_formats,
+ ARRAY_SIZE(writeback_formats));
+}
+
+void rcar_du_writeback_atomic_flush(struct rcar_du_crtc *rcrtc,
+ struct vsp1_du_writeback_config *cfg)
+{
+ struct rcar_du_wb_conn_state *wb_state;
+ struct drm_connector_state *state;
+ struct rcar_du_wb_job *rjob;
+ struct drm_framebuffer *fb;
+ unsigned int i;
+
+ state = rcrtc->writeback.base.state;
+ if (!state || !state->writeback_job || !state->writeback_job->fb)
+ return;
+
+ fb = state->writeback_job->fb;
+ rjob = state->writeback_job->priv;
+ wb_state = to_rcar_wb_conn_state(state);
+
+ cfg->pixelformat = wb_state->format->v4l2;
+ cfg->pitch = fb->pitches[0];
+
+ for (i = 0; i < wb_state->format->planes; ++i)
+ cfg->mem[i] = sg_dma_address(rjob->sg_tables[i].sgl)
+ + fb->offsets[i];
+
+ drm_writeback_queue_job(&rcrtc->writeback, state);
+}
+
+void rcar_du_writeback_complete(struct rcar_du_crtc *rcrtc)
+{
+ drm_writeback_signal_completion(&rcrtc->writeback, 0);
+}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.h b/drivers/gpu/drm/rcar-du/rcar_du_writeback.h
new file mode 100644
index 000000000000..d57c5a0bffe3
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * rcar_du_writeback.h -- R-Car Display Unit Writeback Support
+ *
+ * Copyright (C) 2019 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ */
+
+#ifndef __RCAR_DU_WRITEBACK_H__
+#define __RCAR_DU_WRITEBACK_H__
+
+#include <drm/drm_plane.h>
+
+struct rcar_du_crtc;
+struct rcar_du_device;
+struct vsp1_du_atomic_pipe_config;
+
+#ifdef CONFIG_DRM_RCAR_WRITEBACK
+int rcar_du_writeback_init(struct rcar_du_device *rcdu,
+ struct rcar_du_crtc *rcrtc);
+void rcar_du_writeback_atomic_flush(struct rcar_du_crtc *rcrtc,
+ struct vsp1_du_writeback_config *cfg);
+void rcar_du_writeback_complete(struct rcar_du_crtc *rcrtc);
+#else
+static inline int rcar_du_writeback_init(struct rcar_du_device *rcdu,
+ struct rcar_du_crtc *rcrtc)
+{
+ return -ENXIO;
+}
+static inline void
+rcar_du_writeback_atomic_flush(struct rcar_du_crtc *rcrtc,
+ struct vsp1_du_writeback_config *cfg)
+{
+}
+static inline void rcar_du_writeback_complete(struct rcar_du_crtc *rcrtc)
+{
+}
+#endif
+
+#endif /* __RCAR_DU_WRITEBACK_H__ */
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 18/18] drm: rcar-du: Add writeback support for R-Car Gen3
2019-03-13 0:05 ` [PATCH v6 18/18] drm: rcar-du: Add writeback support for R-Car Gen3 Laurent Pinchart
@ 2019-03-13 12:06 ` Kieran Bingham
2019-03-13 16:08 ` Laurent Pinchart
0 siblings, 1 reply; 36+ messages in thread
From: Kieran Bingham @ 2019-03-13 12:06 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc, Liviu Dudau, linux-media
Hi Laurent,
On 13/03/2019 00:05, Laurent Pinchart wrote:
> Implement writeback support for R-Car Gen3 by exposing writeback
> connectors. Behind the scene the calls are forwarded to the VSP
> backend.
>
> Using writeback connectors will allow implemented writeback support for
> R-Car Gen2 with a consistent API if desired.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
An extra blank line, and I was a bit concerned about a function naming
below - but other than that:
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> Changes since v5:
>
> - Skip writeback connector when configuring output routing
> - Implement writeback connector atomic state operations
> ---
> drivers/gpu/drm/rcar-du/Kconfig | 4 +
> drivers/gpu/drm/rcar-du/Makefile | 3 +-
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 7 +-
> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 7 +-
> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 12 +
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 5 +
> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 243 ++++++++++++++++++++
> drivers/gpu/drm/rcar-du/rcar_du_writeback.h | 39 ++++
> 8 files changed, 317 insertions(+), 3 deletions(-)
> create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_writeback.h
>
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index 7c36e2777a15..1529849e217e 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -36,3 +36,7 @@ config DRM_RCAR_VSP
> depends on VIDEO_RENESAS_VSP1=y || (VIDEO_RENESAS_VSP1 && DRM_RCAR_DU=m)
> help
> Enable support to expose the R-Car VSP Compositor as KMS planes.
> +
> +config DRM_RCAR_WRITEBACK
> + bool
> + default y if ARM64
> diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> index 2a3b8d7972b5..6c2ed9c46467 100644
> --- a/drivers/gpu/drm/rcar-du/Makefile
> +++ b/drivers/gpu/drm/rcar-du/Makefile
> @@ -4,7 +4,7 @@ rcar-du-drm-y := rcar_du_crtc.o \
> rcar_du_encoder.o \
> rcar_du_group.o \
> rcar_du_kms.o \
> - rcar_du_plane.o
> + rcar_du_plane.o \
>
> rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
> rcar_du_of_lvds_r8a7790.dtb.o \
> @@ -13,6 +13,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \
> rcar_du_of_lvds_r8a7795.dtb.o \
> rcar_du_of_lvds_r8a7796.dtb.o
> rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o
> +rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
>
> obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o
> obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 96175d48a902..a95cf6bab4e0 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -645,8 +645,13 @@ static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc,
> rstate->outputs = 0;
>
> drm_for_each_encoder_mask(encoder, crtc->dev, state->encoder_mask) {
> - struct rcar_du_encoder *renc = to_rcar_encoder(encoder);
> + struct rcar_du_encoder *renc;
>
> + /* Skip the writeback encoder. */
> + if (encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL)
> + continue;
> +
> + renc = to_rcar_encoder(encoder);
> rstate->outputs |= BIT(renc->output);
> }
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index c478953be092..92f7d5f3ff80 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -15,6 +15,7 @@
> #include <linux/wait.h>
>
> #include <drm/drm_crtc.h>
> +#include <drm/drm_writeback.h>
>
> #include <media/vsp1.h>
>
> @@ -39,6 +40,7 @@ struct rcar_du_vsp;
> * @group: CRTC group this CRTC belongs to
> * @vsp: VSP feeding video to this CRTC
> * @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
> + * @writeback: the writeback connector
> */
> struct rcar_du_crtc {
> struct drm_crtc crtc;
> @@ -65,9 +67,12 @@ struct rcar_du_crtc {
>
> const char *const *sources;
> unsigned int sources_count;
> +
> + struct drm_writeback_connector writeback;
> };
>
> -#define to_rcar_crtc(c) container_of(c, struct rcar_du_crtc, crtc)
> +#define to_rcar_crtc(c) container_of(c, struct rcar_du_crtc, crtc)
> +#define wb_to_rcar_crtc(c) container_of(c, struct rcar_du_crtc, writeback)
>
> /**
> * struct rcar_du_crtc_state - Driver-specific CRTC state
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 999440c7b258..c729f048626e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -24,6 +24,7 @@
> #include "rcar_du_kms.h"
> #include "rcar_du_regs.h"
> #include "rcar_du_vsp.h"
> +#include "rcar_du_writeback.h"
>
> /* -----------------------------------------------------------------------------
> * Format helpers
> @@ -662,6 +663,17 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
> encoder->possible_clones = (1 << num_encoders) - 1;
> }
>
> + /* Create the writeback connectors. */
> + if (rcdu->info->gen >= 3) {
> + for (i = 0; i < rcdu->num_crtcs; ++i) {
> + struct rcar_du_crtc *rcrtc = &rcdu->crtcs[i];
> +
> + ret = rcar_du_writeback_init(rcdu, rcrtc);
> + if (ret < 0)
> + return ret;
> + }
> + }
> +
> /*
> * Initialize the default DPAD0 source to the index of the first DU
> * channel that can be connected to DPAD0. The exact value doesn't
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index 0806a69c4679..99ae03a1713a 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -25,6 +25,7 @@
> #include "rcar_du_drv.h"
> #include "rcar_du_kms.h"
> #include "rcar_du_vsp.h"
> +#include "rcar_du_writeback.h"
>
> static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
> {
> @@ -35,6 +36,8 @@ static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
>
> if (status & VSP1_DU_STATUS_COMPLETE)
> rcar_du_crtc_finish_page_flip(crtc);
> + if (status & VSP1_DU_STATUS_WRITEBACK)
> + rcar_du_writeback_complete(crtc);
>
> drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
> }
> @@ -106,6 +109,8 @@ void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
> state = to_rcar_crtc_state(crtc->crtc.state);
> cfg.crc = state->crc;
>
> + rcar_du_writeback_atomic_flush(crtc, &cfg.writeback);
Hrm ...the naming here worries me a bit. This doesn't do the actual
flushing (execution?) of the writeback operation, it just configures the
writeback into the VSP cfg structure. The 'flush' to hardware takes
place in vsp1_du_atomic_flush().
Or maybe it is ok becuase it calls drm_writeback_queue_job() as well as
setting up the cfg...
> +
> vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
> }
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> new file mode 100644
> index 000000000000..cfe18e69c1e7
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> @@ -0,0 +1,243 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * rcar_du_writeback.c -- R-Car Display Unit Writeback Support
> + *
> + * Copyright (C) 2019 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + */
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_writeback.h>
> +
> +#include "rcar_du_crtc.h"
> +#include "rcar_du_drv.h"
> +#include "rcar_du_kms.h"
> +
> +/**
> + * struct rcar_du_wb_conn_state - Driver-specific writeback connector state
> + * @state: base DRM connector state
> + * @format: format of the writeback framebuffer
> + */
> +struct rcar_du_wb_conn_state {
> + struct drm_connector_state state;
> + const struct rcar_du_format_info *format;
> +};
> +
Extra blank line here.
> +
> +#define to_rcar_wb_conn_state(s) \
> + container_of(s, struct rcar_du_wb_conn_state, state)
> +
> +/**
> + * struct rcar_du_wb_job - Driver-private data for writeback jobs
> + * @sg_tables: scatter-gather tables for the framebuffer memory
> + */
> +struct rcar_du_wb_job {
> + struct sg_table sg_tables[3];
> +};
> +
> +static int rcar_du_wb_conn_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 int rcar_du_wb_prepare_job(struct drm_writeback_connector *connector,
> + struct drm_writeback_job *job)
> +{
> + struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
> + struct rcar_du_wb_job *rjob;
> + int ret;
> +
> + if (!job->fb)
> + return 0;
> +
> + rjob = kzalloc(sizeof(*rjob), GFP_KERNEL);
> + if (!rjob)
> + return -ENOMEM;
> +
> + /* Map the framebuffer to the VSP. */
> + ret = rcar_du_vsp_map_fb(rcrtc->vsp, job->fb, rjob->sg_tables);
> + if (ret < 0) {
> + kfree(rjob);
> + return ret;
> + }
> +
> + job->priv = rjob;
> + return 0;
> +}
> +
> +static void rcar_du_wb_cleanup_job(struct drm_writeback_connector *connector,
> + struct drm_writeback_job *job)
> +{
> + struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
> + struct rcar_du_wb_job *rjob = job->priv;
> +
> + if (!job->fb)
> + return;
> +
> + rcar_du_vsp_unmap_fb(rcrtc->vsp, job->fb, rjob->sg_tables);
> + kfree(rjob);
> +}
> +
> +static const struct drm_connector_helper_funcs rcar_du_wb_conn_helper_funcs = {
> + .get_modes = rcar_du_wb_conn_get_modes,
> + .prepare_writeback_job = rcar_du_wb_prepare_job,
> + .cleanup_writeback_job = rcar_du_wb_cleanup_job,
> +};
> +
> +static struct drm_connector_state *
> +rcar_du_wb_conn_duplicate_state(struct drm_connector *connector)
> +{
> + struct rcar_du_wb_conn_state *copy;
> +
> + if (WARN_ON(!connector->state))
> + return NULL;
> +
> + copy = kzalloc(sizeof(*copy), GFP_KERNEL);
> + if (!copy)
> + return NULL;
> +
> + __drm_atomic_helper_connector_duplicate_state(connector, ©->state);
> +
> + return ©->state;
> +}
> +
> +static void rcar_du_wb_conn_destroy_state(struct drm_connector *connector,
> + struct drm_connector_state *state)
> +{
> + __drm_atomic_helper_connector_destroy_state(state);
> + kfree(to_rcar_wb_conn_state(state));
> +}
> +
> +static void rcar_du_wb_conn_reset(struct drm_connector *connector)
> +{
> + struct rcar_du_wb_conn_state *state;
> +
> + if (connector->state) {
> + rcar_du_wb_conn_destroy_state(connector, connector->state);
> + connector->state = NULL;
> + }
> +
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (state == NULL)
> + return;
> +
> + __drm_atomic_helper_connector_reset(connector, &state->state);
> +}
> +
> +static const struct drm_connector_funcs rcar_du_wb_conn_funcs = {
> + .reset = rcar_du_wb_conn_reset,
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy = drm_connector_cleanup,
> + .atomic_duplicate_state = rcar_du_wb_conn_duplicate_state,
> + .atomic_destroy_state = rcar_du_wb_conn_destroy_state,
> +};
> +
> +static int rcar_du_wb_enc_atomic_check(struct drm_encoder *encoder,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct rcar_du_wb_conn_state *wb_state =
> + to_rcar_wb_conn_state(conn_state);
> + const struct drm_display_mode *mode = &crtc_state->mode;
> + struct drm_device *dev = encoder->dev;
> + struct drm_framebuffer *fb;
> +
> + if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> + return 0;
> +
> + fb = conn_state->writeback_job->fb;
> +
> + /*
> + * Verify that the framebuffer format is supported and that its size
> + * matches the current mode.
> + */
> + if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
> + dev_dbg(dev->dev, "%s: invalid framebuffer size %ux%u\n",
> + __func__, fb->width, fb->height);
> + return -EINVAL;
> + }
> +
> + wb_state->format = rcar_du_format_info(fb->format->format);
> + if (wb_state->format == NULL) {
> + dev_dbg(dev->dev, "%s: unsupported format %08x\n", __func__,
> + fb->format->format);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct drm_encoder_helper_funcs rcar_du_wb_enc_helper_funcs = {
> + .atomic_check = rcar_du_wb_enc_atomic_check,
> +};
> +
> +/*
> + * Only RGB formats are currently supported as the VSP outputs RGB to the DU
> + * and can't convert to YUV separately for writeback.
> + */
> +static const u32 writeback_formats[] = {
> + DRM_FORMAT_RGB332,
> + DRM_FORMAT_ARGB4444,
> + DRM_FORMAT_XRGB4444,
> + DRM_FORMAT_ARGB1555,
> + DRM_FORMAT_XRGB1555,
> + DRM_FORMAT_RGB565,
> + DRM_FORMAT_BGR888,
> + DRM_FORMAT_RGB888,
> + DRM_FORMAT_BGRA8888,
> + DRM_FORMAT_BGRX8888,
> + DRM_FORMAT_ARGB8888,
> + DRM_FORMAT_XRGB8888,
> +};
> +
> +int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> + struct rcar_du_crtc *rcrtc)
> +{
> + struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
> +
> + wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> + drm_connector_helper_add(&wb_conn->base,
> + &rcar_du_wb_conn_helper_funcs);
> +
> + return drm_writeback_connector_init(rcdu->ddev, wb_conn,
> + &rcar_du_wb_conn_funcs,
> + &rcar_du_wb_enc_helper_funcs,
> + writeback_formats,
> + ARRAY_SIZE(writeback_formats));
> +}
> +
> +void rcar_du_writeback_atomic_flush(struct rcar_du_crtc *rcrtc,
> + struct vsp1_du_writeback_config *cfg)
> +{
> + struct rcar_du_wb_conn_state *wb_state;
> + struct drm_connector_state *state;
> + struct rcar_du_wb_job *rjob;
> + struct drm_framebuffer *fb;
> + unsigned int i;
> +
> + state = rcrtc->writeback.base.state;
> + if (!state || !state->writeback_job || !state->writeback_job->fb)
> + return;
> +
> + fb = state->writeback_job->fb;
> + rjob = state->writeback_job->priv;
> + wb_state = to_rcar_wb_conn_state(state);
> +
> + cfg->pixelformat = wb_state->format->v4l2;
> + cfg->pitch = fb->pitches[0];
> +
> + for (i = 0; i < wb_state->format->planes; ++i)
> + cfg->mem[i] = sg_dma_address(rjob->sg_tables[i].sgl)
> + + fb->offsets[i];
> +
> + drm_writeback_queue_job(&rcrtc->writeback, state);
> +}
> +
> +void rcar_du_writeback_complete(struct rcar_du_crtc *rcrtc)
> +{
> + drm_writeback_signal_completion(&rcrtc->writeback, 0);
> +}
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.h b/drivers/gpu/drm/rcar-du/rcar_du_writeback.h
> new file mode 100644
> index 000000000000..d57c5a0bffe3
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * rcar_du_writeback.h -- R-Car Display Unit Writeback Support
> + *
> + * Copyright (C) 2019 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + */
> +
> +#ifndef __RCAR_DU_WRITEBACK_H__
> +#define __RCAR_DU_WRITEBACK_H__
> +
> +#include <drm/drm_plane.h>
> +
> +struct rcar_du_crtc;
> +struct rcar_du_device;
> +struct vsp1_du_atomic_pipe_config;
> +
> +#ifdef CONFIG_DRM_RCAR_WRITEBACK
> +int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> + struct rcar_du_crtc *rcrtc);
> +void rcar_du_writeback_atomic_flush(struct rcar_du_crtc *rcrtc,
> + struct vsp1_du_writeback_config *cfg);
> +void rcar_du_writeback_complete(struct rcar_du_crtc *rcrtc);
> +#else
> +static inline int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> + struct rcar_du_crtc *rcrtc)
> +{
> + return -ENXIO;
> +}
> +static inline void
> +rcar_du_writeback_atomic_flush(struct rcar_du_crtc *rcrtc,
> + struct vsp1_du_writeback_config *cfg)
> +{
> +}
> +static inline void rcar_du_writeback_complete(struct rcar_du_crtc *rcrtc)
> +{
> +}
> +#endif
> +
> +#endif /* __RCAR_DU_WRITEBACK_H__ */
>
--
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 18/18] drm: rcar-du: Add writeback support for R-Car Gen3
2019-03-13 12:06 ` Kieran Bingham
@ 2019-03-13 16:08 ` Laurent Pinchart
0 siblings, 0 replies; 36+ messages in thread
From: Laurent Pinchart @ 2019-03-13 16:08 UTC (permalink / raw)
To: Kieran Bingham
Cc: Laurent Pinchart, Liviu Dudau, dri-devel, linux-renesas-soc, linux-media
Hi Kieran,
On Wed, Mar 13, 2019 at 12:06:40PM +0000, Kieran Bingham wrote:
> On 13/03/2019 00:05, Laurent Pinchart wrote:
> > Implement writeback support for R-Car Gen3 by exposing writeback
> > connectors. Behind the scene the calls are forwarded to the VSP
> > backend.
> >
> > Using writeback connectors will allow implemented writeback support for
> > R-Car Gen2 with a consistent API if desired.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> An extra blank line, and I was a bit concerned about a function naming
> below - but other than that:
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> > ---
> > Changes since v5:
> >
> > - Skip writeback connector when configuring output routing
> > - Implement writeback connector atomic state operations
> > ---
> > drivers/gpu/drm/rcar-du/Kconfig | 4 +
> > drivers/gpu/drm/rcar-du/Makefile | 3 +-
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 7 +-
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 7 +-
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 12 +
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 5 +
> > drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 243 ++++++++++++++++++++
> > drivers/gpu/drm/rcar-du/rcar_du_writeback.h | 39 ++++
> > 8 files changed, 317 insertions(+), 3 deletions(-)
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_writeback.h
[snip]
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > index 0806a69c4679..99ae03a1713a 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > @@ -25,6 +25,7 @@
> > #include "rcar_du_drv.h"
> > #include "rcar_du_kms.h"
> > #include "rcar_du_vsp.h"
> > +#include "rcar_du_writeback.h"
> >
> > static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
> > {
> > @@ -35,6 +36,8 @@ static void rcar_du_vsp_complete(void *private, unsigned int status, u32 crc)
> >
> > if (status & VSP1_DU_STATUS_COMPLETE)
> > rcar_du_crtc_finish_page_flip(crtc);
> > + if (status & VSP1_DU_STATUS_WRITEBACK)
> > + rcar_du_writeback_complete(crtc);
> >
> > drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
> > }
> > @@ -106,6 +109,8 @@ void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
> > state = to_rcar_crtc_state(crtc->crtc.state);
> > cfg.crc = state->crc;
> >
> > + rcar_du_writeback_atomic_flush(crtc, &cfg.writeback);
>
> Hrm ...the naming here worries me a bit. This doesn't do the actual
> flushing (execution?) of the writeback operation, it just configures the
> writeback into the VSP cfg structure. The 'flush' to hardware takes
> place in vsp1_du_atomic_flush().
>
> Or maybe it is ok becuase it calls drm_writeback_queue_job() as well as
> setting up the cfg...
You've got a point. I've renamed the function to
rcar_du_writeback_setup().
> > +
> > vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
> > }
> >
[snip]
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 36+ messages in thread