Hi,
On Wed, 2018-06-13 at 16:07 +0200, Maxime Ripard wrote:
> During our frame decoding setup, we need to check a number of controls to
> make sure that they are properly filled before trying to access them.
>
> It's not too bad with MPEG2 since there's just a single one, but with the
> upcoming increase of codecs, and the integration of more complex codecs,
> this logic will be duplicated a significant number of times. H264 for
> example uses 4 different controls.
>
> Add a macro that expands to the proper check in order to reduce the
> duplication.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> ---
> .../platform/sunxi/cedrus/sunxi_cedrus_dec.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.c b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.c
> index 8c92af34ebeb..c19acf9626c4 100644
> --- a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.c
> +++ b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.c
> @@ -110,15 +110,16 @@ void sunxi_cedrus_device_run(void *priv)
>
> spin_lock_irqsave(&ctx->dev->irq_lock, flags);
>
> +#define CHECK_CONTROL(ctx, ctrl) \
> + if (!ctx->ctrls[(ctrl)]) { \
Although this was not introduced in this patch, I believe this approach
won't work since ctx->ctrls[i] is a pointer returned from
v4l2_ctrl_new_custom(hdl, &cfg, NULL), which will always be non-null
after calling cedrus_init_ctrls.
Perhaps checking ctx->ctrls[i].p_cur.p would be the right thing to
check, but I'm unsure about that.
> + v4l2_err(&(ctx)->dev->v4l2_dev, "Invalid " #ctrl " control\n"); \
> + (ctx)->job_abort = 1; \
> + goto unlock_complete; \
> + }
> +
> switch (ctx->vpu_src_fmt->fourcc) {
> case V4L2_PIX_FMT_MPEG2_FRAME:
> - if (!ctx->ctrls[SUNXI_CEDRUS_CTRL_DEC_MPEG2_FRAME_HDR]) {
> - v4l2_err(&ctx->dev->v4l2_dev,
> - "Invalid MPEG2 frame header control\n");
> - ctx->job_abort = 1;
> - goto unlock_complete;
> - }
> -
> + CHECK_CONTROL(ctx, SUNXI_CEDRUS_CTRL_DEC_MPEG2_FRAME_HDR);
> run.mpeg2.hdr = get_ctrl_ptr(ctx, SUNXI_CEDRUS_CTRL_DEC_MPEG2_FRAME_HDR);
> sunxi_cedrus_mpeg2_setup(ctx, &run);
>
> @@ -128,6 +129,7 @@ void sunxi_cedrus_device_run(void *priv)
> default:
> ctx->job_abort = 1;
> }
Maybe add a newline here?
Cheers,
Paul
> +#undef CHECK_CONTROL
>
> unlock_complete:
> spin_unlock_irqrestore(&ctx->dev->irq_lock, flags);
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.comAttachment:
signature.asc
Description: This is a digitally signed message part
