Re: [PATCH 6/9] media: cedrus: Add ops structure

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 10462 bytes --]

Hi,

On Wed, 2018-06-13 at 16:07 +0200, Maxime Ripard wrote:
> In order to increase the number of codecs supported, we need to decouple
> the MPEG2 only code that was there up until now and turn it into something
> a bit more generic.
> 
> Do that by introducing an intermediate ops structure that would need to be
> filled by each supported codec. Start by implementing in that structure the
> setup and trigger hooks that are currently the only functions being
> implemented by codecs support.
> 
> To do so, we need to store the current codec in use, which we do at
> start_streaming time.

With the comments below taken in account, this is:

Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  .../platform/sunxi/cedrus/sunxi_cedrus.c      |  2 ++
>  .../sunxi/cedrus/sunxi_cedrus_common.h        | 11 +++++++
>  .../platform/sunxi/cedrus/sunxi_cedrus_dec.c  | 10 +++---
>  .../sunxi/cedrus/sunxi_cedrus_mpeg2.c         | 11 +++++--
>  .../sunxi/cedrus/sunxi_cedrus_mpeg2.h         | 33 -------------------
>  .../sunxi/cedrus/sunxi_cedrus_video.c         | 17 +++++++++-
>  6 files changed, 42 insertions(+), 42 deletions(-)
>  delete mode 100644 drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.h
> 
> diff --git a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus.c b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus.c
> index ccd41d9a3e41..bc80480f5dfd 100644
> --- a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus.c
> +++ b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus.c
> @@ -244,6 +244,8 @@ static int sunxi_cedrus_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	dev->dec_ops[SUNXI_CEDRUS_CODEC_MPEG2] = &sunxi_cedrus_dec_ops_mpeg2;
> +
>  	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
>  	if (ret)
>  		goto unreg_media;
> diff --git a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_common.h b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_common.h
> index a5f83c452006..c2e2c92d103b 100644
> --- a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_common.h
> +++ b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_common.h
> @@ -75,6 +75,7 @@ struct sunxi_cedrus_ctx {
>  	struct v4l2_pix_format_mplane src_fmt;
>  	struct sunxi_cedrus_fmt *vpu_dst_fmt;
>  	struct v4l2_pix_format_mplane dst_fmt;
> +	enum sunxi_cedrus_codec current_codec;

Nit: for consistency with the way things are named, "codec_current"
probably makes more sense.

IMO using the natural English order is fine for temporary variables, but
 less so for variables used in common parts like structures. This allows
seeing "_" as a logical hierarchical delimiter that automatically makes
us end up with consistent prefixes that can easily be grepped for and
derived.

But that's just my 2 cents, it's really not a big deal, especially in
this case!

>  	struct v4l2_ctrl_handler hdl;
>  	struct v4l2_ctrl *ctrls[SUNXI_CEDRUS_CTRL_MAX];
> @@ -107,6 +108,14 @@ struct sunxi_cedrus_buffer *vb2_to_cedrus_buffer(const struct vb2_buffer *p)
>  	return vb2_v4l2_to_cedrus_buffer(to_vb2_v4l2_buffer(p));
>  }
>  
> +struct sunxi_cedrus_dec_ops {
> +	void (*setup)(struct sunxi_cedrus_ctx *ctx,
> +		      struct sunxi_cedrus_run *run);
> +	void (*trigger)(struct sunxi_cedrus_ctx *ctx);

By the way, are we sure that these functions won't ever fail?
I think this is the case for MPEG2 (there is virtually nothing to check
for errors) but perhaps it's different for H264.

> +};
> +
> +extern struct sunxi_cedrus_dec_ops sunxi_cedrus_dec_ops_mpeg2;
> +
>  struct sunxi_cedrus_dev {
>  	struct v4l2_device v4l2_dev;
>  	struct video_device vfd;
> @@ -130,6 +139,8 @@ struct sunxi_cedrus_dev {
>  	struct reset_control *rstc;
>  
>  	struct regmap *syscon;
> +
> +	struct sunxi_cedrus_dec_ops	*dec_ops[SUNXI_CEDRUS_CODEC_LAST];
>  };
>  
>  static inline void sunxi_cedrus_write(struct sunxi_cedrus_dev *dev,
> diff --git a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.c b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.c
> index f274408ab5a7..5e552fa05274 100644
> --- a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.c
> +++ b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_dec.c
> @@ -28,7 +28,6 @@
>  #include <media/v4l2-mem2mem.h>
>  
>  #include "sunxi_cedrus_common.h"
> -#include "sunxi_cedrus_mpeg2.h"
>  #include "sunxi_cedrus_dec.h"
>  #include "sunxi_cedrus_hw.h"
>  
> @@ -77,6 +76,7 @@ void sunxi_cedrus_device_work(struct work_struct *work)
>  void sunxi_cedrus_device_run(void *priv)
>  {
>  	struct sunxi_cedrus_ctx *ctx = priv;
> +	struct sunxi_cedrus_dev *dev = ctx->dev;
>  	struct sunxi_cedrus_run run = { 0 };
>  	struct media_request *src_req, *dst_req;
>  	unsigned long flags;
> @@ -120,8 +120,6 @@ void sunxi_cedrus_device_run(void *priv)
>  	case V4L2_PIX_FMT_MPEG2_FRAME:
>  		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);
> -
>  		break;
>  
>  	default:
> @@ -129,6 +127,9 @@ void sunxi_cedrus_device_run(void *priv)
>  	}
>  #undef CHECK_CONTROL
>  
> +	if (!ctx->job_abort)
> +		dev->dec_ops[ctx->current_codec]->setup(ctx, &run);
> +
>  unlock_complete:
>  	spin_unlock_irqrestore(&ctx->dev->irq_lock, flags);
>  
> @@ -143,8 +144,7 @@ void sunxi_cedrus_device_run(void *priv)
>  	spin_lock_irqsave(&ctx->dev->irq_lock, flags);
>  
>  	if (!ctx->job_abort) {
> -		if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_MPEG2_FRAME)
> -			sunxi_cedrus_mpeg2_trigger(ctx);
> +		dev->dec_ops[ctx->current_codec]->trigger(ctx);
>  	} else {
>  		v4l2_m2m_buf_done(run.src, VB2_BUF_STATE_ERROR);
>  		v4l2_m2m_buf_done(run.dst, VB2_BUF_STATE_ERROR);
> diff --git a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.c b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.c
> index d1d7a3cfce0d..e25075bb5779 100644
> --- a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.c
> +++ b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.c
> @@ -52,8 +52,8 @@ static const u8 mpeg_default_non_intra_quant[64] = {
>  
>  #define m_niq(i) ((i << 8) | mpeg_default_non_intra_quant[i])
>  
> -void sunxi_cedrus_mpeg2_setup(struct sunxi_cedrus_ctx *ctx,
> -			      struct sunxi_cedrus_run *run)
> +static void sunxi_cedrus_mpeg2_setup(struct sunxi_cedrus_ctx *ctx,
> +				     struct sunxi_cedrus_run *run)
>  {
>  	struct sunxi_cedrus_dev *dev = ctx->dev;
>  	const struct v4l2_ctrl_mpeg2_frame_hdr *frame_hdr = run->mpeg2.hdr;
> @@ -148,9 +148,14 @@ void sunxi_cedrus_mpeg2_setup(struct sunxi_cedrus_ctx *ctx,
>  	sunxi_cedrus_write(dev, src_buf_addr + VBV_SIZE - 1, VE_MPEG_VLD_END);
>  }
>  
> -void sunxi_cedrus_mpeg2_trigger(struct sunxi_cedrus_ctx *ctx)
> +static void sunxi_cedrus_mpeg2_trigger(struct sunxi_cedrus_ctx *ctx)
>  {
>  	struct sunxi_cedrus_dev *dev = ctx->dev;
>  
>  	sunxi_cedrus_write(dev, VE_TRIG_MPEG2, VE_MPEG_TRIGGER);
>  }
> +
> +struct sunxi_cedrus_dec_ops sunxi_cedrus_dec_ops_mpeg2 = {
> +	.setup		= sunxi_cedrus_mpeg2_setup,
> +	.trigger	= sunxi_cedrus_mpeg2_trigger,
> +};
> diff --git a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.h b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.h
> deleted file mode 100644
> index 4c380becfa1a..000000000000
> --- a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_mpeg2.h
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/*
> - * Sunxi-Cedrus VPU driver
> - *
> - * Copyright (C) 2018 Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> - * Copyright (C) 2016 Florent Revest <florent.revest@free-electrons.com>
> - *
> - * Based on the vim2m driver, that is:
> - *
> - * Copyright (c) 2009-2010 Samsung Electronics Co., Ltd.
> - * Pawel Osciak, <pawel@osciak.com>
> - * Marek Szyprowski, <m.szyprowski@samsung.com>
> - *
> - * This software is licensed under the terms of the GNU General Public
> - * License version 2, as published by the Free Software Foundation, and
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - */
> -
> -#ifndef _SUNXI_CEDRUS_MPEG2_H_
> -#define _SUNXI_CEDRUS_MPEG2_H_
> -
> -struct sunxi_cedrus_ctx;
> -struct sunxi_cedrus_run;
> -
> -void sunxi_cedrus_mpeg2_setup(struct sunxi_cedrus_ctx *ctx,
> -			      struct sunxi_cedrus_run *run);
> -void sunxi_cedrus_mpeg2_trigger(struct sunxi_cedrus_ctx *ctx);
> -
> -#endif
> diff --git a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.c b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.c
> index 089abfe6bfeb..fb7b081a5bb7 100644
> --- a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.c
> +++ b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.c
> @@ -28,7 +28,6 @@
>  #include <media/v4l2-mem2mem.h>
>  
>  #include "sunxi_cedrus_common.h"
> -#include "sunxi_cedrus_mpeg2.h"
>  #include "sunxi_cedrus_dec.h"
>  #include "sunxi_cedrus_hw.h"
>  
> @@ -414,6 +413,21 @@ static int sunxi_cedrus_buf_prepare(struct vb2_buffer *vb)
>  	return 0;
>  }
>  
> +static int sunxi_cedrus_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> +	struct sunxi_cedrus_ctx *ctx = vb2_get_drv_priv(q);
> +
> +	switch (ctx->vpu_src_fmt->fourcc) {
> +	case V4L2_PIX_FMT_MPEG2_FRAME:
> +		ctx->current_codec = SUNXI_CEDRUS_CODEC_MPEG2;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static void sunxi_cedrus_stop_streaming(struct vb2_queue *q)
>  {
>  	struct sunxi_cedrus_ctx *ctx = vb2_get_drv_priv(q);
> @@ -462,6 +476,7 @@ static struct vb2_ops sunxi_cedrus_qops = {
>  	.buf_cleanup		= sunxi_cedrus_buf_cleanup,
>  	.buf_queue		= sunxi_cedrus_buf_queue,
>  	.buf_request_complete	= sunxi_cedrus_buf_request_complete,
> +	.start_streaming	= sunxi_cedrus_start_streaming,
>  	.stop_streaming		= sunxi_cedrus_stop_streaming,
>  	.wait_prepare		= vb2_ops_wait_prepare,
>  	.wait_finish		= vb2_ops_wait_finish,
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]