Digging into the documentation we find that the DT_ID bitfield is used to
map the six bit DT to a two bit ID code. This value is concatenated to the
VC bitfield to create a CID value. DT_ID is the two least significant bits
of CID and VC the most significant bits.
Originally we set dt_id = vc * 4 in and then subsequently set dt_id = vc.
commit 3c4ed72a16bc ("media: camss: sm8250: Virtual channels for CSID")
silently fixed the multiplication by four which would give a better
value for the generated CID without mentioning what was being done or why.
Next up I haplessly changed the value back to "dt_id = vc * 4" since there
didn't appear to be any logic behind it.
Hans asked what the change was for and I honestly couldn't remember the
provenance of it, so I dug in.
Link: https://lore.kernel.org/linux-arm-msm/[email protected]/
Add a comment so the next hapless programmer doesn't make this same
mistake.
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/media/platform/qcom/camss/camss-csid-gen2.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
index 6ba2b10326444..791f27b18c394 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
@@ -352,6 +352,18 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
phy_sel = csid->phy.csiphy_id;
if (enable) {
+ /*
+ * DT_ID is a two bit bitfield that is concatenated with
+ * the four least significant bits of the six bit VC
+ * bitfield to generate an internal CID value.
+ *
+ * CSID_RDI_CFG0(vc)
+ * DT_ID : 28:27
+ * VC : 26:22
+ * DT : 21:16
+ *
+ * CID : VC 3:0 << 2 | DT_ID 1:0
+ */
u8 dt_id = vc;
if (tg->enabled) {
--
2.42.0
On 25/09/2023 17:47, Bryan O'Donoghue wrote:
> Digging into the documentation we find that the DT_ID bitfield is used to
> map the six bit DT to a two bit ID code. This value is concatenated to the
> VC bitfield to create a CID value. DT_ID is the two least significant bits
> of CID and VC the most significant bits.
>
> Originally we set dt_id = vc * 4 in and then subsequently set dt_id = vc.
>
> commit 3c4ed72a16bc ("media: camss: sm8250: Virtual channels for CSID")
> silently fixed the multiplication by four which would give a better
> value for the generated CID without mentioning what was being done or why.
>
> Next up I haplessly changed the value back to "dt_id = vc * 4" since there
> didn't appear to be any logic behind it.
>
> Hans asked what the change was for and I honestly couldn't remember the
> provenance of it, so I dug in.
>
> Link: https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> Add a comment so the next hapless programmer doesn't make this same
> mistake.
>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> drivers/media/platform/qcom/camss/camss-csid-gen2.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> index 6ba2b10326444..791f27b18c394 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> @@ -352,6 +352,18 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
> phy_sel = csid->phy.csiphy_id;
>
> if (enable) {
> + /*
> + * DT_ID is a two bit bitfield that is concatenated with
> + * the four least significant bits of the six bit VC
> + * bitfield to generate an internal CID value.
> + *
> + * CSID_RDI_CFG0(vc)
> + * DT_ID : 28:27
> + * VC : 26:22
This is 5 bits, not 6 bits as the comment above says. Which is right?
> + * DT : 21:16
> + *
> + * CID : VC 3:0 << 2 | DT_ID 1:0
> + */
> u8 dt_id = vc;
So if dt_id is 2 bits, and vc can be more than 2 bits in the future,
shouldn't this read "vc & 3"?
Regards,
Hans
>
> if (tg->enabled) {
On 27/09/2023 14:03, Bryan O'Donoghue wrote:
> On 27/09/2023 12:42, Hans Verkuil wrote:
>>> + /*
>>> + * DT_ID is a two bit bitfield that is concatenated with
>>> + * the four least significant bits of the six bit VC
>>> + * bitfield to generate an internal CID value.
>>> + *
>>> + * CSID_RDI_CFG0(vc)
>>> + * DT_ID : 28:27
>>> + * VC : 26:22
>> This is 5 bits, not 6 bits as the comment above says. Which is right?
>
> Yes you're right I had "DT" which is six bits in my head when I wrote this. The VC bitfield is five.
>
>>
>>> + * DT : 21:16
>>> + *
>>> + * CID : VC 3:0 << 2 | DT_ID 1:0
>>> + */
>>> u8 dt_id = vc;
>> So if dt_id is 2 bits, and vc can be more than 2 bits in the future,
>> shouldn't this read "vc & 3"?
>
> Hmm...
>
> val |= dt_id << RDI_CFG0_DT_ID;
>
> yes that would overrun otherwise.
No need to repost the whole series, just post a v6.1 for this patch only.
Regards,
Hans
On 27/09/2023 12:42, Hans Verkuil wrote:
>> + /*
>> + * DT_ID is a two bit bitfield that is concatenated with
>> + * the four least significant bits of the six bit VC
>> + * bitfield to generate an internal CID value.
>> + *
>> + * CSID_RDI_CFG0(vc)
>> + * DT_ID : 28:27
>> + * VC : 26:22
> This is 5 bits, not 6 bits as the comment above says. Which is right?
Yes you're right I had "DT" which is six bits in my head when I wrote
this. The VC bitfield is five.
>
>> + * DT : 21:16
>> + *
>> + * CID : VC 3:0 << 2 | DT_ID 1:0
>> + */
>> u8 dt_id = vc;
> So if dt_id is 2 bits, and vc can be more than 2 bits in the future,
> shouldn't this read "vc & 3"?
Hmm...
val |= dt_id << RDI_CFG0_DT_ID;
yes that would overrun otherwise.
---
bod
Digging into the documentation we find that the DT_ID bitfield is used to
map the six bit DT to a two bit ID code. This value is concatenated to the
VC bitfield to create a CID value. DT_ID is the two least significant bits
of CID and VC the most significant bits.
Originally we set dt_id = vc * 4 in and then subsequently set dt_id = vc.
commit 3c4ed72a16bc ("media: camss: sm8250: Virtual channels for CSID")
silently fixed the multiplication by four which would give a better
value for the generated CID without mentioning what was being done or why.
Next up I haplessly changed the value back to "dt_id = vc * 4" since there
didn't appear to be any logic behind it.
Hans asked what the change was for and I honestly couldn't remember the
provenance of it, so I dug in.
Link: https://lore.kernel.org/linux-arm-msm/[email protected]/
Add a comment so the next hapless programmer doesn't make this same
mistake.
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
.../media/platform/qcom/camss/camss-csid-gen2.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
index 6ba2b10326444..05ff5fa8095a8 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
@@ -352,7 +352,19 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
phy_sel = csid->phy.csiphy_id;
if (enable) {
- u8 dt_id = vc;
+ /*
+ * DT_ID is a two bit bitfield that is concatenated with
+ * the four least significant bits of the five bit VC
+ * bitfield to generate an internal CID value.
+ *
+ * CSID_RDI_CFG0(vc)
+ * DT_ID : 28:27
+ * VC : 26:22
+ * DT : 21:16
+ *
+ * CID : VC 3:0 << 2 | DT_ID 1:0
+ */
+ u8 dt_id = vc & 0x03;
if (tg->enabled) {
/* configure one DT, infinite frames */
--
2.42.0