| Message ID | b04341887fb1acb9ed4adc28d109f9e21f146c7d.1594230107.git-series.maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Support BCM2711 Display Pipeline | expand |
Hi Maxime, On 7/9/20 2:41 AM, Maxime Ripard wrote: > In order to prevent timeouts and stalls in the pipeline, the core clock > needs to be maxed at 500MHz during a modeset on the BCM2711. > > Reviewed-by: Eric Anholt <eric@anholt.net> > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ > drivers/gpu/drm/vc4/vc4_hvs.c | 9 +++++++++ > drivers/gpu/drm/vc4/vc4_kms.c | 9 +++++++++ > 3 files changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > index e4cde1f9224b..6358f6ca8d56 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.h > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > @@ -320,6 +320,8 @@ struct vc4_hvs { > void __iomem *regs; > u32 __iomem *dlist; > > + struct clk *core_clk; > + > /* Memory manager for CRTCs to allocate space in the display > * list. Units are dwords. > */ > diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c > index 836d8799d79e..091fdf4908aa 100644 > --- a/drivers/gpu/drm/vc4/vc4_hvs.c > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c > @@ -19,6 +19,7 @@ > * each CRTC. > */ > > +#include <linux/clk.h> > #include <linux/component.h> > #include <linux/platform_device.h> > > @@ -540,6 +541,14 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) > hvs->regset.regs = hvs_regs; > hvs->regset.nregs = ARRAY_SIZE(hvs_regs); > > + if (hvs->hvs5) { > + hvs->core_clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(hvs->core_clk)) { > + dev_err(&pdev->dev, "Couldn't get core clock\n"); > + return PTR_ERR(hvs->core_clk); > + } > + } > + > if (!hvs->hvs5) > hvs->dlist = hvs->regs + SCALER_DLIST_START; > else > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 08318e69061b..210cc2408087 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -11,6 +11,8 @@ > * crtc, HDMI encoder). > */ > > +#include <linux/clk.h> > + > #include <drm/drm_atomic.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_crtc.h> > @@ -149,6 +151,7 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) > { > struct drm_device *dev = state->dev; > struct vc4_dev *vc4 = to_vc4_dev(dev); > + struct vc4_hvs *hvs = vc4->hvs; > struct vc4_crtc *vc4_crtc; > int i; > > @@ -160,6 +163,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) > vc4_hvs_mask_underrun(dev, vc4_crtc->channel); > } > > + if (vc4->hvs->hvs5) > + clk_set_min_rate(hvs->core_clk, 500000000); > + > drm_atomic_helper_wait_for_fences(dev, state, false); > > drm_atomic_helper_wait_for_dependencies(state); > @@ -182,6 +188,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) > > drm_atomic_helper_commit_cleanup_done(state); > > + if (vc4->hvs->hvs5) > + clk_set_min_rate(hvs->core_clk, 0); > + > drm_atomic_state_put(state); > > up(&vc4->async_modeset); > This patch doesn't control the enable/disable of core_clk. So, I think that it need to handle the clock as following: diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index 4ef88c0b51ab..355d67fd8beb 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -588,6 +588,12 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) dev_err(&pdev->dev, "Couldn't get core clock\n"); return PTR_ERR(hvs->core_clk); } + + ret = clk_prepare_enable(hvs->core_clk); + if (ret) { + dev_err(&pdev->dev, "Couldn't enable core clock\n"); + return ret; + } } if (!hvs->hvs5) @@ -681,6 +687,8 @@ static void vc4_hvs_unbind(struct device *dev, struct device *master, drm_mm_takedown(&vc4->hvs->dlist_mm); drm_mm_takedown(&vc4->hvs->lbm_mm); + clk_prepare_enable(vc4->hvs->core_clk); + vc4->hvs = NULL; }
On 9/1/20 8:21 PM, Chanwoo Choi wrote: > Hi Maxime, > > On 7/9/20 2:41 AM, Maxime Ripard wrote: >> In order to prevent timeouts and stalls in the pipeline, the core clock >> needs to be maxed at 500MHz during a modeset on the BCM2711. >> >> Reviewed-by: Eric Anholt <eric@anholt.net> >> Signed-off-by: Maxime Ripard <maxime@cerno.tech> >> --- >> drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ >> drivers/gpu/drm/vc4/vc4_hvs.c | 9 +++++++++ >> drivers/gpu/drm/vc4/vc4_kms.c | 9 +++++++++ >> 3 files changed, 20 insertions(+) >> >> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h >> index e4cde1f9224b..6358f6ca8d56 100644 >> --- a/drivers/gpu/drm/vc4/vc4_drv.h >> +++ b/drivers/gpu/drm/vc4/vc4_drv.h >> @@ -320,6 +320,8 @@ struct vc4_hvs { >> void __iomem *regs; >> u32 __iomem *dlist; >> >> + struct clk *core_clk; >> + >> /* Memory manager for CRTCs to allocate space in the display >> * list. Units are dwords. >> */ >> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c >> index 836d8799d79e..091fdf4908aa 100644 >> --- a/drivers/gpu/drm/vc4/vc4_hvs.c >> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c >> @@ -19,6 +19,7 @@ >> * each CRTC. >> */ >> >> +#include <linux/clk.h> >> #include <linux/component.h> >> #include <linux/platform_device.h> >> >> @@ -540,6 +541,14 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) >> hvs->regset.regs = hvs_regs; >> hvs->regset.nregs = ARRAY_SIZE(hvs_regs); >> >> + if (hvs->hvs5) { >> + hvs->core_clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(hvs->core_clk)) { >> + dev_err(&pdev->dev, "Couldn't get core clock\n"); >> + return PTR_ERR(hvs->core_clk); >> + } >> + } >> + >> if (!hvs->hvs5) >> hvs->dlist = hvs->regs + SCALER_DLIST_START; >> else >> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c >> index 08318e69061b..210cc2408087 100644 >> --- a/drivers/gpu/drm/vc4/vc4_kms.c >> +++ b/drivers/gpu/drm/vc4/vc4_kms.c >> @@ -11,6 +11,8 @@ >> * crtc, HDMI encoder). >> */ >> >> +#include <linux/clk.h> >> + >> #include <drm/drm_atomic.h> >> #include <drm/drm_atomic_helper.h> >> #include <drm/drm_crtc.h> >> @@ -149,6 +151,7 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) >> { >> struct drm_device *dev = state->dev; >> struct vc4_dev *vc4 = to_vc4_dev(dev); >> + struct vc4_hvs *hvs = vc4->hvs; >> struct vc4_crtc *vc4_crtc; >> int i; >> >> @@ -160,6 +163,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) >> vc4_hvs_mask_underrun(dev, vc4_crtc->channel); >> } >> >> + if (vc4->hvs->hvs5) >> + clk_set_min_rate(hvs->core_clk, 500000000); >> + >> drm_atomic_helper_wait_for_fences(dev, state, false); >> >> drm_atomic_helper_wait_for_dependencies(state); >> @@ -182,6 +188,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) >> >> drm_atomic_helper_commit_cleanup_done(state); >> >> + if (vc4->hvs->hvs5) >> + clk_set_min_rate(hvs->core_clk, 0); >> + >> drm_atomic_state_put(state); >> >> up(&vc4->async_modeset); >> > > This patch doesn't control the enable/disable of core_clk. > So, I think that it need to handle the clock as following: > > diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c > index 4ef88c0b51ab..355d67fd8beb 100644 > --- a/drivers/gpu/drm/vc4/vc4_hvs.c > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c > @@ -588,6 +588,12 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) > dev_err(&pdev->dev, "Couldn't get core clock\n"); > return PTR_ERR(hvs->core_clk); > } > + > + ret = clk_prepare_enable(hvs->core_clk); > + if (ret) { > + dev_err(&pdev->dev, "Couldn't enable core clock\n"); > + return ret; > + } > } > > if (!hvs->hvs5) > @@ -681,6 +687,8 @@ static void vc4_hvs_unbind(struct device *dev, struct device *master, > drm_mm_takedown(&vc4->hvs->dlist_mm); > drm_mm_takedown(&vc4->hvs->lbm_mm); > > + clk_prepare_enable(vc4->hvs->core_clk); I'm sorry. Change to clk_disable_unprepare(vc4->hvs->core_clk); > + > vc4->hvs = NULL; > } > > >
Hi, On Tue, Sep 01, 2020 at 08:21:36PM +0900, Chanwoo Choi wrote: > Hi Maxime, > > On 7/9/20 2:41 AM, Maxime Ripard wrote: > > In order to prevent timeouts and stalls in the pipeline, the core clock > > needs to be maxed at 500MHz during a modeset on the BCM2711. > > > > Reviewed-by: Eric Anholt <eric@anholt.net> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ > > drivers/gpu/drm/vc4/vc4_hvs.c | 9 +++++++++ > > drivers/gpu/drm/vc4/vc4_kms.c | 9 +++++++++ > > 3 files changed, 20 insertions(+) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > > index e4cde1f9224b..6358f6ca8d56 100644 > > --- a/drivers/gpu/drm/vc4/vc4_drv.h > > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > > @@ -320,6 +320,8 @@ struct vc4_hvs { > > void __iomem *regs; > > u32 __iomem *dlist; > > > > + struct clk *core_clk; > > + > > /* Memory manager for CRTCs to allocate space in the display > > * list. Units are dwords. > > */ > > diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c > > index 836d8799d79e..091fdf4908aa 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hvs.c > > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c > > @@ -19,6 +19,7 @@ > > * each CRTC. > > */ > > > > +#include <linux/clk.h> > > #include <linux/component.h> > > #include <linux/platform_device.h> > > > > @@ -540,6 +541,14 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) > > hvs->regset.regs = hvs_regs; > > hvs->regset.nregs = ARRAY_SIZE(hvs_regs); > > > > + if (hvs->hvs5) { > > + hvs->core_clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(hvs->core_clk)) { > > + dev_err(&pdev->dev, "Couldn't get core clock\n"); > > + return PTR_ERR(hvs->core_clk); > > + } > > + } > > + > > if (!hvs->hvs5) > > hvs->dlist = hvs->regs + SCALER_DLIST_START; > > else > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > > index 08318e69061b..210cc2408087 100644 > > --- a/drivers/gpu/drm/vc4/vc4_kms.c > > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > > @@ -11,6 +11,8 @@ > > * crtc, HDMI encoder). > > */ > > > > +#include <linux/clk.h> > > + > > #include <drm/drm_atomic.h> > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_crtc.h> > > @@ -149,6 +151,7 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) > > { > > struct drm_device *dev = state->dev; > > struct vc4_dev *vc4 = to_vc4_dev(dev); > > + struct vc4_hvs *hvs = vc4->hvs; > > struct vc4_crtc *vc4_crtc; > > int i; > > > > @@ -160,6 +163,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) > > vc4_hvs_mask_underrun(dev, vc4_crtc->channel); > > } > > > > + if (vc4->hvs->hvs5) > > + clk_set_min_rate(hvs->core_clk, 500000000); > > + > > drm_atomic_helper_wait_for_fences(dev, state, false); > > > > drm_atomic_helper_wait_for_dependencies(state); > > @@ -182,6 +188,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) > > > > drm_atomic_helper_commit_cleanup_done(state); > > > > + if (vc4->hvs->hvs5) > > + clk_set_min_rate(hvs->core_clk, 0); > > + > > drm_atomic_state_put(state); > > > > up(&vc4->async_modeset); > > > > This patch doesn't control the enable/disable of core_clk. > So, I think that it need to handle the clock as following: > > diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c > index 4ef88c0b51ab..355d67fd8beb 100644 > --- a/drivers/gpu/drm/vc4/vc4_hvs.c > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c > @@ -588,6 +588,12 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) > dev_err(&pdev->dev, "Couldn't get core clock\n"); > return PTR_ERR(hvs->core_clk); > } > + > + ret = clk_prepare_enable(hvs->core_clk); > + if (ret) { > + dev_err(&pdev->dev, "Couldn't enable core clock\n"); > + return ret; > + } > } > > if (!hvs->hvs5) > @@ -681,6 +687,8 @@ static void vc4_hvs_unbind(struct device *dev, struct device *master, > drm_mm_takedown(&vc4->hvs->dlist_mm); > drm_mm_takedown(&vc4->hvs->lbm_mm); > > + clk_prepare_enable(vc4->hvs->core_clk); > + > vc4->hvs = NULL; > } Good catch, thanks! Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index e4cde1f9224b..6358f6ca8d56 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -320,6 +320,8 @@ struct vc4_hvs { void __iomem *regs; u32 __iomem *dlist; + struct clk *core_clk; + /* Memory manager for CRTCs to allocate space in the display * list. Units are dwords. */ diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index 836d8799d79e..091fdf4908aa 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -19,6 +19,7 @@ * each CRTC. */ +#include <linux/clk.h> #include <linux/component.h> #include <linux/platform_device.h> @@ -540,6 +541,14 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) hvs->regset.regs = hvs_regs; hvs->regset.nregs = ARRAY_SIZE(hvs_regs); + if (hvs->hvs5) { + hvs->core_clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(hvs->core_clk)) { + dev_err(&pdev->dev, "Couldn't get core clock\n"); + return PTR_ERR(hvs->core_clk); + } + } + if (!hvs->hvs5) hvs->dlist = hvs->regs + SCALER_DLIST_START; else diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 08318e69061b..210cc2408087 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -11,6 +11,8 @@ * crtc, HDMI encoder). */ +#include <linux/clk.h> + #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc.h> @@ -149,6 +151,7 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); + struct vc4_hvs *hvs = vc4->hvs; struct vc4_crtc *vc4_crtc; int i; @@ -160,6 +163,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) vc4_hvs_mask_underrun(dev, vc4_crtc->channel); } + if (vc4->hvs->hvs5) + clk_set_min_rate(hvs->core_clk, 500000000); + drm_atomic_helper_wait_for_fences(dev, state, false); drm_atomic_helper_wait_for_dependencies(state); @@ -182,6 +188,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) drm_atomic_helper_commit_cleanup_done(state); + if (vc4->hvs->hvs5) + clk_set_min_rate(hvs->core_clk, 0); + drm_atomic_state_put(state); up(&vc4->async_modeset);