Hi Laurent,
On 22/04/18 23:34, Laurent Pinchart wrote:
The DISCOM is used to compute CRCs on display frames. Integrate it in the display pipeline at the output of the blending unit to process output frames.
Computing CRCs on input frames is possible by positioning the DISCOM at a different point in the pipeline. This use case isn't supported at the moment and could be implemented by extending the API between the VSP1 and DU drivers if needed.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Only a couple of small questions - but nothing to block an RB tag.
Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
drivers/media/platform/vsp1/vsp1_drm.c | 115 ++++++++++++++++++++++++++++++++- drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++ 2 files changed, 124 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index 5fc31578f9b0..7864b43a90e1 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -22,6 +22,7 @@ #include "vsp1_lif.h" #include "vsp1_pipe.h" #include "vsp1_rwpf.h" +#include "vsp1_uif.h"
#define BRX_NAME(e) (e)->type == VSP1_ENTITY_BRU ? "BRU" : "BRS"
@@ -35,8 +36,13 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe, struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe); bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
- if (drm_pipe->du_complete)
drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
if (drm_pipe->du_complete) {
struct vsp1_entity *uif = drm_pipe->uif;u32 crc;crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;drm_pipe->du_complete(drm_pipe->du_private, complete, crc);}
if (completion & VSP1_DL_FRAME_END_INTERNAL) { drm_pipe->force_brx_release = false;
@@ -48,10 +54,65 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
- Pipeline Configuration
*/
+/*
- Insert the UIF in the pipeline between the prev and next entities. If no UIF
- is available connect the two entities directly.
- */
+static int vsp1_du_insert_uif(struct vsp1_device *vsp1,
struct vsp1_pipeline *pipe,struct vsp1_entity *uif,struct vsp1_entity *prev, unsigned int prev_pad,struct vsp1_entity *next, unsigned int next_pad)+{
- int ret;
- if (uif) {
struct v4l2_subdev_format format;prev->sink = uif;prev->sink_pad = UIF_PAD_SINK;memset(&format, 0, sizeof(format));format.which = V4L2_SUBDEV_FORMAT_ACTIVE;format.pad = prev_pad;ret = v4l2_subdev_call(&prev->subdev, pad, get_fmt, NULL,&format);if (ret < 0)return ret;format.pad = UIF_PAD_SINK;ret = v4l2_subdev_call(&uif->subdev, pad, set_fmt, NULL,&format);if (ret < 0)return ret;dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on UIF sink\n",__func__, format.format.width, format.format.height,format.format.code);/** The UIF doesn't mangle the format between its sink and* source pads, so there is no need to retrieve the format on* its source pad.*/uif->sink = next;uif->sink_pad = next_pad;- } else {
prev->sink = next;prev->sink_pad = next_pad;- }
- return 0;
+}
/* Setup one RPF and the connected BRx sink pad. */ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe, struct vsp1_rwpf *rpf,
struct vsp1_entity *uif, unsigned int brx_input){ struct v4l2_subdev_selection sel; @@ -122,6 +183,12 @@ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1, if (ret < 0) return ret;
- /* Insert and configure the UIF if available. */
- ret = vsp1_du_insert_uif(vsp1, pipe, uif, &rpf->entity, RWPF_PAD_SOURCE,
pipe->brx, brx_input);- if (ret < 0)
return ret;- /* BRx sink, propagate the format from the RPF source. */ format.pad = brx_input;
@@ -297,7 +364,10 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1, struct vsp1_rwpf *rpf) static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe) {
- struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe); struct vsp1_rwpf *inputs[VSP1_MAX_RPF] = { NULL, };
- struct vsp1_entity *uif;
- bool use_uif = false; struct vsp1_brx *brx; unsigned int i; int ret;
@@ -358,7 +428,11 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, dev_dbg(vsp1->dev, "%s: connecting RPF.%u to %s:%u\n", __func__, rpf->entity.index, BRX_NAME(pipe->brx), i);
ret = vsp1_du_pipeline_setup_rpf(vsp1, pipe, rpf, i);
uif = drm_pipe->crc.source == VSP1_DU_CRC_PLANE &&drm_pipe->crc.index == i ? drm_pipe->uif : NULL;if (uif)use_uif = true;ret = vsp1_du_pipeline_setup_rpf(vsp1, pipe, rpf, uif, i);@@ -367,6 +441,31 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1, } }
- /* Insert and configure the UIF at the BRx output if available. */
- uif = drm_pipe->crc.source == VSP1_DU_CRC_OUTPUT ? drm_pipe->uif : NULL;
- if (uif)
use_uif = true;- ret = vsp1_du_insert_uif(vsp1, pipe, uif,
pipe->brx, pipe->brx->source_pad,&pipe->output->entity, 0);- if (ret < 0)
dev_err(vsp1->dev, "%s: failed to setup UIF after %s\n",__func__, BRX_NAME(pipe->brx));- /*
* If the UIF is not in use schedule it for removal by setting its pipe* pointer to NULL, vsp1_du_pipeline_configure() will remove it from the* hardware pipeline and from the pipeline's list of entities. Otherwise* make sure it is present in the pipeline's list of entities if it* wasn't already.*/- if (!use_uif) {
Do we need use_uif here? Wouldn't uif suffice? - Oh - no it wouldn't. A UIF at RPF would get overwritten by the lack of UIF at BRx.
Nothing to see here. Move along...
drm_pipe->uif->pipe = NULL;- } else if (!drm_pipe->uif->pipe) {
drm_pipe->uif->pipe = pipe;list_add_tail(&drm_pipe->uif->list_pipe, &pipe->entities);- }
- return 0;
}
@@ -748,6 +847,9 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index, struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index]; struct vsp1_pipeline *pipe = &drm_pipe->pipe;
- drm_pipe->crc.source = cfg->crc.source;
- drm_pipe->crc.index = cfg->crc.index;
I think this could be shortened to
drm_pipe->crc = cfg->crc;
Or is that a GCC extension. Either way, it's just a matter of taste, and you might prefer to be more explicit.
- vsp1_du_pipeline_setup_inputs(vsp1, pipe); vsp1_du_pipeline_configure(pipe); mutex_unlock(&vsp1->drm->lock);
@@ -816,6 +918,13 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
pipe->lif->pipe = pipe; list_add_tail(&pipe->lif->list_pipe, &pipe->entities);
/** CRC computation is initially disabled, don't add the UIF to* the pipeline.*/if (i < vsp1->info->uif_count)drm_pipe->uif = &vsp1->uif[i]->entity;}
/* Disable all RPFs initially. */
diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h index e5b88b28806c..1e7670955ef0 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.h +++ b/drivers/media/platform/vsp1/vsp1_drm.h @@ -13,6 +13,8 @@ #include <linux/videodev2.h> #include <linux/wait.h>
+#include <media/vsp1.h>
#include "vsp1_pipe.h"
/** @@ -22,6 +24,9 @@
- @height: output display height
- @force_brx_release: when set, release the BRx during the next reconfiguration
- @wait_queue: wait queue to wait for BRx release completion
- @uif: UIF entity if available for the pipeline
- @crc.source: source for CRC calculation
*/
- @crc.index: index of the CRC source plane (when crc.source is set to plane)
- @du_complete: frame completion callback for the DU driver (optional)
- @du_private: data to be passed to the du_complete callback
@@ -34,6 +39,13 @@ struct vsp1_drm_pipeline { bool force_brx_release; wait_queue_head_t wait_queue;
- struct vsp1_entity *uif;
- struct {
enum vsp1_du_crc_source source;unsigned int index;- } crc;
Does this have to be duplicated ? Or can it be included from the API header...
- /* Frame synchronisation */ void (*du_complete)(void *data, bool completed, u32 crc); void *du_private;