* Proposal for a v4l2_ctrl_new_std_compound() function @ 2019-09-12 13:35 Hans Verkuil 2019-09-13 16:08 ` kbuild test robot 2019-09-17 16:19 ` [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() Ricardo Ribalda Delgado 0 siblings, 2 replies; 7+ messages in thread From: Hans Verkuil @ 2019-09-12 13:35 UTC (permalink / raw) To: Ricardo Ribalda Delgado; +Cc: Linux Media Mailing List Hi Ricardo, As per our irc discussion, here is a proposal how I think it can be done. The core problem is that for compound types we want to provide a default value that can be used in std_init_compound() without having to provide our own type ops. The new v4l2_ctrl_new_std_compound would pass a const pointer to the struct with the default value, and std_init_compound should use that if p_def != NULL. I think this is beneficial for the various codec compound types as well. Implementing this in v4l2-ctrls.c is left as an exercise for the reader (i.e., you!). Regards, Hans Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h index 570ff4b0205a..7fdbf3abe49b 100644 --- a/include/media/v4l2-ctrls.h +++ b/include/media/v4l2-ctrls.h @@ -254,6 +254,7 @@ struct v4l2_ctrl { s32 val; } cur; + const union v4l2_ctrl_ptr p_def; union v4l2_ctrl_ptr p_new; union v4l2_ctrl_ptr p_cur; }; @@ -667,6 +668,27 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, u32 id, u8 max, u8 def, const s64 *qmenu_int); +/** + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2 + * compound control. + * + * @hdl: The control handler. + * @ops: The control ops. + * @id: The control ID. + * @min: The control's minimum value. + * @max: The control's maximum value. + * @step: The control's step value + * @p_def: The control's default value. + * + * If the &v4l2_ctrl struct could not be allocated, or the control + * ID is not known, then NULL is returned and @hdl->error is set to the + * appropriate error code (if it wasn't set already). + */ +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, + const struct v4l2_ctrl_ops *ops, + u32 id, s64 min, s64 max, u64 step, + const union v4l2_ctrl_ptr p_def); + /** * typedef v4l2_ctrl_filter - Typedef to define the filter function to be * used when adding a control handler. ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: Proposal for a v4l2_ctrl_new_std_compound() function 2019-09-12 13:35 Proposal for a v4l2_ctrl_new_std_compound() function Hans Verkuil @ 2019-09-13 16:08 ` kbuild test robot 2019-09-17 16:19 ` [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() Ricardo Ribalda Delgado 1 sibling, 0 replies; 7+ messages in thread From: kbuild test robot @ 2019-09-13 16:08 UTC (permalink / raw) To: Hans Verkuil Cc: kbuild-all, Ricardo Ribalda Delgado, Linux Media Mailing List [-- Attachment #1: Type: text/plain, Size: 32905 bytes --] Hi Hans, I love your patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [cannot apply to v5.3-rc8 next-20190904] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Hans-Verkuil/Proposal-for-a-v4l2_ctrl_new_std_compound-function/20190913-222917 base: git://linuxtv.org/media_tree.git master reproduce: make htmldocs If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All warnings (new ones prefixed by >>): Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme. WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) include/linux/w1.h:272: warning: Function parameter or member 'of_match_table' not described in 'w1_family' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quotactl' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quota_on' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'getprocattr' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'setprocattr' not described in 'security_list_options' drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:142: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_read_lock' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:347: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor ' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:348: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor ' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:494: warning: Function parameter or member 'start' not described in 'amdgpu_vm_pt_first_dfs' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_dfs_safe' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'entry' not described in 'for_each_amdgpu_vm_pt_dfs_safe' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:823: warning: Function parameter or member 'level' not described in 'amdgpu_vm_bo_param' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'level' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2822: warning: Function parameter or member 'pasid' not described in 'amdgpu_vm_make_compute' drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:378: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch' drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:379: warning: Function parameter or member 'ih' not described in 'amdgpu_irq_dispatch' drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:379: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch' drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c:1: warning: no structured comments found drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1: warning: no structured comments found drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:1: warning: 'pp_dpm_sclk pp_dpm_mclk pp_dpm_pcie' not found drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:131: warning: Incorrect use of kernel-doc format: * @atomic_obj drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:237: warning: Incorrect use of kernel-doc format: * gpu_info FW provided soc bounding box struct or 0 if not drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'atomic_obj' not described in 'amdgpu_display_manager' drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'backlight_link' not described in 'amdgpu_display_manager' drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'backlight_caps' not described in 'amdgpu_display_manager' drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'freesync_module' not described in 'amdgpu_display_manager' drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'fw_dmcu' not described in 'amdgpu_display_manager' drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'dmcu_fw_version' not described in 'amdgpu_display_manager' drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'soc_bounding_box' not described in 'amdgpu_display_manager' drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: 'dm_pflip_high_irq' not found drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: 'register_hpd_handlers' not found drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: 'dm_crtc_high_irq' not found include/linux/i2c.h:337: warning: Function parameter or member 'init_irq' not described in 'i2c_client' include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints' include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops' include/linux/spi/spi.h:190: warning: Function parameter or member 'driver_override' not described in 'spi_device' drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found fs/direct-io.c:258: warning: Excess function parameter 'offset' description in 'dio_complete' fs/libfs.c:496: warning: Excess function parameter 'available' description in 'simple_write_end' fs/posix_acl.c:647: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode' fs/posix_acl.c:647: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode' fs/posix_acl.c:647: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode' >> include/media/v4l2-ctrls.h:260: warning: Function parameter or member 'p_def' not described in 'v4l2_ctrl' mm/util.c:1: warning: 'get_user_pages_fast' not found mm/slab.c:4215: warning: Function parameter or member 'objp' not described in '__ksize' include/linux/input/sparse-keymap.h:43: warning: Function parameter or member 'sw' not described in 'key_entry' include/linux/skbuff.h:893: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'list' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'head_frag' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'encapsulation' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'csum_valid' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'vlan_present' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'csum_level' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff' include/net/sock.h:233: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_portpair' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_cookie' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_listener' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common' include/net/sock.h:515: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_wq_raw' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock' include/net/sock.h:2439: warning: Function parameter or member 'tcp_rx_skb_cache_key' not described in 'DECLARE_STATIC_KEY_FALSE' include/net/sock.h:2439: warning: Excess function parameter 'sk' description in 'DECLARE_STATIC_KEY_FALSE' include/net/sock.h:2439: warning: Excess function parameter 'skb' description in 'DECLARE_STATIC_KEY_FALSE' include/linux/netdevice.h:2040: warning: Function parameter or member 'gso_partial_features' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'name_assign_type' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'mpls_ptr' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'xdp_prog' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'qdisc_hash' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device' include/linux/netdevice.h:2040: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device' include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state' include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state' drivers/net/phy/phylink.c:595: warning: Function parameter or member 'config' not described in 'phylink_create' drivers/net/phy/phylink.c:595: warning: Excess function parameter 'ndev' description in 'phylink_create' lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found lib/genalloc.c:1: warning: 'gen_pool_alloc' not found lib/genalloc.c:1: warning: 'gen_pool_free' not found lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver' include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver' include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver' include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver' include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver' include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver' include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver' include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver' include/drm/drm_modeset_helper_vtables.h:1053: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs' include/drm/drm_modeset_helper_vtables.h:1053: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs' include/drm/drm_atomic_state_helper.h:1: warning: no structured comments found drivers/gpu/drm/mcde/mcde_drv.c:1: warning: 'ST-Ericsson MCDE DRM Driver' not found include/net/cfg80211.h:1092: warning: Function parameter or member 'txpwr' not described in 'station_parameters' include/net/mac80211.h:4043: warning: Function parameter or member 'sta_set_txpwr' not described in 'ieee80211_ops' include/net/mac80211.h:2006: warning: Function parameter or member 'txpwr' not described in 'ieee80211_sta' Documentation/admin-guide/xfs.rst:257: WARNING: Block quote ends without a blank line; unexpected unindent. Documentation/index.rst:94: WARNING: toctree contains reference to nonexisting document 'virtual/index' Documentation/crypto/crypto_engine.rst:2: WARNING: Explicit markup ends without a blank line; unexpected unindent. Documentation/kbuild/makefiles.rst:1142: WARNING: Inline emphasis start-string without end-string. Documentation/kbuild/makefiles.rst:1152: WARNING: Inline emphasis start-string without end-string. Documentation/kbuild/makefiles.rst:1154: WARNING: Inline emphasis start-string without end-string. Documentation/security/keys/core.rst:1110: WARNING: Inline emphasis start-string without end-string. Documentation/security/keys/core.rst:1110: WARNING: Inline emphasis start-string without end-string. Documentation/security/keys/core.rst:1108: WARNING: Inline emphasis start-string without end-string. Documentation/security/keys/core.rst:1108: WARNING: Inline emphasis start-string without end-string. Documentation/security/keys/core.rst:1108: WARNING: Inline emphasis start-string without end-string. Documentation/trace/kprobetrace.rst:99: WARNING: Explicit markup ends without a blank line; unexpected unindent. include/uapi/linux/firewire-cdev.h:312: WARNING: Inline literal start-string without end-string. drivers/firewire/core-transaction.c:606: WARNING: Inline strong start-string without end-string. drivers/message/fusion/mptbase.c:5057: WARNING: Definition list ends without a blank line; unexpected unindent. drivers/tty/serial/serial_core.c:1964: WARNING: Definition list ends without a blank line; unexpected unindent. drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c:245: WARNING: Unexpected indentation. drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c:248: WARNING: Block quote ends without a blank line; unexpected unindent. drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:271: WARNING: Unexpected indentation. vim +260 include/media/v4l2-ctrls.h 8ac7a9493a4380 Hans Verkuil 2012-09-07 126 8c2721d57a4bac Mauro Carvalho Chehab 2015-08-22 127 /** 8c2721d57a4bac Mauro Carvalho Chehab 2015-08-22 128 * struct v4l2_ctrl - The control structure. 8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22 129 * 0996517cf8eade Hans Verkuil 2010-08-01 130 * @node: The list node. 77068d36d8b9e9 Hans Verkuil 2011-06-13 131 * @ev_subs: The list of control event subscriptions. 0996517cf8eade Hans Verkuil 2010-08-01 132 * @handler: The handler that owns the control. 0996517cf8eade Hans Verkuil 2010-08-01 133 * @cluster: Point to start of cluster array. 0996517cf8eade Hans Verkuil 2010-08-01 134 * @ncontrols: Number of controls in cluster array. 0996517cf8eade Hans Verkuil 2010-08-01 135 * @done: Internal flag: set for each processed control. 2a863793beaa0f Hans Verkuil 2011-01-11 136 * @is_new: Set when the user specified a new value for this control. It 8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22 137 * is also set when called from v4l2_ctrl_handler_setup(). Drivers 2a863793beaa0f Hans Verkuil 2011-01-11 138 * should never set this flag. 9ea1b7a4b66fdd Hans Verkuil 2014-01-17 139 * @has_changed: Set when the current value differs from the new value. Drivers 9ea1b7a4b66fdd Hans Verkuil 2014-01-17 140 * should never use this flag. 0996517cf8eade Hans Verkuil 2010-08-01 141 * @is_private: If set, then this control is private to its handler and it 0996517cf8eade Hans Verkuil 2010-08-01 142 * will not be added to any other handlers. Drivers can set 0996517cf8eade Hans Verkuil 2010-08-01 143 * this flag. 72d877cac07c8d Hans Verkuil 2011-06-10 144 * @is_auto: If set, then this control selects whether the other cluster 72d877cac07c8d Hans Verkuil 2011-06-10 145 * members are in 'automatic' mode or 'manual' mode. This is 72d877cac07c8d Hans Verkuil 2011-06-10 146 * used for autogain/gain type clusters. Drivers should never 72d877cac07c8d Hans Verkuil 2011-06-10 147 * set this flag directly. d9a2547150245f Hans Verkuil 2014-06-12 148 * @is_int: If set, then this control has a simple integer value (i.e. it d9a2547150245f Hans Verkuil 2014-06-12 149 * uses ctrl->val). 8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22 150 * @is_string: If set, then this control has type %V4L2_CTRL_TYPE_STRING. 8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22 151 * @is_ptr: If set, then this control is an array and/or has type >= 8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22 152 * %V4L2_CTRL_COMPOUND_TYPES 8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22 153 * and/or has type %V4L2_CTRL_TYPE_STRING. In other words, &struct d9a2547150245f Hans Verkuil 2014-06-12 154 * v4l2_ext_control uses field p to point to the data. 998e7659150760 Hans Verkuil 2014-06-10 155 * @is_array: If set, then this control contains an N-dimensional array. 5626b8c75bc13a Hans Verkuil 2011-08-26 156 * @has_volatiles: If set, then one or more members of the cluster are volatile. 5626b8c75bc13a Hans Verkuil 2011-08-26 157 * Drivers should never touch this flag. 8ac7a9493a4380 Hans Verkuil 2012-09-07 158 * @call_notify: If set, then call the handler's notify function whenever the 8ac7a9493a4380 Hans Verkuil 2012-09-07 159 * control's value changes. 72d877cac07c8d Hans Verkuil 2011-06-10 160 * @manual_mode_value: If the is_auto flag is set, then this is the value 72d877cac07c8d Hans Verkuil 2011-06-10 161 * of the auto control that determines if that control is in 72d877cac07c8d Hans Verkuil 2011-06-10 162 * manual mode. So if the value of the auto control equals this 72d877cac07c8d Hans Verkuil 2011-06-10 163 * value, then the whole cluster is in manual mode. Drivers should 72d877cac07c8d Hans Verkuil 2011-06-10 164 * never set this flag directly. 0996517cf8eade Hans Verkuil 2010-08-01 165 * @ops: The control ops. 0176077a813933 Hans Verkuil 2014-04-27 166 * @type_ops: The control type ops. 0996517cf8eade Hans Verkuil 2010-08-01 167 * @id: The control ID. 0996517cf8eade Hans Verkuil 2010-08-01 168 * @name: The control name. 0996517cf8eade Hans Verkuil 2010-08-01 169 * @type: The control type. 0996517cf8eade Hans Verkuil 2010-08-01 170 * @minimum: The control's minimum value. 0996517cf8eade Hans Verkuil 2010-08-01 171 * @maximum: The control's maximum value. 0996517cf8eade Hans Verkuil 2010-08-01 172 * @default_value: The control's default value. 0996517cf8eade Hans Verkuil 2010-08-01 173 * @step: The control's step value for non-menu controls. 20d88eef66a869 Hans Verkuil 2014-06-12 174 * @elems: The number of elements in the N-dimensional array. d9a2547150245f Hans Verkuil 2014-06-12 175 * @elem_size: The size in bytes of the control. 20d88eef66a869 Hans Verkuil 2014-06-12 176 * @dims: The size of each dimension. 20d88eef66a869 Hans Verkuil 2014-06-12 177 * @nr_of_dims:The number of dimensions in @dims. 0996517cf8eade Hans Verkuil 2010-08-01 178 * @menu_skip_mask: The control's skip mask for menu controls. This makes it 0996517cf8eade Hans Verkuil 2010-08-01 179 * easy to skip menu items that are not valid. If bit X is set, 0996517cf8eade Hans Verkuil 2010-08-01 180 * then menu item X is skipped. Of course, this only works for 0996517cf8eade Hans Verkuil 2010-08-01 181 * menus with <= 32 menu items. There are no menus that come 0996517cf8eade Hans Verkuil 2010-08-01 182 * close to that number, so this is OK. Should we ever need more, 0996517cf8eade Hans Verkuil 2010-08-01 183 * then this will have to be extended to a u64 or a bit array. 0996517cf8eade Hans Verkuil 2010-08-01 184 * @qmenu: A const char * array for all menu items. Array entries that are 0996517cf8eade Hans Verkuil 2010-08-01 185 * empty strings ("") correspond to non-existing menu items (this 0996517cf8eade Hans Verkuil 2010-08-01 186 * is in addition to the menu_skip_mask above). The last entry 0996517cf8eade Hans Verkuil 2010-08-01 187 * must be NULL. 20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27 188 * Used only if the @type is %V4L2_CTRL_TYPE_MENU. 20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27 189 * @qmenu_int: A 64-bit integer array for with integer menu items. 20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27 190 * The size of array must be equal to the menu size, e. g.: 20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27 191 * :math:`ceil(\frac{maximum - minimum}{step}) + 1`. 20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27 192 * Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU. 0996517cf8eade Hans Verkuil 2010-08-01 193 * @flags: The control's flags. 20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27 194 * @cur: Structure to store the current value. 20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27 195 * @cur.val: The control's current value, if the @type is represented via 20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27 196 * a u32 integer (see &enum v4l2_ctrl_type). 0996517cf8eade Hans Verkuil 2010-08-01 197 * @val: The control's new s32 value. 0996517cf8eade Hans Verkuil 2010-08-01 198 * @priv: The control's private pointer. For use by the driver. It is 0996517cf8eade Hans Verkuil 2010-08-01 199 * untouched by the control framework. Note that this pointer is 0996517cf8eade Hans Verkuil 2010-08-01 200 * not freed when the control is deleted. Should this be needed 0996517cf8eade Hans Verkuil 2010-08-01 201 * then a new internal bitfield can be added to tell the framework 0996517cf8eade Hans Verkuil 2010-08-01 202 * to free this pointer. bf7b70482704bb Baruch Siach 2018-07-05 203 * @p_cur: The control's current value represented via a union which 7dc879190f55f4 Mauro Carvalho Chehab 2015-08-22 204 * provides a standard way of accessing control types 7dc879190f55f4 Mauro Carvalho Chehab 2015-08-22 205 * through a pointer. bf7b70482704bb Baruch Siach 2018-07-05 206 * @p_new: The control's new value represented via a union which provides 7dc879190f55f4 Mauro Carvalho Chehab 2015-08-22 207 * a standard way of accessing control types 7dc879190f55f4 Mauro Carvalho Chehab 2015-08-22 208 * through a pointer. 0996517cf8eade Hans Verkuil 2010-08-01 209 */ 0996517cf8eade Hans Verkuil 2010-08-01 210 struct v4l2_ctrl { 0996517cf8eade Hans Verkuil 2010-08-01 211 /* Administrative fields */ 0996517cf8eade Hans Verkuil 2010-08-01 212 struct list_head node; 77068d36d8b9e9 Hans Verkuil 2011-06-13 213 struct list_head ev_subs; 0996517cf8eade Hans Verkuil 2010-08-01 214 struct v4l2_ctrl_handler *handler; 0996517cf8eade Hans Verkuil 2010-08-01 215 struct v4l2_ctrl **cluster; 8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22 216 unsigned int ncontrols; 8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22 217 0996517cf8eade Hans Verkuil 2010-08-01 218 unsigned int done:1; 0996517cf8eade Hans Verkuil 2010-08-01 219 2a863793beaa0f Hans Verkuil 2011-01-11 220 unsigned int is_new:1; 9ea1b7a4b66fdd Hans Verkuil 2014-01-17 221 unsigned int has_changed:1; 0996517cf8eade Hans Verkuil 2010-08-01 222 unsigned int is_private:1; 72d877cac07c8d Hans Verkuil 2011-06-10 223 unsigned int is_auto:1; d9a2547150245f Hans Verkuil 2014-06-12 224 unsigned int is_int:1; d9a2547150245f Hans Verkuil 2014-06-12 225 unsigned int is_string:1; d9a2547150245f Hans Verkuil 2014-06-12 226 unsigned int is_ptr:1; 998e7659150760 Hans Verkuil 2014-06-10 227 unsigned int is_array:1; 5626b8c75bc13a Hans Verkuil 2011-08-26 228 unsigned int has_volatiles:1; 8ac7a9493a4380 Hans Verkuil 2012-09-07 229 unsigned int call_notify:1; 82a7c049449ec5 Hans Verkuil 2011-06-28 230 unsigned int manual_mode_value:8; 0996517cf8eade Hans Verkuil 2010-08-01 231 0996517cf8eade Hans Verkuil 2010-08-01 232 const struct v4l2_ctrl_ops *ops; 0176077a813933 Hans Verkuil 2014-04-27 233 const struct v4l2_ctrl_type_ops *type_ops; 0996517cf8eade Hans Verkuil 2010-08-01 234 u32 id; 0996517cf8eade Hans Verkuil 2010-08-01 235 const char *name; 0996517cf8eade Hans Verkuil 2010-08-01 236 enum v4l2_ctrl_type type; 0ba2aeb6dab809 Hans Verkuil 2014-04-16 237 s64 minimum, maximum, default_value; 20d88eef66a869 Hans Verkuil 2014-06-12 238 u32 elems; d9a2547150245f Hans Verkuil 2014-06-12 239 u32 elem_size; 20d88eef66a869 Hans Verkuil 2014-06-12 240 u32 dims[V4L2_CTRL_MAX_DIMS]; 20d88eef66a869 Hans Verkuil 2014-06-12 241 u32 nr_of_dims; 0996517cf8eade Hans Verkuil 2010-08-01 242 union { 0ba2aeb6dab809 Hans Verkuil 2014-04-16 243 u64 step; 0ba2aeb6dab809 Hans Verkuil 2014-04-16 244 u64 menu_skip_mask; 0996517cf8eade Hans Verkuil 2010-08-01 245 }; ce580fe5190dec Sakari Ailus 2011-08-04 246 union { 513521eaee4375 Hans Verkuil 2010-12-29 247 const char * const *qmenu; ce580fe5190dec Sakari Ailus 2011-08-04 248 const s64 *qmenu_int; ce580fe5190dec Sakari Ailus 2011-08-04 249 }; 0996517cf8eade Hans Verkuil 2010-08-01 250 unsigned long flags; d9a2547150245f Hans Verkuil 2014-06-12 251 void *priv; 0996517cf8eade Hans Verkuil 2010-08-01 252 s32 val; 2a9ec3731137f9 Hans Verkuil 2014-04-27 253 struct { 0996517cf8eade Hans Verkuil 2010-08-01 254 s32 val; d9a2547150245f Hans Verkuil 2014-06-12 255 } cur; 0176077a813933 Hans Verkuil 2014-04-27 256 4d9433f96f76aa Hans Verkuil 2019-09-12 257 const union v4l2_ctrl_ptr p_def; 0176077a813933 Hans Verkuil 2014-04-27 258 union v4l2_ctrl_ptr p_new; 0176077a813933 Hans Verkuil 2014-04-27 259 union v4l2_ctrl_ptr p_cur; 0996517cf8eade Hans Verkuil 2010-08-01 @260 }; 0996517cf8eade Hans Verkuil 2010-08-01 261 :::::: The code at line 260 was first introduced by commit :::::: 0996517cf8eaded69b8502c8f5abeb8cec62b6d4 V4L/DVB: v4l2: Add new control handling framework :::::: TO: Hans Verkuil <hverkuil@xs4all.nl> :::::: CC: Mauro Carvalho Chehab <mchehab@redhat.com> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 7282 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() 2019-09-12 13:35 Proposal for a v4l2_ctrl_new_std_compound() function Hans Verkuil 2019-09-13 16:08 ` kbuild test robot @ 2019-09-17 16:19 ` Ricardo Ribalda Delgado 2019-09-17 16:21 ` Ricardo Ribalda Delgado 1 sibling, 1 reply; 7+ messages in thread From: Ricardo Ribalda Delgado @ 2019-09-17 16:19 UTC (permalink / raw) To: hverkuil, linux-media; +Cc: Ricardo Ribalda Delgado Implement v4l2_ctrl_new_std_compound. This is just a discussion patch, do not merge as is, and be gentle with the author ;P. Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> --- drivers/media/i2c/imx214.c | 13 +++-- drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++----------- include/media/v4l2-ctrls.h | 25 +++++++++ 3 files changed, 81 insertions(+), 36 deletions(-) diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c index 625617d4c81a..e18fed9f31f1 100644 --- a/drivers/media/i2c/imx214.c +++ b/drivers/media/i2c/imx214.c @@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client) .width = 1120, .height = 1120, }; + union v4l2_ctrl_ptr p_def = { + .p_area = &unit_size, + }; int ret; ret = imx214_parse_fwnode(dev); @@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client) V4L2_CID_EXPOSURE, 0, 3184, 1, 0x0c70); - imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls, - &imx214_ctrl_ops, - V4L2_CID_UNIT_CELL_SIZE, - &unit_size); - + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, + NULL, + V4L2_CID_UNIT_CELL_SIZE, + 0, 0x7ffffff, 1, 0, + p_def); ret = imx214->ctrls.error; if (ret) { dev_err(&client->dev, "%s control init failed (%d)\n", diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 3d2fa1868982..04813ba2262b 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx, p_mpeg2_slice_params->picture.picture_coding_type = V4L2_MPEG2_PICTURE_CODING_TYPE_I; break; + default: + if (ctrl->has_p_def) + memcpy(p, ctrl->p_def.p, ctrl->elem_size); + break; } } @@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, s64 min, s64 max, u64 step, s64 def, const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size, u32 flags, const char * const *qmenu, - const s64 *qmenu_int, void *priv) + const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def, + void *priv) { struct v4l2_ctrl *ctrl; unsigned sz_extra; @@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, is_array) sz_extra += 2 * tot_ctrl_size; + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) + sz_extra += elem_size; + ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL); if (ctrl == NULL) { handler_set_err(hdl, -ENOMEM); @@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, ctrl->p_new.p = &ctrl->val; ctrl->p_cur.p = &ctrl->cur.val; } + + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) { + ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size; + memcpy(ctrl->p_def.p, p_def.p, elem_size); + ctrl->has_p_def = true; + } + for (idx = 0; idx < elems; idx++) { ctrl->type_ops->init(ctrl, idx, ctrl->p_cur); ctrl->type_ops->init(ctrl, idx, ctrl->p_new); @@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, s64 max = cfg->max; u64 step = cfg->step; s64 def = cfg->def; + const union v4l2_ctrl_ptr p_def = {}; if (name == NULL) v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step, @@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, type, min, max, is_menu ? cfg->menu_skip_mask : step, def, cfg->dims, cfg->elem_size, - flags, qmenu, qmenu_int, priv); + flags, qmenu, qmenu_int, p_def, priv); if (ctrl) ctrl->is_private = cfg->is_private; return ctrl; @@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, const char *name; enum v4l2_ctrl_type type; u32 flags; + const union v4l2_ctrl_ptr p_def = {}; v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); if (type == V4L2_CTRL_TYPE_MENU || @@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, } return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, min, max, step, def, NULL, 0, - flags, NULL, NULL, NULL); + flags, NULL, NULL, p_def, NULL); } EXPORT_SYMBOL(v4l2_ctrl_new_std); @@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, s64 def = _def; u64 step; u32 flags; + const union v4l2_ctrl_ptr p_def = {}; v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); @@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, } return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, 0, max, mask, def, NULL, 0, - flags, qmenu, qmenu_int, NULL); + flags, qmenu, qmenu_int, p_def, NULL); } EXPORT_SYMBOL(v4l2_ctrl_new_std_menu); @@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, s64 min; s64 max = _max; s64 def = _def; + const union v4l2_ctrl_ptr p_def = {}; /* v4l2_ctrl_new_std_menu_items() should only be called for * standard controls without a standard menu. @@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, } return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, 0, max, mask, def, NULL, 0, - flags, qmenu, NULL, NULL); + flags, qmenu, NULL, p_def, NULL); } EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); +/* Helper function for standard compound controls */ +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, + const struct v4l2_ctrl_ops *ops, + u32 id, s64 min, s64 max, u64 step, s64 def, + const union v4l2_ctrl_ptr p_def) +{ + const char *name; + enum v4l2_ctrl_type type; + u32 flags; + + v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); + if (type == V4L2_CTRL_TYPE_MENU || + type == V4L2_CTRL_TYPE_INTEGER_MENU) { + handler_set_err(hdl, -EINVAL); + return NULL; + } + return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, + min, max, step, def, NULL, 0, + flags, NULL, NULL, p_def, NULL); +} +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound); + /* Helper function for standard integer menu controls */ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, const struct v4l2_ctrl_ops *ops, @@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, s64 max = _max; s64 def = _def; u32 flags; + const union v4l2_ctrl_ptr p_def = {}; v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); if (type != V4L2_CTRL_TYPE_INTEGER_MENU) { @@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, } return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, 0, max, 0, def, NULL, 0, - flags, NULL, qmenu_int, NULL); + flags, NULL, qmenu_int, p_def, NULL); } EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); @@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags) return try_or_set_cluster(fh, master, true, ch_flags); } -/* Helper function for area controls */ -struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, - const struct v4l2_ctrl_ops *ops, - u32 id, const struct v4l2_area *area) -{ - static struct v4l2_ctrl_config ctrl = {}; - struct v4l2_ctrl *c; - int ret; - - ctrl.id = id; - c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); - if (!c) - return NULL; - - memcpy(c->p_new.p_area, area, sizeof(*area)); - ret = set_ctrl(NULL, c, 0); - if (ret){ - hdl->error = ret; - return NULL; - } - - return c; -} -EXPORT_SYMBOL(v4l2_ctrl_new_area); - /* Helper function for VIDIOC_S_CTRL compatibility */ static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c) diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h index b5b42777a756..8dc7e9827056 100644 --- a/include/media/v4l2-ctrls.h +++ b/include/media/v4l2-ctrls.h @@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); * manual mode. So if the value of the auto control equals this * value, then the whole cluster is in manual mode. Drivers should * never set this flag directly. + * @has_p_def: If set, the p_def field points to the default value of the control. * @ops: The control ops. * @type_ops: The control type ops. * @id: The control ID. @@ -230,6 +231,7 @@ struct v4l2_ctrl { unsigned int has_volatiles:1; unsigned int call_notify:1; unsigned int manual_mode_value:8; + unsigned int has_p_def:1; const struct v4l2_ctrl_ops *ops; const struct v4l2_ctrl_type_ops *type_ops; @@ -256,6 +258,7 @@ struct v4l2_ctrl { s32 val; } cur; + union v4l2_ctrl_ptr p_def; union v4l2_ctrl_ptr p_new; union v4l2_ctrl_ptr p_cur; }; @@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, u32 id, u8 max, u64 mask, u8 def, const char * const *qmenu); +/** + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2 + * compound control. + * + * @hdl: The control handler. + * @ops: The control ops. + * @id: The control ID. + * @min: The control's minimum value. + * @max: The control's maximum value. + * @step: The control's step value + * @def: The control's default value. + * @p_def: The control's p_def value. + * + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to + * the @p_def field. + * + */ +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, + const struct v4l2_ctrl_ops *ops, + u32 id, s64 min, s64 max, u64 step, + s64 def, const union v4l2_ctrl_ptr p_def); + /** * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control. -- 2.23.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() 2019-09-17 16:19 ` [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() Ricardo Ribalda Delgado @ 2019-09-17 16:21 ` Ricardo Ribalda Delgado 2019-09-20 10:06 ` Hans Verkuil 0 siblings, 1 reply; 7+ messages in thread From: Ricardo Ribalda Delgado @ 2019-09-17 16:21 UTC (permalink / raw) To: Hans Verkuil, linux-media Hi Hans Is this something close to what you were having in mind? Right now it sits on https://github.com/ribalda/linux/commit/de21dbc2f57b58b22f5d73bc314dd8e59dff5c7d but I will make it as the beginning of my patchset if you think that I am on the right track. Thanks! On Tue, Sep 17, 2019 at 6:19 PM Ricardo Ribalda Delgado <ribalda@kernel.org> wrote: > > Implement v4l2_ctrl_new_std_compound. This is just a discussion patch, > do not merge as is, and be gentle with the author ;P. > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> > --- > drivers/media/i2c/imx214.c | 13 +++-- > drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++----------- > include/media/v4l2-ctrls.h | 25 +++++++++ > 3 files changed, 81 insertions(+), 36 deletions(-) > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > index 625617d4c81a..e18fed9f31f1 100644 > --- a/drivers/media/i2c/imx214.c > +++ b/drivers/media/i2c/imx214.c > @@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client) > .width = 1120, > .height = 1120, > }; > + union v4l2_ctrl_ptr p_def = { > + .p_area = &unit_size, > + }; > int ret; > > ret = imx214_parse_fwnode(dev); > @@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client) > V4L2_CID_EXPOSURE, > 0, 3184, 1, 0x0c70); > > - imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls, > - &imx214_ctrl_ops, > - V4L2_CID_UNIT_CELL_SIZE, > - &unit_size); > - > + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, > + NULL, > + V4L2_CID_UNIT_CELL_SIZE, > + 0, 0x7ffffff, 1, 0, > + p_def); > ret = imx214->ctrls.error; > if (ret) { > dev_err(&client->dev, "%s control init failed (%d)\n", > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 3d2fa1868982..04813ba2262b 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx, > p_mpeg2_slice_params->picture.picture_coding_type = > V4L2_MPEG2_PICTURE_CODING_TYPE_I; > break; > + default: > + if (ctrl->has_p_def) > + memcpy(p, ctrl->p_def.p, ctrl->elem_size); > + break; > } > } > > @@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > s64 min, s64 max, u64 step, s64 def, > const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size, > u32 flags, const char * const *qmenu, > - const s64 *qmenu_int, void *priv) > + const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def, > + void *priv) > { > struct v4l2_ctrl *ctrl; > unsigned sz_extra; > @@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > is_array) > sz_extra += 2 * tot_ctrl_size; > > + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) > + sz_extra += elem_size; > + > ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL); > if (ctrl == NULL) { > handler_set_err(hdl, -ENOMEM); > @@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > ctrl->p_new.p = &ctrl->val; > ctrl->p_cur.p = &ctrl->cur.val; > } > + > + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) { > + ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size; > + memcpy(ctrl->p_def.p, p_def.p, elem_size); > + ctrl->has_p_def = true; > + } > + > for (idx = 0; idx < elems; idx++) { > ctrl->type_ops->init(ctrl, idx, ctrl->p_cur); > ctrl->type_ops->init(ctrl, idx, ctrl->p_new); > @@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, > s64 max = cfg->max; > u64 step = cfg->step; > s64 def = cfg->def; > + const union v4l2_ctrl_ptr p_def = {}; > > if (name == NULL) > v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step, > @@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, > type, min, max, > is_menu ? cfg->menu_skip_mask : step, def, > cfg->dims, cfg->elem_size, > - flags, qmenu, qmenu_int, priv); > + flags, qmenu, qmenu_int, p_def, priv); > if (ctrl) > ctrl->is_private = cfg->is_private; > return ctrl; > @@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, > const char *name; > enum v4l2_ctrl_type type; > u32 flags; > + const union v4l2_ctrl_ptr p_def = {}; > > v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); > if (type == V4L2_CTRL_TYPE_MENU || > @@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, > } > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > min, max, step, def, NULL, 0, > - flags, NULL, NULL, NULL); > + flags, NULL, NULL, p_def, NULL); > } > EXPORT_SYMBOL(v4l2_ctrl_new_std); > > @@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, > s64 def = _def; > u64 step; > u32 flags; > + const union v4l2_ctrl_ptr p_def = {}; > > v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); > > @@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, > } > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > 0, max, mask, def, NULL, 0, > - flags, qmenu, qmenu_int, NULL); > + flags, qmenu, qmenu_int, p_def, NULL); > } > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu); > > @@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > s64 min; > s64 max = _max; > s64 def = _def; > + const union v4l2_ctrl_ptr p_def = {}; > > /* v4l2_ctrl_new_std_menu_items() should only be called for > * standard controls without a standard menu. > @@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > } > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > 0, max, mask, def, NULL, 0, > - flags, qmenu, NULL, NULL); > + flags, qmenu, NULL, p_def, NULL); > > } > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > > +/* Helper function for standard compound controls */ > +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > + const struct v4l2_ctrl_ops *ops, > + u32 id, s64 min, s64 max, u64 step, s64 def, > + const union v4l2_ctrl_ptr p_def) > +{ > + const char *name; > + enum v4l2_ctrl_type type; > + u32 flags; > + > + v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); > + if (type == V4L2_CTRL_TYPE_MENU || > + type == V4L2_CTRL_TYPE_INTEGER_MENU) { > + handler_set_err(hdl, -EINVAL); > + return NULL; > + } > + return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > + min, max, step, def, NULL, 0, > + flags, NULL, NULL, p_def, NULL); > +} > +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound); > + > /* Helper function for standard integer menu controls */ > struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > const struct v4l2_ctrl_ops *ops, > @@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > s64 max = _max; > s64 def = _def; > u32 flags; > + const union v4l2_ctrl_ptr p_def = {}; > > v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); > if (type != V4L2_CTRL_TYPE_INTEGER_MENU) { > @@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > } > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > 0, max, 0, def, NULL, 0, > - flags, NULL, qmenu_int, NULL); > + flags, NULL, qmenu_int, p_def, NULL); > } > EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); > > @@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags) > return try_or_set_cluster(fh, master, true, ch_flags); > } > > -/* Helper function for area controls */ > -struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, > - const struct v4l2_ctrl_ops *ops, > - u32 id, const struct v4l2_area *area) > -{ > - static struct v4l2_ctrl_config ctrl = {}; > - struct v4l2_ctrl *c; > - int ret; > - > - ctrl.id = id; > - c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); > - if (!c) > - return NULL; > - > - memcpy(c->p_new.p_area, area, sizeof(*area)); > - ret = set_ctrl(NULL, c, 0); > - if (ret){ > - hdl->error = ret; > - return NULL; > - } > - > - return c; > -} > -EXPORT_SYMBOL(v4l2_ctrl_new_area); > - > /* Helper function for VIDIOC_S_CTRL compatibility */ > static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, > struct v4l2_ext_control *c) > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index b5b42777a756..8dc7e9827056 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); > * manual mode. So if the value of the auto control equals this > * value, then the whole cluster is in manual mode. Drivers should > * never set this flag directly. > + * @has_p_def: If set, the p_def field points to the default value of the control. > * @ops: The control ops. > * @type_ops: The control type ops. > * @id: The control ID. > @@ -230,6 +231,7 @@ struct v4l2_ctrl { > unsigned int has_volatiles:1; > unsigned int call_notify:1; > unsigned int manual_mode_value:8; > + unsigned int has_p_def:1; > > const struct v4l2_ctrl_ops *ops; > const struct v4l2_ctrl_type_ops *type_ops; > @@ -256,6 +258,7 @@ struct v4l2_ctrl { > s32 val; > } cur; > > + union v4l2_ctrl_ptr p_def; > union v4l2_ctrl_ptr p_new; > union v4l2_ctrl_ptr p_cur; > }; > @@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > u32 id, u8 max, > u64 mask, u8 def, > const char * const *qmenu); > +/** > + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2 > + * compound control. > + * > + * @hdl: The control handler. > + * @ops: The control ops. > + * @id: The control ID. > + * @min: The control's minimum value. > + * @max: The control's maximum value. > + * @step: The control's step value > + * @def: The control's default value. > + * @p_def: The control's p_def value. > + * > + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to > + * the @p_def field. > + * > + */ > +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > + const struct v4l2_ctrl_ops *ops, > + u32 id, s64 min, s64 max, u64 step, > + s64 def, const union v4l2_ctrl_ptr p_def); > + > > /** > * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control. > -- > 2.23.0 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() 2019-09-17 16:21 ` Ricardo Ribalda Delgado @ 2019-09-20 10:06 ` Hans Verkuil 2019-09-20 10:33 ` Ricardo Ribalda Delgado 0 siblings, 1 reply; 7+ messages in thread From: Hans Verkuil @ 2019-09-20 10:06 UTC (permalink / raw) To: Ricardo Ribalda Delgado, linux-media On 9/17/19 6:21 PM, Ricardo Ribalda Delgado wrote: > Hi Hans > > Is this something close to what you were having in mind? Right now it > sits on https://github.com/ribalda/linux/commit/de21dbc2f57b58b22f5d73bc314dd8e59dff5c7d > but I will make it as the beginning of my patchset if you think that I > am on the right track. > > Thanks! > > On Tue, Sep 17, 2019 at 6:19 PM Ricardo Ribalda Delgado > <ribalda@kernel.org> wrote: >> >> Implement v4l2_ctrl_new_std_compound. This is just a discussion patch, >> do not merge as is, and be gentle with the author ;P. >> >> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> >> --- >> drivers/media/i2c/imx214.c | 13 +++-- >> drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++----------- >> include/media/v4l2-ctrls.h | 25 +++++++++ >> 3 files changed, 81 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c >> index 625617d4c81a..e18fed9f31f1 100644 >> --- a/drivers/media/i2c/imx214.c >> +++ b/drivers/media/i2c/imx214.c >> @@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client) >> .width = 1120, >> .height = 1120, >> }; >> + union v4l2_ctrl_ptr p_def = { >> + .p_area = &unit_size, >> + }; >> int ret; >> >> ret = imx214_parse_fwnode(dev); >> @@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client) >> V4L2_CID_EXPOSURE, >> 0, 3184, 1, 0x0c70); >> >> - imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls, >> - &imx214_ctrl_ops, >> - V4L2_CID_UNIT_CELL_SIZE, >> - &unit_size); >> - >> + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, >> + NULL, >> + V4L2_CID_UNIT_CELL_SIZE, >> + 0, 0x7ffffff, 1, 0, >> + p_def); >> ret = imx214->ctrls.error; >> if (ret) { >> dev_err(&client->dev, "%s control init failed (%d)\n", >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c >> index 3d2fa1868982..04813ba2262b 100644 >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >> @@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx, >> p_mpeg2_slice_params->picture.picture_coding_type = >> V4L2_MPEG2_PICTURE_CODING_TYPE_I; >> break; >> + default: >> + if (ctrl->has_p_def) >> + memcpy(p, ctrl->p_def.p, ctrl->elem_size); >> + break; It makes more sense to do this at the beginning of this function: if (ctrl->p_def.p) memcpy(p, ctrl->p_def.p, ctrl->elem_size); else memset(p, 0, ctrl->elem_size); and then enter the switch. This avoids calling memset followed by a memcpy. >> } >> } >> >> @@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >> s64 min, s64 max, u64 step, s64 def, >> const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size, >> u32 flags, const char * const *qmenu, >> - const s64 *qmenu_int, void *priv) >> + const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def, >> + void *priv) >> { >> struct v4l2_ctrl *ctrl; >> unsigned sz_extra; >> @@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >> is_array) >> sz_extra += 2 * tot_ctrl_size; >> >> + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) >> + sz_extra += elem_size; >> + >> ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL); >> if (ctrl == NULL) { >> handler_set_err(hdl, -ENOMEM); >> @@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >> ctrl->p_new.p = &ctrl->val; >> ctrl->p_cur.p = &ctrl->cur.val; >> } >> + >> + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) { >> + ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size; >> + memcpy(ctrl->p_def.p, p_def.p, elem_size); >> + ctrl->has_p_def = true; Is this needed? ctrl->p_def.p would be NULL if there is no p_def. >> + } >> + >> for (idx = 0; idx < elems; idx++) { >> ctrl->type_ops->init(ctrl, idx, ctrl->p_cur); >> ctrl->type_ops->init(ctrl, idx, ctrl->p_new); >> @@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, >> s64 max = cfg->max; >> u64 step = cfg->step; >> s64 def = cfg->def; >> + const union v4l2_ctrl_ptr p_def = {}; I think it is cleaner to have a 'static const union v4l2_ctrl_ptr ptr_null;' at the start of the source and just use that. >> >> if (name == NULL) >> v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step, >> @@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, >> type, min, max, >> is_menu ? cfg->menu_skip_mask : step, def, >> cfg->dims, cfg->elem_size, >> - flags, qmenu, qmenu_int, priv); >> + flags, qmenu, qmenu_int, p_def, priv); >> if (ctrl) >> ctrl->is_private = cfg->is_private; >> return ctrl; >> @@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, >> const char *name; >> enum v4l2_ctrl_type type; >> u32 flags; >> + const union v4l2_ctrl_ptr p_def = {}; >> >> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >> if (type == V4L2_CTRL_TYPE_MENU || >> @@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, >> } >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >> min, max, step, def, NULL, 0, >> - flags, NULL, NULL, NULL); >> + flags, NULL, NULL, p_def, NULL); >> } >> EXPORT_SYMBOL(v4l2_ctrl_new_std); >> >> @@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, >> s64 def = _def; >> u64 step; >> u32 flags; >> + const union v4l2_ctrl_ptr p_def = {}; >> >> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >> >> @@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, >> } >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >> 0, max, mask, def, NULL, 0, >> - flags, qmenu, qmenu_int, NULL); >> + flags, qmenu, qmenu_int, p_def, NULL); >> } >> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu); >> >> @@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >> s64 min; >> s64 max = _max; >> s64 def = _def; >> + const union v4l2_ctrl_ptr p_def = {}; >> >> /* v4l2_ctrl_new_std_menu_items() should only be called for >> * standard controls without a standard menu. >> @@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >> } >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >> 0, max, mask, def, NULL, 0, >> - flags, qmenu, NULL, NULL); >> + flags, qmenu, NULL, p_def, NULL); >> >> } >> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); >> >> +/* Helper function for standard compound controls */ >> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, >> + const struct v4l2_ctrl_ops *ops, >> + u32 id, s64 min, s64 max, u64 step, s64 def, The def arg makes no sense, since that's superseded by p_def. >> + const union v4l2_ctrl_ptr p_def) >> +{ >> + const char *name; >> + enum v4l2_ctrl_type type; >> + u32 flags; Add: s64 def = min; It will be discarded anyway. >> + >> + v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >> + if (type == V4L2_CTRL_TYPE_MENU || >> + type == V4L2_CTRL_TYPE_INTEGER_MENU) { >> + handler_set_err(hdl, -EINVAL); >> + return NULL; >> + } This makes no sense. It should just check that the type is a compound type and return an error if it isn't. >> + return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >> + min, max, step, def, NULL, 0, >> + flags, NULL, NULL, p_def, NULL); >> +} >> +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound); >> + >> /* Helper function for standard integer menu controls */ >> struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, >> const struct v4l2_ctrl_ops *ops, >> @@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, >> s64 max = _max; >> s64 def = _def; >> u32 flags; >> + const union v4l2_ctrl_ptr p_def = {}; >> >> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >> if (type != V4L2_CTRL_TYPE_INTEGER_MENU) { >> @@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, >> } >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >> 0, max, 0, def, NULL, 0, >> - flags, NULL, qmenu_int, NULL); >> + flags, NULL, qmenu_int, p_def, NULL); >> } >> EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); >> >> @@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags) >> return try_or_set_cluster(fh, master, true, ch_flags); >> } >> >> -/* Helper function for area controls */ >> -struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, >> - const struct v4l2_ctrl_ops *ops, >> - u32 id, const struct v4l2_area *area) >> -{ >> - static struct v4l2_ctrl_config ctrl = {}; >> - struct v4l2_ctrl *c; >> - int ret; >> - >> - ctrl.id = id; >> - c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); >> - if (!c) >> - return NULL; >> - >> - memcpy(c->p_new.p_area, area, sizeof(*area)); >> - ret = set_ctrl(NULL, c, 0); >> - if (ret){ >> - hdl->error = ret; >> - return NULL; >> - } >> - >> - return c; >> -} >> -EXPORT_SYMBOL(v4l2_ctrl_new_area); >> - >> /* Helper function for VIDIOC_S_CTRL compatibility */ >> static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, >> struct v4l2_ext_control *c) >> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h >> index b5b42777a756..8dc7e9827056 100644 >> --- a/include/media/v4l2-ctrls.h >> +++ b/include/media/v4l2-ctrls.h >> @@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); >> * manual mode. So if the value of the auto control equals this >> * value, then the whole cluster is in manual mode. Drivers should >> * never set this flag directly. >> + * @has_p_def: If set, the p_def field points to the default value of the control. >> * @ops: The control ops. >> * @type_ops: The control type ops. >> * @id: The control ID. >> @@ -230,6 +231,7 @@ struct v4l2_ctrl { >> unsigned int has_volatiles:1; >> unsigned int call_notify:1; >> unsigned int manual_mode_value:8; >> + unsigned int has_p_def:1; >> >> const struct v4l2_ctrl_ops *ops; >> const struct v4l2_ctrl_type_ops *type_ops; >> @@ -256,6 +258,7 @@ struct v4l2_ctrl { >> s32 val; >> } cur; >> >> + union v4l2_ctrl_ptr p_def; >> union v4l2_ctrl_ptr p_new; >> union v4l2_ctrl_ptr p_cur; >> }; >> @@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >> u32 id, u8 max, >> u64 mask, u8 def, >> const char * const *qmenu); >> +/** >> + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2 >> + * compound control. >> + * >> + * @hdl: The control handler. >> + * @ops: The control ops. >> + * @id: The control ID. >> + * @min: The control's minimum value. >> + * @max: The control's maximum value. >> + * @step: The control's step value >> + * @def: The control's default value. >> + * @p_def: The control's p_def value. >> + * >> + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to >> + * the @p_def field. >> + * >> + */ >> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, >> + const struct v4l2_ctrl_ops *ops, >> + u32 id, s64 min, s64 max, u64 step, >> + s64 def, const union v4l2_ctrl_ptr p_def); >> + >> >> /** >> * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control. >> -- >> 2.23.0 >> So other than these fairly minor points, this is indeed what I was looking for. Regards, Hans ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() 2019-09-20 10:06 ` Hans Verkuil @ 2019-09-20 10:33 ` Ricardo Ribalda Delgado 2019-09-20 10:38 ` Hans Verkuil 0 siblings, 1 reply; 7+ messages in thread From: Ricardo Ribalda Delgado @ 2019-09-20 10:33 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media Hello Hans Thanks for your review! I will implement the changes and resend, just one comment. On Fri, Sep 20, 2019 at 12:07 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On 9/17/19 6:21 PM, Ricardo Ribalda Delgado wrote: > > Hi Hans > > > > Is this something close to what you were having in mind? Right now it > > sits on https://github.com/ribalda/linux/commit/de21dbc2f57b58b22f5d73bc314dd8e59dff5c7d > > but I will make it as the beginning of my patchset if you think that I > > am on the right track. > > > > Thanks! > > > > On Tue, Sep 17, 2019 at 6:19 PM Ricardo Ribalda Delgado > > <ribalda@kernel.org> wrote: > >> > >> Implement v4l2_ctrl_new_std_compound. This is just a discussion patch, > >> do not merge as is, and be gentle with the author ;P. > >> > >> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> > >> --- > >> drivers/media/i2c/imx214.c | 13 +++-- > >> drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++----------- > >> include/media/v4l2-ctrls.h | 25 +++++++++ > >> 3 files changed, 81 insertions(+), 36 deletions(-) > >> > >> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > >> index 625617d4c81a..e18fed9f31f1 100644 > >> --- a/drivers/media/i2c/imx214.c > >> +++ b/drivers/media/i2c/imx214.c > >> @@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client) > >> .width = 1120, > >> .height = 1120, > >> }; > >> + union v4l2_ctrl_ptr p_def = { > >> + .p_area = &unit_size, > >> + }; > >> int ret; > >> > >> ret = imx214_parse_fwnode(dev); > >> @@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client) > >> V4L2_CID_EXPOSURE, > >> 0, 3184, 1, 0x0c70); > >> > >> - imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls, > >> - &imx214_ctrl_ops, > >> - V4L2_CID_UNIT_CELL_SIZE, > >> - &unit_size); > >> - > >> + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, > >> + NULL, > >> + V4L2_CID_UNIT_CELL_SIZE, > >> + 0, 0x7ffffff, 1, 0, > >> + p_def); > >> ret = imx214->ctrls.error; > >> if (ret) { > >> dev_err(&client->dev, "%s control init failed (%d)\n", > >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > >> index 3d2fa1868982..04813ba2262b 100644 > >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c > >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > >> @@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx, > >> p_mpeg2_slice_params->picture.picture_coding_type = > >> V4L2_MPEG2_PICTURE_CODING_TYPE_I; > >> break; > >> + default: > >> + if (ctrl->has_p_def) > >> + memcpy(p, ctrl->p_def.p, ctrl->elem_size); > >> + break; > > It makes more sense to do this at the beginning of this function: > > if (ctrl->p_def.p) > memcpy(p, ctrl->p_def.p, ctrl->elem_size); > else > memset(p, 0, ctrl->elem_size); > > and then enter the switch. > > This avoids calling memset followed by a memcpy. > > >> } > >> } > >> > >> @@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > >> s64 min, s64 max, u64 step, s64 def, > >> const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size, > >> u32 flags, const char * const *qmenu, > >> - const s64 *qmenu_int, void *priv) > >> + const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def, > >> + void *priv) > >> { > >> struct v4l2_ctrl *ctrl; > >> unsigned sz_extra; > >> @@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > >> is_array) > >> sz_extra += 2 * tot_ctrl_size; > >> > >> + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) > >> + sz_extra += elem_size; > >> + > >> ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL); > >> if (ctrl == NULL) { > >> handler_set_err(hdl, -ENOMEM); > >> @@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > >> ctrl->p_new.p = &ctrl->val; > >> ctrl->p_cur.p = &ctrl->cur.val; > >> } > >> + > >> + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) { > >> + ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size; > >> + memcpy(ctrl->p_def.p, p_def.p, elem_size); > >> + ctrl->has_p_def = true; > > Is this needed? ctrl->p_def.p would be NULL if there is no p_def. > > >> + } > >> + > >> for (idx = 0; idx < elems; idx++) { > >> ctrl->type_ops->init(ctrl, idx, ctrl->p_cur); > >> ctrl->type_ops->init(ctrl, idx, ctrl->p_new); > >> @@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, > >> s64 max = cfg->max; > >> u64 step = cfg->step; > >> s64 def = cfg->def; > >> + const union v4l2_ctrl_ptr p_def = {}; > > I think it is cleaner to have a 'static const union v4l2_ctrl_ptr ptr_null;' > at the start of the source and just use that. > > >> > >> if (name == NULL) > >> v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step, > >> @@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, > >> type, min, max, > >> is_menu ? cfg->menu_skip_mask : step, def, > >> cfg->dims, cfg->elem_size, > >> - flags, qmenu, qmenu_int, priv); > >> + flags, qmenu, qmenu_int, p_def, priv); > >> if (ctrl) > >> ctrl->is_private = cfg->is_private; > >> return ctrl; > >> @@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, > >> const char *name; > >> enum v4l2_ctrl_type type; > >> u32 flags; > >> + const union v4l2_ctrl_ptr p_def = {}; > >> > >> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); > >> if (type == V4L2_CTRL_TYPE_MENU || > >> @@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, > >> } > >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > >> min, max, step, def, NULL, 0, > >> - flags, NULL, NULL, NULL); > >> + flags, NULL, NULL, p_def, NULL); > >> } > >> EXPORT_SYMBOL(v4l2_ctrl_new_std); > >> > >> @@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, > >> s64 def = _def; > >> u64 step; > >> u32 flags; > >> + const union v4l2_ctrl_ptr p_def = {}; > >> > >> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); > >> > >> @@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, > >> } > >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > >> 0, max, mask, def, NULL, 0, > >> - flags, qmenu, qmenu_int, NULL); > >> + flags, qmenu, qmenu_int, p_def, NULL); > >> } > >> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu); > >> > >> @@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > >> s64 min; > >> s64 max = _max; > >> s64 def = _def; > >> + const union v4l2_ctrl_ptr p_def = {}; > >> > >> /* v4l2_ctrl_new_std_menu_items() should only be called for > >> * standard controls without a standard menu. > >> @@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > >> } > >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > >> 0, max, mask, def, NULL, 0, > >> - flags, qmenu, NULL, NULL); > >> + flags, qmenu, NULL, p_def, NULL); > >> > >> } > >> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > >> > >> +/* Helper function for standard compound controls */ > >> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > >> + const struct v4l2_ctrl_ops *ops, > >> + u32 id, s64 min, s64 max, u64 step, s64 def, > > The def arg makes no sense, since that's superseded by p_def. > If def does not make sense, then for the same reason min, max and step should not make any sense (because they are not a pointer but a integer). What about removing them completely from the function prototype and add them later if we change our mind, it is a internal API. > >> + const union v4l2_ctrl_ptr p_def) > >> +{ > >> + const char *name; > >> + enum v4l2_ctrl_type type; > >> + u32 flags; > > Add: > > s64 def = min; > > It will be discarded anyway. > > >> + > >> + v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); > >> + if (type == V4L2_CTRL_TYPE_MENU || > >> + type == V4L2_CTRL_TYPE_INTEGER_MENU) { > >> + handler_set_err(hdl, -EINVAL); > >> + return NULL; > >> + } > > This makes no sense. It should just check that the type is a compound type and > return an error if it isn't. > > >> + return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > >> + min, max, step, def, NULL, 0, > >> + flags, NULL, NULL, p_def, NULL); > >> +} > >> +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound); > >> + > >> /* Helper function for standard integer menu controls */ > >> struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > >> const struct v4l2_ctrl_ops *ops, > >> @@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > >> s64 max = _max; > >> s64 def = _def; > >> u32 flags; > >> + const union v4l2_ctrl_ptr p_def = {}; > >> > >> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); > >> if (type != V4L2_CTRL_TYPE_INTEGER_MENU) { > >> @@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > >> } > >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > >> 0, max, 0, def, NULL, 0, > >> - flags, NULL, qmenu_int, NULL); > >> + flags, NULL, qmenu_int, p_def, NULL); > >> } > >> EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); > >> > >> @@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags) > >> return try_or_set_cluster(fh, master, true, ch_flags); > >> } > >> > >> -/* Helper function for area controls */ > >> -struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, > >> - const struct v4l2_ctrl_ops *ops, > >> - u32 id, const struct v4l2_area *area) > >> -{ > >> - static struct v4l2_ctrl_config ctrl = {}; > >> - struct v4l2_ctrl *c; > >> - int ret; > >> - > >> - ctrl.id = id; > >> - c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); > >> - if (!c) > >> - return NULL; > >> - > >> - memcpy(c->p_new.p_area, area, sizeof(*area)); > >> - ret = set_ctrl(NULL, c, 0); > >> - if (ret){ > >> - hdl->error = ret; > >> - return NULL; > >> - } > >> - > >> - return c; > >> -} > >> -EXPORT_SYMBOL(v4l2_ctrl_new_area); > >> - > >> /* Helper function for VIDIOC_S_CTRL compatibility */ > >> static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, > >> struct v4l2_ext_control *c) > >> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > >> index b5b42777a756..8dc7e9827056 100644 > >> --- a/include/media/v4l2-ctrls.h > >> +++ b/include/media/v4l2-ctrls.h > >> @@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); > >> * manual mode. So if the value of the auto control equals this > >> * value, then the whole cluster is in manual mode. Drivers should > >> * never set this flag directly. > >> + * @has_p_def: If set, the p_def field points to the default value of the control. > >> * @ops: The control ops. > >> * @type_ops: The control type ops. > >> * @id: The control ID. > >> @@ -230,6 +231,7 @@ struct v4l2_ctrl { > >> unsigned int has_volatiles:1; > >> unsigned int call_notify:1; > >> unsigned int manual_mode_value:8; > >> + unsigned int has_p_def:1; > >> > >> const struct v4l2_ctrl_ops *ops; > >> const struct v4l2_ctrl_type_ops *type_ops; > >> @@ -256,6 +258,7 @@ struct v4l2_ctrl { > >> s32 val; > >> } cur; > >> > >> + union v4l2_ctrl_ptr p_def; > >> union v4l2_ctrl_ptr p_new; > >> union v4l2_ctrl_ptr p_cur; > >> }; > >> @@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > >> u32 id, u8 max, > >> u64 mask, u8 def, > >> const char * const *qmenu); > >> +/** > >> + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2 > >> + * compound control. > >> + * > >> + * @hdl: The control handler. > >> + * @ops: The control ops. > >> + * @id: The control ID. > >> + * @min: The control's minimum value. > >> + * @max: The control's maximum value. > >> + * @step: The control's step value > >> + * @def: The control's default value. > >> + * @p_def: The control's p_def value. > >> + * > >> + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to > >> + * the @p_def field. > >> + * > >> + */ > >> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > >> + const struct v4l2_ctrl_ops *ops, > >> + u32 id, s64 min, s64 max, u64 step, > >> + s64 def, const union v4l2_ctrl_ptr p_def); > >> + > >> > >> /** > >> * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control. > >> -- > >> 2.23.0 > >> > > So other than these fairly minor points, this is indeed what I was looking for. > > Regards, > > Hans ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() 2019-09-20 10:33 ` Ricardo Ribalda Delgado @ 2019-09-20 10:38 ` Hans Verkuil 0 siblings, 0 replies; 7+ messages in thread From: Hans Verkuil @ 2019-09-20 10:38 UTC (permalink / raw) To: Ricardo Ribalda Delgado; +Cc: linux-media On 9/20/19 12:33 PM, Ricardo Ribalda Delgado wrote: > Hello Hans > > Thanks for your review! I will implement the changes and resend, just > one comment. > > > On Fri, Sep 20, 2019 at 12:07 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> On 9/17/19 6:21 PM, Ricardo Ribalda Delgado wrote: >>> Hi Hans >>> >>> Is this something close to what you were having in mind? Right now it >>> sits on https://github.com/ribalda/linux/commit/de21dbc2f57b58b22f5d73bc314dd8e59dff5c7d >>> but I will make it as the beginning of my patchset if you think that I >>> am on the right track. >>> >>> Thanks! >>> >>> On Tue, Sep 17, 2019 at 6:19 PM Ricardo Ribalda Delgado >>> <ribalda@kernel.org> wrote: >>>> >>>> Implement v4l2_ctrl_new_std_compound. This is just a discussion patch, >>>> do not merge as is, and be gentle with the author ;P. >>>> >>>> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> >>>> --- >>>> drivers/media/i2c/imx214.c | 13 +++-- >>>> drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++----------- >>>> include/media/v4l2-ctrls.h | 25 +++++++++ >>>> 3 files changed, 81 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c >>>> index 625617d4c81a..e18fed9f31f1 100644 >>>> --- a/drivers/media/i2c/imx214.c >>>> +++ b/drivers/media/i2c/imx214.c >>>> @@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client) >>>> .width = 1120, >>>> .height = 1120, >>>> }; >>>> + union v4l2_ctrl_ptr p_def = { >>>> + .p_area = &unit_size, >>>> + }; >>>> int ret; >>>> >>>> ret = imx214_parse_fwnode(dev); >>>> @@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client) >>>> V4L2_CID_EXPOSURE, >>>> 0, 3184, 1, 0x0c70); >>>> >>>> - imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls, >>>> - &imx214_ctrl_ops, >>>> - V4L2_CID_UNIT_CELL_SIZE, >>>> - &unit_size); >>>> - >>>> + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, >>>> + NULL, >>>> + V4L2_CID_UNIT_CELL_SIZE, >>>> + 0, 0x7ffffff, 1, 0, >>>> + p_def); >>>> ret = imx214->ctrls.error; >>>> if (ret) { >>>> dev_err(&client->dev, "%s control init failed (%d)\n", >>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c >>>> index 3d2fa1868982..04813ba2262b 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >>>> @@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx, >>>> p_mpeg2_slice_params->picture.picture_coding_type = >>>> V4L2_MPEG2_PICTURE_CODING_TYPE_I; >>>> break; >>>> + default: >>>> + if (ctrl->has_p_def) >>>> + memcpy(p, ctrl->p_def.p, ctrl->elem_size); >>>> + break; >> >> It makes more sense to do this at the beginning of this function: >> >> if (ctrl->p_def.p) >> memcpy(p, ctrl->p_def.p, ctrl->elem_size); >> else >> memset(p, 0, ctrl->elem_size); >> >> and then enter the switch. >> >> This avoids calling memset followed by a memcpy. >> >>>> } >>>> } >>>> >>>> @@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >>>> s64 min, s64 max, u64 step, s64 def, >>>> const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size, >>>> u32 flags, const char * const *qmenu, >>>> - const s64 *qmenu_int, void *priv) >>>> + const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def, >>>> + void *priv) >>>> { >>>> struct v4l2_ctrl *ctrl; >>>> unsigned sz_extra; >>>> @@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >>>> is_array) >>>> sz_extra += 2 * tot_ctrl_size; >>>> >>>> + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) >>>> + sz_extra += elem_size; >>>> + >>>> ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL); >>>> if (ctrl == NULL) { >>>> handler_set_err(hdl, -ENOMEM); >>>> @@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >>>> ctrl->p_new.p = &ctrl->val; >>>> ctrl->p_cur.p = &ctrl->cur.val; >>>> } >>>> + >>>> + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) { >>>> + ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size; >>>> + memcpy(ctrl->p_def.p, p_def.p, elem_size); >>>> + ctrl->has_p_def = true; >> >> Is this needed? ctrl->p_def.p would be NULL if there is no p_def. >> >>>> + } >>>> + >>>> for (idx = 0; idx < elems; idx++) { >>>> ctrl->type_ops->init(ctrl, idx, ctrl->p_cur); >>>> ctrl->type_ops->init(ctrl, idx, ctrl->p_new); >>>> @@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, >>>> s64 max = cfg->max; >>>> u64 step = cfg->step; >>>> s64 def = cfg->def; >>>> + const union v4l2_ctrl_ptr p_def = {}; >> >> I think it is cleaner to have a 'static const union v4l2_ctrl_ptr ptr_null;' >> at the start of the source and just use that. >> >>>> >>>> if (name == NULL) >>>> v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step, >>>> @@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, >>>> type, min, max, >>>> is_menu ? cfg->menu_skip_mask : step, def, >>>> cfg->dims, cfg->elem_size, >>>> - flags, qmenu, qmenu_int, priv); >>>> + flags, qmenu, qmenu_int, p_def, priv); >>>> if (ctrl) >>>> ctrl->is_private = cfg->is_private; >>>> return ctrl; >>>> @@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, >>>> const char *name; >>>> enum v4l2_ctrl_type type; >>>> u32 flags; >>>> + const union v4l2_ctrl_ptr p_def = {}; >>>> >>>> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >>>> if (type == V4L2_CTRL_TYPE_MENU || >>>> @@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, >>>> } >>>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >>>> min, max, step, def, NULL, 0, >>>> - flags, NULL, NULL, NULL); >>>> + flags, NULL, NULL, p_def, NULL); >>>> } >>>> EXPORT_SYMBOL(v4l2_ctrl_new_std); >>>> >>>> @@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, >>>> s64 def = _def; >>>> u64 step; >>>> u32 flags; >>>> + const union v4l2_ctrl_ptr p_def = {}; >>>> >>>> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >>>> >>>> @@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, >>>> } >>>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >>>> 0, max, mask, def, NULL, 0, >>>> - flags, qmenu, qmenu_int, NULL); >>>> + flags, qmenu, qmenu_int, p_def, NULL); >>>> } >>>> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu); >>>> >>>> @@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >>>> s64 min; >>>> s64 max = _max; >>>> s64 def = _def; >>>> + const union v4l2_ctrl_ptr p_def = {}; >>>> >>>> /* v4l2_ctrl_new_std_menu_items() should only be called for >>>> * standard controls without a standard menu. >>>> @@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >>>> } >>>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >>>> 0, max, mask, def, NULL, 0, >>>> - flags, qmenu, NULL, NULL); >>>> + flags, qmenu, NULL, p_def, NULL); >>>> >>>> } >>>> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); >>>> >>>> +/* Helper function for standard compound controls */ >>>> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, >>>> + const struct v4l2_ctrl_ops *ops, >>>> + u32 id, s64 min, s64 max, u64 step, s64 def, >> >> The def arg makes no sense, since that's superseded by p_def. >> > > If def does not make sense, then for the same reason min, max and step > should not make any sense (because they are not a pointer but a > integer). > What about removing them completely from the function prototype and > add them later if we change our mind, it is a internal API. I've been going back and forth about whether or not min/max/step is useful for e.g. this AREA control as it would still help in validating a range for the width and height. But I think you are right, and it is better to just drop this and mark min/max/step/def as N/A in the documentation for these compound controls. We DO need basic validation for AREA (i.e. the width/height shall be > 0), but that's easy enough. Regards, Hans > >>>> + const union v4l2_ctrl_ptr p_def) >>>> +{ >>>> + const char *name; >>>> + enum v4l2_ctrl_type type; >>>> + u32 flags; >> >> Add: >> >> s64 def = min; >> >> It will be discarded anyway. >> >>>> + >>>> + v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >>>> + if (type == V4L2_CTRL_TYPE_MENU || >>>> + type == V4L2_CTRL_TYPE_INTEGER_MENU) { >>>> + handler_set_err(hdl, -EINVAL); >>>> + return NULL; >>>> + } >> >> This makes no sense. It should just check that the type is a compound type and >> return an error if it isn't. >> >>>> + return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >>>> + min, max, step, def, NULL, 0, >>>> + flags, NULL, NULL, p_def, NULL); >>>> +} >>>> +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound); >>>> + >>>> /* Helper function for standard integer menu controls */ >>>> struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, >>>> const struct v4l2_ctrl_ops *ops, >>>> @@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, >>>> s64 max = _max; >>>> s64 def = _def; >>>> u32 flags; >>>> + const union v4l2_ctrl_ptr p_def = {}; >>>> >>>> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags); >>>> if (type != V4L2_CTRL_TYPE_INTEGER_MENU) { >>>> @@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, >>>> } >>>> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >>>> 0, max, 0, def, NULL, 0, >>>> - flags, NULL, qmenu_int, NULL); >>>> + flags, NULL, qmenu_int, p_def, NULL); >>>> } >>>> EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); >>>> >>>> @@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags) >>>> return try_or_set_cluster(fh, master, true, ch_flags); >>>> } >>>> >>>> -/* Helper function for area controls */ >>>> -struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl, >>>> - const struct v4l2_ctrl_ops *ops, >>>> - u32 id, const struct v4l2_area *area) >>>> -{ >>>> - static struct v4l2_ctrl_config ctrl = {}; >>>> - struct v4l2_ctrl *c; >>>> - int ret; >>>> - >>>> - ctrl.id = id; >>>> - c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area); >>>> - if (!c) >>>> - return NULL; >>>> - >>>> - memcpy(c->p_new.p_area, area, sizeof(*area)); >>>> - ret = set_ctrl(NULL, c, 0); >>>> - if (ret){ >>>> - hdl->error = ret; >>>> - return NULL; >>>> - } >>>> - >>>> - return c; >>>> -} >>>> -EXPORT_SYMBOL(v4l2_ctrl_new_area); >>>> - >>>> /* Helper function for VIDIOC_S_CTRL compatibility */ >>>> static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, >>>> struct v4l2_ext_control *c) >>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h >>>> index b5b42777a756..8dc7e9827056 100644 >>>> --- a/include/media/v4l2-ctrls.h >>>> +++ b/include/media/v4l2-ctrls.h >>>> @@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); >>>> * manual mode. So if the value of the auto control equals this >>>> * value, then the whole cluster is in manual mode. Drivers should >>>> * never set this flag directly. >>>> + * @has_p_def: If set, the p_def field points to the default value of the control. >>>> * @ops: The control ops. >>>> * @type_ops: The control type ops. >>>> * @id: The control ID. >>>> @@ -230,6 +231,7 @@ struct v4l2_ctrl { >>>> unsigned int has_volatiles:1; >>>> unsigned int call_notify:1; >>>> unsigned int manual_mode_value:8; >>>> + unsigned int has_p_def:1; >>>> >>>> const struct v4l2_ctrl_ops *ops; >>>> const struct v4l2_ctrl_type_ops *type_ops; >>>> @@ -256,6 +258,7 @@ struct v4l2_ctrl { >>>> s32 val; >>>> } cur; >>>> >>>> + union v4l2_ctrl_ptr p_def; >>>> union v4l2_ctrl_ptr p_new; >>>> union v4l2_ctrl_ptr p_cur; >>>> }; >>>> @@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >>>> u32 id, u8 max, >>>> u64 mask, u8 def, >>>> const char * const *qmenu); >>>> +/** >>>> + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2 >>>> + * compound control. >>>> + * >>>> + * @hdl: The control handler. >>>> + * @ops: The control ops. >>>> + * @id: The control ID. >>>> + * @min: The control's minimum value. >>>> + * @max: The control's maximum value. >>>> + * @step: The control's step value >>>> + * @def: The control's default value. >>>> + * @p_def: The control's p_def value. >>>> + * >>>> + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to >>>> + * the @p_def field. >>>> + * >>>> + */ >>>> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, >>>> + const struct v4l2_ctrl_ops *ops, >>>> + u32 id, s64 min, s64 max, u64 step, >>>> + s64 def, const union v4l2_ctrl_ptr p_def); >>>> + >>>> >>>> /** >>>> * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control. >>>> -- >>>> 2.23.0 >>>> >> >> So other than these fairly minor points, this is indeed what I was looking for. >> >> Regards, >> >> Hans ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-20 10:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-12 13:35 Proposal for a v4l2_ctrl_new_std_compound() function Hans Verkuil 2019-09-13 16:08 ` kbuild test robot 2019-09-17 16:19 ` [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() Ricardo Ribalda Delgado 2019-09-17 16:21 ` Ricardo Ribalda Delgado 2019-09-20 10:06 ` Hans Verkuil 2019-09-20 10:33 ` Ricardo Ribalda Delgado 2019-09-20 10:38 ` Hans Verkuil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).