Re: [PATCH 6/7] drm/exynos/decon5433: signal vblank only on odd fields

[Ville Syrjälä]

On Thu, Jan 26, 2017 at 09:22:27AM +0100, Andrzej Hajda wrote:
> On 25.01.2017 15:06, Ville Syrjälä wrote:
> > On Mon, Jan 23, 2017 at 10:15:16AM +0100, Andrzej Hajda wrote:
> >> On 20.01.2017 14:55, Ville Syrjälä wrote:
> >>> On Fri, Jan 20, 2017 at 07:52:24AM +0100, Andrzej Hajda wrote:
>  In case of interlace mode irq is generated for odd and even fields, but
>  vblank should be signaled only for the last emitted field.
> >>> I'm pretty sure most drivers signal it for both fields. At least i915
> >>> does.
> >> The question is which behavior is correct? I have not found any clear
> >> statement in the documentation, or drm core code.
> > That's very typical for us unfortunately.
> >
> > I would say what we should do what i915 does. It allows more flexibility
> > in how you use the hardware. Eg. then you can actually scan out
> > interlaced material to an interlaced display and not mess up the fields,
> > and you can also do 3:2 pulldown type of stuff. Or you can even just
> > stuff progressive frames down the pipe at field rate.
> >
> > One problem with interlaced stuff is that we don't have any field
> > indication in the events, nor do we have a way to flip on a specific
> > field. I tried to specify the latter for the SETPLANE ioctl way
> > back when, but it didn't end up being implemented and now we would
> > need something different for atomic.
> >
> >> I have guessed that since vblank event is used to signal end of scan-out
> >> of buffer it should be called after scan-out of whole buffer - in case
> >> of interlaced mode after scan-out of 2nd field.
> > Each field has a proper vertical blanking interval, so you'd just end up
> > totally wasting one of them.
> 
> The problem in this particular case is that hardware does not allow to
> change buffers between fields, or more precisely it updates its internal
> registers after 2nd field - ie after reading full frame.

Oh. That's a rather odd piece of hw then. In that case it might indeed
be better to not signal vblank for the field that can't do the flip.

> I am still investigating the issue, but it is possible this limitation
> cannot be overcome.
> 
> Regards
> Andrzej
> 
> >
> >> Maybe my assumption is wrong, in such case this patch should be dropped
> >> and mixer driver also should be fixed, but before doing that it would be
> >> good to know for sure how it should be handled correctly.
> >>
> >> Regards
> >> Andrzej
> 

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] drm/exynos/decon5433: signal vblank only on odd fields

[Andrzej Hajda]

On 25.01.2017 15:06, Ville Syrjälä wrote:
> On Mon, Jan 23, 2017 at 10:15:16AM +0100, Andrzej Hajda wrote:
>> On 20.01.2017 14:55, Ville Syrjälä wrote:
>>> On Fri, Jan 20, 2017 at 07:52:24AM +0100, Andrzej Hajda wrote:
 In case of interlace mode irq is generated for odd and even fields, but
 vblank should be signaled only for the last emitted field.
>>> I'm pretty sure most drivers signal it for both fields. At least i915
>>> does.
>> The question is which behavior is correct? I have not found any clear
>> statement in the documentation, or drm core code.
> That's very typical for us unfortunately.
>
> I would say what we should do what i915 does. It allows more flexibility
> in how you use the hardware. Eg. then you can actually scan out
> interlaced material to an interlaced display and not mess up the fields,
> and you can also do 3:2 pulldown type of stuff. Or you can even just
> stuff progressive frames down the pipe at field rate.
>
> One problem with interlaced stuff is that we don't have any field
> indication in the events, nor do we have a way to flip on a specific
> field. I tried to specify the latter for the SETPLANE ioctl way
> back when, but it didn't end up being implemented and now we would
> need something different for atomic.
>
>> I have guessed that since vblank event is used to signal end of scan-out
>> of buffer it should be called after scan-out of whole buffer - in case
>> of interlaced mode after scan-out of 2nd field.
> Each field has a proper vertical blanking interval, so you'd just end up
> totally wasting one of them.

The problem in this particular case is that hardware does not allow to
change buffers between fields, or more precisely it updates its internal
registers after 2nd field - ie after reading full frame.
I am still investigating the issue, but it is possible this limitation
cannot be overcome.

Regards
Andrzej

>
>> Maybe my assumption is wrong, in such case this patch should be dropped
>> and mixer driver also should be fixed, but before doing that it would be
>> good to know for sure how it should be handled correctly.
>>
>> Regards
>> Andrzej


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] drm/exynos/decon5433: signal vblank only on odd fields

[Ville Syrjälä]

On Mon, Jan 23, 2017 at 10:15:16AM +0100, Andrzej Hajda wrote:
> On 20.01.2017 14:55, Ville Syrjälä wrote:
> > On Fri, Jan 20, 2017 at 07:52:24AM +0100, Andrzej Hajda wrote:
> >> In case of interlace mode irq is generated for odd and even fields, but
> >> vblank should be signaled only for the last emitted field.
> > I'm pretty sure most drivers signal it for both fields. At least i915
> > does.
> 
> The question is which behavior is correct? I have not found any clear
> statement in the documentation, or drm core code.

That's very typical for us unfortunately.

I would say what we should do what i915 does. It allows more flexibility
in how you use the hardware. Eg. then you can actually scan out
interlaced material to an interlaced display and not mess up the fields,
and you can also do 3:2 pulldown type of stuff. Or you can even just
stuff progressive frames down the pipe at field rate.

One problem with interlaced stuff is that we don't have any field
indication in the events, nor do we have a way to flip on a specific
field. I tried to specify the latter for the SETPLANE ioctl way
back when, but it didn't end up being implemented and now we would
need something different for atomic.

> I have guessed that since vblank event is used to signal end of scan-out
> of buffer it should be called after scan-out of whole buffer - in case
> of interlaced mode after scan-out of 2nd field.

Each field has a proper vertical blanking interval, so you'd just end up
totally wasting one of them.

> Maybe my assumption is wrong, in such case this patch should be dropped
> and mixer driver also should be fixed, but before doing that it would be
> good to know for sure how it should be handled correctly.
> 
> Regards
> Andrzej

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] drm/exynos/decon5433: signal vblank only on odd fields

[Andrzej Hajda]

On 20.01.2017 14:55, Ville Syrjälä wrote:
> On Fri, Jan 20, 2017 at 07:52:24AM +0100, Andrzej Hajda wrote:
>> In case of interlace mode irq is generated for odd and even fields, but
>> vblank should be signaled only for the last emitted field.
> I'm pretty sure most drivers signal it for both fields. At least i915
> does.

The question is which behavior is correct? I have not found any clear
statement in the documentation, or drm core code.
I have guessed that since vblank event is used to signal end of scan-out
of buffer it should be called after scan-out of whole buffer - in case
of interlaced mode after scan-out of 2nd field.
Maybe my assumption is wrong, in such case this patch should be dropped
and mixer driver also should be fixed, but before doing that it would be
good to know for sure how it should be handled correctly.

Regards
Andrzej

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] drm/exynos/decon5433: signal vblank only on odd fields

[Ville Syrjälä]

On Fri, Jan 20, 2017 at 07:52:24AM +0100, Andrzej Hajda wrote:
> In case of interlace mode irq is generated for odd and even fields, but
> vblank should be signaled only for the last emitted field.

I'm pretty sure most drivers signal it for both fields. At least i915
does.

> 
> Signed-off-by: Andrzej Hajda 
> ---
>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 7 +++
>  include/video/exynos5433_decon.h  | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c 
> b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> index 46434ba9..ad8b93a 100644
> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> @@ -591,6 +591,13 @@ static irqreturn_t decon_irq_handler(int irq, void 
> *dev_id)
>  
>   if (val) {
>   writel(val, ctx->addr + DECON_VIDINTCON1);
> + if (ctx->out_type & IFTYPE_HDMI) {
> + val = readl(ctx->addr + DECON_VIDOUTCON0);
> + val &= VIDOUT_INTERLACE_EN_F | VIDOUT_INTERLACE_FIELD_F;
> + if (val ==
> + (VIDOUT_INTERLACE_EN_F | VIDOUT_INTERLACE_FIELD_F))
> + return IRQ_HANDLED;
> + }
>   drm_crtc_handle_vblank(>crtc->base);
>   }
>  
> diff --git a/include/video/exynos5433_decon.h 
> b/include/video/exynos5433_decon.h
> index b26511a..ef8e2a8 100644
> --- a/include/video/exynos5433_decon.h
> +++ b/include/video/exynos5433_decon.h
> @@ -89,6 +89,7 @@
>  #define VIDCON0_ENVID_F  (1 << 0)
>  
>  /* VIDOUTCON0 */
> +#define VIDOUT_INTERLACE_FIELD_F (1 << 29)
>  #define VIDOUT_INTERLACE_EN_F(1 << 28)
>  #define VIDOUT_LCD_ON(1 << 24)
>  #define VIDOUT_IF_F_MASK (0x3 << 20)
> -- 
> 2.7.4
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel