Re: [PATCH v4 5/7] drm/vc4: hdmi: Rework hdmi_enable_4kp60 detection code

[Dave Stevenson]

Hi Maxime

On Thu, 20 Oct 2022 at 10:14, <maxime@xxxxxxxxxx> wrote:
>
> In order to support higher HDMI frequencies, users have to set the
> hdmi_enable_4kp60 parameter in their config.txt file.
>
> This will have the side-effect of raising the maximum of the core clock,
> tied to the HVS, and managed by the HVS driver.
>
> However, we are querying this in the HDMI driver by poking into the HVS
> structure to get our struct clk handle.
>
> Let's make this part of the HVS bind implementation to have all the core
> clock related setup in the same place.
>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.h  | 10 ++++++++++
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 15 ++++-----------
>  drivers/gpu/drm/vc4/vc4_hdmi.h |  8 --------
>  drivers/gpu/drm/vc4/vc4_hvs.c  | 23 +++++++++++++++++++++++
>  4 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 418a8242691f..8b2b1af565f9 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -326,6 +326,8 @@ struct vc4_hvs {
>
>         struct clk *core_clk;
>
> +       unsigned long max_core_rate;
> +
>         /* Memory manager for CRTCs to allocate space in the display
>          * list.  Units are dwords.
>          */
> @@ -337,6 +339,14 @@ struct vc4_hvs {
>         struct drm_mm_node mitchell_netravali_filter;
>
>         struct debugfs_regset32 regset;
> +
> +       /*
> +        * Even if HDMI0 on the RPi4 can output modes requiring a pixel
> +        * rate higher than 297MHz, it needs some adjustments in the
> +        * config.txt file to be able to do so and thus won't always be
> +        * available.
> +        */
> +       bool vc5_hdmi_enable_scrambling;
>  };
>
>  struct vc4_plane {
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 87961d4de5aa..afe3daa2173e 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -46,7 +46,6 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/rational.h>
>  #include <linux/reset.h>
> -#include <soc/bcm2835/raspberrypi-clocks.h>
>  #include <sound/dmaengine_pcm.h>
>  #include <sound/hdmi-codec.h>
>  #include <sound/pcm_drm_eld.h>
> @@ -460,6 +459,7 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector,
>  static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
>  {
>         struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> +       struct vc4_dev *vc4 = to_vc4_dev(connector->dev);
>         int ret = 0;
>         struct edid *edid;
>
> @@ -483,7 +483,7 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
>         ret = drm_add_edid_modes(connector, edid);
>         kfree(edid);
>
> -       if (vc4_hdmi->disable_4kp60) {
> +       if (!vc4->hvs->vc5_hdmi_enable_scrambling) {
>                 struct drm_device *drm = connector->dev;
>                 const struct drm_display_mode *mode;
>
> @@ -1757,11 +1757,12 @@ vc4_hdmi_encoder_clock_valid(const struct vc4_hdmi *vc4_hdmi,
>  {
>         const struct drm_connector *connector = &vc4_hdmi->connector;
>         const struct drm_display_info *info = &connector->display_info;
> +       struct vc4_dev *vc4 = to_vc4_dev(connector->dev);
>
>         if (clock > vc4_hdmi->variant->max_pixel_clock)
>                 return MODE_CLOCK_HIGH;
>
> -       if (vc4_hdmi->disable_4kp60 && clock > HDMI_14_MAX_TMDS_CLK)
> +       if (!vc4->hvs->vc5_hdmi_enable_scrambling && clock > HDMI_14_MAX_TMDS_CLK)
>                 return MODE_CLOCK_HIGH;
>
>         if (info->max_tmds_clock && clock > (info->max_tmds_clock * 1000))
> @@ -3428,14 +3429,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>         vc4_hdmi->disable_wifi_frequencies =
>                 of_property_read_bool(dev->of_node, "wifi-2.4ghz-coexistence");
>
> -       if (variant->max_pixel_clock == 600000000) {
> -               struct vc4_dev *vc4 = to_vc4_dev(drm);
> -               unsigned long max_rate = rpi_firmware_clk_get_max_rate(vc4->hvs->core_clk);
> -
> -               if (max_rate < 550000000)
> -                       vc4_hdmi->disable_4kp60 = true;
> -       }
> -
>         ret = devm_pm_runtime_enable(dev);
>         if (ret)
>                 return ret;
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> index db823efb2563..e3619836ca17 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> @@ -156,14 +156,6 @@ struct vc4_hdmi {
>          */
>         bool disable_wifi_frequencies;
>
> -       /*
> -        * Even if HDMI0 on the RPi4 can output modes requiring a pixel
> -        * rate higher than 297MHz, it needs some adjustments in the
> -        * config.txt file to be able to do so and thus won't always be
> -        * available.
> -        */
> -       bool disable_4kp60;
> -
>         struct cec_adapter *cec_adap;
>         struct cec_msg cec_rx_msg;
>         bool cec_tx_ok;
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 4ac9f5a2d5f9..300ac0b57571 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -28,6 +28,8 @@
>  #include <drm/drm_drv.h>
>  #include <drm/drm_vblank.h>
>
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
>  #include "vc4_drv.h"
>  #include "vc4_regs.h"
>
> @@ -791,12 +793,33 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>         hvs->regset.nregs = ARRAY_SIZE(hvs_regs);
>
>         if (vc4->is_vc5) {
> +               struct rpi_firmware *firmware;
> +               struct device_node *node;
> +               unsigned long max_rate;

As commented on patch 4, the value returned by
rpi_firmware_clk_get_max_rate is only an unsigned int.
I guess in this case it is passed into the clock frame work which
wants unsigned long, but do we need to keep it as an unsigned long?

> +
> +               node = rpi_firmware_find_node();
> +               if (!node)
> +                       return -EINVAL;
> +
> +               firmware = rpi_firmware_get(node);
> +               of_node_put(node);
> +               if (!firmware)
> +                       return -EPROBE_DEFER;
> +
>                 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);
>                 }
>
> +               max_rate = rpi_firmware_clk_get_max_rate(firmware,
> +                                                        RPI_FIRMWARE_CORE_CLK_ID);
> +               rpi_firmware_put(firmware);
> +               if (max_rate >= 550000000)
> +                       hvs->vc5_hdmi_enable_scrambling = true;
> +
> +               hvs->max_core_rate = max_rate;

I was going to query the reason for storing this value, but it's used
when we get to patch 7/7.
I won't quibble about having it as an unused value for 2 patches.

If you're happy in both cases, then I'll send an R-b.

  Dave

>                 ret = clk_prepare_enable(hvs->core_clk);
>                 if (ret) {
>                         dev_err(&pdev->dev, "Couldn't enable the core clock\n");
>
> --
> b4 0.10.1