Re: [PATCH 13/13] media: qcom: camss: Add support for VFE hardware version Titan 780

[Depeng Shao <quic_depengs@quicinc.com>]

Hi Bryan,


On 8/19/2024 7:05 PM, Bryan O'Donoghue wrote:
> On 12/08/2024 15:41, Depeng Shao wrote:
>> +#define VFE_BUS_WM_CFG(n)        (BUS_REG_BASE + 0x200 + (n) * 0x100)
> 
> <snip>
> 
>> +#define RDI_WM(n)            ((vfe_is_lite(vfe) ? 0x0 : 0x17) + (n))
>> +
>> +static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct 
>> vfe_line *line)
>> +{
>> +    struct v4l2_pix_format_mplane *pix =
>> +        &line->video_out.active_fmt.fmt.pix_mp;
>> +
>> +    wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
> 
> OK so one more point here.
> 
> The non-lite VFE has I think in the case of sm8550 twenty seven 
> different bus clients.
> 
> The above code takes a given index - take the example of index 0 meaning 
> RDI0 and
> 
> 1. Determines if is_lite() is true deriving a jump of 0 or 0x17
> 2. Uses this index as a further offset to functions such as
>     VFE_BUS_WM_CFG(n)
> 3. In no way articulates which bus client is which.
> 
> So for a non lite case -> RDI0 is bus client # 23
> 
> The code we have for CAMSS just assumes RDI is the only client we are 
> programming - which I'm not proposing to change for now, however the 
> code is very not obvious in what it is doing here.
> 
> This BTW isn't a criticism of what you've done here but, even though I 
> have access to the registers in front of me, I had to spend about 30 
> minutes looking up and verifying these offsets.
> 
> That's not sustainable.
> 
> Could you please add a comment which details what each index relates to.
> 
> /*
>   * Bus client mapping
>   *
>   * 0 = VID_Y ?
>   * 1 = VID_C
>   * .. etc
>   * .. etc
>   * 23 = RDI0
>   * 24 = RDI1
>   */
> 
> I'll try to apply a similar level of index documentation for existing 
> upstream submissions so that working out client mappings is less tedious 
> and will be requiring these mappings for new VFE silicon enabling code 
> upstream.
> 

Sure, I will add the comment for the bus client mapping in next version 
patch.

But the comment will occupy too many lines, I will fold the comment, e.g.,

/*
  * Bus client mapping
  *
  * Full VFE:
  * 0 = VID_Y, 1 = VID_C, 2 = VID 4:1, 3 = VID 16:1, 4 = DISP Y, 5 = 
DISP C, 6 = DISP 4:1,
  * 7 = DISP 16:1, 8 = FD_Y, 9 = FD_C, ...
  * ...
  * 23 = RDI0, 24 = RDI1, 25 = RDI2, 26 = LTM STATS
  *
  * VFE LITE:
  * 0 = RDI0, 1 = RDI1, 2 = RDI3, 4 = RDI4
  */

Since the full VFE has many ports, can we just add comment for the RDI 
client?


Thanks,
Depeng