vsp1: fix video output on R8A77970
PATCH v2 1/1 vsp1: fix video output on R8A77970
Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but the video output that VSP2-D sends to DU has a greenish garbage-like line repeated every 8 or so screen rows. It turns out that V3M has a teeny LIF register (at least it's documented!) that you need to set to some kind of a magic value for the LIF to work correctly...
Based on the original (and large) patch by Daisuke Matsushita <[email protected]>.
---
This patch is against the 'media_tree.git' repo's 'master' branch.
Changes in version 2:
- added a comment before the V3M SoC check;
- fixed indetation in that check;
- reformatted the patch description.
drivers/media/platform/vsp1/vsp1_lif.c | 12 ++++++++++++ drivers/media/platform/vsp1/vsp1_regs.h | 5 +++++ 2 files changed, 17 insertions(+)
drivers/media/platform/vsp1/vsp1_lif.c +12 -0
@@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en (obth <<VI6_LIF_CTRL_OBTH_SHIFT ) | (format->code ==0 ?VI6_LIF_CTRL_CFMT :0 ) | VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); + +/* +* R-Car V3M has the buffer attribute register you absolutely need +* to write kinda magic value to for the LIF to work correctly... +*/ +if ((entity->vsp1->version & + (VI6_IP_VERSION_MODEL_MASK |VI6_IP_VERSION_SOC_MASK )) == + (VI6_IP_VERSION_MODEL_VSPD_V3 |VI6_IP_VERSION_SOC_V3M )) { +vsp1_lif_write (lif, dl,VI6_LIF_LBA , +VI6_LIF_LBA_LBA0 | + (1536 <<VI6_LIF_LBA_LBA1_SHIFT )); + } }static const struct vsp1_entity_operations lif_entity_ops = {
drivers/media/platform/vsp1/vsp1_regs.h +5 -0
@@ -693,6 +693,11 @@#define VI6_LIF_CSBTH_LBTH_MASK (0x7ff << 0)#define VI6_LIF_CSBTH_LBTH_SHIFT 0 +#define VI6_LIF_LBA 0x3b0c +#define VI6_LIF_LBA_LBA0 (1 << 31) +#define VI6_LIF_LBA_LBA1_MASK (0xfff << 16) +#define VI6_LIF_LBA_LBA1_SHIFT 16 +/* ----------------------------------------------------------------------------- * Security Control Registers */
Hi Sergei,
Thank you for the patch.
Quoted 1 line · On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote:
Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but
I'm not sure there's a need to state my name in the commit message.
Quoted 1 line
the video output that VSP2-D sends to DU has a greenish garbage-like line
Why does the text in your patches (commit message, comments, ...) sometime have double spaces between words ?
Quoted 1 line
repeated every 8 or so screen rows.
Is it every "8 or so" rows, or exactly every 8 rows ?
Quoted 6 lines
It turns out that V3M has a teeny LIF register (at least it's documented!) that you need to set to some kind of a magic value for the LIF to work correctly...
Based on the original (and large) patch by Daisuke Matsushita <[email protected]>.
What else is in the big patch ? Is it available somewhere ?
Quoted 21 lines
Fixes d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL andVSP2-D instances")
Signed-off-by Sergei Shtylyov <[email protected]>---
This patch is against the 'media_tree.git' repo's 'master' branch.Changes in version 2:
- added a comment before the V3M SoC check;
- fixed indetation in that check;
- reformatted the patch description.drivers/media/platform/vsp1/vsp1_lif.c | 12 ++++++++++++ drivers/media/platform/vsp1/vsp1_regs.h | 5 +++++ 2 files changed, 17 insertions(+)Index:
media_tree/drivers/media/platform/vsp1/vsp1_lif.c
===================================================================
drivers/media/platform/vsp1/vsp1_lif.c static void lif_configure(struct vsp1_en · line 162
Quoted 8 lines
drivers/media/platform/vsp1/vsp1_lif.c +5
-0@@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en (obth <<VI6_LIF_CTRL_OBTH_SHIFT ) | (format->code ==0 ?VI6_LIF_CTRL_CFMT :0 ) | VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); + +/* +* R-Car V3M has the buffer attribute register you absolutely need +* to write kinda magic value to for the LIF to work correctly... +*/
I'm not sure about the "kinda" magic value. 1536 is very likely a buffer size. How about the following text ?
/*
* On V3M the LBA register has to be set to a non-default value to
* guarantee proper operation (otherwise artifacts may appear on the
* output). The value required by the datasheet is not documented but
* is likely a buffer size or threshold.
*/
The commit message should also be updated to feel a bit less magic.
drivers/media/platform/vsp1/vsp1_lif.c static void lif_configure(struct vsp1_en · line 170
Quoted 8 lines
drivers/media/platform/vsp1/vsp1_lif.c +7
-0+if ((entity->vsp1->version & + (VI6_IP_VERSION_MODEL_MASK |VI6_IP_VERSION_SOC_MASK )) == + (VI6_IP_VERSION_MODEL_VSPD_V3 |VI6_IP_VERSION_SOC_V3M )) { +vsp1_lif_write (lif, dl,VI6_LIF_LBA , +VI6_LIF_LBA_LBA0 | + (1536 <<VI6_LIF_LBA_LBA1_SHIFT )); + } }
The datasheet documents the register as being present on both V3M and M3-W (and the test I've just run on H3 shows that the register is present there as well). Should we program it on M3-W or leave it to the default value that should be what is recommended by the datasheet for that SoC ?
Quoted 17 lines
drivers/media/platform/vsp1/vsp1_lif.c +0
-0static const struct vsp1_entity_operations lif_entity_ops = {Index:
media_tree/drivers/media/platform/vsp1/vsp1_regs.h
===================================================================drivers/media/platform/vsp1/vsp1_regs.h +5
-0@@ -693,6 +693,11 @@#define VI6_LIF_CSBTH_LBTH_MASK (0x7ff << 0)#define VI6_LIF_CSBTH_LBTH_SHIFT 0 +#define VI6_LIF_LBA 0x3b0c +#define VI6_LIF_LBA_LBA0 (1 << 31) +#define VI6_LIF_LBA_LBA1_MASK (0xfff << 16) +#define VI6_LIF_LBA_LBA1_SHIFT 16 +/* ------------------------------------------------------------------------ * Security Control Registers */
Signature · 3 lines
Regards, Laurent Pinchart
Hello!
Quoted 4 lines · Laurent Pinchart
Quoted 1 line · On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote:
Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but
I'm not sure there's a need to state my name in the commit message.
You were the author of the patch this one has in the Fixes: tag, so I
thought that was appropriate. I can remove that if you don't want you name mentioned...
Quoted 4 lines
Quoted 1 line
the video output that VSP2-D sends to DU has a greenish garbage-like line
Why does the text in your patches (commit message, comments, ...) sometime have double spaces between words ?
The text looks more pleasant (at least to me). Can remove them if you want...
Quoted 3 lines
Quoted 1 line
repeated every 8 or so screen rows.
Is it every "8 or so" rows, or exactly every 8 rows ?
You really want me to count the pixels?
Quoted 8 lines
Quoted 6 lines
It turns out that V3M has a teeny LIF register (at least it's documented!) that you need to set to some kind of a magic value for the LIF to work correctly...
Based on the original (and large) patch by Daisuke Matsushita <[email protected]>.
What else is in the big patch ? Is it available somewhere ?
Assorted changes gathered together only because they all bring the support
for R8A77970. If you're really curious, here you are:
Quoted 16 lines
Quoted 16 lines
Fixes d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL andVSP2-D instances")
Signed-off-by Sergei Shtylyov <[email protected]>---
This patch is against the 'media_tree.git' repo's 'master' branch.Changes in version 2:
- added a comment before the V3M SoC check;
- fixed indetation in that check;
- reformatted the patch description.drivers/media/platform/vsp1/vsp1_lif.c | 12 ++++++++++++ drivers/media/platform/vsp1/vsp1_regs.h | 5 +++++ 2 files changed, 17 insertions(+)
Quoted 15 lines
drivers/media/platform/vsp1/vsp1_lif.c static void lif_configure(struct vsp1_en · line 162
Quoted 13 lines
Index:
media_tree/drivers/media/platform/vsp1/vsp1_lif.c
===================================================================drivers/media/platform/vsp1/vsp1_lif.c +5
-0@@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en (obth <<VI6_LIF_CTRL_OBTH_SHIFT ) | (format->code ==0 ?VI6_LIF_CTRL_CFMT :0 ) | VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); + +/* +* R-Car V3M has the buffer attribute register you absolutely need +* to write kinda magic value to for the LIF to work correctly... +*/ I'm not sure about the "kinda" magic value. 1536 is very likely a buffer size.
Well, that's only guessing. The manual doesn't say anything about what the
number is.
Quoted 4 lines
How about the following text ?
/* * On V3M the LBA register has to be set to a non-default value to
I'd then spell its full name, LIF0 Buffer Attribute register.
Quoted 5 lines
* guarantee proper operation (otherwise artifacts may appear on the
* output). The value required by the datasheet is not documented but
* is likely a buffer size or threshold.
*/
OK, can change that (modulo the name).
Quoted 1 line
The commit message should also be updated to feel a bit less magic.
I'll see about it.
Quoted 5 lines
Quoted 5 lines
drivers/media/platform/vsp1/vsp1_lif.c +5
-0+if ((entity->vsp1->version & + (VI6_IP_VERSION_MODEL_MASK |VI6_IP_VERSION_SOC_MASK )) == + (VI6_IP_VERSION_MODEL_VSPD_V3 |VI6_IP_VERSION_SOC_V3M )) { + vsp1_lif_write(lif, dl, VI6_LIF_LBA, + VI6_LIF_LBA_LBA0 |
Quoted 8 lines
drivers/media/platform/vsp1/vsp1_lif.c static void lif_configure(struct vsp1_en · line 170
Quoted 3 lines
drivers/media/platform/vsp1/vsp1_lif.c +2
-0+ (1536 << VI6_LIF_LBA_LBA1_SHIFT)); + } }The datasheet documents the register as being present on both V3M and M3-W (and the test I've just run on H3 shows that the register is present there as well). Should we program it on M3-W or leave it to the default value that should be what is recommended by the datasheet for that SoC ?
If the default value matches what's recommended by the manual, then I'd
leave the register alone. But my task was R8A77970 support only anyway...
[..]
MBR, Sergei
Hi Sergei,
Quoted 10 lines · On Monday, 15 January 2018 18:06:48 EET Sergei Shtylyov wrote:
Quoted 5 lines · Laurent Pinchart
Quoted 2 lines · On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote:
Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but
I'm not sure there's a need to state my name in the commit message.
You were the author of the patch this one has in the Fixes: tag, so I thought that was appropriate. I can remove that if you don't want you name mentioned...
It's just that I thought it was more important to refer to the patch than to my name, developers reading the log will be more interested in the technical details than in my person :-)
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
the video output that VSP2-D sends to DU has a greenish garbage-like line
Why does the text in your patches (commit message, comments, ...) sometime have double spaces between words ?
The text looks more pleasant (at least to me). Can remove them if you want...
I don't think we try to do typesetting in kernel code or commit messages, so I'd stick to single spaces if you don't mind.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
repeated every 8 or so screen rows.
Is it every "8 or so" rows, or exactly every 8 rows ?
You really want me to count the pixels?
I'd like to know a bit more about the systems, whether the green lines appear at the exact same interval, whether they bounce up and down or are always at the same place, ...
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
It turns out that V3M has a teeny LIF register (at least it's documented!) that you need to set to some kind of a magic value for the LIF to work correctly...
Based on the original (and large) patch by Daisuke Matsushita <[email protected]>.
Quoted 8 lines
Quoted 1 line
What else is in the big patch ? Is it available somewhere ?
Assorted changes gathered together only because they all bring the support for R8A77970. If you're really curious, here you are:
https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/reci
pes-kernel/linux/linux-renesas/0030-arm64-renesas-r8a7797-Add-Renesas-R8A779 7-SoC-suppor.patch
Thank you.
Quoted 33 lines
Quoted 16 lines
Quoted 16 lines
Fixes d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL andVSP2-D instances")
Signed-off-by Sergei Shtylyov <[email protected]>---
This patch is against the 'media_tree.git' repo's 'master' branch.Changes in version 2:
- added a comment before the V3M SoC check;
- fixed indetation in that check;
- reformatted the patch description.drivers/media/platform/vsp1/vsp1_lif.c | 12 ++++++++++++ drivers/media/platform/vsp1/vsp1_regs.h | 5 +++++ 2 files changed, 17 insertions(+)Quoted 16 lines
drivers/media/platform/vsp1/vsp1_lif.c static void lif_configure(struct vsp1_en · line 162
Quoted 13 lines
Index:
media_tree/drivers/media/platform/vsp1/vsp1_lif.c
===================================================================drivers/media/platform/vsp1/vsp1_lif.c +5
-0@@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en (obth <<VI6_LIF_CTRL_OBTH_SHIFT ) | (format->code ==0 ?VI6_LIF_CTRL_CFMT :0 ) | VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); + +/* +* R-Car V3M has the buffer attribute register you absolutely need +* to write kinda magic value to for the LIF to work correctly... +*/ I'm not sure about the "kinda" magic value. 1536 is very likely a buffer size.
Quoted 2 lines
Well, that's only guessing. The manual doesn't say anything about what the number is.
Yes it's just an educated guess.
Quoted 7 lines
Quoted 5 lines
How about the following text ?
/* * On V3M the LBA register has to be set to a non-default value toI'd then spell its full name, LIF0 Buffer Attribute register.
Sounds good to me.
Quoted 26 lines
Quoted 4 lines
* guarantee proper operation (otherwise artifacts may appear on the
* output). The value required by the datasheet is not documented but
* is likely a buffer size or threshold.
*/OK, can change that (modulo the name).
Quoted 1 line
The commit message should also be updated to feel a bit less magic.
I'll see about it.
Quoted 8 lines
Quoted 8 lines
drivers/media/platform/vsp1/vsp1_lif.c +7
-0+if ((entity->vsp1->version & + (VI6_IP_VERSION_MODEL_MASK |VI6_IP_VERSION_SOC_MASK )) == + (VI6_IP_VERSION_MODEL_VSPD_V3 |VI6_IP_VERSION_SOC_V3M )) { +vsp1_lif_write (lif, dl,VI6_LIF_LBA , +VI6_LIF_LBA_LBA0 | + (1536 <<VI6_LIF_LBA_LBA1_SHIFT )); + }Quoted 6 lines
drivers/media/platform/vsp1/vsp1_lif.c static void lif_configure(struct vsp1_en · line 171
Quoted 1 line
drivers/media/platform/vsp1/vsp1_lif.c +0
-0}The datasheet documents the register as being present on both V3M and M3-W (and the test I've just run on H3 shows that the register is present there as well). Should we program it on M3-W or leave it to the default value that should be what is recommended by the datasheet for that SoC ?
Quoted 2 lines
If the default value matches what's recommended by the manual, then I'd leave the register alone.
OK, I agree.
Quoted 1 line
But my task was R8A77970 support only anyway...
Sure, and my task is to push you to do more when it makes sense ;-)
Signature · 3 lines
Regards, Laurent Pinchart