| Message ID | 20180422223430.16407-8-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
HI Laurent, On Mon, Apr 23, 2018 at 01:34:29AM +0300, 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> > --- > 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; Isn't the !uif case better handled in the caller? See below... Or otherwise, if you prefer handling it here, shouldn't the indentation be reduced with if (!uif) { prev->sink = next; prev->sink_pad = next_pad; return 0; } > + } > + > + 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) { ... here. If you don't use uif, don't call vspi1_du_insert_uif(). True, you have to link entities explicitly here if there is not uif, but I may be missing where this was happening before this code was added. Thanks j > + 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; > + > 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) Why 'initially disabled'? This seems to me to conditionally enable the UIF unit. Or is the lif count always the same as the uif count? Thanks j > + 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; > + > /* Frame synchronisation */ > void (*du_complete)(void *data, bool completed, u32 crc); > void *du_private; > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Saturday, 28 April 2018 14:00:26 EEST jacopo mondi wrote: > On Mon, Apr 23, 2018 at 01:34:29AM +0300, 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> > > --- > > > > 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 [snip] > > @@ -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; > > Isn't the !uif case better handled in the caller? See below... > Or otherwise, if you prefer handling it here, shouldn't the > indentation be reduced with I don't think it's better handled in the caller, I prefer keeping the entities linking code in a single place. I'll reduce the indentation. > if (!uif) { > prev->sink = next; > prev->sink_pad = next_pad; > return 0; > } > > > + } > > + > > + return 0; > > +} [snip] > > @@ -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) { > > ... here. If you don't use uif, don't call vspi1_du_insert_uif(). > True, you have to link entities explicitly here if there is not uif, > but I may be missing where this was happening before this code was > added. > > > + 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; > > + > > 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) > > Why 'initially disabled'? This seems to me to conditionally enable the > UIF unit. Or is the lif count always the same as the uif count? Setting drm_pipe->uif isn't adding the UIF to the pipeline, that would be done by adding it to the pipe->entities list and setting the uif->pipe pointer. > > + drm_pipe->uif = &vsp1->uif[i]->entity; > > }
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); > 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; >
Hi Kieran, On Saturday, 28 April 2018 21:58:53 EEST Kieran Bingham wrote: > 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 [snip] > > @@ -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. That's what I had written in the first place, only to have gcc throwing an error because the crc fields are anonymous structures. > > + > > vsp1_du_pipeline_setup_inputs(vsp1, pipe); > > vsp1_du_pipeline_configure(pipe); > > mutex_unlock(&vsp1->drm->lock); [snip] > > 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... It's a good point. I thought it might not be worth it just for two fields, but I see no compelling reason against it. I'll introduce a new structure. > > + > > /* Frame synchronisation */ > > void (*du_complete)(void *data, bool completed, u32 crc); > > void *du_private;
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) { + 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; + 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; + /* Frame synchronisation */ void (*du_complete)(void *data, bool completed, u32 crc); void *du_private;
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> --- drivers/media/platform/vsp1/vsp1_drm.c | 115 ++++++++++++++++++++++++++++++++- drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++ 2 files changed, 124 insertions(+), 3 deletions(-)