Hi!

On Thu, Jun 21, 2018 at 11:49:54AM +0200, Paul Kocialkowski wrote:

Hi,

On Wed, 2018-06-13 at 16:07 +0200, Maxime Ripard wrote:

quoted

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 <redacted>

Thanks!

quoted

Signed-off-by: Maxime Ripard <redacted>
---
 .../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.

I'm not quite sure what you mean by consitency here. This structure
has 5 other variables with two words: vpu_src_fmt, src_fmt,
vpu_dst_fmt, dst_fmt and dst_bufs. codec_current would be going
against the consistency of that structure.

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!

quoted

 	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.

It won't fail either, and if we need to change it somewhere down the
line, it's quite easy to do.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180625/e35865d9/attachment.sig>