On 11/06/18 08:44, Keiichi Watanabe wrote: > Hi, Hans. > Thank you for the review. > Your idea sounds good. > > However, I think that changing V4L2_CID_MPEG_VIDEO_VPX_PROFILE to an enum > breaks both of s5p-mfc and venus drivers. This is because they call > 'v4l2_ctrl_new_std' for it. For menu controls, > 'v4l2_ctrl_new_std_menu' must be used. > > https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L2678 > https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/qcom/venus/vdec_ctrls.c#L133 > > I can fix them within the commit for adding VP8_PROFILE control, but > cannot confirm that it works on real devices since I don't have them. > What should I do? Fix it in both drivers with a Cc to stanimir.varbanov@xxxxxxxxxx (venus) and s.nawrocki@xxxxxxxxxxx (s5p) to let them test/ack the patch. Venus is no problem, the s5p driver can decide to keep the old INT control since it has been in use for such a long time, but that is up to Sylwester to decide. Regards, Hans > > Best regards, > Keiichi > On Fri, Jun 8, 2018 at 10:00 PM Nicolas Dufresne <nicolas@xxxxxxxxxxxx> wrote: >> >> >> >> Le ven. 8 juin 2018 08:56, Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx> a écrit : >>> >>> Hi Hans, >>> >>> On 06/08/2018 12:29 PM, Hans Verkuil wrote: >>>> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote: >>>>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired >>>>> profile for VP9 encoder and querying for supported profiles by VP9 encoder >>>>> or decoder. >>>>> >>>>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be >>>>> used for querying since it is not a menu control but an integer >>>>> control, which cannot return an arbitrary set of supported profiles. >>>>> >>>>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as >>>>> with controls for other codec profiles. (e.g. H264) >>>> >>>> Please ignore my reply to patch 2/2. I looked at this a bit more and what you >>>> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum. >>>> >>>> All other codec profile controls are all enums, so the fact that VPX_PROFILE >>>> isn't is a bug. Changing the type should not cause any problems since the same >>>> control value is used when you set the control. >>>> >>>> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added >>>> as an integer type control, so the s5p-mfc driver should not be affected by >>>> changing the type of this control. >>>> >>>> Stanimir: this will slightly change the venus driver, but since it is a very >>>> recent driver I think we can get away with changing the core type of the >>>> VPX_PROFILE control. I think that's better than ending up with two controls >>>> that do the same thing. >>> >>> I agree. Actually the changes shouldn't be so much in venus driver. >> >> >> Also fine on userspace side, since profiles enumeration isn't implemented yet in FFMPEG, GStreamer, Chrome >> >> >>> >>> -- >>> regards, >>> Stan
