diff mbox series

[04/11] media: cedrus: Add helper for checking capabilities

Message ID 20221024201515.34129-5-jernej.skrabec@gmail.com
State New
Headers
Series media: cedrus: Format handling improvements and 10-bit HEVC support |

Commit Message

Jernej Škrabec Oct. 24, 2022, 8:15 p.m. UTC 
  There is several different Cedrus cores with varying capabilities, so
some operations like listing formats depends on checks if feature is
supported or not.

Currently check for capabilities is only in format enumeration helper,
but it will be used also elsewhere later. Let's convert this check to
helper and while at it, also simplify it. There is no need to check if
capability mask is zero, condition will still work properly.

No functional changes intended.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/sunxi/cedrus/cedrus.h       | 6 ++++++
 drivers/staging/media/sunxi/cedrus/cedrus_video.c | 8 +-------
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Dan Carpenter Oct. 25, 2022, 6:30 a.m. UTC | #1 
On Mon, Oct 24, 2022 at 10:15:08PM +0200, Jernej Skrabec wrote:
> There is several different Cedrus cores with varying capabilities, so
> some operations like listing formats depends on checks if feature is
> supported or not.
> 
> Currently check for capabilities is only in format enumeration helper,
> but it will be used also elsewhere later. Let's convert this check to
> helper and while at it, also simplify it. There is no need to check if
> capability mask is zero, condition will still work properly.

Sure.  That's true.  Out of curiousity, can cedrus_formats[i].capabilities
be zero?  Because it feels like that's what should be checked.

regards,
dan carpenter
Jernej Škrabec Oct. 25, 2022, 3:17 p.m. UTC | #2 
Dne torek, 25. oktober 2022 ob 08:30:28 CEST je Dan Carpenter napisal(a):
> On Mon, Oct 24, 2022 at 10:15:08PM +0200, Jernej Skrabec wrote:
> > There is several different Cedrus cores with varying capabilities, so
> > some operations like listing formats depends on checks if feature is
> > supported or not.
> > 
> > Currently check for capabilities is only in format enumeration helper,
> > but it will be used also elsewhere later. Let's convert this check to
> > helper and while at it, also simplify it. There is no need to check if
> > capability mask is zero, condition will still work properly.
> 
> Sure.  That's true.  Out of curiousity, can cedrus_formats[i].capabilities
> be zero?  Because it feels like that's what should be checked.

Yes, it can be. It's the case for V4L2_PIX_FMT_NV12_32L32. All variants 
supports it, so there is no special capability needed in order to be listed. 
What would you check in such case? Condition still works for this case.

Best regards,
Jernej
Paul Kocialkowski Oct. 25, 2022, 3:22 p.m. UTC | #3 
Hi Jernej,

On Tue 25 Oct 22, 17:17, Jernej Škrabec wrote:
> Dne torek, 25. oktober 2022 ob 08:30:28 CEST je Dan Carpenter napisal(a):
> > On Mon, Oct 24, 2022 at 10:15:08PM +0200, Jernej Skrabec wrote:
> > > There is several different Cedrus cores with varying capabilities, so
> > > some operations like listing formats depends on checks if feature is
> > > supported or not.
> > > 
> > > Currently check for capabilities is only in format enumeration helper,
> > > but it will be used also elsewhere later. Let's convert this check to
> > > helper and while at it, also simplify it. There is no need to check if
> > > capability mask is zero, condition will still work properly.
> > 
> > Sure.  That's true.  Out of curiousity, can cedrus_formats[i].capabilities
> > be zero?  Because it feels like that's what should be checked.
> 
> Yes, it can be. It's the case for V4L2_PIX_FMT_NV12_32L32. All variants 
> supports it, so there is no special capability needed in order to be listed. 
> What would you check in such case? Condition still works for this case.

I think the problem is that (bits & 0) == 0 is always true.
So if the input caps are 0, we need to make sure to return false.

Cheers,

Paul
Jernej Škrabec Oct. 25, 2022, 3:28 p.m. UTC | #4 
Dne torek, 25. oktober 2022 ob 17:22:59 CEST je Paul Kocialkowski napisal(a):
> Hi Jernej,
> 
> On Tue 25 Oct 22, 17:17, Jernej Škrabec wrote:
> > Dne torek, 25. oktober 2022 ob 08:30:28 CEST je Dan Carpenter napisal(a):
> > > On Mon, Oct 24, 2022 at 10:15:08PM +0200, Jernej Skrabec wrote:
> > > > There is several different Cedrus cores with varying capabilities, so
> > > > some operations like listing formats depends on checks if feature is
> > > > supported or not.
> > > > 
> > > > Currently check for capabilities is only in format enumeration helper,
> > > > but it will be used also elsewhere later. Let's convert this check to
> > > > helper and while at it, also simplify it. There is no need to check if
> > > > capability mask is zero, condition will still work properly.
> > > 
> > > Sure.  That's true.  Out of curiousity, can
> > > cedrus_formats[i].capabilities
> > > be zero?  Because it feels like that's what should be checked.
> > 
> > Yes, it can be. It's the case for V4L2_PIX_FMT_NV12_32L32. All variants
> > supports it, so there is no special capability needed in order to be
> > listed. What would you check in such case? Condition still works for this
> > case.
> I think the problem is that (bits & 0) == 0 is always true.
> So if the input caps are 0, we need to make sure to return false.

No. If format (or any other) capabilities are 0, means they are supported by 
all variants and it's expected from cedrus_is_capable() to return true.

Regards,
Jernej
Paul Kocialkowski Oct. 25, 2022, 3:35 p.m. UTC | #5 
On Tue 25 Oct 22, 17:28, Jernej Škrabec wrote:
> Dne torek, 25. oktober 2022 ob 17:22:59 CEST je Paul Kocialkowski napisal(a):
> > Hi Jernej,
> > 
> > On Tue 25 Oct 22, 17:17, Jernej Škrabec wrote:
> > > Dne torek, 25. oktober 2022 ob 08:30:28 CEST je Dan Carpenter napisal(a):
> > > > On Mon, Oct 24, 2022 at 10:15:08PM +0200, Jernej Skrabec wrote:
> > > > > There is several different Cedrus cores with varying capabilities, so
> > > > > some operations like listing formats depends on checks if feature is
> > > > > supported or not.
> > > > > 
> > > > > Currently check for capabilities is only in format enumeration helper,
> > > > > but it will be used also elsewhere later. Let's convert this check to
> > > > > helper and while at it, also simplify it. There is no need to check if
> > > > > capability mask is zero, condition will still work properly.
> > > > 
> > > > Sure.  That's true.  Out of curiousity, can
> > > > cedrus_formats[i].capabilities
> > > > be zero?  Because it feels like that's what should be checked.
> > > 
> > > Yes, it can be. It's the case for V4L2_PIX_FMT_NV12_32L32. All variants
> > > supports it, so there is no special capability needed in order to be
> > > listed. What would you check in such case? Condition still works for this
> > > case.
> > I think the problem is that (bits & 0) == 0 is always true.
> > So if the input caps are 0, we need to make sure to return false.
> 
> No. If format (or any other) capabilities are 0, means they are supported by 
> all variants and it's expected from cedrus_is_capable() to return true.

Mhh, yeah. Not sure what I was thinking. Sorry for the noise.

Paul
diff mbox series

Patch

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 9cffaf228422..1a98790a99af 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -268,6 +268,12 @@  vb2_to_cedrus_buffer(const struct vb2_buffer *p)
 	return vb2_v4l2_to_cedrus_buffer(to_vb2_v4l2_buffer(p));
 }
 
+static inline bool
+cedrus_is_capable(struct cedrus_ctx *ctx, unsigned int capabilities)
+{
+	return (ctx->dev->capabilities & capabilities) == capabilities;
+}
+
 void *cedrus_find_control_data(struct cedrus_ctx *ctx, u32 id);
 u32 cedrus_get_num_of_controls(struct cedrus_ctx *ctx, u32 id);
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 27a7120fa6fb..04b7b87ef0b7 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -177,19 +177,13 @@  static int cedrus_enum_fmt(struct file *file, struct v4l2_fmtdesc *f,
 			   u32 direction)
 {
 	struct cedrus_ctx *ctx = cedrus_file2ctx(file);
-	struct cedrus_dev *dev = ctx->dev;
-	unsigned int capabilities = dev->capabilities;
-	struct cedrus_format *fmt;
 	unsigned int i, index;
 
 	/* Index among formats that match the requested direction. */
 	index = 0;
 
 	for (i = 0; i < CEDRUS_FORMATS_COUNT; i++) {
-		fmt = &cedrus_formats[i];
-
-		if (fmt->capabilities && (fmt->capabilities & capabilities) !=
-		    fmt->capabilities)
+		if (!cedrus_is_capable(ctx, cedrus_formats[i].capabilities))
 			continue;
 
 		if (!(cedrus_formats[i].directions & direction))