This patch series enable a QHD HDMI monitor to work at native resolution. Tested on a Sapphire board with RK3399 connected to a Q27q-10 monitor at 2560x1440@60
Vicente Bergas (3): drm: rockchip: remove vop_crtc_mode_fixup to fix clock handling drm: rockchip: HDMI: allow any clock that is within the range drm: rockchip: HDMI: add higher pixel clock frequencies
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 8 +++- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 45 --------------------- 2 files changed, 7 insertions(+), 46 deletions(-)
Under certain conditions vop_crtc_mode_fixup rounds the clock 148500000 to 148501000 which leads to the following error: dwhdmi-rockchip ff940000.hdmi: PHY configuration failed (clock 148501000)
The issue was found on RK3399 booting with u-boot. U-boot configures the display at 2560x1440 and then linux comes up with a black screen. A workaround was to un-plug and re-plug the HDMI display.
Signed-off-by: Vicente Bergas vicencb@gmail.com Tested-by: Vicente Bergas vicencb@gmail.com --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 45 --------------------- 1 file changed, 45 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index c80f7d9fd13f..fe80da652994 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1142,50 +1142,6 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc) spin_unlock_irqrestore(&vop->irq_lock, flags); }
-static bool vop_crtc_mode_fixup(struct drm_crtc *crtc, - const struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode) -{ - struct vop *vop = to_vop(crtc); - unsigned long rate; - - /* - * Clock craziness. - * - * Key points: - * - * - DRM works in in kHz. - * - Clock framework works in Hz. - * - Rockchip's clock driver picks the clock rate that is the - * same _OR LOWER_ than the one requested. - * - * Action plan: - * - * 1. When DRM gives us a mode, we should add 999 Hz to it. That way - * if the clock we need is 60000001 Hz (~60 MHz) and DRM tells us to - * make 60000 kHz then the clock framework will actually give us - * the right clock. - * - * NOTE: if the PLL (maybe through a divider) could actually make - * a clock rate 999 Hz higher instead of the one we want then this - * could be a problem. Unfortunately there's not much we can do - * since it's baked into DRM to use kHz. It shouldn't matter in - * practice since Rockchip PLLs are controlled by tables and - * even if there is a divider in the middle I wouldn't expect PLL - * rates in the table that are just a few kHz different. - * - * 2. Get the clock framework to round the rate for us to tell us - * what it will actually make. - * - * 3. Store the rounded up rate so that we don't need to worry about - * this in the actual clk_set_rate(). - */ - rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999); - adjusted_mode->clock = DIV_ROUND_UP(rate, 1000); - - return true; -} - static bool vop_dsp_lut_is_enabled(struct vop *vop) { return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en); @@ -1512,7 +1468,6 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, }
static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = { - .mode_fixup = vop_crtc_mode_fixup, .atomic_check = vop_crtc_atomic_check, .atomic_begin = vop_crtc_atomic_begin, .atomic_flush = vop_crtc_atomic_flush,
Add our HDMI driver owner Algea to list.
On 9/22/20 2:18 AM, Vicente Bergas wrote:
Under certain conditions vop_crtc_mode_fixup rounds the clock 148500000 to 148501000 which leads to the following error: dwhdmi-rockchip ff940000.hdmi: PHY configuration failed (clock 148501000)
The issue was found on RK3399 booting with u-boot. U-boot configures the display at 2560x1440 and then linux comes up with a black screen. A workaround was to un-plug and re-plug the HDMI display.
Signed-off-by: Vicente Bergas vicencb@gmail.com Tested-by: Vicente Bergas vicencb@gmail.com
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 45 --------------------- 1 file changed, 45 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index c80f7d9fd13f..fe80da652994 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1142,50 +1142,6 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc) spin_unlock_irqrestore(&vop->irq_lock, flags); }
-static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
const struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode)-{
- struct vop *vop = to_vop(crtc);
- unsigned long rate;
- /*
* Clock craziness.** Key points:** - DRM works in in kHz.* - Clock framework works in Hz.* - Rockchip's clock driver picks the clock rate that is the* same _OR LOWER_ than the one requested.** Action plan:** 1. When DRM gives us a mode, we should add 999 Hz to it. That way* if the clock we need is 60000001 Hz (~60 MHz) and DRM tells us to* make 60000 kHz then the clock framework will actually give us* the right clock.** NOTE: if the PLL (maybe through a divider) could actually make* a clock rate 999 Hz higher instead of the one we want then this* could be a problem. Unfortunately there's not much we can do* since it's baked into DRM to use kHz. It shouldn't matter in* practice since Rockchip PLLs are controlled by tables and* even if there is a divider in the middle I wouldn't expect PLL* rates in the table that are just a few kHz different.** 2. Get the clock framework to round the rate for us to tell us* what it will actually make.** 3. Store the rounded up rate so that we don't need to worry about* this in the actual clk_set_rate().*/- rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
- adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
- return true;
-}
- static bool vop_dsp_lut_is_enabled(struct vop *vop) { return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en);
@@ -1512,7 +1468,6 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, }
static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
- .mode_fixup = vop_crtc_mode_fixup, .atomic_check = vop_crtc_atomic_check, .atomic_begin = vop_crtc_atomic_begin, .atomic_flush = vop_crtc_atomic_flush,
Hello Vicente,
在 2020/9/22 15:40, Andy Yan 写道:
Add our HDMI driver owner Algea to list.
On 9/22/20 2:18 AM, Vicente Bergas wrote:
Under certain conditions vop_crtc_mode_fixup rounds the clock
May I ask under what conditions that the clock of HDMI will
be changed to 148501000? In general, the description of clock
in EDID will not be detailed below the thousands place.
148500000 to 148501000 which leads to the following error: dwhdmi-rockchip ff940000.hdmi: PHY configuration failed (clock 148501000)
The issue was found on RK3399 booting with u-boot. U-boot configures the display at 2560x1440 and then linux comes up with a black screen. A workaround was to un-plug and re-plug the HDMI display.
Signed-off-by: Vicente Bergas vicencb@gmail.com Tested-by: Vicente Bergas vicencb@gmail.com
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 45 --------------------- 1 file changed, 45 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index c80f7d9fd13f..fe80da652994 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1142,50 +1142,6 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc) spin_unlock_irqrestore(&vop->irq_lock, flags); } -static bool vop_crtc_mode_fixup(struct drm_crtc *crtc, - const struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode) -{ - struct vop *vop = to_vop(crtc); - unsigned long rate;
- /* - * Clock craziness. - * - * Key points: - * - * - DRM works in in kHz. - * - Clock framework works in Hz. - * - Rockchip's clock driver picks the clock rate that is the - * same _OR LOWER_ than the one requested. - * - * Action plan: - * - * 1. When DRM gives us a mode, we should add 999 Hz to it. That way - * if the clock we need is 60000001 Hz (~60 MHz) and DRM tells us to - * make 60000 kHz then the clock framework will actually give us - * the right clock. - * - * NOTE: if the PLL (maybe through a divider) could actually make - * a clock rate 999 Hz higher instead of the one we want then this - * could be a problem. Unfortunately there's not much we can do - * since it's baked into DRM to use kHz. It shouldn't matter in - * practice since Rockchip PLLs are controlled by tables and - * even if there is a divider in the middle I wouldn't expect PLL - * rates in the table that are just a few kHz different. - * - * 2. Get the clock framework to round the rate for us to tell us - * what it will actually make. - * - * 3. Store the rounded up rate so that we don't need to worry about - * this in the actual clk_set_rate(). - */ - rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999); - adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
- return true; -}
static bool vop_dsp_lut_is_enabled(struct vop *vop) { return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en); @@ -1512,7 +1468,6 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, } static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = { - .mode_fixup = vop_crtc_mode_fixup, .atomic_check = vop_crtc_atomic_check, .atomic_begin = vop_crtc_atomic_begin, .atomic_flush = vop_crtc_atomic_flush,
On Tue, Sep 22, 2020 at 11:24 AM crj algea.cao@rock-chips.com wrote:
Hello Vicente,
在 2020/9/22 15:40, Andy Yan 写道:
Add our HDMI driver owner Algea to list.
On 9/22/20 2:18 AM, Vicente Bergas wrote:
Under certain conditions vop_crtc_mode_fixup rounds the clock
May I ask under what conditions that the clock of HDMI will
be changed to 148501000? In general, the description of clock
in EDID will not be detailed below the thousands place.
There is no clock in the EDID with 1KHz resolution, the clock is 148500000 which has 500KHz resolution. It is the function vop_crtc_mode_fixup that gets xxx0000 and returns xxx1000
148500000 to 148501000 which leads to the following error: dwhdmi-rockchip ff940000.hdmi: PHY configuration failed (clock 148501000)
The issue was found on RK3399 booting with u-boot. U-boot configures the display at 2560x1440 and then linux comes up with a black screen. A workaround was to un-plug and re-plug the HDMI display.
Signed-off-by: Vicente Bergas vicencb@gmail.com Tested-by: Vicente Bergas vicencb@gmail.com
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 45 --------------------- 1 file changed, 45 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index c80f7d9fd13f..fe80da652994 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1142,50 +1142,6 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc) spin_unlock_irqrestore(&vop->irq_lock, flags); } -static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
const struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode)-{
- struct vop *vop = to_vop(crtc);
- unsigned long rate;
- /*
* Clock craziness.** Key points:** - DRM works in in kHz.* - Clock framework works in Hz.* - Rockchip's clock driver picks the clock rate that is the* same _OR LOWER_ than the one requested.** Action plan:** 1. When DRM gives us a mode, we should add 999 Hz to it.That way
* if the clock we need is 60000001 Hz (~60 MHz) and DRMtells us to
* make 60000 kHz then the clock framework will actually give us* the right clock.** NOTE: if the PLL (maybe through a divider) could actuallymake
* a clock rate 999 Hz higher instead of the one we want thenthis
* could be a problem. Unfortunately there's not much we can do* since it's baked into DRM to use kHz. It shouldn't matter in* practice since Rockchip PLLs are controlled by tables and* even if there is a divider in the middle I wouldn't expectPLL
* rates in the table that are just a few kHz different.** 2. Get the clock framework to round the rate for us to tell us* what it will actually make.** 3. Store the rounded up rate so that we don't need to worryabout
* this in the actual clk_set_rate().*/- rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 +
999);
- adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
- return true;
-}
- static bool vop_dsp_lut_is_enabled(struct vop *vop) { return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en);
@@ -1512,7 +1468,6 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, } static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
- .mode_fixup = vop_crtc_mode_fixup, .atomic_check = vop_crtc_atomic_check, .atomic_begin = vop_crtc_atomic_begin, .atomic_flush = vop_crtc_atomic_flush,
Hi, Douglas
在 2020/9/22 17:31, Vicente Bergas 写道:
On Tue, Sep 22, 2020 at 11:24 AM crj algea.cao@rock-chips.com wrote:
Hello Vicente,
在 2020/9/22 15:40, Andy Yan 写道:
Add our HDMI driver owner Algea to list.
On 9/22/20 2:18 AM, Vicente Bergas wrote:
Under certain conditions vop_crtc_mode_fixup rounds the clock
May I ask under what conditions that the clock of HDMI will
be changed to 148501000? In general, the description of clock
in EDID will not be detailed below the thousands place.
There is no clock in the EDID with 1KHz resolution, the clock is 148500000 which has 500KHz resolution. It is the function vop_crtc_mode_fixup that gets xxx0000 and returns xxx1000
I checked the commit msg of commit 287422a95fe2 ("drm/rockchip: Round up _before_ giving to the clock framework").
Round up hdmi clock is for some panels with special clocks. Are these panels clock can't be divided correctly common?
148500000 to 148501000 which leads to the following error: dwhdmi-rockchip ff940000.hdmi: PHY configuration failed (clock 148501000)
The issue was found on RK3399 booting with u-boot. U-boot configures the display at 2560x1440 and then linux comes up with a black screen. A workaround was to un-plug and re-plug the HDMI display.
Signed-off-by: Vicente Bergas vicencb@gmail.com Tested-by: Vicente Bergas vicencb@gmail.com
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 45 --------------------- 1 file changed, 45 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index c80f7d9fd13f..fe80da652994 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1142,50 +1142,6 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc) spin_unlock_irqrestore(&vop->irq_lock, flags); } -static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
const struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode)-{
- struct vop *vop = to_vop(crtc);
- unsigned long rate;
- /*
* Clock craziness.** Key points:** - DRM works in in kHz.* - Clock framework works in Hz.* - Rockchip's clock driver picks the clock rate that is the* same _OR LOWER_ than the one requested.** Action plan:** 1. When DRM gives us a mode, we should add 999 Hz to it.That way
* if the clock we need is 60000001 Hz (~60 MHz) and DRMtells us to
* make 60000 kHz then the clock framework will actually give us* the right clock.** NOTE: if the PLL (maybe through a divider) could actuallymake
* a clock rate 999 Hz higher instead of the one we want thenthis
* could be a problem. Unfortunately there's not much we can do* since it's baked into DRM to use kHz. It shouldn't matter in* practice since Rockchip PLLs are controlled by tables and* even if there is a divider in the middle I wouldn't expectPLL
* rates in the table that are just a few kHz different.** 2. Get the clock framework to round the rate for us to tell us* what it will actually make.** 3. Store the rounded up rate so that we don't need to worryabout
* this in the actual clk_set_rate().*/- rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 +
999);
- adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
- return true;
-}
- static bool vop_dsp_lut_is_enabled(struct vop *vop) { return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en);
@@ -1512,7 +1468,6 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, } static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
- .mode_fixup = vop_crtc_mode_fixup, .atomic_check = vop_crtc_atomic_check, .atomic_begin = vop_crtc_atomic_begin, .atomic_flush = vop_crtc_atomic_flush,
Hi,
On Tue, Sep 22, 2020 at 3:13 AM crj algea.cao@rock-chips.com wrote:
Hi, Douglas
在 2020/9/22 17:31, Vicente Bergas 写道:
On Tue, Sep 22, 2020 at 11:24 AM crj algea.cao@rock-chips.com wrote:
Hello Vicente,
在 2020/9/22 15:40, Andy Yan 写道:
Add our HDMI driver owner Algea to list.
On 9/22/20 2:18 AM, Vicente Bergas wrote:
Under certain conditions vop_crtc_mode_fixup rounds the clock
May I ask under what conditions that the clock of HDMI will
be changed to 148501000? In general, the description of clock
in EDID will not be detailed below the thousands place.
There is no clock in the EDID with 1KHz resolution, the clock is 148500000 which has 500KHz resolution. It is the function vop_crtc_mode_fixup that gets xxx0000 and returns xxx1000
I checked the commit msg of commit 287422a95fe2 ("drm/rockchip: Round up _before_ giving to the clock framework").
Round up hdmi clock is for some panels with special clocks. Are these panels clock can't be divided correctly common?
I'm sorry, but I don't understand the question. Can you restate? I think the commit message that you refer to is pretty thorough. Specifically the problem is all around the fact that, internally, DRM often refers to clocks in kHz. We end up with issues when converting back and forth between numbers in kHz and in MHz. Since DRM always rounds down when going to kHz we end up with problems.
I'm curious how you're ending up with an error, though. How could adding 999 to 148500000 and then rounding down cause you to get 148501000?
148500000 to 148501000 which leads to the following error: dwhdmi-rockchip ff940000.hdmi: PHY configuration failed (clock 148501000)
The issue was found on RK3399 booting with u-boot. U-boot configures the display at 2560x1440 and then linux comes up with a black screen. A workaround was to un-plug and re-plug the HDMI display.
Signed-off-by: Vicente Bergas vicencb@gmail.com Tested-by: Vicente Bergas vicencb@gmail.com
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 45 --------------------- 1 file changed, 45 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index c80f7d9fd13f..fe80da652994 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1142,50 +1142,6 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc) spin_unlock_irqrestore(&vop->irq_lock, flags); } -static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
const struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode)-{
- struct vop *vop = to_vop(crtc);
- unsigned long rate;
- /*
* Clock craziness.** Key points:** - DRM works in in kHz.* - Clock framework works in Hz.* - Rockchip's clock driver picks the clock rate that is the* same _OR LOWER_ than the one requested.** Action plan:** 1. When DRM gives us a mode, we should add 999 Hz to it.That way
* if the clock we need is 60000001 Hz (~60 MHz) and DRMtells us to
* make 60000 kHz then the clock framework will actually give us* the right clock.** NOTE: if the PLL (maybe through a divider) could actuallymake
* a clock rate 999 Hz higher instead of the one we want thenthis
* could be a problem. Unfortunately there's not much we can do* since it's baked into DRM to use kHz. It shouldn't matter in* practice since Rockchip PLLs are controlled by tables and* even if there is a divider in the middle I wouldn't expectPLL
* rates in the table that are just a few kHz different.** 2. Get the clock framework to round the rate for us to tell us* what it will actually make.** 3. Store the rounded up rate so that we don't need to worryabout
* this in the actual clk_set_rate().*/- rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 +
999);
- adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
- return true;
-}
- static bool vop_dsp_lut_is_enabled(struct vop *vop) { return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en);
@@ -1512,7 +1468,6 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, } static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
- .mode_fixup = vop_crtc_mode_fixup, .atomic_check = vop_crtc_atomic_check, .atomic_begin = vop_crtc_atomic_begin, .atomic_flush = vop_crtc_atomic_flush,
On Tue, Sep 22, 2020 at 4:28 PM Doug Anderson dianders@chromium.org wrote:
Hi,
On Tue, Sep 22, 2020 at 3:13 AM crj algea.cao@rock-chips.com wrote:
Hi, Douglas
在 2020/9/22 17:31, Vicente Bergas 写道:
On Tue, Sep 22, 2020 at 11:24 AM crj algea.cao@rock-chips.com wrote:
Hello Vicente,
在 2020/9/22 15:40, Andy Yan 写道:
Add our HDMI driver owner Algea to list.
On 9/22/20 2:18 AM, Vicente Bergas wrote:
Under certain conditions vop_crtc_mode_fixup rounds the clock
May I ask under what conditions that the clock of HDMI will
be changed to 148501000? In general, the description of clock
in EDID will not be detailed below the thousands place.
There is no clock in the EDID with 1KHz resolution, the clock is 148500000 which has 500KHz resolution. It is the function vop_crtc_mode_fixup that gets xxx0000 and returns xxx1000
I checked the commit msg of commit 287422a95fe2 ("drm/rockchip: Round up _before_ giving to the clock framework").
Round up hdmi clock is for some panels with special clocks. Are these panels clock can't be divided correctly common?
I'm sorry, but I don't understand the question. Can you restate? I think the commit message that you refer to is pretty thorough. Specifically the problem is all around the fact that, internally, DRM often refers to clocks in kHz. We end up with issues when converting back and forth between numbers in kHz and in MHz. Since DRM always rounds down when going to kHz we end up with problems.
I'm curious how you're ending up with an error, though. How could adding 999 to 148500000 and then rounding down cause you to get 148501000?
The name of the macro is DIV_ROUND_UP, or is clk_round_rate who should round down?
148500000 to 148501000 which leads to the following error: dwhdmi-rockchip ff940000.hdmi: PHY configuration failed (clock 148501000)
The issue was found on RK3399 booting with u-boot. U-boot configures the display at 2560x1440 and then linux comes up with a black screen. A workaround was to un-plug and re-plug the HDMI display.
Signed-off-by: Vicente Bergas vicencb@gmail.com Tested-by: Vicente Bergas vicencb@gmail.com
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 45 --------------------- 1 file changed, 45 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index c80f7d9fd13f..fe80da652994 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1142,50 +1142,6 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc) spin_unlock_irqrestore(&vop->irq_lock, flags); } -static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
const struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode)-{
- struct vop *vop = to_vop(crtc);
- unsigned long rate;
- /*
* Clock craziness.** Key points:** - DRM works in in kHz.* - Clock framework works in Hz.* - Rockchip's clock driver picks the clock rate that is the* same _OR LOWER_ than the one requested.** Action plan:** 1. When DRM gives us a mode, we should add 999 Hz to it.That way
* if the clock we need is 60000001 Hz (~60 MHz) and DRMtells us to
* make 60000 kHz then the clock framework will actually give us* the right clock.** NOTE: if the PLL (maybe through a divider) could actuallymake
* a clock rate 999 Hz higher instead of the one we want thenthis
* could be a problem. Unfortunately there's not much we can do* since it's baked into DRM to use kHz. It shouldn't matter in* practice since Rockchip PLLs are controlled by tables and* even if there is a divider in the middle I wouldn't expectPLL
* rates in the table that are just a few kHz different.** 2. Get the clock framework to round the rate for us to tell us* what it will actually make.** 3. Store the rounded up rate so that we don't need to worryabout
* this in the actual clk_set_rate().*/- rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 +
999);
- adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
- return true;
-}
- static bool vop_dsp_lut_is_enabled(struct vop *vop) { return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en);
@@ -1512,7 +1468,6 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc, } static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
- .mode_fixup = vop_crtc_mode_fixup, .atomic_check = vop_crtc_atomic_check, .atomic_begin = vop_crtc_atomic_begin, .atomic_flush = vop_crtc_atomic_flush,
Hi,
On Tue, Sep 22, 2020 at 7:52 AM Vicente Bergas vicencb@gmail.com wrote:
On Tue, Sep 22, 2020 at 4:28 PM Doug Anderson dianders@chromium.org wrote:
Hi,
On Tue, Sep 22, 2020 at 3:13 AM crj algea.cao@rock-chips.com wrote:
Hi, Douglas
在 2020/9/22 17:31, Vicente Bergas 写道:
On Tue, Sep 22, 2020 at 11:24 AM crj algea.cao@rock-chips.com wrote:
Hello Vicente,
在 2020/9/22 15:40, Andy Yan 写道:
Add our HDMI driver owner Algea to list.
On 9/22/20 2:18 AM, Vicente Bergas wrote: > Under certain conditions vop_crtc_mode_fixup rounds the clock
May I ask under what conditions that the clock of HDMI will
be changed to 148501000? In general, the description of clock
in EDID will not be detailed below the thousands place.
There is no clock in the EDID with 1KHz resolution, the clock is 148500000 which has 500KHz resolution. It is the function vop_crtc_mode_fixup that gets xxx0000 and returns xxx1000
I checked the commit msg of commit 287422a95fe2 ("drm/rockchip: Round up _before_ giving to the clock framework").
Round up hdmi clock is for some panels with special clocks. Are these panels clock can't be divided correctly common?
I'm sorry, but I don't understand the question. Can you restate? I think the commit message that you refer to is pretty thorough. Specifically the problem is all around the fact that, internally, DRM often refers to clocks in kHz. We end up with issues when converting back and forth between numbers in kHz and in MHz. Since DRM always rounds down when going to kHz we end up with problems.
I'm curious how you're ending up with an error, though. How could adding 999 to 148500000 and then rounding down cause you to get 148501000?
The name of the macro is DIV_ROUND_UP, or is clk_round_rate who should round down?
Here's the code:
rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999); adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
Input clock is in kHz and DRM always rounds down (last I checked--I guess you could confirm if this is still true).
Imagine that you want an input clock of 999999 kHz and the PLL can actually make this.
DRM will request a clock of 999 kHz because it always rounds down.
First: rate = 999 * 1000 + 999 = 999999 Hz
Now we'll ask the clock framework if it can make this. It can, so clk_round_rate() will return 999999 kHz. Note that, at least on all Rockchip platforms I looked at in the past, clk_round_rate() and clk_set_rate() always round down. Thus, if we _hadn't_ added the 999 here we would not have gotten back 999999 Hz.
We have to return a rate in terms of kHz. While we could round down like DRM does, it seemed better at the time to do the rounding here. Thus, I now rounded up. We should end up storing
(999999 + 999) / 1000 = 1000 kHz
Then, when we use it in vop_crtc_atomic_enable() we don't have to do any more rounding.
I guess it's possible that the problem is that the function is starting with an input where it knows that "adjusted_mode->clock" was rounded down and it ends with it rounded up. That shouldn't cause problems unless somehow the function is being called twice or someone else is making assumptions about the rounding. You could, potentially, change this to:
adjusted_mode->clock = rate / 1000;
...and then in vop_crtc_atomic_enable() you add the "999" back in, like:
clk_set_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
That would make it more consistent / stable. Does it work for you?
> 148500000 to 148501000 which leads to the following error: > dwhdmi-rockchip ff940000.hdmi: PHY configuration failed (clock > 148501000) > > The issue was found on RK3399 booting with u-boot. U-boot configures the > display at 2560x1440 and then linux comes up with a black screen. > A workaround was to un-plug and re-plug the HDMI display. > > Signed-off-by: Vicente Bergas vicencb@gmail.com > Tested-by: Vicente Bergas vicencb@gmail.com > --- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 45 --------------------- > 1 file changed, 45 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index c80f7d9fd13f..fe80da652994 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -1142,50 +1142,6 @@ static void vop_crtc_disable_vblank(struct > drm_crtc *crtc) > spin_unlock_irqrestore(&vop->irq_lock, flags); > } > -static bool vop_crtc_mode_fixup(struct drm_crtc *crtc, > - const struct drm_display_mode *mode, > - struct drm_display_mode *adjusted_mode) > -{ > - struct vop *vop = to_vop(crtc); > - unsigned long rate; > - > - /* > - * Clock craziness. > - * > - * Key points: > - * > - * - DRM works in in kHz. > - * - Clock framework works in Hz. > - * - Rockchip's clock driver picks the clock rate that is the > - * same _OR LOWER_ than the one requested. > - * > - * Action plan: > - * > - * 1. When DRM gives us a mode, we should add 999 Hz to it. > That way > - * if the clock we need is 60000001 Hz (~60 MHz) and DRM > tells us to > - * make 60000 kHz then the clock framework will actually give us > - * the right clock. > - * > - * NOTE: if the PLL (maybe through a divider) could actually > make > - * a clock rate 999 Hz higher instead of the one we want then > this > - * could be a problem. Unfortunately there's not much we can do > - * since it's baked into DRM to use kHz. It shouldn't matter in > - * practice since Rockchip PLLs are controlled by tables and > - * even if there is a divider in the middle I wouldn't expect > PLL > - * rates in the table that are just a few kHz different. > - * > - * 2. Get the clock framework to round the rate for us to tell us > - * what it will actually make. > - * > - * 3. Store the rounded up rate so that we don't need to worry > about > - * this in the actual clk_set_rate(). > - */ > - rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + > 999); > - adjusted_mode->clock = DIV_ROUND_UP(rate, 1000); > - > - return true; > -} > - > static bool vop_dsp_lut_is_enabled(struct vop *vop) > { > return vop_read_reg(vop, 0, &vop->data->common->dsp_lut_en); > @@ -1512,7 +1468,6 @@ static void vop_crtc_atomic_flush(struct > drm_crtc *crtc, > } > static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = { > - .mode_fixup = vop_crtc_mode_fixup, > .atomic_check = vop_crtc_atomic_check, > .atomic_begin = vop_crtc_atomic_begin, > .atomic_flush = vop_crtc_atomic_flush,
On Tuesday, September 22, 2020 5:26:17 PM CEST, Doug Anderson wrote:
Hi,
On Tue, Sep 22, 2020 at 7:52 AM Vicente Bergas vicencb@gmail.com wrote:
On Tue, Sep 22, 2020 at 4:28 PM Doug Anderson dianders@chromium.org wrote: ...
Here's the code:
rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999); adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
Input clock is in kHz and DRM always rounds down (last I checked--I guess you could confirm if this is still true).
Imagine that you want an input clock of 999999 kHz and the PLL can actually make this.
DRM will request a clock of 999 kHz because it always rounds down.
First: rate = 999 * 1000 + 999 = 999999 Hz
Now we'll ask the clock framework if it can make this. It can, so clk_round_rate() will return 999999 kHz. Note that, at least on all Rockchip platforms I looked at in the past, clk_round_rate() and clk_set_rate() always round down. Thus, if we _hadn't_ added the 999 here we would not have gotten back 999999 Hz.
We have to return a rate in terms of kHz. While we could round down like DRM does, it seemed better at the time to do the rounding here. Thus, I now rounded up. We should end up storing
(999999 + 999) / 1000 = 1000 kHz
Then, when we use it in vop_crtc_atomic_enable() we don't have to do any more rounding.
I guess it's possible that the problem is that the function is starting with an input where it knows that "adjusted_mode->clock" was rounded down and it ends with it rounded up. That shouldn't cause problems unless somehow the function is being called twice or someone else is making assumptions about the rounding. You could, potentially, change this to:
adjusted_mode->clock = rate / 1000;
...and then in vop_crtc_atomic_enable() you add the "999" back in, like:
clk_set_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
That would make it more consistent / stable. Does it work for you?
Hi Douglas,
i've tested this as suggested: --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1181,7 +1181,7 @@ * this in the actual clk_set_rate(). */ rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999); - adjusted_mode->clock = DIV_ROUND_UP(rate, 1000); + adjusted_mode->clock = rate / 1000;
return true; } @@ -1380,7 +1380,7 @@
VOP_REG_SET(vop, intr, line_flag_num[0], vact_end);
- clk_set_rate(vop->dclk, adjusted_mode->clock * 1000); + clk_set_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
VOP_REG_SET(vop, common, standby, 0); mutex_unlock(&vop->vop_lock); and it also works fine. Should i sent a V2 of this patch series including your approach?
For completeness i've added some printks to the original code to show the clock values: 1.- Provided adjusted_mode->clock adjusted_mode->clock (before) = 148500KHz rate = 148500998Hz adjusted_mode->clock (after) = 148501KHz <= this is the problematic clock
2.- Overwrite adjusted_mode->clock with the comment's value of 60000.001KHz adjusted_mode->clock (before) = 60000KHz rate = 60000998Hz adjusted_mode->clock (after) = 60001KHz
3.- Overwrite adjusted_mode->clock with your mentioned value of 999.999KHz adjusted_mode->clock (before) = 999KHz rate = 999999Hz adjusted_mode->clock (after) = 1000KHz
Regards, Vicente.
Hi,
On Tue, Sep 22, 2020 at 12:10 PM Vicente Bergas vicencb@gmail.com wrote:
On Tuesday, September 22, 2020 5:26:17 PM CEST, Doug Anderson wrote:
Hi,
On Tue, Sep 22, 2020 at 7:52 AM Vicente Bergas vicencb@gmail.com wrote:
On Tue, Sep 22, 2020 at 4:28 PM Doug Anderson dianders@chromium.org wrote: ...
Here's the code:
rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999); adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
Input clock is in kHz and DRM always rounds down (last I checked--I guess you could confirm if this is still true).
Imagine that you want an input clock of 999999 kHz and the PLL can actually make this.
DRM will request a clock of 999 kHz because it always rounds down.
First: rate = 999 * 1000 + 999 = 999999 Hz
Now we'll ask the clock framework if it can make this. It can, so clk_round_rate() will return 999999 kHz. Note that, at least on all Rockchip platforms I looked at in the past, clk_round_rate() and clk_set_rate() always round down. Thus, if we _hadn't_ added the 999 here we would not have gotten back 999999 Hz.
We have to return a rate in terms of kHz. While we could round down like DRM does, it seemed better at the time to do the rounding here. Thus, I now rounded up. We should end up storing
(999999 + 999) / 1000 = 1000 kHz
Then, when we use it in vop_crtc_atomic_enable() we don't have to do any more rounding.
I guess it's possible that the problem is that the function is starting with an input where it knows that "adjusted_mode->clock" was rounded down and it ends with it rounded up. That shouldn't cause problems unless somehow the function is being called twice or someone else is making assumptions about the rounding. You could, potentially, change this to:
adjusted_mode->clock = rate / 1000;
...and then in vop_crtc_atomic_enable() you add the "999" back in, like:
clk_set_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
That would make it more consistent / stable. Does it work for you?
Hi Douglas,
i've tested this as suggested: --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1181,7 +1181,7 @@ * this in the actual clk_set_rate(). */ rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
adjusted_mode->clock = rate / 1000;
You'll also want to change the comment above. Specifically it says that we're storing the rounded up state.
return true;} @@ -1380,7 +1380,7 @@
VOP_REG_SET(vop, intr, line_flag_num[0], vact_end);
clk_set_rate(vop->dclk, adjusted_mode->clock * 1000);
clk_set_rate(vop->dclk, adjusted_mode->clock * 1000 + 999); VOP_REG_SET(vop, common, standby, 0); mutex_unlock(&vop->vop_lock);and it also works fine. Should i sent a V2 of this patch series including your approach?
That would be good w/ me.
-Doug
For a video mode to work it suffices that the available bandwidth is large enough. There is no need to have an exact match.
This greatly expands the list of supported monitors.
Signed-off-by: Vicente Bergas vicencb@gmail.com Tested-by: Vicente Bergas vicencb@gmail.com --- drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c index 23de359a1dec..87a9198f7494 100644 --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c @@ -230,7 +230,7 @@ dw_hdmi_rockchip_mode_valid(struct dw_hdmi *hdmi, void *data, int i;
for (i = 0; mpll_cfg[i].mpixelclock != (~0UL); i++) { - if (pclk == mpll_cfg[i].mpixelclock) { + if (pclk <= mpll_cfg[i].mpixelclock) { valid = true; break; }
In order to support video resolutions beyond FHD more bandwidth is needed. The new entry values have been taken from u-boot: https://gitlab.denx.de/u-boot/u-boot/-/blob/ba2a0cbb053951ed6d36161989d38da7...
Signed-off-by: Vicente Bergas vicencb@gmail.com Tested-by: Vicente Bergas vicencb@gmail.com --- drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c index 87a9198f7494..db4a946f92aa 100644 --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c @@ -148,6 +148,10 @@ static const struct dw_hdmi_mpll_config rockchip_mpll_cfg[] = { { 0x214c, 0x0003}, { 0x4064, 0x0003} }, + }, { + 272000000, { + { 0x0040, 0x0003}, + }, }, { ~0UL, { { 0x00a0, 0x000a }, @@ -173,6 +177,8 @@ static const struct dw_hdmi_curr_ctrl rockchip_cur_ctr[] = { 146250000, { 0x0038, 0x0038, 0x0038 }, }, { 148500000, { 0x0000, 0x0038, 0x0038 }, + }, { + 272000000, { 0x0000, }, }, { ~0UL, { 0x0000, 0x0000, 0x0000}, }
dri-devel@lists.freedesktop.org
-
Andy Yan -
crj -
Doug Anderson -
Vicente Bergas