Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] media: venus: support frame rate control
@ 2020-02-13 21:30 Jeffrey Kardatzke
  2020-02-17 10:15 ` Stanimir Varbanov
  2020-02-18 19:13 ` Jeffrey Kardatzke
  0 siblings, 2 replies; 6+ messages in thread
From: Jeffrey Kardatzke @ 2020-02-13 21:30 UTC (permalink / raw)
  To: linux-media
  Cc: Stanimir Varbanov, Andy Gross, Mauro Carvalho Chehab ),
	linux-arm-msm, linux-kernel, Jeffrey Kardatzke

Frame rate control is always enabled in this driver, so make it silently
support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.

Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
---
 drivers/media/platform/qcom/venus/venc_ctrls.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 877c0b3299e9..9ede692f77c5 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 		}
 		mutex_unlock(&inst->lock);
 		break;
+	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
+		// Silently ignore, it's always enabled.
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -351,6 +354,9 @@ int venc_ctrl_init(struct venus_inst *inst)
 	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
 			  V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
 
+	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
+			  V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 0, 1, 1, 1);
+
 	ret = inst->ctrl_handler.error;
 	if (ret)
 		goto err;
-- 
2.25.0.225.g125e21ebc7-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: venus: support frame rate control
  2020-02-13 21:30 [PATCH] media: venus: support frame rate control Jeffrey Kardatzke
@ 2020-02-17 10:15 ` Stanimir Varbanov
  2020-02-18 19:09   ` Jeffrey Kardatzke
  2020-02-18 19:13 ` Jeffrey Kardatzke
  1 sibling, 1 reply; 6+ messages in thread
From: Stanimir Varbanov @ 2020-02-17 10:15 UTC (permalink / raw)
  To: Jeffrey Kardatzke, linux-media
  Cc: Stanimir Varbanov, Andy Gross, Mauro Carvalho Chehab ),
	linux-arm-msm, linux-kernel

Hi Jeff,

Thanks for the patch!

On 2/13/20 11:30 PM, Jeffrey Kardatzke wrote:
> Frame rate control is always enabled in this driver, so make it silently
> support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> 
> Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
> ---
>  drivers/media/platform/qcom/venus/venc_ctrls.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> index 877c0b3299e9..9ede692f77c5 100644
> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> @@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>  		}
>  		mutex_unlock(&inst->lock);
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
> +		// Silently ignore, it's always enabled.

Please, use C comments and follow the kernel coding style.

I wonder shouldn't it be better to add rc_enable field in struct
venc_controls and give the user choice to disable the rate control? We
can keep the default to be "enabled".

> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -351,6 +354,9 @@ int venc_ctrl_init(struct venus_inst *inst)
>  	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
>  			  V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
>  
> +	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
> +			  V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 0, 1, 1, 1);
> +

you forgot to increment the number of controls in the call to
v4l2_ctrl_handler_init.

>  	ret = inst->ctrl_handler.error;
>  	if (ret)
>  		goto err;
> 

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: venus: support frame rate control
  2020-02-17 10:15 ` Stanimir Varbanov
@ 2020-02-18 19:09   ` Jeffrey Kardatzke
  2020-02-19  7:56     ` Stanimir Varbanov
  0 siblings, 1 reply; 6+ messages in thread
From: Jeffrey Kardatzke @ 2020-02-18 19:09 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: linux-media, Andy Gross, Mauro Carvalho Chehab ),
	linux-arm-msm, linux-kernel

Sorry for the duplicate, accidentally used HTML format and it got
bounced from the mailing lists so resending.

On Mon, Feb 17, 2020 at 2:15 AM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Jeff,
>
> Thanks for the patch!
>
> On 2/13/20 11:30 PM, Jeffrey Kardatzke wrote:
> > Frame rate control is always enabled in this driver, so make it silently
> > support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> >
> > Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
> > ---
> >  drivers/media/platform/qcom/venus/venc_ctrls.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> > index 877c0b3299e9..9ede692f77c5 100644
> > --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> > +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> > @@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >               }
> >               mutex_unlock(&inst->lock);
> >               break;
> > +     case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
> > +             // Silently ignore, it's always enabled.
>
> Please, use C comments and follow the kernel coding style.

OK, hopefully I've got that now. I didn't see any issues aside from
the comment style though.
I'll upload a new patch shortly.
>
>
> I wonder shouldn't it be better to add rc_enable field in struct
> venc_controls and give the user choice to disable the rate control? We
> can keep the default to be "enabled".
>
That'd be fine. Is there a way to actually disable the rate control though?
>
> > +             break;
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -351,6 +354,9 @@ int venc_ctrl_init(struct venus_inst *inst)
> >       v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
> >                         V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
> >
> > +     v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
> > +                       V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 0, 1, 1, 1);
> > +
>
> you forgot to increment the number of controls in the call to
> v4l2_ctrl_handler_init.
>
Done, thanks.
>
> >       ret = inst->ctrl_handler.error;
> >       if (ret)
> >               goto err;
> >
>
> --
> regards,
> Stan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] media: venus: support frame rate control
  2020-02-13 21:30 [PATCH] media: venus: support frame rate control Jeffrey Kardatzke
  2020-02-17 10:15 ` Stanimir Varbanov
@ 2020-02-18 19:13 ` Jeffrey Kardatzke
  1 sibling, 0 replies; 6+ messages in thread
From: Jeffrey Kardatzke @ 2020-02-18 19:13 UTC (permalink / raw)
  To: linux-media
  Cc: Stanimir Varbanov, Andy Gross, Mauro Carvalho Chehab ),
	linux-arm-msm, linux-kernel, Jeffrey Kardatzke

Frame rate control is always enabled in this driver, so make it silently
support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.

Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
---
 drivers/media/platform/qcom/venus/venc_ctrls.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 877c0b3299e9..15852bc26231 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 		}
 		mutex_unlock(&inst->lock);
 		break;
+	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
+		/* Silently ignore, it's always enabled. */
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -214,7 +217,7 @@ int venc_ctrl_init(struct venus_inst *inst)
 {
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 30);
+	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 31);
 	if (ret)
 		return ret;
 
@@ -351,6 +354,9 @@ int venc_ctrl_init(struct venus_inst *inst)
 	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
 			  V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
 
+	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
+			  V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 0, 1, 1, 1);
+
 	ret = inst->ctrl_handler.error;
 	if (ret)
 		goto err;
-- 
2.25.0.265.gbab2e86ba0-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: venus: support frame rate control
  2020-02-18 19:09   ` Jeffrey Kardatzke
@ 2020-02-19  7:56     ` Stanimir Varbanov
  2020-02-19 16:37       ` Jeffrey Kardatzke
  0 siblings, 1 reply; 6+ messages in thread
From: Stanimir Varbanov @ 2020-02-19  7:56 UTC (permalink / raw)
  To: Jeffrey Kardatzke, Stanimir Varbanov
  Cc: linux-media, Andy Gross, Mauro Carvalho Chehab ),
	linux-arm-msm, linux-kernel

Hi Jeff,

On 2/18/20 9:09 PM, Jeffrey Kardatzke wrote:
> Sorry for the duplicate, accidentally used HTML format and it got
> bounced from the mailing lists so resending.
> 
> On Mon, Feb 17, 2020 at 2:15 AM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Hi Jeff,
>>
>> Thanks for the patch!
>>
>> On 2/13/20 11:30 PM, Jeffrey Kardatzke wrote:
>>> Frame rate control is always enabled in this driver, so make it silently
>>> support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
>>>
>>> Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
>>> ---
>>>  drivers/media/platform/qcom/venus/venc_ctrls.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
>>> index 877c0b3299e9..9ede692f77c5 100644
>>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>>> @@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>               }
>>>               mutex_unlock(&inst->lock);
>>>               break;
>>> +     case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
>>> +             // Silently ignore, it's always enabled.
>>
>> Please, use C comments and follow the kernel coding style.
> 
> OK, hopefully I've got that now. I didn't see any issues aside from
> the comment style though.
> I'll upload a new patch shortly.
>>
>>
>> I wonder shouldn't it be better to add rc_enable field in struct
>> venc_controls and give the user choice to disable the rate control? We
>> can keep the default to be "enabled".
>>
> That'd be fine. Is there a way to actually disable the rate control though?

The rate control property values are here [1], and [2] is where we set
the control.

-- 
regards,
Stan

[1]
https://elixir.bootlin.com/linux/v5.6-rc2/source/drivers/media/platform/qcom/venus/hfi_helper.h#L229
[2]
https://elixir.bootlin.com/linux/v5.6-rc2/source/drivers/media/platform/qcom/venus/venc.c#L734

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: venus: support frame rate control
  2020-02-19  7:56     ` Stanimir Varbanov
@ 2020-02-19 16:37       ` Jeffrey Kardatzke
  0 siblings, 0 replies; 6+ messages in thread
From: Jeffrey Kardatzke @ 2020-02-19 16:37 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: linux-media, Andy Gross, Mauro Carvalho Chehab ),
	linux-arm-msm, linux-kernel

Got it, thanks. I'll submit a new patch with that.

On Tue, Feb 18, 2020 at 11:56 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Jeff,
>
> On 2/18/20 9:09 PM, Jeffrey Kardatzke wrote:
> > Sorry for the duplicate, accidentally used HTML format and it got
> > bounced from the mailing lists so resending.
> >
> > On Mon, Feb 17, 2020 at 2:15 AM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >> Hi Jeff,
> >>
> >> Thanks for the patch!
> >>
> >> On 2/13/20 11:30 PM, Jeffrey Kardatzke wrote:
> >>> Frame rate control is always enabled in this driver, so make it silently
> >>> support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> >>>
> >>> Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
> >>> ---
> >>>  drivers/media/platform/qcom/venus/venc_ctrls.c | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>> index 877c0b3299e9..9ede692f77c5 100644
> >>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>> @@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >>>               }
> >>>               mutex_unlock(&inst->lock);
> >>>               break;
> >>> +     case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
> >>> +             // Silently ignore, it's always enabled.
> >>
> >> Please, use C comments and follow the kernel coding style.
> >
> > OK, hopefully I've got that now. I didn't see any issues aside from
> > the comment style though.
> > I'll upload a new patch shortly.
> >>
> >>
> >> I wonder shouldn't it be better to add rc_enable field in struct
> >> venc_controls and give the user choice to disable the rate control? We
> >> can keep the default to be "enabled".
> >>
> > That'd be fine. Is there a way to actually disable the rate control though?
>
> The rate control property values are here [1], and [2] is where we set
> the control.
>
> --
> regards,
> Stan
>
> [1]
> https://elixir.bootlin.com/linux/v5.6-rc2/source/drivers/media/platform/qcom/venus/hfi_helper.h#L229
> [2]
> https://elixir.bootlin.com/linux/v5.6-rc2/source/drivers/media/platform/qcom/venus/venc.c#L734



-- 
Jeffrey Kardatzke
jkardatzke@google.com
Google, Inc.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13 21:30 [PATCH] media: venus: support frame rate control Jeffrey Kardatzke
2020-02-17 10:15 ` Stanimir Varbanov
2020-02-18 19:09   ` Jeffrey Kardatzke
2020-02-19  7:56     ` Stanimir Varbanov
2020-02-19 16:37       ` Jeffrey Kardatzke
2020-02-18 19:13 ` Jeffrey Kardatzke

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git