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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 28.04.26 um 09:55 schrieb Yongbang Shi:
>> Hi
>>
>> Am 23.04.26 um 08:32 schrieb Yongbang Shi:
>>> From: Lin He <helin52@huawei.com>
>>>
>>> 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@huawei.com>
>>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>>> ---
>>> ChangeLog:
>>> v1 -> v2:
>>>     - remove tag "Reviewed-by: Tao Tian <tiantao6@hisilicon.com>", 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.

Sounds good. Thanks for following up on this.

Best regards
Thomas

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

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