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

[Yongbang Shi]

> 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.
> 

We discussed this with the chip design team and confirmed, after reviewing the chip's RTL code, that FPVDDEN, FPEN and
VBIASEN are not used in the chip's RTL implementation; only PANELDATE is used, and this bit controls the output of the
data, hsync and vsync signals simultaneously. We will remove FPVDDEN, FPEN and VBIASEN in v6, and do some tests.

This PANELDATE should be enabled during initialization. In older versions of the driver, since there was only one
encoder-connector(VGA), placing it in `encoder_helper` posed no practical issues. Now that it has been moved to the
initialization phase, it is enabled only once during initialization, in accordance with the chip's original design
intent, which better aligns with the original design.

Regarding suspend/resume operations and power saving, we have implemented `atomic_enable` and `atomic_disable` for CRTC.
These hook functions configure hsync and vsync for the DPMS switch[1]. Moving the `DISPLAY_CONTROL_HISILE.PANELDATE`
enable flag to the initialization phase does not affect power saving.

[1] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c#L190

Thanks,
Yongbang.

> 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,
>