[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <60d737b2-4ae6-42aa-abc3-5843dde7f7b2@quicinc.com>
Date: Mon, 19 Aug 2024 21:07:45 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <rfoss@...nel.org>,
<todor.too@...il.com>, <mchehab@...nel.org>, <robh@...nel.org>,
<krzk+dt@...nel.org>, <conor+dt@...nel.org>
CC: <linux-arm-msm@...r.kernel.org>, <linux-media@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<kernel@...cinc.com>, Yongsheng Li <quic_yon@...cinc.com>
Subject: Re: [PATCH 13/13] media: qcom: camss: Add support for VFE hardware
version Titan 780
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
Powered by blists - more mailing lists