On Tue, Sep 22, 2020 at 4:28 PM Doug Anderson <dianders at chromium.org> wrote:
>
> Hi,
>
> On Tue, Sep 22, 2020 at 3:13 AM crj <algea.cao at 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 at 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 at gmail.com>
> > >>>> Tested-by: Vicente Bergas <vicencb at 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,
> > >
> >
> >