[PATCH 1/2] drm/exynos: calculate vrefresh instead of use a fixed value

[PATCH 1/2] drm/exynos: calculate vrefresh instead of use a fixed value

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

When mode's vrefresh is zero we should ask DRM core to calculate vrefresh
for us so we can get the correct value instead of relying on fixed value
defined in a macro.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 9819fa6..08f7197 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -42,7 +42,6 @@
  * CPU Interface.
  */
 
-#define FIMD_DEFAULT_FRAMERATE 60
 #define MIN_FB_WIDTH_FOR_16WORD_BURST 128
 
 /* position control register for hardware window 0, 2 ~ 4.*/
@@ -329,7 +328,7 @@ static bool fimd_mode_fixup(struct exynos_drm_crtc *crtc,
 		struct drm_display_mode *adjusted_mode)
 {
 	if (adjusted_mode->vrefresh == 0)
-		adjusted_mode->vrefresh = FIMD_DEFAULT_FRAMERATE;
+		adjusted_mode->vrefresh = drm_mode_vrefresh(mode);
 
 	return true;
 }
-- 
2.1.0

[PATCH 2/2] drm/exynos: WARN_ON if ideal_clk is zero

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

ideal_clk is the divisor in an operation to find the clock divider so
it can't never be zero or we will end up with a division by zero error.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 08f7197..294f9cf 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -309,6 +309,8 @@ static u32 fimd_calc_clkdiv(struct fimd_context *ctx,
 	unsigned long ideal_clk = mode->htotal * mode->vtotal * mode->vrefresh;
 	u32 clkdiv;
 
+	WARN_ON(ideal_clk == 0);
+
 	if (ctx->i80_if) {
 		/*
 		 * The frame done interrupt should be occurred prior to the
-- 
2.1.0

Re: [PATCH 2/2] drm/exynos: WARN_ON if ideal_clk is zero

[Tobias Jakobi]

Hmm,

I wonder if that really 'fixes' anything, because now we get a WARN_ON 
which is immediately followed by a div-by-zero. Furthermore we then 
still use the result of that operation as input for a hw register (bad 
idea?)

I thought of something like this. Change fimd_calc_clkdiv() to return a 
signed value (this shouldn't be a problem, since the returned number 
range is small anywary). Let fimd_calc_clkdiv() return -1 when it 
encounters the 'ideal_clk == 0' case. Move computation of clkdiv in 
fimd_commit() to the top of the function (right after the {h,v}total 
checks). If 'clkdiv == -1' case is encountered, then do WARN_ON and 
return immediately from fimd_commit().

Should I prepare a patch for this, or does someone see an issue with 
this approach?

With best wishes,
Tobias


On 2015-05-20 16:33, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> ideal_clk is the divisor in an operation to find the clock divider so
> it can't never be zero or we will end up with a division by zero error.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 08f7197..294f9cf 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -309,6 +309,8 @@ static u32 fimd_calc_clkdiv(struct fimd_context 
> *ctx,
>  	unsigned long ideal_clk = mode->htotal * mode->vtotal * 
> mode->vrefresh;
>  	u32 clkdiv;
> 
> +	WARN_ON(ideal_clk == 0);
> +
>  	if (ctx->i80_if) {
>  		/*
>  		 * The frame done interrupt should be occurred prior to the

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/exynos: WARN_ON if ideal_clk is zero

[Daniel Stone]

Hi,

On 20 May 2015 at 17:04, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> Hmm,
>
> I wonder if that really 'fixes' anything, because now we get a WARN_ON which
> is immediately followed by a div-by-zero. Furthermore we then still use the
> result of that operation as input for a hw register (bad idea?)
>
> I thought of something like this. Change fimd_calc_clkdiv() to return a
> signed value (this shouldn't be a problem, since the returned number range
> is small anywary). Let fimd_calc_clkdiv() return -1 when it encounters the
> 'ideal_clk == 0' case. Move computation of clkdiv in fimd_commit() to the
> top of the function (right after the {h,v}total checks). If 'clkdiv == -1'
> case is encountered, then do WARN_ON and return immediately from
> fimd_commit().
>
> Should I prepare a patch for this, or does someone see an issue with this
> approach?

Commit should never fail; as I said earlier, the right fix is to
reject these modes during checking, by making sure that any mode which
passes mode_fixup() and/or mode_valid() never trips this condition.

Cheers,
Daniel

Re: [PATCH 2/2] drm/exynos: WARN_ON if ideal_clk is zero

[Tobias Jakobi]

On 2015-05-20 18:14, Daniel Stone wrote:
> Hi,
> 
> On 20 May 2015 at 17:04, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> 
> wrote:
>> Hmm,
>> 
>> I wonder if that really 'fixes' anything, because now we get a WARN_ON 
>> which
>> is immediately followed by a div-by-zero. Furthermore we then still 
>> use the
>> result of that operation as input for a hw register (bad idea?)
>> 
>> I thought of something like this. Change fimd_calc_clkdiv() to return 
>> a
>> signed value (this shouldn't be a problem, since the returned number 
>> range
>> is small anywary). Let fimd_calc_clkdiv() return -1 when it encounters 
>> the
>> 'ideal_clk == 0' case. Move computation of clkdiv in fimd_commit() to 
>> the
>> top of the function (right after the {h,v}total checks). If 'clkdiv == 
>> -1'
>> case is encountered, then do WARN_ON and return immediately from
>> fimd_commit().
>> 
>> Should I prepare a patch for this, or does someone see an issue with 
>> this
>> approach?
> 
> Commit should never fail; as I said earlier, the right fix is to
> reject these modes during checking, by making sure that any mode which
> passes mode_fixup() and/or mode_valid() never trips this condition.
But then in this case it wouldn't help to call drm_mode_vrefresh() 
during fixup, since it still returns zero, so we're in the same 
situation as before.

I even wonder if fixup is doing anything at all. See my previous log in:
http://www.spinics.net/lists/linux-samsung-soc/msg44683.html

This is with the old code in fixup that should set vrefresh to 60.

And we see that it's actually called:
[  135.978878] [drm:fimd_mode_fixup] vrefresh 0

But then a small while later:
[  135.979048] [drm:fimd_calc_clkdiv] vrefresh 0

So apparantly nothing was fixed up here anyway?!

With best wishes,
Tobias




> 
> Cheers,
> Daniel

Re: [PATCH 1/2] drm/exynos: calculate vrefresh instead of use a fixed value

[Tobias Jakobi]

On 2015-05-20 16:33, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> When mode's vrefresh is zero we should ask DRM core to calculate 
> vrefresh
> for us so we can get the correct value instead of relying on fixed 
> value
> defined in a macro.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 9819fa6..08f7197 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -42,7 +42,6 @@
>   * CPU Interface.
>   */
> 
> -#define FIMD_DEFAULT_FRAMERATE 60
>  #define MIN_FB_WIDTH_FOR_16WORD_BURST 128
> 
>  /* position control register for hardware window 0, 2 ~ 4.*/
> @@ -329,7 +328,7 @@ static bool fimd_mode_fixup(struct exynos_drm_crtc 
> *crtc,
>  		struct drm_display_mode *adjusted_mode)
>  {
>  	if (adjusted_mode->vrefresh == 0)
Well, I'm not completly sure how this all works, but shouldn't we check 
'mode' here, and not 'adjusted_mode'?

With best wishes,
Tobias


> -		adjusted_mode->vrefresh = FIMD_DEFAULT_FRAMERATE;
> +		adjusted_mode->vrefresh = drm_mode_vrefresh(mode);
> 
>  	return true;
>  }

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/exynos: WARN_ON if ideal_clk is zero

[Daniel Stone]

Hi,

On 20 May 2015 at 17:31, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> On 2015-05-20 18:14, Daniel Stone wrote:
>> On 20 May 2015 at 17:04, Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> wrote:
>>> I wonder if that really 'fixes' anything, because now we get a WARN_ON
>>> which
>>> is immediately followed by a div-by-zero. Furthermore we then still use
>>> the
>>> result of that operation as input for a hw register (bad idea?)
>>>
>>> I thought of something like this. Change fimd_calc_clkdiv() to return a
>>> signed value (this shouldn't be a problem, since the returned number
>>> range
>>> is small anywary). Let fimd_calc_clkdiv() return -1 when it encounters
>>> the
>>> 'ideal_clk == 0' case. Move computation of clkdiv in fimd_commit() to the
>>> top of the function (right after the {h,v}total checks). If 'clkdiv ==
>>> -1'
>>> case is encountered, then do WARN_ON and return immediately from
>>> fimd_commit().
>>>
>>> Should I prepare a patch for this, or does someone see an issue with this
>>> approach?
>>
>>
>> Commit should never fail; as I said earlier, the right fix is to
>> reject these modes during checking, by making sure that any mode which
>> passes mode_fixup() and/or mode_valid() never trips this condition.
>
> But then in this case it wouldn't help to call drm_mode_vrefresh() during
> fixup, since it still returns zero, so we're in the same situation as
> before.

fixup can reject a mode entirely by returning false, which is the sane
thing to do here if the clock is zero.

> I even wonder if fixup is doing anything at all. See my previous log in:
> http://www.spinics.net/lists/linux-samsung-soc/msg44683.html
>
> This is with the old code in fixup that should set vrefresh to 60.
>
> And we see that it's actually called:
> [  135.978878] [drm:fimd_mode_fixup] vrefresh 0
>
> But then a small while later:
> [  135.979048] [drm:fimd_calc_clkdiv] vrefresh 0
>
> So apparantly nothing was fixed up here anyway?!

Ah. That would be because fimd_calc_clkdiv uses &crtc->base.mode
(passed by fimd_commit), instead of crtc->state->adjusted_mode. Does
making that change help?

Cheers,
Daniel

Re: [PATCH 1/2] drm/exynos: calculate vrefresh instead of use a fixed value

[Daniel Stone]

Hi,

On 20 May 2015 at 17:58, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> On 2015-05-20 16:33, Gustavo Padovan wrote:
>> When mode's vrefresh is zero we should ask DRM core to calculate vrefresh
>> for us so we can get the correct value instead of relying on fixed value
>> defined in a macro.
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> index 9819fa6..08f7197 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> @@ -42,7 +42,6 @@
>>   * CPU Interface.
>>   */
>>
>> -#define FIMD_DEFAULT_FRAMERATE 60
>>  #define MIN_FB_WIDTH_FOR_16WORD_BURST 128
>>
>>  /* position control register for hardware window 0, 2 ~ 4.*/
>> @@ -329,7 +328,7 @@ static bool fimd_mode_fixup(struct exynos_drm_crtc
>> *crtc,
>>                 struct drm_display_mode *adjusted_mode)
>>  {
>>         if (adjusted_mode->vrefresh == 0)
>
> Well, I'm not completly sure how this all works, but shouldn't we check
> 'mode' here, and not 'adjusted_mode'?

adjusted_mode is fine; it is pre-populated with the value from mode.
'mode' itself _must not_ be modified.

Mind you, this is missing a hunk to reject entirely invalid modes, i.e.:
if (adjusted_mode->vrefresh == 0)
        adjusted_mode->vrefresh = drm_mode_vrefresh(adjusted_mode);
if (adjusted_mode->vrefresh == 0)
        return false;

Cheers,
Daniel

Re: [PATCH 1/2] drm/exynos: calculate vrefresh instead of use a fixed value

[Gustavo Padovan]

Hi,

2015-05-20 Daniel Stone <daniel@fooishbar.org>:

> Hi,
> 
> On 20 May 2015 at 17:58, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> > On 2015-05-20 16:33, Gustavo Padovan wrote:
> >> When mode's vrefresh is zero we should ask DRM core to calculate vrefresh
> >> for us so we can get the correct value instead of relying on fixed value
> >> defined in a macro.
> >>
> >> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >> ---
> >>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> >> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> >> index 9819fa6..08f7197 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> >> @@ -42,7 +42,6 @@
> >>   * CPU Interface.
> >>   */
> >>
> >> -#define FIMD_DEFAULT_FRAMERATE 60
> >>  #define MIN_FB_WIDTH_FOR_16WORD_BURST 128
> >>
> >>  /* position control register for hardware window 0, 2 ~ 4.*/
> >> @@ -329,7 +328,7 @@ static bool fimd_mode_fixup(struct exynos_drm_crtc
> >> *crtc,
> >>                 struct drm_display_mode *adjusted_mode)
> >>  {
> >>         if (adjusted_mode->vrefresh == 0)
> >
> > Well, I'm not completly sure how this all works, but shouldn't we check
> > 'mode' here, and not 'adjusted_mode'?
> 
> adjusted_mode is fine; it is pre-populated with the value from mode.
> 'mode' itself _must not_ be modified.
> 
> Mind you, this is missing a hunk to reject entirely invalid modes, i.e.:
> if (adjusted_mode->vrefresh == 0)
>         adjusted_mode->vrefresh = drm_mode_vrefresh(adjusted_mode);
> if (adjusted_mode->vrefresh == 0)
>         return false;

I have sent a v2 with the proposed change. I can't really test this here
as I don't have the hardware where it fails. So in v1 I was just
guessing that it would fix the issue.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel