Re: [PATCH for drm-misc-fixes v5 3/4] drm/hisilicon/hibmc: move display contrl config to hibmc_probe()

[Thomas Zimmermann]

Hi

Am 23.04.26 um 08:32 schrieb Yongbang Shi:
> From: Lin He <helin52@xxxxxxxxxx>
>
> If there's no VGA output, this encoder modeset won't be called, which
> will cause displaying data from GPU being cut off. It's actually a
> common display config for DP and VGA, so move the vdac encoder modeset
> to driver load stage.
>
> Fixes: 5294967f4ae4 ("drm/hisilicon/hibmc: Add support for VDAC")
> Signed-off-by: Lin He <helin52@xxxxxxxxxx>
> Signed-off-by: Yongbang Shi <shiyongbang@xxxxxxxxxx>
> ---
> ChangeLog:
> v1 -> v2:
>    - remove tag "Reviewed-by: Tao Tian <tiantao6@xxxxxxxxxxxxx>", witch will
>      be given in public.
>    - add 'drm-misc-fixes' in subject prefix.
> ---
>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   | 14 ++++++++++++
>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  | 22 -------------------
>   2 files changed, 14 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index d409efc34215..f8ab471f92e7 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -217,6 +217,18 @@ void hibmc_set_current_gate(struct hibmc_drm_private *priv, unsigned int gate)
>   	writel(gate, mmio + gate_reg);
>   }
>   
> +static void hibmc_display_ctrl(struct hibmc_drm_private *priv)
> +{
> +	u32 reg;
> +
> +	reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
> +	reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
> +	reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
> +	reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
> +	reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);

That might be stupid question: IDK what these bits mean, but the FPVDDEN 
and FPEN sound like they enable something. If you do this 
unconditionally in HW init, does it prevent power saving?  And is the 
system goes through suspend/resume,  does it need to be re-enabled?

My gut feeling would be to put this code into the CRTC.

Best regards
Thomas

> +	writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
> +}
> +
>   static void hibmc_hw_config(struct hibmc_drm_private *priv)
>   {
>   	u32 reg;
> @@ -248,6 +260,8 @@ static void hibmc_hw_config(struct hibmc_drm_private *priv)
>   	reg |= HIBMC_MSCCTL_LOCALMEM_RESET(1);
>   
>   	writel(reg, priv->mmio + HIBMC_MISC_CTRL);
> +
> +	hibmc_display_ctrl(priv);
>   }
>   
>   static int hibmc_hw_map(struct hibmc_drm_private *priv)
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> index dc40018dbd21..9d924740c110 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> @@ -86,26 +86,6 @@ static const struct drm_connector_funcs hibmc_connector_funcs = {
>   	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>   };
>   
> -static void hibmc_encoder_mode_set(struct drm_encoder *encoder,
> -				   struct drm_display_mode *mode,
> -				   struct drm_display_mode *adj_mode)
> -{
> -	u32 reg;
> -	struct drm_device *dev = encoder->dev;
> -	struct hibmc_drm_private *priv = to_hibmc_drm_private(dev);
> -
> -	reg = readl(priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
> -	reg |= HIBMC_DISPLAY_CONTROL_FPVDDEN(1);
> -	reg |= HIBMC_DISPLAY_CONTROL_PANELDATE(1);
> -	reg |= HIBMC_DISPLAY_CONTROL_FPEN(1);
> -	reg |= HIBMC_DISPLAY_CONTROL_VBIASEN(1);
> -	writel(reg, priv->mmio + HIBMC_DISPLAY_CONTROL_HISILE);
> -}
> -
> -static const struct drm_encoder_helper_funcs hibmc_encoder_helper_funcs = {
> -	.mode_set = hibmc_encoder_mode_set,
> -};
> -
>   int hibmc_vdac_init(struct hibmc_drm_private *priv)
>   {
>   	struct drm_device *dev = &priv->dev;
> @@ -128,8 +108,6 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>   		goto err;
>   	}
>   
> -	drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
> -
>   	ret = drm_connector_init_with_ddc(dev, connector,
>   					  &hibmc_connector_funcs,
>   					  DRM_MODE_CONNECTOR_VGA,

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)