| Message ID | 20200921181803.1160-2-vicencb@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: rockchip: hdmi: enable higher resolutions than FHD | expand |
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, > >
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 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
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,