Re: [PATCH v2 1/5] media: Add MIPI CCI register access helper functions

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/15/23 13:54, Tommaso Merciai wrote:
> On Thu, Jun 15, 2023 at 01:26:25PM +0200, Tommaso Merciai wrote:
>> Hi Hans,
>>
>> On Thu, Jun 15, 2023 at 01:10:40PM +0200, Hans de Goede wrote:
>>> Hi Tommaso,
>>>
>>> On 6/15/23 12:05, Tommaso Merciai wrote:
>>>> Hi Hans, Laurent, Sakari,
>>>>
>>>> Can I cherry-pick this patch and use these new functions also
>>>> for cci regs of the alvium driver?
>>>
>>> Yes that sounds like a good plan.
>>>
>>>> Are on going to be merge?
>>>
>>> Yes this will hopefully get merged upstream soon.
>>
>> Thanks for the info!
>>
>> I want to ask you your opinion about this:
>>
>> Into alvium driver actually I'm using the following defines
>> manipulations:
>>
>> #define REG_BCRM_REG_ADDR_R				REG_BCRM_CCI_16BIT(0x0014)
>>
>> #define REG_BCRM_FEATURE_INQUIRY_R			REG_BCRM_V4L2_64BIT(0x0008)
>> #define REG_BCRM_DEVICE_FIRMWARE_VERSION_R		REG_BCRM_V4L2_64BIT(0x0010)
>>
>> My plan is to use your cci API for cci register in this way defines
>> became like:
>>
>> #define REG_BCRM_REG_ADDR_R				CCI_REG16(0x0014)
>>
>> And leave v4l2 regs are it are right now:
>>
>> #define REG_BCRM_FEATURE_INQUIRY_R			REG_BCRM_V4L2_64BIT(0x0008)
>> #define REG_BCRM_DEVICE_FIRMWARE_VERSION_R		REG_BCRM_V4L2_64BIT(0x0010)
>>
>> What do you think about?
> 
> Or maybe is worth don't use v4l2 bit for v4l2 regs, I think is implicit
> that what regs that are not CCI are v4l2, then we return wit the
> following defines:
> 
> 
> 
> #define REG_BCRM_REG_ADDR_R                           CCI_REG16(0x0014)
> ^CCI regs
> 
> #define REG_BCRM_FEATURE_INQUIRY_R                    0x0008 
> #define REG_BCRM_DEVICE_FIRMWARE_VERSION_R            0x0010
> ^v4l2 regs

I'm not sure what you mean with "V4L2" registers ? I guess you mean
registers which cannot be accessed through the CCI helper functions,
but starting with v2 this is no longer true. There now is a CCI_REG64()
so you can simply use that.

Regards,

Hans



> 
> ?
> 
>>
>>>
>>> Note I'm about to send out a v3 addressing some small
>>> remarks on this v2. I'll Cc you on that.
>>
>> Thanks, in this way I can test that and let you know my feedback.
>>
>> Regards,
>> Tommaso
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>>
>>>> Let me know.
>>>> Thanks! :)
>>>>
>>>> Regards,
>>>> Tommaso
>>>>
>>>> On Thu, Jun 15, 2023 at 12:21:00PM +0300, Laurent Pinchart wrote:
>>>>> On Thu, Jun 15, 2023 at 11:11:20AM +0200, Hans de Goede wrote:
>>>>>> Hi Sakari,
>>>>>>
>>>>>> On 6/14/23 23:48, Sakari Ailus wrote:
>>>>>>> Hi Laurent,
>>>>>>>
>>>>>>> On Thu, Jun 15, 2023 at 12:34:29AM +0300, Laurent Pinchart wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> On Wed, Jun 14, 2023 at 08:39:56PM +0000, Sakari Ailus wrote:
>>>>>>>>> On Wed, Jun 14, 2023 at 09:23:39PM +0200, Hans de Goede wrote:
>>>>>>>>>> The CSI2 specification specifies a standard method to access camera sensor
>>>>>>>>>> registers called "Camera Control Interface (CCI)".
>>>>>>>>>>
>>>>>>>>>> This uses either 8 or 16 bit (big-endian wire order) register addresses
>>>>>>>>>> and supports 8, 16, 24 or 32 bit (big-endian wire order) register widths.
>>>>>>>>>>
>>>>>>>>>> Currently a lot of Linux camera sensor drivers all have their own custom
>>>>>>>>>> helpers for this, often copy and pasted from other drivers.
>>>>>>>>>>
>>>>>>>>>> Add a set of generic helpers for this so that all sensor drivers can
>>>>>>>>>> switch to a single common implementation.
>>>>>>>>>>
>>>>>>>>>> These helpers take an extra optional "int *err" function parameter,
>>>>>>>>>> this can be used to chain a bunch of register accesses together with
>>>>>>>>>> only a single error check at the end, rather then needing to error
>>>>>>>>>> check each individual register access. The first failing call will
>>>>>>>>>> set the contents of err to a non 0 value and all other calls will
>>>>>>>>>> then become no-ops.
>>>>>>>>>>
>>>>>>>>>> Link: https://lore.kernel.org/linux-media/59aefa7f-7bf9-6736-6040-39551329cd0a@redhat.com/
>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>>>>> ---
>>>>>>>>>> Changes in v2:
>>>>>>>>>> - Drop cci_reg_type enum
>>>>>>>>>> - Make having an encoded reg-width mandatory rather then using 0 to encode
>>>>>>>>>>   8 bit width making reg-addresses without an encoded width default to
>>>>>>>>>>   a width of 8
>>>>>>>>>> - Add support for 64 bit wide registers
>>>>>>>>
>>>>>>>> I'm in two minds about this. This means that the read and write
>>>>>>>> functions take a u64 argument, which will be less efficient on 32-bit
>>>>>>>> platforms. I think it would be possible, with some macro magic, to
>>>>>>>> accept different argument sizes, but maybe that's a micro-optimization
>>>>>>>> that would actually result in worse code. 
>>>>>>>>
>>>>>>>> 64-bit support could be useful, but as far as I can tell, it's not used
>>>>>>>> in this series, so maybe we could leave this for later ?
>>>>>>>
>>>>>>> I prefer to have it now, I just told Tommaso working on the Alvium driver
>>>>>>> to use this, and he needs 64-bit access. :-)
>>>>>>>
>>>>>>> You could also easily have 32-bit and 64-bit variant of the functions, with
>>>>>>> C11 _Generic(). Introducing it now would be easier than later.
>>>>>>
>>>>>> I took a quick look at C11 _Generic() and that looks at the type
>>>>>> of "things" so in this case it would look at type of the val argument.
>>>>>>
>>>>>> Problem is that that can still be e.g. an int when doing a
>>>>>> read/write from a 64 bit registers.
>>>>>>
>>>>>> So we would then need to handle the 64 bit width case in the 32
>>>>>> bit versions of the functions too.
>>>>>>
>>>>>> And likewise I can see someone passing a long on a 64 bit
>>>>>> arch while doing a cci_write() to a non 64 bit register.
>>>>>>
>>>>>> So this would basically mean copy and pasting cci_read()
>>>>>> + cci_write() 2x with the only difference being one
>>>>>> variant taking a 32 bit val argument and the other a
>>>>>> 64 bit val argument.
>>>>>>
>>>>>> This seems like premature optimization to me.
>>>>>>
>>>>>> As mentioned in my reply to Laurent if we want to
>>>>>> optimize things we really should look at avoiding
>>>>>> unnecessary i2c transfers, or packing multiple
>>>>>> writes into a single i2c transfer for writes to
>>>>>> subsequent registers. That is where significant
>>>>>> speedups can be made.
>>>>>
>>>>> This is something I'd really like to see, but it's way more work.
>>>>>
>>>>> There's an important need of applying changes atomically, which is often
>>>>> not possible to strictly guarantee over I2C. Userspace ends up writing
>>>>> V4L2 controls as quickly as it can after the start of a frame, hoping
>>>>> they will all reach the sensor before the end of the frame. Some
>>>>> platforms have camera-specific I2C controllers that have the ability to
>>>>> buffer I2C transfers and issue them based on a hardware trigger. How to
>>>>> fit this in thé kernel I2C API will be an interesting exercise.
>>>>>
>>>>> -- 
>>>>> Regards,
>>>>>
>>>>> Laurent Pinchart
>>>>
>>>
>