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@xxxxxxxxxxxxxxxx>
Only a couple of small questions - but nothing to block an RB tag.
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> ---
> 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);
> if (ret < 0) {
> dev_err(vsp1->dev,
> "%s: failed to setup RPF.%u\n",
> @@ -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;
>
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
