* 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.