Re: [PATCH v2] vsp1: fix video output on R8A77970
Hi Sergei,
On Monday, 15 January 2018 18:06:48 EET Sergei Shtylyov wrote:
> On 01/15/2018 03:51 PM, Laurent Pinchart wrote:
> > 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 :-)
> >> 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.
> >> 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, ...
> >> 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
> >> .
> >
> > 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.
> >> Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and
> >> VSP2-D instances")
> >> Signed-off-by: Sergei Shtylyov
> >>
> >> ---
> >> 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
> >> ===
> >> --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c
> >> +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c
> >> @@ -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.
Yes it's just an educated guess.
> > 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.
Sounds good to me.
> > * 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).
>
> > The commit message should also be updated to feel a bit less magic.
>
> I'll see about it.
>
> >> + 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 ?
>
> If the default value matches what's recommended by the manual, then I'd
> leave the register alone.
OK, I agree.
Re: [PATCH v2] vsp1: fix video output on R8A77970
Hello!
On 01/15/2018 03:51 PM, Laurent Pinchart wrote:
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...
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...
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?
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
.
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/recipes-kernel/linux/linux-renesas/0030-arm64-renesas-r8a7797-Add-Renesas-R8A7797-SoC-suppor.patch
Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and
VSP2-D instances")
Signed-off-by: Sergei Shtylyov
---
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
===
--- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c
+++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c
@@ -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.
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.
* 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).
The commit message should also be updated to feel a bit less magic.
I'll see about it.
+ 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 ?
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
Re: [PATCH v2] vsp1: fix video output on R8A77970
Hi Sergei,
Thank you for the patch.
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.
> 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 ?
> repeated every 8 or so screen rows.
Is it every "8 or so" rows, or exactly every 8 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
> .
What else is in the big patch ? Is it available somewhere ?
> Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and
> VSP2-D instances")
> Signed-off-by: Sergei Shtylyov
>
> ---
> 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
> ===
> --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c
> +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c
> @@ -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.
> + 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 ?
> static const struct vsp1_entity_operations lif_entity_ops = {
> Index: media_tree/drivers/media/platform/vsp1/vsp1_regs.h
> ===
> --- media_tree.orig/drivers/media/platform/vsp1/vsp1_regs.h
> +++ media_tree/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -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
> */
--
Regards,
Laurent Pinchart
[PATCH v2] 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
.
Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and
VSP2-D instances")
Signed-off-by: Sergei Shtylyov
---
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
===
--- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c
+++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c
@@ -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 = {
Index: media_tree/drivers/media/platform/vsp1/vsp1_regs.h
===
--- media_tree.orig/drivers/media/platform/vsp1/vsp1_regs.h
+++ media_tree/drivers/media/platform/vsp1/vsp1_regs.h
@@ -693,6 +693,11 @@
#define VI6_LIF_CSBTH_LBTH_MASK(0x7ff << 0)
#define VI6_LIF_CSBTH_LBTH_SHIFT 0
+#define VI6_LIF_LBA0x3b0c
+#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
*/
