On 08/08/2024 14:03, Jacopo Mondi wrote: > Hi Hans > > On Thu, Aug 08, 2024 at 12:58:46PM GMT, Hans Verkuil wrote: <snip> >>>>>>>> I see no check against this in the rkisp1 code. Shouldn't this be checked? >>>>>>>> If the version is unsupported, then just return an error. >>>>>>> >>>>>>> Do we need this for the first version ? There are no other versions >>>>>>> userspace can use at the moment. I can add a check during validation >>>>>>> though. >>>>>> >>>>>> Yes: if a V2 is added in the future, and an application wants to use that >>>>>> against a driver that only support V1, then that should fail. >>>>>> >>>>>>>> Also, how does userspace know which version(s) is/are supported by the driver? >>>>>>> >>>>>>> Good question, there is no API for that atm. Defining a new format >>>>>>> version should only happen when a non-backward compatible change to >>>>>>> the format is made. I understand an application can be compiled >>>>>>> against a newer kernel header that provides a new format version but >>>>>>> then run on an older kernel where the new format is not supported. >>>>>>> >>>>>>> Probably userspace should be able to identify what versions are >>>>>>> supported by the driver it runs with and use the most appropriate one >>>>>>> by selecting it at runtime. >>>>>>> >>>>>>> What API would you use for that ? Is this something required for this >>>>>>> first version where a single format version is available ? >>>>>> >>>>>> You need this also for this first version for the reason explained above. >>>>>> >>>>>> Personally I would just make a read-only control that returns the highest >>>>>> supported version. >>>>> >>>>> Can't userspace use the version number reported through the media device >>>>> to determine the features the driver support ? We've done that in >>>>> libcamera for some drivers already, either to work around bugs, or to >>>>> make use of new features. >>>> >>>> You can, but this will fall down if the driver is backported to an older >>>> kernel for whatever reason. Since the version is just the kernel version, >>>> it will drop back to that older kernel version. >>> >>> I recall discussing this issue in the past (I'm not sure it was with >>> you). If my memory doesn't fail me, there was a consensus that, when >>> backporting the whole V4L2 subsystem, the version number reported would >>> tbe the one corresponding to the more recent kernel, not the kernel the >>> code has been backported to. >> >> That was when using the old https://git.linuxtv.org/media_build.git/ system. >> That's no longer in use. >> >> I'm talking if a vendor is on an old kernel and backports rkisp1 to it (not >> exactly uncommon!), then this will not work since the version will be that >> of the old kernel. >> > > In this case it will backport the driver and the uAPI from the same > version, so this shouldn't be an issue. > > What is concerning is an application compiled against a uAPI newer > than the kernel it will be run on. The uAPI could advertise newer > format revisions the driver doesn't know about and will fail to > validate. In this case the application should be give a way to know > what version is the most recent one supported by the driver, and a > control might be the way forward. As long as we have a single format > revision I think we might omit implementing the control for now and > only expose it when multiple revisions will be implemented ? If the > control is not there, it means only V1 is available. Should I document > it ? I think it is sufficient for now to just add a version check in the driver and return an error if it is not supported. And when a V2 is introduced, then we can think about using a control. Or perhaps just rely on the version check. Regards, Hans > >> Regards, >> >> Hans >> >>> >>> That would help here, but will not solve the issue of how to deal with >>> backports of a single driver. Jacopo, what do you think ? >>> >>>> Whether that is acceptable or not is up to you. >>>> >>>> In any case, this would have to be documented so you know at which kernel >>>> version a new RKISP1_EXT_PARAM_BUFFER_Vx is introduced. >>>> >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> +/** >>>>>>>>> + * struct rkisp1_ext_params_cfg - RkISP1 extensible parameters configuration >>>>>>>>> + * >>>>>>>>> + * This struct contains the configuration parameters of the RkISP1 ISP >>>>>>>>> + * algorithms, serialized by userspace into a data buffer. Each configuration >>>>>>>>> + * parameter block is represented by a block-specific structure which contains a >>>>>>>>> + * :c:type:`rkisp1_ext_params_block_header` entry as first member. Userspace >>>>>>>>> + * populates the @data buffer with configuration parameters for the blocks that >>>>>>>>> + * it intends to configure. As a consequence, the data buffer effective size >>>>>>>>> + * changes according to the number of ISP blocks that userspace intends to >>>>>>>>> + * configure and is set by userspace in the @data_size field. >>>>>>>>> + * >>>>>>>>> + * The parameters buffer is versioned by the @version field to allow modifying >>>>>>>>> + * and extending its definition. Userspace shall populate the @version field to >>>>>>>>> + * inform the driver about the version it intends to use. The driver will parse >>>>>>>>> + * and handle the @data buffer according to the data layout specific to the >>>>>>>>> + * indicated version and return an error if the desired version is not >>>>>>>>> + * supported. >>>>>>>>> + * >>>>>>>>> + * For each ISP block that userspace wants to configure, a block-specific >>>>>>>>> + * structure is appended to the @data buffer, one after the other without gaps >>>>>>>>> + * in between nor overlaps. Userspace shall populate the @data_size field with >>>>>>>>> + * the effective size, in bytes, of the @data buffer. >>>>>>>>> + * >>>>>>>>> + * The expected memory layout of the parameters buffer is:: >>>>>>>>> + * >>>>>>>>> + * +-------------------- struct rkisp1_ext_params_cfg -------------------+ >>>>>>>>> + * | version = RKISP_EXT_PARAMS_BUFFER_V1; | >>>>>>>>> + * | data_size = sizeof(struct rkisp1_ext_params_bls_config) | >>>>>>>>> + * | + sizeof(struct rkisp1_ext_params_dpcc_config); | >>>>>>>>> + * | +------------------------- data ---------------------------------+ | >>>>>>>>> + * | | +------------- struct rkisp1_ext_params_bls_config -----------+ | | >>>>>>>>> + * | | | +-------- struct rkisp1_ext_params_block_header ---------+ | | | >>>>>>>>> + * | | | | type = RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS; | | | | >>>>>>>>> + * | | | | flags = RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE; | | | | >>>>>>>>> + * | | | | size = sizeof(struct rkisp1_ext_params_bls_config); | | | | >>>>>>>>> + * | | | +---------------------------------------------------------+ | | | >>>>>>>>> + * | | | +---------- struct rkisp1_cif_isp_bls_config -------------+ | | | >>>>>>>>> + * | | | | enable_auto = 0; | | | | >>>>>>>>> + * | | | | fixed_val.r = 256; | | | | >>>>>>>>> + * | | | | fixed_val.gr = 256; | | | | >>>>>>>>> + * | | | | fixed_val.gb = 256; | | | | >>>>>>>>> + * | | | | fixed_val.b = 256; | | | | >>>>>>>>> + * | | | +---------------------------------------------------------+ | | | >>>>>>>>> + * | | +------------ struct rkisp1_ext_params_dpcc_config -----------+ | | >>>>>>>>> + * | | | +-------- struct rkisp1_ext_params_block_header ---------+ | | | >>>>>>>>> + * | | | | type = RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC; | | | | >>>>>>>>> + * | | | | flags = RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE; | | | | >>>>>>>>> + * | | | | size = sizeof(struct rkisp1_ext_params_dpcc_config); | | | | >>>>>>>>> + * | | | +---------------------------------------------------------+ | | | >>>>>>>>> + * | | | +---------- struct rkisp1_cif_isp_dpcc_config ------------+ | | | >>>>>>>>> + * | | | | mode = RKISP1_CIF_ISP_DPCC_MODE_STAGE1_ENABLE; | | | | >>>>>>>>> + * | | | | output_mode = | | | | >>>>>>>>> + * | | | | RKISP1_CIF_ISP_DPCC_OUTPUT_MODE_STAGE1_INCL_G_CENTER; | | | | >>>>>>>>> + * | | | | set_use = ... ; | | | | >>>>>>>>> + * | | | | ... = ... ; | | | | >>>>>>>>> + * | | | +---------------------------------------------------------+ | | | >>>>>>>>> + * | | +-------------------------------------------------------------+ | | >>>>>>>>> + * | +-----------------------------------------------------------------+ | >>>>>>>>> + * +---------------------------------------------------------------------+ >>>>>>>>> + * >>>>>>>>> + * @version: The RkISP1 extensible parameters buffer version, see >>>>>>>>> + * :c:type:`rksip1_ext_param_buffer_version` >>>>>>>>> + * @data_size: The RkISP1 configuration data effective size, excluding this >>>>>>>>> + * header >>>>>>>>> + * @data: The RkISP1 extensible configuration data blocks >>>>>>>>> + */ >>>>>>>>> +struct rkisp1_ext_params_cfg { >>>>>>>>> + __u32 version; >>>>>>>>> + __u32 data_size; >>>>>>>>> + __u8 data[RKISP1_EXT_PARAMS_MAX_SIZE]; >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> #endif /* _UAPI_RKISP1_CONFIG_H */ >>>>> >>>> >>> >>
