Re: [PATCH 1/9] drm/meson: hdmi: move encoder settings out of phy driver

[Neil Armstrong <neil.armstrong@linaro.org>]

On 30/07/2024 14:50, Jerome Brunet wrote:
> This relocates register pokes of the HDMI VPU encoder out of the
> HDMI phy driver. As far as HDMI is concerned, the sequence in which
> the setup is done remains mostly the same.
> 
> This was tested with modetest, cycling through the following resolutions:
>    #0 3840x2160 60.00
>    #1 3840x2160 59.94
>    #2 3840x2160 50.00
>    #3 3840x2160 30.00
>    #4 3840x2160 29.97
>    #5 3840x2160 25.00
>    #6 3840x2160 24.00
>    #7 3840x2160 23.98
>    #8 1920x1080 60.00
>    #9 1920x1080 60.00
>    #10 1920x1080 59.94
>    #11 1920x1080i 30.00
>    #12 1920x1080i 29.97
>    #13 1920x1080 50.00
>    #14 1920x1080i 25.00
>    #15 1920x1080 30.00
>    #16 1920x1080 29.97
>    #17 1920x1080 25.00
>    #18 1920x1080 24.00
>    #19 1920x1080 23.98
>    #20 1280x1024 60.02
>    #21 1152x864 59.97
>    #22 1280x720 60.00
>    #23 1280x720 59.94
>    #24 1280x720 50.00
>    #25 1024x768 60.00
>    #26 800x600 60.32
>    #27 720x576 50.00
>    #28 720x480 59.94
> 
> No regression to report.
> 
> This is part of an effort to clean up Amlogic HDMI related drivers which
> should eventually allow to stop using the component API and HHI syscon.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/gpu/drm/meson/meson_dw_hdmi.c      | 38 ----------------------
>   drivers/gpu/drm/meson/meson_encoder_hdmi.c | 16 +++++++++
>   2 files changed, 16 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 5565f7777529..bcf4f83582f2 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -115,12 +115,6 @@
>   
>   static DEFINE_SPINLOCK(reg_lock);
>   
> -enum meson_venc_source {
> -	MESON_VENC_SOURCE_NONE = 0,
> -	MESON_VENC_SOURCE_ENCI = 1,
> -	MESON_VENC_SOURCE_ENCP = 2,
> -};
> -
>   struct meson_dw_hdmi;
>   
>   struct meson_dw_hdmi_data {
> @@ -376,8 +370,6 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>   	struct meson_dw_hdmi *dw_hdmi = (struct meson_dw_hdmi *)data;
>   	bool is_hdmi2_sink = display->hdmi.scdc.supported;
>   	struct meson_drm *priv = dw_hdmi->priv;
> -	unsigned int wr_clk =
> -		readl_relaxed(priv->io_base + _REG(VPU_HDMI_SETTING));
>   	bool mode_is_420 = false;
>   
>   	DRM_DEBUG_DRIVER("\"%s\" div%d\n", mode->name,
> @@ -421,36 +413,6 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>   	meson_dw_hdmi_phy_reset(dw_hdmi);
>   	meson_dw_hdmi_phy_reset(dw_hdmi);
>   
> -	/* Temporary Disable VENC video stream */
> -	if (priv->venc.hdmi_use_enci)
> -		writel_relaxed(0, priv->io_base + _REG(ENCI_VIDEO_EN));
> -	else
> -		writel_relaxed(0, priv->io_base + _REG(ENCP_VIDEO_EN));
> -
> -	/* Temporary Disable HDMI video stream to HDMI-TX */
> -	writel_bits_relaxed(0x3, 0,
> -			    priv->io_base + _REG(VPU_HDMI_SETTING));
> -	writel_bits_relaxed(0xf << 8, 0,
> -			    priv->io_base + _REG(VPU_HDMI_SETTING));
> -
> -	/* Re-Enable VENC video stream */
> -	if (priv->venc.hdmi_use_enci)
> -		writel_relaxed(1, priv->io_base + _REG(ENCI_VIDEO_EN));
> -	else
> -		writel_relaxed(1, priv->io_base + _REG(ENCP_VIDEO_EN));
> -
> -	/* Push back HDMI clock settings */
> -	writel_bits_relaxed(0xf << 8, wr_clk & (0xf << 8),
> -			    priv->io_base + _REG(VPU_HDMI_SETTING));
> -
> -	/* Enable and Select HDMI video source for HDMI-TX */
> -	if (priv->venc.hdmi_use_enci)
> -		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCI,
> -				    priv->io_base + _REG(VPU_HDMI_SETTING));
> -	else
> -		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCP,
> -				    priv->io_base + _REG(VPU_HDMI_SETTING));
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> index 0593a1cde906..1c3e3e5526eb 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> @@ -45,6 +45,12 @@ struct meson_encoder_hdmi {
>   	struct cec_notifier *cec_notifier;
>   };
>   
> +enum meson_venc_source {
> +	MESON_VENC_SOURCE_NONE = 0,
> +	MESON_VENC_SOURCE_ENCI = 1,
> +	MESON_VENC_SOURCE_ENCP = 2,
> +};
> +
>   #define bridge_to_meson_encoder_hdmi(x) \
>   	container_of(x, struct meson_encoder_hdmi, bridge)
>   
> @@ -247,6 +253,14 @@ static void meson_encoder_hdmi_atomic_enable(struct drm_bridge *bridge,
>   		writel_relaxed(1, priv->io_base + _REG(ENCI_VIDEO_EN));
>   	else
>   		writel_relaxed(1, priv->io_base + _REG(ENCP_VIDEO_EN));
> +
> +	/* Enable and Select HDMI video source for HDMI-TX */
> +	if (priv->venc.hdmi_use_enci)
> +		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCI,
> +				    priv->io_base + _REG(VPU_HDMI_SETTING));
> +	else
> +		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCP,
> +				    priv->io_base + _REG(VPU_HDMI_SETTING));
>   }
>   
>   static void meson_encoder_hdmi_atomic_disable(struct drm_bridge *bridge,
> @@ -257,6 +271,8 @@ static void meson_encoder_hdmi_atomic_disable(struct drm_bridge *bridge,
>   
>   	writel_bits_relaxed(0x3, 0,
>   			    priv->io_base + _REG(VPU_HDMI_SETTING));
> +	writel_bits_relaxed(0xf << 8, 0,
> +			    priv->io_base + _REG(VPU_HDMI_SETTING));
>   
>   	writel_relaxed(0, priv->io_base + _REG(ENCI_VIDEO_EN));
>   	writel_relaxed(0, priv->io_base + _REG(ENCP_VIDEO_EN));

Nice usage of the split bridge architecture!
Now we must make sure the atomic enable order doesn't change...

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>