diff mbox

[PATCH/RFC] v4l: vsp1: Change VSP1 LIF linebuffer FIFO

Message ID 1426430018-3172-1-git-send-email-ykaneko0929@gmail.com (mailing list archive)
State RFC
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Yoshihiro Kaneko March 15, 2015, 2:33 p.m. UTC 
From: Yoshifumi Hosoya <yoshifumi.hosoya.wj@renesas.com>

Change to VSPD hardware recommended value.
Purpose is highest pixel clock without underruns.
In the default R-Car Linux BSP config this value is
wrong and therefore there are many underruns.

Here are the original settings:
HBTH = 1300 (VSPD stops when 1300 pixels are buffered)
LBTH = 200 (VSPD resumes when buffer level has decreased
            below 200 pixels)

The display underruns can be eliminated
by applying the following settings:
HBTH = 1504
LBTH = 1248

Reported-by: Peter Fiedler <peter.fiedler@renesas.com>
Signed-off-by: Yoshifumi Hosoya <yoshifumi.hosoya.wj@renesas.com>
Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
---

This patch is based on the master branch of linuxtv.org/media_tree.git.

 drivers/media/platform/vsp1/vsp1_lif.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Geert Uytterhoeven March 16, 2015, 8:06 a.m. UTC | #1 
Hi Kaneko-san, Hosoya-san,

On Sun, Mar 15, 2015 at 3:33 PM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> From: Yoshifumi Hosoya <yoshifumi.hosoya.wj@renesas.com>
>
> Change to VSPD hardware recommended value.
> Purpose is highest pixel clock without underruns.
> In the default R-Car Linux BSP config this value is
> wrong and therefore there are many underruns.
>
> Here are the original settings:
> HBTH = 1300 (VSPD stops when 1300 pixels are buffered)
> LBTH = 200 (VSPD resumes when buffer level has decreased
>             below 200 pixels)
>
> The display underruns can be eliminated
> by applying the following settings:
> HBTH = 1504
> LBTH = 1248

> --- a/drivers/media/platform/vsp1/vsp1_lif.c
> +++ b/drivers/media/platform/vsp1/vsp1_lif.c
> @@ -44,9 +44,9 @@ static int lif_s_stream(struct v4l2_subdev *subdev, int enable)
>  {
>         const struct v4l2_mbus_framefmt *format;
>         struct vsp1_lif *lif = to_lif(subdev);
> -       unsigned int hbth = 1300;
> -       unsigned int obth = 400;
> -       unsigned int lbth = 200;
> +       unsigned int hbth = 1536;
> +       unsigned int obth = 128;
> +       unsigned int lbth = 1520;

These values don't match the patch description?

BTW, what's the significance of changing obth?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart March 18, 2015, 1:23 p.m. UTC | #2 
Hello,

On Monday 16 March 2015 09:06:22 Geert Uytterhoeven wrote:
> On Sun, Mar 15, 2015 at 3:33 PM, Yoshihiro Kaneko wrote:
> > From: Yoshifumi Hosoya <yoshifumi.hosoya.wj@renesas.com>
> > 
> > Change to VSPD hardware recommended value.
> > Purpose is highest pixel clock without underruns.
> > In the default R-Car Linux BSP config this value is
> > wrong and therefore there are many underruns.
> > 
> > Here are the original settings:
> > HBTH = 1300 (VSPD stops when 1300 pixels are buffered)
> > LBTH = 200 (VSPD resumes when buffer level has decreased
> >             below 200 pixels)
> > 
> > The display underruns can be eliminated
> > by applying the following settings:
> > HBTH = 1504
> > LBTH = 1248
> > 
> > --- a/drivers/media/platform/vsp1/vsp1_lif.c
> > +++ b/drivers/media/platform/vsp1/vsp1_lif.c
> > @@ -44,9 +44,9 @@ static int lif_s_stream(struct v4l2_subdev *subdev, int
> > enable)
> >  {
> >         const struct v4l2_mbus_framefmt *format;
> >         struct vsp1_lif *lif = to_lif(subdev);
> > -       unsigned int hbth = 1300;
> > -       unsigned int obth = 400;
> > -       unsigned int lbth = 200;
> > +       unsigned int hbth = 1536;
> > +       unsigned int obth = 128;
> > +       unsigned int lbth = 1520;
> 
> These values don't match the patch description?

Indeed. And where do these values come from ? A 16 bytes hysteresis is very 
small, the VSP1 will constantly start and stop. Isn't that bad from a power 
consumption point of view ?

> BTW, what's the significance of changing obth?
Yoshihiro Kaneko March 29, 2015, 1:12 p.m. UTC | #3 
Hi Geert, Hi Laurent,

Thanks for your review. There do indeed seem to be some problems with
this patch.
I'm happy get some feedback from the BSP team if you think it is
worthwhile. Else I suggest we simply drop this issue for now.

Thanks,
Kaneko

2015-03-18 22:23 GMT+09:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hello,
>
> On Monday 16 March 2015 09:06:22 Geert Uytterhoeven wrote:
>> On Sun, Mar 15, 2015 at 3:33 PM, Yoshihiro Kaneko wrote:
>> > From: Yoshifumi Hosoya <yoshifumi.hosoya.wj@renesas.com>
>> >
>> > Change to VSPD hardware recommended value.
>> > Purpose is highest pixel clock without underruns.
>> > In the default R-Car Linux BSP config this value is
>> > wrong and therefore there are many underruns.
>> >
>> > Here are the original settings:
>> > HBTH = 1300 (VSPD stops when 1300 pixels are buffered)
>> > LBTH = 200 (VSPD resumes when buffer level has decreased
>> >             below 200 pixels)
>> >
>> > The display underruns can be eliminated
>> > by applying the following settings:
>> > HBTH = 1504
>> > LBTH = 1248
>> >
>> > --- a/drivers/media/platform/vsp1/vsp1_lif.c
>> > +++ b/drivers/media/platform/vsp1/vsp1_lif.c
>> > @@ -44,9 +44,9 @@ static int lif_s_stream(struct v4l2_subdev *subdev, int
>> > enable)
>> >  {
>> >         const struct v4l2_mbus_framefmt *format;
>> >         struct vsp1_lif *lif = to_lif(subdev);
>> > -       unsigned int hbth = 1300;
>> > -       unsigned int obth = 400;
>> > -       unsigned int lbth = 200;
>> > +       unsigned int hbth = 1536;
>> > +       unsigned int obth = 128;
>> > +       unsigned int lbth = 1520;
>>
>> These values don't match the patch description?
>
> Indeed. And where do these values come from ? A 16 bytes hysteresis is very
> small, the VSP1 will constantly start and stop. Isn't that bad from a power
> consumption point of view ?
>
>> BTW, what's the significance of changing obth?
>
> --
> Regards,
>
> Laurent Pinchart
>
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/platform/vsp1/vsp1_lif.c b/drivers/media/platform/vsp1/vsp1_lif.c
index 17a6ca7..7d0f7eb 100644
--- a/drivers/media/platform/vsp1/vsp1_lif.c
+++ b/drivers/media/platform/vsp1/vsp1_lif.c
@@ -1,7 +1,7 @@ 
 /*
  * vsp1_lif.c  --  R-Car VSP1 LCD Controller Interface
  *
- * Copyright (C) 2013-2014 Renesas Electronics Corporation
+ * Copyright (C) 2013-2015 Renesas Electronics Corporation
  *
  * Contact: Laurent Pinchart (laurent.pinchart@ideasonboard.com)
  *
@@ -44,9 +44,9 @@  static int lif_s_stream(struct v4l2_subdev *subdev, int enable)
 {
 	const struct v4l2_mbus_framefmt *format;
 	struct vsp1_lif *lif = to_lif(subdev);
-	unsigned int hbth = 1300;
-	unsigned int obth = 400;
-	unsigned int lbth = 200;
+	unsigned int hbth = 1536;
+	unsigned int obth = 128;
+	unsigned int lbth = 1520;
 
 	if (!enable) {
 		vsp1_lif_write(lif, VI6_LIF_CTRL, 0);