| Message ID | 5919dccdd4a792936e6cb7eb55983c530c9a468d.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 Wed, 8 Jul 2020 at 18:44, Maxime Ripard <maxime@cerno.tech> wrote: > > The HSM clock needs to be setup at around 101% of the pixel rate. This > was done previously by setting the clock rate to 163.7MHz at probe time and > only check in mode_valid whether the mode pixel clock was under the pixel > clock +1% or not. > > However, with 4k we need to change that frequency to a higher frequency > than 163.7MHz, and yet want to have the lowest clock as possible to have a > decent power saving. > > Let's change that logic a bit by setting the clock rate of the HSM clock > to the pixel rate at encoder_enable time. This would work for the > BCM2711 that support 4k resolutions and has a clock that can provide it, > but we still have to take care of a 4k panel plugged on a BCM283x SoCs > that wouldn't be able to use those modes, so let's define the limit in > the variant. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 79 ++++++++++++++++------------------- > drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +- > 2 files changed, 41 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 17797b14cde4..9f30fab744f2 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -53,7 +53,6 @@ > #include "vc4_hdmi_regs.h" > #include "vc4_regs.h" > > -#define HSM_CLOCK_FREQ 163682864 > #define CEC_CLOCK_FREQ 40000 > > static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused) > @@ -326,6 +325,7 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder) > HDMI_WRITE(HDMI_VID_CTL, > HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE); > > + clk_disable_unprepare(vc4_hdmi->hsm_clock); > clk_disable_unprepare(vc4_hdmi->pixel_clock); > > ret = pm_runtime_put(&vc4_hdmi->pdev->dev); > @@ -423,6 +423,7 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) > struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); > struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); > bool debug_dump_regs = false; > + unsigned long pixel_rate, hsm_rate; > int ret; > > ret = pm_runtime_get_sync(&vc4_hdmi->pdev->dev); > @@ -431,9 +432,8 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) > return; > } > > - ret = clk_set_rate(vc4_hdmi->pixel_clock, > - mode->clock * 1000 * > - ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1)); > + pixel_rate = mode->clock * 1000 * ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1); > + ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate); > if (ret) { > DRM_ERROR("Failed to set pixel clock rate: %d\n", ret); > return; > @@ -445,6 +445,36 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) > return; > } > > + /* > + * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must > + * be faster than pixel clock, infinitesimally faster, tested in > + * simulation. Otherwise, exact value is unimportant for HDMI > + * operation." This conflicts with bcm2835's vc4 documentation, which > + * states HSM's clock has to be at least 108% of the pixel clock. > + * > + * Real life tests reveal that vc4's firmware statement holds up, and > + * users are able to use pixel clocks closer to HSM's, namely for > + * 1920x1200@60Hz. So it was decided to have leave a 1% margin between > + * both clocks. Which, for RPi0-3 implies a maximum pixel clock of > + * 162MHz. > + * > + * Additionally, the AXI clock needs to be at least 25% of > + * pixel clock, but HSM ends up being the limiting factor. > + */ > + hsm_rate = max_t(unsigned long, 120000000, (pixel_rate / 100) * 101); > + ret = clk_set_rate(vc4_hdmi->hsm_clock, hsm_rate); > + if (ret) { > + DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); > + return; > + } > + > + ret = clk_prepare_enable(vc4_hdmi->hsm_clock); > + if (ret) { > + DRM_ERROR("Failed to turn on HSM clock: %d\n", ret); > + clk_disable_unprepare(vc4_hdmi->pixel_clock); > + return; > + } > + > if (vc4_hdmi->variant->reset) > vc4_hdmi->variant->reset(vc4_hdmi); > > @@ -559,23 +589,9 @@ static enum drm_mode_status > vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder, > const struct drm_display_mode *mode) > { > - /* > - * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must > - * be faster than pixel clock, infinitesimally faster, tested in > - * simulation. Otherwise, exact value is unimportant for HDMI > - * operation." This conflicts with bcm2835's vc4 documentation, which > - * states HSM's clock has to be at least 108% of the pixel clock. > - * > - * Real life tests reveal that vc4's firmware statement holds up, and > - * users are able to use pixel clocks closer to HSM's, namely for > - * 1920x1200@60Hz. So it was decided to have leave a 1% margin between > - * both clocks. Which, for RPi0-3 implies a maximum pixel clock of > - * 162MHz. > - * > - * Additionally, the AXI clock needs to be at least 25% of > - * pixel clock, but HSM ends up being the limiting factor. > - */ > - if (mode->clock > HSM_CLOCK_FREQ / (1000 * 101 / 100)) > + struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); > + > + if ((mode->clock * 1000) > vc4_hdmi->variant->max_pixel_clock) > return MODE_CLOCK_HIGH; > > return MODE_OK; > @@ -1349,23 +1365,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > return -EPROBE_DEFER; > } > > - /* This is the rate that is set by the firmware. The number > - * needs to be a bit higher than the pixel clock rate > - * (generally 148.5Mhz). > - */ > - ret = clk_set_rate(vc4_hdmi->hsm_clock, HSM_CLOCK_FREQ); > - if (ret) { > - DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); > - goto err_put_i2c; > - } > - > - ret = clk_prepare_enable(vc4_hdmi->hsm_clock); > - if (ret) { > - DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n", > - ret); > - goto err_put_i2c; > - } > - > /* Only use the GPIO HPD pin if present in the DT, otherwise > * we'll use the HDMI core's register. > */ > @@ -1413,9 +1412,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > err_destroy_encoder: > drm_encoder_cleanup(encoder); > err_unprepare_hsm: > - clk_disable_unprepare(vc4_hdmi->hsm_clock); > pm_runtime_disable(dev); > -err_put_i2c: > put_device(&vc4_hdmi->ddc->dev); > > return ret; > @@ -1454,7 +1451,6 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master, > vc4_hdmi_connector_destroy(&vc4_hdmi->connector); > drm_encoder_cleanup(&vc4_hdmi->encoder.base.base); > > - clk_disable_unprepare(vc4_hdmi->hsm_clock); > pm_runtime_disable(dev); > > put_device(&vc4_hdmi->ddc->dev); > @@ -1479,6 +1475,7 @@ static int vc4_hdmi_dev_remove(struct platform_device *pdev) > static const struct vc4_hdmi_variant bcm2835_variant = { > .encoder_type = VC4_ENCODER_TYPE_HDMI0, > .debugfs_name = "hdmi_regs", > + .max_pixel_clock = 162000000, > .cec_available = true, > .registers = vc4_hdmi_fields, > .num_registers = ARRAY_SIZE(vc4_hdmi_fields), > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h > index 3f07aebe89f1..342f6e0227a2 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h > @@ -36,6 +36,9 @@ struct vc4_hdmi_variant { > /* Set to true when the CEC support is available */ > bool cec_available; > > + /* Maximum pixel clock supported by the controller (in Hz) */ > + unsigned long long max_pixel_clock; > + > /* List of the registers available on that variant */ > const struct vc4_hdmi_register *registers; > > -- > git-series 0.9.1
Hi Maxime, On 7/9/20 2:42 AM, Maxime Ripard wrote: > The HSM clock needs to be setup at around 101% of the pixel rate. This > was done previously by setting the clock rate to 163.7MHz at probe time and > only check in mode_valid whether the mode pixel clock was under the pixel > clock +1% or not. > > However, with 4k we need to change that frequency to a higher frequency > than 163.7MHz, and yet want to have the lowest clock as possible to have a > decent power saving. > > Let's change that logic a bit by setting the clock rate of the HSM clock > to the pixel rate at encoder_enable time. This would work for the > BCM2711 that support 4k resolutions and has a clock that can provide it, > but we still have to take care of a 4k panel plugged on a BCM283x SoCs > that wouldn't be able to use those modes, so let's define the limit in > the variant. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 79 ++++++++++++++++------------------- > drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +- > 2 files changed, 41 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 17797b14cde4..9f30fab744f2 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -53,7 +53,6 @@ > #include "vc4_hdmi_regs.h" > #include "vc4_regs.h" > > -#define HSM_CLOCK_FREQ 163682864 > #define CEC_CLOCK_FREQ 40000 > > static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused) > @@ -326,6 +325,7 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder) > HDMI_WRITE(HDMI_VID_CTL, > HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE); > > + clk_disable_unprepare(vc4_hdmi->hsm_clock); > clk_disable_unprepare(vc4_hdmi->pixel_clock); > > ret = pm_runtime_put(&vc4_hdmi->pdev->dev); > @@ -423,6 +423,7 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) > struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); > struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); > bool debug_dump_regs = false; > + unsigned long pixel_rate, hsm_rate; > int ret; > > ret = pm_runtime_get_sync(&vc4_hdmi->pdev->dev); > @@ -431,9 +432,8 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) > return; > } > > - ret = clk_set_rate(vc4_hdmi->pixel_clock, > - mode->clock * 1000 * > - ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1)); > + pixel_rate = mode->clock * 1000 * ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1); > + ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate); > if (ret) { > DRM_ERROR("Failed to set pixel clock rate: %d\n", ret); > return; > @@ -445,6 +445,36 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) > return; > } > > + /* > + * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must > + * be faster than pixel clock, infinitesimally faster, tested in > + * simulation. Otherwise, exact value is unimportant for HDMI > + * operation." This conflicts with bcm2835's vc4 documentation, which > + * states HSM's clock has to be at least 108% of the pixel clock. > + * > + * Real life tests reveal that vc4's firmware statement holds up, and > + * users are able to use pixel clocks closer to HSM's, namely for > + * 1920x1200@60Hz. So it was decided to have leave a 1% margin between > + * both clocks. Which, for RPi0-3 implies a maximum pixel clock of > + * 162MHz. > + * > + * Additionally, the AXI clock needs to be at least 25% of > + * pixel clock, but HSM ends up being the limiting factor. > + */ > + hsm_rate = max_t(unsigned long, 120000000, (pixel_rate / 100) * 101); > + ret = clk_set_rate(vc4_hdmi->hsm_clock, hsm_rate); > + if (ret) { > + DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); > + return; > + } > + > + ret = clk_prepare_enable(vc4_hdmi->hsm_clock); > + if (ret) { > + DRM_ERROR("Failed to turn on HSM clock: %d\n", ret); > + clk_disable_unprepare(vc4_hdmi->pixel_clock); > + return; > + } About vc4_hdmi->hsm_clock instance, usually, we need to enable the clock with clk_prepare_enable() and then touch the clock like clk_set_rate(). I think that need to enable the clock before calling clk_set_rate(). When I tested this patchset, it is well working because I think that vc4_hdmi->hsm_clock was already enabled on other side. > + > if (vc4_hdmi->variant->reset) > vc4_hdmi->variant->reset(vc4_hdmi); > > @@ -559,23 +589,9 @@ static enum drm_mode_status > vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder, > const struct drm_display_mode *mode) > { > - /* > - * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must > - * be faster than pixel clock, infinitesimally faster, tested in > - * simulation. Otherwise, exact value is unimportant for HDMI > - * operation." This conflicts with bcm2835's vc4 documentation, which > - * states HSM's clock has to be at least 108% of the pixel clock. > - * > - * Real life tests reveal that vc4's firmware statement holds up, and > - * users are able to use pixel clocks closer to HSM's, namely for > - * 1920x1200@60Hz. So it was decided to have leave a 1% margin between > - * both clocks. Which, for RPi0-3 implies a maximum pixel clock of > - * 162MHz. > - * > - * Additionally, the AXI clock needs to be at least 25% of > - * pixel clock, but HSM ends up being the limiting factor. > - */ > - if (mode->clock > HSM_CLOCK_FREQ / (1000 * 101 / 100)) > + struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); > + > + if ((mode->clock * 1000) > vc4_hdmi->variant->max_pixel_clock) > return MODE_CLOCK_HIGH; > > return MODE_OK; > @@ -1349,23 +1365,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > return -EPROBE_DEFER; > } > > - /* This is the rate that is set by the firmware. The number > - * needs to be a bit higher than the pixel clock rate > - * (generally 148.5Mhz). > - */ > - ret = clk_set_rate(vc4_hdmi->hsm_clock, HSM_CLOCK_FREQ); > - if (ret) { > - DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); > - goto err_put_i2c; > - } > - > - ret = clk_prepare_enable(vc4_hdmi->hsm_clock); > - if (ret) { > - DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n", > - ret); > - goto err_put_i2c; > - } > - > /* Only use the GPIO HPD pin if present in the DT, otherwise > * we'll use the HDMI core's register. > */ > @@ -1413,9 +1412,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > err_destroy_encoder: > drm_encoder_cleanup(encoder); > err_unprepare_hsm: > - clk_disable_unprepare(vc4_hdmi->hsm_clock); > pm_runtime_disable(dev); > -err_put_i2c: > put_device(&vc4_hdmi->ddc->dev); > > return ret; > @@ -1454,7 +1451,6 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master, > vc4_hdmi_connector_destroy(&vc4_hdmi->connector); > drm_encoder_cleanup(&vc4_hdmi->encoder.base.base); > > - clk_disable_unprepare(vc4_hdmi->hsm_clock); > pm_runtime_disable(dev); > > put_device(&vc4_hdmi->ddc->dev); > @@ -1479,6 +1475,7 @@ static int vc4_hdmi_dev_remove(struct platform_device *pdev) > static const struct vc4_hdmi_variant bcm2835_variant = { > .encoder_type = VC4_ENCODER_TYPE_HDMI0, > .debugfs_name = "hdmi_regs", > + .max_pixel_clock = 162000000, > .cec_available = true, > .registers = vc4_hdmi_fields, > .num_registers = ARRAY_SIZE(vc4_hdmi_fields), > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h > index 3f07aebe89f1..342f6e0227a2 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h > @@ -36,6 +36,9 @@ struct vc4_hdmi_variant { > /* Set to true when the CEC support is available */ > bool cec_available; > > + /* Maximum pixel clock supported by the controller (in Hz) */ > + unsigned long long max_pixel_clock; > + > /* List of the registers available on that variant */ > const struct vc4_hdmi_register *registers; > >
Hi Chanwoo, On Tue, Sep 01, 2020 at 01:36:17PM +0900, Chanwoo Choi wrote: > On 7/9/20 2:42 AM, Maxime Ripard wrote: > > The HSM clock needs to be setup at around 101% of the pixel rate. This > > was done previously by setting the clock rate to 163.7MHz at probe time and > > only check in mode_valid whether the mode pixel clock was under the pixel > > clock +1% or not. > > > > However, with 4k we need to change that frequency to a higher frequency > > than 163.7MHz, and yet want to have the lowest clock as possible to have a > > decent power saving. > > > > Let's change that logic a bit by setting the clock rate of the HSM clock > > to the pixel rate at encoder_enable time. This would work for the > > BCM2711 that support 4k resolutions and has a clock that can provide it, > > but we still have to take care of a 4k panel plugged on a BCM283x SoCs > > that wouldn't be able to use those modes, so let's define the limit in > > the variant. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 79 ++++++++++++++++------------------- > > drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +- > > 2 files changed, 41 insertions(+), 41 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index 17797b14cde4..9f30fab744f2 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -53,7 +53,6 @@ > > #include "vc4_hdmi_regs.h" > > #include "vc4_regs.h" > > > > -#define HSM_CLOCK_FREQ 163682864 > > #define CEC_CLOCK_FREQ 40000 > > > > static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused) > > @@ -326,6 +325,7 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder) > > HDMI_WRITE(HDMI_VID_CTL, > > HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE); > > > > + clk_disable_unprepare(vc4_hdmi->hsm_clock); > > clk_disable_unprepare(vc4_hdmi->pixel_clock); > > > > ret = pm_runtime_put(&vc4_hdmi->pdev->dev); > > @@ -423,6 +423,7 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) > > struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); > > struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); > > bool debug_dump_regs = false; > > + unsigned long pixel_rate, hsm_rate; > > int ret; > > > > ret = pm_runtime_get_sync(&vc4_hdmi->pdev->dev); > > @@ -431,9 +432,8 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) > > return; > > } > > > > - ret = clk_set_rate(vc4_hdmi->pixel_clock, > > - mode->clock * 1000 * > > - ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1)); > > + pixel_rate = mode->clock * 1000 * ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1); > > + ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate); > > if (ret) { > > DRM_ERROR("Failed to set pixel clock rate: %d\n", ret); > > return; > > @@ -445,6 +445,36 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) > > return; > > } > > > > + /* > > + * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must > > + * be faster than pixel clock, infinitesimally faster, tested in > > + * simulation. Otherwise, exact value is unimportant for HDMI > > + * operation." This conflicts with bcm2835's vc4 documentation, which > > + * states HSM's clock has to be at least 108% of the pixel clock. > > + * > > + * Real life tests reveal that vc4's firmware statement holds up, and > > + * users are able to use pixel clocks closer to HSM's, namely for > > + * 1920x1200@60Hz. So it was decided to have leave a 1% margin between > > + * both clocks. Which, for RPi0-3 implies a maximum pixel clock of > > + * 162MHz. > > + * > > + * Additionally, the AXI clock needs to be at least 25% of > > + * pixel clock, but HSM ends up being the limiting factor. > > + */ > > + hsm_rate = max_t(unsigned long, 120000000, (pixel_rate / 100) * 101); > > + ret = clk_set_rate(vc4_hdmi->hsm_clock, hsm_rate); > > + if (ret) { > > + DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); > > + return; > > + } > > + > > + ret = clk_prepare_enable(vc4_hdmi->hsm_clock); > > + if (ret) { > > + DRM_ERROR("Failed to turn on HSM clock: %d\n", ret); > > + clk_disable_unprepare(vc4_hdmi->pixel_clock); > > + return; > > + } > > About vc4_hdmi->hsm_clock instance, usually, we need to enable the clock > with clk_prepare_enable() and then touch the clock like clk_set_rate(). > I think that need to enable the clock before calling clk_set_rate(). > > When I tested this patchset, it is well working because I think that > vc4_hdmi->hsm_clock was already enabled on other side. There's no clear rule here on the ordering (at least enforced by the framework). There's clocks that need to be disabled to change their rate (CLK_SET_RATE_GATE) and some that need to be enabled to change their rate (CLK_SET_RATE_UNGATE). Generally speaking, it seems more logical to me to have first the rate changed and then the clock enabled since it won't create any "hiccup", but I could very well see the opposite to be preferred. Maxime
Hi Maxime, On 9/1/20 6:45 PM, Maxime Ripard wrote: > Hi Chanwoo, > > On Tue, Sep 01, 2020 at 01:36:17PM +0900, Chanwoo Choi wrote: >> On 7/9/20 2:42 AM, Maxime Ripard wrote: >>> The HSM clock needs to be setup at around 101% of the pixel rate. This >>> was done previously by setting the clock rate to 163.7MHz at probe time and >>> only check in mode_valid whether the mode pixel clock was under the pixel >>> clock +1% or not. >>> >>> However, with 4k we need to change that frequency to a higher frequency >>> than 163.7MHz, and yet want to have the lowest clock as possible to have a >>> decent power saving. >>> >>> Let's change that logic a bit by setting the clock rate of the HSM clock >>> to the pixel rate at encoder_enable time. This would work for the >>> BCM2711 that support 4k resolutions and has a clock that can provide it, >>> but we still have to take care of a 4k panel plugged on a BCM283x SoCs >>> that wouldn't be able to use those modes, so let's define the limit in >>> the variant. >>> >>> Signed-off-by: Maxime Ripard <maxime@cerno.tech> >>> --- >>> drivers/gpu/drm/vc4/vc4_hdmi.c | 79 ++++++++++++++++------------------- >>> drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +- >>> 2 files changed, 41 insertions(+), 41 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c >>> index 17797b14cde4..9f30fab744f2 100644 >>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c >>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c >>> @@ -53,7 +53,6 @@ >>> #include "vc4_hdmi_regs.h" >>> #include "vc4_regs.h" >>> >>> -#define HSM_CLOCK_FREQ 163682864 >>> #define CEC_CLOCK_FREQ 40000 >>> >>> static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused) >>> @@ -326,6 +325,7 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder) >>> HDMI_WRITE(HDMI_VID_CTL, >>> HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE); >>> >>> + clk_disable_unprepare(vc4_hdmi->hsm_clock); >>> clk_disable_unprepare(vc4_hdmi->pixel_clock); >>> >>> ret = pm_runtime_put(&vc4_hdmi->pdev->dev); >>> @@ -423,6 +423,7 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) >>> struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); >>> struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); >>> bool debug_dump_regs = false; >>> + unsigned long pixel_rate, hsm_rate; >>> int ret; >>> >>> ret = pm_runtime_get_sync(&vc4_hdmi->pdev->dev); >>> @@ -431,9 +432,8 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) >>> return; >>> } >>> >>> - ret = clk_set_rate(vc4_hdmi->pixel_clock, >>> - mode->clock * 1000 * >>> - ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1)); >>> + pixel_rate = mode->clock * 1000 * ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1); >>> + ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate); >>> if (ret) { >>> DRM_ERROR("Failed to set pixel clock rate: %d\n", ret); >>> return; >>> @@ -445,6 +445,36 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) >>> return; >>> } >>> >>> + /* >>> + * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must >>> + * be faster than pixel clock, infinitesimally faster, tested in >>> + * simulation. Otherwise, exact value is unimportant for HDMI >>> + * operation." This conflicts with bcm2835's vc4 documentation, which >>> + * states HSM's clock has to be at least 108% of the pixel clock. >>> + * >>> + * Real life tests reveal that vc4's firmware statement holds up, and >>> + * users are able to use pixel clocks closer to HSM's, namely for >>> + * 1920x1200@60Hz. So it was decided to have leave a 1% margin between >>> + * both clocks. Which, for RPi0-3 implies a maximum pixel clock of >>> + * 162MHz. >>> + * >>> + * Additionally, the AXI clock needs to be at least 25% of >>> + * pixel clock, but HSM ends up being the limiting factor. >>> + */ >>> + hsm_rate = max_t(unsigned long, 120000000, (pixel_rate / 100) * 101); >>> + ret = clk_set_rate(vc4_hdmi->hsm_clock, hsm_rate); >>> + if (ret) { >>> + DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); >>> + return; >>> + } >>> + >>> + ret = clk_prepare_enable(vc4_hdmi->hsm_clock); >>> + if (ret) { >>> + DRM_ERROR("Failed to turn on HSM clock: %d\n", ret); >>> + clk_disable_unprepare(vc4_hdmi->pixel_clock); >>> + return; >>> + } >> >> About vc4_hdmi->hsm_clock instance, usually, we need to enable the clock >> with clk_prepare_enable() and then touch the clock like clk_set_rate(). >> I think that need to enable the clock before calling clk_set_rate(). >> >> When I tested this patchset, it is well working because I think that >> vc4_hdmi->hsm_clock was already enabled on other side. > > There's no clear rule here on the ordering (at least enforced by the > framework). There's clocks that need to be disabled to change their rate > (CLK_SET_RATE_GATE) and some that need to be enabled to change their > rate (CLK_SET_RATE_UNGATE). > > Generally speaking, it seems more logical to me to have first the rate > changed and then the clock enabled since it won't create any "hiccup", > but I could very well see the opposite to be preferred. If it doesn't cause the problem on h/w, I agree. Thanks for the explanation.
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 17797b14cde4..9f30fab744f2 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -53,7 +53,6 @@ #include "vc4_hdmi_regs.h" #include "vc4_regs.h" -#define HSM_CLOCK_FREQ 163682864 #define CEC_CLOCK_FREQ 40000 static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused) @@ -326,6 +325,7 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder) HDMI_WRITE(HDMI_VID_CTL, HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE); + clk_disable_unprepare(vc4_hdmi->hsm_clock); clk_disable_unprepare(vc4_hdmi->pixel_clock); ret = pm_runtime_put(&vc4_hdmi->pdev->dev); @@ -423,6 +423,7 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); bool debug_dump_regs = false; + unsigned long pixel_rate, hsm_rate; int ret; ret = pm_runtime_get_sync(&vc4_hdmi->pdev->dev); @@ -431,9 +432,8 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) return; } - ret = clk_set_rate(vc4_hdmi->pixel_clock, - mode->clock * 1000 * - ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1)); + pixel_rate = mode->clock * 1000 * ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1); + ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate); if (ret) { DRM_ERROR("Failed to set pixel clock rate: %d\n", ret); return; @@ -445,6 +445,36 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) return; } + /* + * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must + * be faster than pixel clock, infinitesimally faster, tested in + * simulation. Otherwise, exact value is unimportant for HDMI + * operation." This conflicts with bcm2835's vc4 documentation, which + * states HSM's clock has to be at least 108% of the pixel clock. + * + * Real life tests reveal that vc4's firmware statement holds up, and + * users are able to use pixel clocks closer to HSM's, namely for + * 1920x1200@60Hz. So it was decided to have leave a 1% margin between + * both clocks. Which, for RPi0-3 implies a maximum pixel clock of + * 162MHz. + * + * Additionally, the AXI clock needs to be at least 25% of + * pixel clock, but HSM ends up being the limiting factor. + */ + hsm_rate = max_t(unsigned long, 120000000, (pixel_rate / 100) * 101); + ret = clk_set_rate(vc4_hdmi->hsm_clock, hsm_rate); + if (ret) { + DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); + return; + } + + ret = clk_prepare_enable(vc4_hdmi->hsm_clock); + if (ret) { + DRM_ERROR("Failed to turn on HSM clock: %d\n", ret); + clk_disable_unprepare(vc4_hdmi->pixel_clock); + return; + } + if (vc4_hdmi->variant->reset) vc4_hdmi->variant->reset(vc4_hdmi); @@ -559,23 +589,9 @@ static enum drm_mode_status vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder, const struct drm_display_mode *mode) { - /* - * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must - * be faster than pixel clock, infinitesimally faster, tested in - * simulation. Otherwise, exact value is unimportant for HDMI - * operation." This conflicts with bcm2835's vc4 documentation, which - * states HSM's clock has to be at least 108% of the pixel clock. - * - * Real life tests reveal that vc4's firmware statement holds up, and - * users are able to use pixel clocks closer to HSM's, namely for - * 1920x1200@60Hz. So it was decided to have leave a 1% margin between - * both clocks. Which, for RPi0-3 implies a maximum pixel clock of - * 162MHz. - * - * Additionally, the AXI clock needs to be at least 25% of - * pixel clock, but HSM ends up being the limiting factor. - */ - if (mode->clock > HSM_CLOCK_FREQ / (1000 * 101 / 100)) + struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + + if ((mode->clock * 1000) > vc4_hdmi->variant->max_pixel_clock) return MODE_CLOCK_HIGH; return MODE_OK; @@ -1349,23 +1365,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) return -EPROBE_DEFER; } - /* This is the rate that is set by the firmware. The number - * needs to be a bit higher than the pixel clock rate - * (generally 148.5Mhz). - */ - ret = clk_set_rate(vc4_hdmi->hsm_clock, HSM_CLOCK_FREQ); - if (ret) { - DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); - goto err_put_i2c; - } - - ret = clk_prepare_enable(vc4_hdmi->hsm_clock); - if (ret) { - DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n", - ret); - goto err_put_i2c; - } - /* Only use the GPIO HPD pin if present in the DT, otherwise * we'll use the HDMI core's register. */ @@ -1413,9 +1412,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) err_destroy_encoder: drm_encoder_cleanup(encoder); err_unprepare_hsm: - clk_disable_unprepare(vc4_hdmi->hsm_clock); pm_runtime_disable(dev); -err_put_i2c: put_device(&vc4_hdmi->ddc->dev); return ret; @@ -1454,7 +1451,6 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master, vc4_hdmi_connector_destroy(&vc4_hdmi->connector); drm_encoder_cleanup(&vc4_hdmi->encoder.base.base); - clk_disable_unprepare(vc4_hdmi->hsm_clock); pm_runtime_disable(dev); put_device(&vc4_hdmi->ddc->dev); @@ -1479,6 +1475,7 @@ static int vc4_hdmi_dev_remove(struct platform_device *pdev) static const struct vc4_hdmi_variant bcm2835_variant = { .encoder_type = VC4_ENCODER_TYPE_HDMI0, .debugfs_name = "hdmi_regs", + .max_pixel_clock = 162000000, .cec_available = true, .registers = vc4_hdmi_fields, .num_registers = ARRAY_SIZE(vc4_hdmi_fields), diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 3f07aebe89f1..342f6e0227a2 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -36,6 +36,9 @@ struct vc4_hdmi_variant { /* Set to true when the CEC support is available */ bool cec_available; + /* Maximum pixel clock supported by the controller (in Hz) */ + unsigned long long max_pixel_clock; + /* List of the registers available on that variant */ const struct vc4_hdmi_register *registers;
The HSM clock needs to be setup at around 101% of the pixel rate. This was done previously by setting the clock rate to 163.7MHz at probe time and only check in mode_valid whether the mode pixel clock was under the pixel clock +1% or not. However, with 4k we need to change that frequency to a higher frequency than 163.7MHz, and yet want to have the lowest clock as possible to have a decent power saving. Let's change that logic a bit by setting the clock rate of the HSM clock to the pixel rate at encoder_enable time. This would work for the BCM2711 that support 4k resolutions and has a clock that can provide it, but we still have to take care of a 4k panel plugged on a BCM283x SoCs that wouldn't be able to use those modes, so let's define the limit in the variant. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 79 ++++++++++++++++------------------- drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +- 2 files changed, 41 insertions(+), 41 deletions(-)