[PATCH 1/2] media: ipu3-imgu: Use MENU type for mode control

[PATCH 1/2] media: ipu3-imgu: Use MENU type for mode control

[Yong Zhi]

This addresses the below TODO item.
- Use V4L2_CTRL_TYPE_MENU for dual-pipe mode control. (Sakari)

Signed-off-by: Yong Zhi <yong.zhi@intel.com>
---
 drivers/staging/media/ipu3/TODO                 |  2 --
 drivers/staging/media/ipu3/include/intel-ipu3.h |  6 ------
 drivers/staging/media/ipu3/ipu3-v4l2.c          | 18 +++++++++++++-----
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/ipu3/TODO b/drivers/staging/media/ipu3/TODO
index 905bbb190217..0dc9a2e79978 100644
--- a/drivers/staging/media/ipu3/TODO
+++ b/drivers/staging/media/ipu3/TODO
@@ -11,8 +11,6 @@ staging directory.
 - Prefix imgu for all public APIs, i.e. change ipu3_v4l2_register() to
   imgu_v4l2_register(). (Sakari)
 
-- Use V4L2_CTRL_TYPE_MENU for dual-pipe mode control. (Sakari)
-
 - IPU3 driver documentation (Laurent)
   Add diagram in driver rst to describe output capability.
   Comments on configuring v4l2 subdevs for CIO2 and ImgU.
diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index ec0b74829351..eb6f52aca992 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -16,12 +16,6 @@
 #define V4L2_CID_INTEL_IPU3_BASE	(V4L2_CID_USER_BASE + 0x10c0)
 #define V4L2_CID_INTEL_IPU3_MODE	(V4L2_CID_INTEL_IPU3_BASE + 1)
 
-/* custom ctrl to set pipe mode */
-enum ipu3_running_mode {
-	IPU3_RUNNING_MODE_VIDEO = 0,
-	IPU3_RUNNING_MODE_STILL = 1,
-};
-
 /******************* ipu3_uapi_stats_3a *******************/
 
 #define IPU3_UAPI_MAX_STRIPES				2
diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index c7936032beb9..d2a0b62d5688 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -12,6 +12,9 @@
 
 /******************** v4l2_subdev_ops ********************/
 
+#define	IPU3_RUNNING_MODE_VIDEO		0
+#define	IPU3_RUNNING_MODE_STILL		1
+
 static int ipu3_subdev_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
 	struct imgu_v4l2_subdev *imgu_sd = container_of(sd,
@@ -1035,15 +1038,20 @@ static const struct v4l2_ctrl_ops ipu3_subdev_ctrl_ops = {
 	.s_ctrl = ipu3_sd_s_ctrl,
 };
 
+static const char * const ipu3_ctrl_mode_strings[] = {
+	"Video mode",
+	"Still mode",
+	NULL,
+};
+
 static const struct v4l2_ctrl_config ipu3_subdev_ctrl_mode = {
 	.ops = &ipu3_subdev_ctrl_ops,
 	.id = V4L2_CID_INTEL_IPU3_MODE,
 	.name = "IPU3 Pipe Mode",
-	.type = V4L2_CTRL_TYPE_INTEGER,
-	.min = IPU3_RUNNING_MODE_VIDEO,
-	.max = IPU3_RUNNING_MODE_STILL,
-	.step = 1,
-	.def = IPU3_RUNNING_MODE_VIDEO,
+	.type = V4L2_CTRL_TYPE_MENU,
+	.max = ARRAY_SIZE(ipu3_ctrl_mode_strings) - 2,
+	.def = 0,
+	.qmenu = ipu3_ctrl_mode_strings,
 };
 
 /******************** Framework registration ********************/
-- 
2.7.4

[PATCH 2/2] media: ipu3-imgu: Remove dead code for NULL check

[Yong Zhi]

Since ipu3_css_buf_dequeue() never returns NULL, remove the
dead code to fix static checker warning:

drivers/staging/media/ipu3/ipu3.c:493 imgu_isr_threaded()
warn: 'b' is an error pointer or valid

Signed-off-by: Yong Zhi <yong.zhi@intel.com>
---
Link to Dan's bug report:
https://www.spinics.net/lists/linux-media/msg145043.html

 drivers/staging/media/ipu3/ipu3.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c
index d521b3afb8b1..839d9398f8e9 100644
--- a/drivers/staging/media/ipu3/ipu3.c
+++ b/drivers/staging/media/ipu3/ipu3.c
@@ -489,12 +489,11 @@ static irqreturn_t imgu_isr_threaded(int irq, void *imgu_ptr)
 			mutex_unlock(&imgu->lock);
 		} while (PTR_ERR(b) == -EAGAIN);
 
-		if (IS_ERR_OR_NULL(b)) {
-			if (!b || PTR_ERR(b) == -EBUSY)	/* All done */
-				break;
-			dev_err(&imgu->pci_dev->dev,
-				"failed to dequeue buffers (%ld)\n",
-				PTR_ERR(b));
+		if (IS_ERR(b)) {
+			if (PTR_ERR(b) != -EBUSY)	/* All done */
+				dev_err(&imgu->pci_dev->dev,
+					"failed to dequeue buffers (%ld)\n",
+					PTR_ERR(b));
 			break;
 		}
 
-- 
2.7.4

Re: [PATCH 2/2] media: ipu3-imgu: Remove dead code for NULL check

[Tomasz Figa]

Hi Yong,

On Tue, Jan 15, 2019 at 12:38 PM Yong Zhi <yong.zhi@intel.com> wrote:
>
> Since ipu3_css_buf_dequeue() never returns NULL, remove the
> dead code to fix static checker warning:
>
> drivers/staging/media/ipu3/ipu3.c:493 imgu_isr_threaded()
> warn: 'b' is an error pointer or valid
>
> Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> ---
> Link to Dan's bug report:
> https://www.spinics.net/lists/linux-media/msg145043.html

You can add Dan's Reported-by above your Signed-off-by to properly
credit him. I'd also add a comment below that Reported-by, e.g.

[Bug report: https://www.spinics.net/lists/linux-media/msg145043.html]

so that it doesn't get removed when applying the patch, as it would
get now, because any text right in this area is ignored by git.

With that fixes, feel free to add my Reviewed-by.

Best regards,
Tomasz

>
>  drivers/staging/media/ipu3/ipu3.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c
> index d521b3afb8b1..839d9398f8e9 100644
> --- a/drivers/staging/media/ipu3/ipu3.c
> +++ b/drivers/staging/media/ipu3/ipu3.c
> @@ -489,12 +489,11 @@ static irqreturn_t imgu_isr_threaded(int irq, void *imgu_ptr)
>                         mutex_unlock(&imgu->lock);
>                 } while (PTR_ERR(b) == -EAGAIN);
>
> -               if (IS_ERR_OR_NULL(b)) {
> -                       if (!b || PTR_ERR(b) == -EBUSY) /* All done */
> -                               break;
> -                       dev_err(&imgu->pci_dev->dev,
> -                               "failed to dequeue buffers (%ld)\n",
> -                               PTR_ERR(b));
> +               if (IS_ERR(b)) {
> +                       if (PTR_ERR(b) != -EBUSY)       /* All done */
> +                               dev_err(&imgu->pci_dev->dev,
> +                                       "failed to dequeue buffers (%ld)\n",
> +                                       PTR_ERR(b));
>                         break;
>                 }
>
> --
> 2.7.4
>

RE: [PATCH 2/2] media: ipu3-imgu: Remove dead code for NULL check

[Zhi, Yong]

Hi, Tomasz,

> -----Original Message-----
> From: Tomasz Figa [mailto:tfiga@chromium.org]
> Sent: Monday, January 14, 2019 11:38 PM
> To: Zhi, Yong <yong.zhi@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
> <sakari.ailus@linux.intel.com>; Mani, Rajmohan
> <rajmohan.mani@intel.com>; Qiu, Tian Shu <tian.shu.qiu@intel.com>;
> Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Hans Verkuil
> <hans.verkuil@cisco.com>; Mauro Carvalho Chehab <mchehab@kernel.org>;
> Cao, Bingbu <bingbu.cao@intel.com>; dan.carpenter@oracle.com
> Subject: Re: [PATCH 2/2] media: ipu3-imgu: Remove dead code for NULL
> check
> 
> Hi Yong,
> 
> On Tue, Jan 15, 2019 at 12:38 PM Yong Zhi <yong.zhi@intel.com> wrote:
> >
> > Since ipu3_css_buf_dequeue() never returns NULL, remove the dead code
> > to fix static checker warning:
> >
> > drivers/staging/media/ipu3/ipu3.c:493 imgu_isr_threaded()
> > warn: 'b' is an error pointer or valid
> >
> > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> > ---
> > Link to Dan's bug report:
> > https://www.spinics.net/lists/linux-media/msg145043.html
> 
> You can add Dan's Reported-by above your Signed-off-by to properly credit
> him. I'd also add a comment below that Reported-by, e.g.
> 
> [Bug report: https://www.spinics.net/lists/linux-media/msg145043.html]
> 
> so that it doesn't get removed when applying the patch, as it would get now,
> because any text right in this area is ignored by git.
> 
> With that fixes, feel free to add my Reviewed-by.

Thanks a lot for the detailed instructions :)

> 
> Best regards,
> Tomasz
> 
> >
> >  drivers/staging/media/ipu3/ipu3.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/media/ipu3/ipu3.c
> > b/drivers/staging/media/ipu3/ipu3.c
> > index d521b3afb8b1..839d9398f8e9 100644
> > --- a/drivers/staging/media/ipu3/ipu3.c
> > +++ b/drivers/staging/media/ipu3/ipu3.c
> > @@ -489,12 +489,11 @@ static irqreturn_t imgu_isr_threaded(int irq, void
> *imgu_ptr)
> >                         mutex_unlock(&imgu->lock);
> >                 } while (PTR_ERR(b) == -EAGAIN);
> >
> > -               if (IS_ERR_OR_NULL(b)) {
> > -                       if (!b || PTR_ERR(b) == -EBUSY) /* All done */
> > -                               break;
> > -                       dev_err(&imgu->pci_dev->dev,
> > -                               "failed to dequeue buffers (%ld)\n",
> > -                               PTR_ERR(b));
> > +               if (IS_ERR(b)) {
> > +                       if (PTR_ERR(b) != -EBUSY)       /* All done */
> > +                               dev_err(&imgu->pci_dev->dev,
> > +                                       "failed to dequeue buffers (%ld)\n",
> > +                                       PTR_ERR(b));
> >                         break;
> >                 }
> >
> > --
> > 2.7.4
> >

Re: [PATCH 2/2] media: ipu3-imgu: Remove dead code for NULL check

[Laurent Pinchart]

Hi Tomasz,

On Tuesday, 15 January 2019 07:38:01 EET Tomasz Figa wrote:
> On Tue, Jan 15, 2019 at 12:38 PM Yong Zhi <yong.zhi@intel.com> wrote:
> > Since ipu3_css_buf_dequeue() never returns NULL, remove the
> > dead code to fix static checker warning:
> > 
> > drivers/staging/media/ipu3/ipu3.c:493 imgu_isr_threaded()
> > warn: 'b' is an error pointer or valid
> > 
> > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> > ---
> > Link to Dan's bug report:
> > https://www.spinics.net/lists/linux-media/msg145043.html
> 
> You can add Dan's Reported-by above your Signed-off-by to properly
> credit him. I'd also add a comment below that Reported-by, e.g.
> 
> [Bug report: https://www.spinics.net/lists/linux-media/msg145043.html]

How about pointing to https://lore.kernel.org/linux-media/
20190104122856.GA1169@kadam/ instead, now that we have a shiny new archive 
that should be stable until the end of times ? :-)

> so that it doesn't get removed when applying the patch, as it would
> get now, because any text right in this area is ignored by git.
> 
> With that fixes, feel free to add my Reviewed-by.
> 
> Best regards,
> Tomasz
> 
> >  drivers/staging/media/ipu3/ipu3.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/media/ipu3/ipu3.c
> > b/drivers/staging/media/ipu3/ipu3.c index d521b3afb8b1..839d9398f8e9
> > 100644
> > --- a/drivers/staging/media/ipu3/ipu3.c
> > +++ b/drivers/staging/media/ipu3/ipu3.c
> > @@ -489,12 +489,11 @@ static irqreturn_t imgu_isr_threaded(int irq, void
> > *imgu_ptr)> 
> >                         mutex_unlock(&imgu->lock);
> >                 
> >                 } while (PTR_ERR(b) == -EAGAIN);
> > 
> > -               if (IS_ERR_OR_NULL(b)) {
> > -                       if (!b || PTR_ERR(b) == -EBUSY) /* All done */
> > -                               break;
> > -                       dev_err(&imgu->pci_dev->dev,
> > -                               "failed to dequeue buffers (%ld)\n",
> > -                               PTR_ERR(b));
> > +               if (IS_ERR(b)) {
> > +                       if (PTR_ERR(b) != -EBUSY)       /* All done */
> > +                               dev_err(&imgu->pci_dev->dev,
> > +                                       "failed to dequeue buffers
> > (%ld)\n", +                                       PTR_ERR(b));
> > 
> >                         break;
> >                 
> >                 }
> > 
> > --
> > 2.7.4


-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: ipu3-imgu: Remove dead code for NULL check

[Tomasz Figa]

On Wed, Jan 16, 2019 at 1:38 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> On Tuesday, 15 January 2019 07:38:01 EET Tomasz Figa wrote:
> > On Tue, Jan 15, 2019 at 12:38 PM Yong Zhi <yong.zhi@intel.com> wrote:
> > > Since ipu3_css_buf_dequeue() never returns NULL, remove the
> > > dead code to fix static checker warning:
> > >
> > > drivers/staging/media/ipu3/ipu3.c:493 imgu_isr_threaded()
> > > warn: 'b' is an error pointer or valid
> > >
> > > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> > > ---
> > > Link to Dan's bug report:
> > > https://www.spinics.net/lists/linux-media/msg145043.html
> >
> > You can add Dan's Reported-by above your Signed-off-by to properly
> > credit him. I'd also add a comment below that Reported-by, e.g.
> >
> > [Bug report: https://www.spinics.net/lists/linux-media/msg145043.html]
>
> How about pointing to https://lore.kernel.org/linux-media/
> 20190104122856.GA1169@kadam/ instead, now that we have a shiny new archive
> that should be stable until the end of times ? :-)
>

Even better, thanks! (I often use the lore patchwork, but somehow I
wasn't able to look that patch up there. :))

> > so that it doesn't get removed when applying the patch, as it would
> > get now, because any text right in this area is ignored by git.
> >
> > With that fixes, feel free to add my Reviewed-by.
> >
> > Best regards,
> > Tomasz
> >
> > >  drivers/staging/media/ipu3/ipu3.c | 11 +++++------
> > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/ipu3/ipu3.c
> > > b/drivers/staging/media/ipu3/ipu3.c index d521b3afb8b1..839d9398f8e9
> > > 100644
> > > --- a/drivers/staging/media/ipu3/ipu3.c
> > > +++ b/drivers/staging/media/ipu3/ipu3.c
> > > @@ -489,12 +489,11 @@ static irqreturn_t imgu_isr_threaded(int irq, void
> > > *imgu_ptr)>
> > >                         mutex_unlock(&imgu->lock);
> > >
> > >                 } while (PTR_ERR(b) == -EAGAIN);
> > >
> > > -               if (IS_ERR_OR_NULL(b)) {
> > > -                       if (!b || PTR_ERR(b) == -EBUSY) /* All done */
> > > -                               break;
> > > -                       dev_err(&imgu->pci_dev->dev,
> > > -                               "failed to dequeue buffers (%ld)\n",
> > > -                               PTR_ERR(b));
> > > +               if (IS_ERR(b)) {
> > > +                       if (PTR_ERR(b) != -EBUSY)       /* All done */
> > > +                               dev_err(&imgu->pci_dev->dev,
> > > +                                       "failed to dequeue buffers
> > > (%ld)\n", +                                       PTR_ERR(b));
> > >
> > >                         break;
> > >
> > >                 }
> > >
> > > --
> > > 2.7.4
>
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Re: [PATCH 1/2] media: ipu3-imgu: Use MENU type for mode control

[Tomasz Figa]

Hi Yong,

On Tue, Jan 15, 2019 at 12:38 PM Yong Zhi <yong.zhi@intel.com> wrote:
>
> This addresses the below TODO item.
> - Use V4L2_CTRL_TYPE_MENU for dual-pipe mode control. (Sakari)
>
> Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> ---
>  drivers/staging/media/ipu3/TODO                 |  2 --
>  drivers/staging/media/ipu3/include/intel-ipu3.h |  6 ------
>  drivers/staging/media/ipu3/ipu3-v4l2.c          | 18 +++++++++++++-----
>  3 files changed, 13 insertions(+), 13 deletions(-)
>

Thanks for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/ipu3/TODO b/drivers/staging/media/ipu3/TODO
> index 905bbb190217..0dc9a2e79978 100644
> --- a/drivers/staging/media/ipu3/TODO
> +++ b/drivers/staging/media/ipu3/TODO
> @@ -11,8 +11,6 @@ staging directory.
>  - Prefix imgu for all public APIs, i.e. change ipu3_v4l2_register() to
>    imgu_v4l2_register(). (Sakari)
>
> -- Use V4L2_CTRL_TYPE_MENU for dual-pipe mode control. (Sakari)
> -
>  - IPU3 driver documentation (Laurent)
>    Add diagram in driver rst to describe output capability.
>    Comments on configuring v4l2 subdevs for CIO2 and ImgU.
> diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> index ec0b74829351..eb6f52aca992 100644
> --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> @@ -16,12 +16,6 @@
>  #define V4L2_CID_INTEL_IPU3_BASE       (V4L2_CID_USER_BASE + 0x10c0)
>  #define V4L2_CID_INTEL_IPU3_MODE       (V4L2_CID_INTEL_IPU3_BASE + 1)
>
> -/* custom ctrl to set pipe mode */
> -enum ipu3_running_mode {
> -       IPU3_RUNNING_MODE_VIDEO = 0,
> -       IPU3_RUNNING_MODE_STILL = 1,
> -};
> -
>  /******************* ipu3_uapi_stats_3a *******************/
>
>  #define IPU3_UAPI_MAX_STRIPES                          2
> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index c7936032beb9..d2a0b62d5688 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> @@ -12,6 +12,9 @@
>
>  /******************** v4l2_subdev_ops ********************/
>
> +#define        IPU3_RUNNING_MODE_VIDEO         0
> +#define        IPU3_RUNNING_MODE_STILL         1

Just a single space after "#define" please.

> +
>  static int ipu3_subdev_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  {
>         struct imgu_v4l2_subdev *imgu_sd = container_of(sd,
> @@ -1035,15 +1038,20 @@ static const struct v4l2_ctrl_ops ipu3_subdev_ctrl_ops = {
>         .s_ctrl = ipu3_sd_s_ctrl,
>  };
>
> +static const char * const ipu3_ctrl_mode_strings[] = {
> +       "Video mode",
> +       "Still mode",
> +       NULL,

Do you need this NULL entry? I don't see it in other drivers.

> +};
> +
>  static const struct v4l2_ctrl_config ipu3_subdev_ctrl_mode = {
>         .ops = &ipu3_subdev_ctrl_ops,
>         .id = V4L2_CID_INTEL_IPU3_MODE,
>         .name = "IPU3 Pipe Mode",
> -       .type = V4L2_CTRL_TYPE_INTEGER,
> -       .min = IPU3_RUNNING_MODE_VIDEO,
> -       .max = IPU3_RUNNING_MODE_STILL,
> -       .step = 1,
> -       .def = IPU3_RUNNING_MODE_VIDEO,
> +       .type = V4L2_CTRL_TYPE_MENU,
> +       .max = ARRAY_SIZE(ipu3_ctrl_mode_strings) - 2,
> +       .def = 0,

IPU3_RUNNING_MODE_VIDEO?

> +       .qmenu = ipu3_ctrl_mode_strings,
>  };
>
>  /******************** Framework registration ********************/
> --
> 2.7.4
>

Best regards,
Tomasz

Re: [PATCH 1/2] media: ipu3-imgu: Use MENU type for mode control

[Sakari Ailus]

Hi Yong,

On Mon, Jan 14, 2019 at 09:37:44PM -0600, Yong Zhi wrote:
> This addresses the below TODO item.
> - Use V4L2_CTRL_TYPE_MENU for dual-pipe mode control. (Sakari)
> 
> Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> ---
>  drivers/staging/media/ipu3/TODO                 |  2 --
>  drivers/staging/media/ipu3/include/intel-ipu3.h |  6 ------
>  drivers/staging/media/ipu3/ipu3-v4l2.c          | 18 +++++++++++++-----
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/TODO b/drivers/staging/media/ipu3/TODO
> index 905bbb190217..0dc9a2e79978 100644
> --- a/drivers/staging/media/ipu3/TODO
> +++ b/drivers/staging/media/ipu3/TODO
> @@ -11,8 +11,6 @@ staging directory.
>  - Prefix imgu for all public APIs, i.e. change ipu3_v4l2_register() to
>    imgu_v4l2_register(). (Sakari)
>  
> -- Use V4L2_CTRL_TYPE_MENU for dual-pipe mode control. (Sakari)
> -

It's good to see TODO entries being addressed. :-)

With Tomasz's comments addressed, this is good to go in IMO.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com