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; }
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
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
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
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
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
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;
}
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
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@lists.freedesktop.org
-
Daniel Stone -
Gustavo Padovan -
Tobias Jakobi