Message-ID: <19ff3766-a73e-761d-8965-015817fa9c0c@quicinc.com>
Date:   Fri, 14 Jul 2023 17:59:03 -0700
From:   Jessica Zhang <quic_jesszhan@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC:     Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
        "Marijn Suijten" <marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>, <quic_abhinavk@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        <freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drm/msm/dsi: Enable DATABUS_WIDEN for DSI command mode



On 7/14/2023 3:30 PM, Dmitry Baryshkov wrote:
> On Fri, 14 Jul 2023 at 22:03, Jessica Zhang <quic_jesszhan@...cinc.com> wrote:
>>
>>
>>
>> On 7/13/2023 6:23 PM, Dmitry Baryshkov wrote:
>>> On 14/07/2023 03:21, Jessica Zhang wrote:
>>>> DSI 6G v2.5.x+ and DPU 7.x+ support a data-bus widen mode that allows DSI
>>>> to send 48 bits of compressed data per pclk instead of 24.
>>>>
>>>> For all chipsets that support this mode, enable it whenever DSC is
>>>> enabled as recommended by the hardware programming guide.
>>>>
>>>> Only enable this for command mode as we are currently unable to validate
>>>> it for video mode.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
>>>> ---
>>>> Note: The dsi.xml.h changes were generated using the headergen2 script in
>>>> envytools [2], but the changes to the copyright and rules-ng-ng source
>>>> file
>>>> paths were dropped.
>>>
>>> Separate commit please, so that it can be replaced by headers sync with
>>> Mesa3d.
>>
>> Hi Dmitry,
>>
>> Acked.
>>
>>>
>>>>
>>>> [1] https://patchwork.freedesktop.org/series/120580/
>>>> [2] https://github.com/freedreno/envytools/
>>>>
>>>> --
>>>> Changes in v2:
>>>> - Rebased on top of "drm/msm/dpu: Re-introduce dpu core revision"
>>>> - Squashed all commits to avoid breaking feature if the series is only
>>>> partially applied
>>>
>>> No. Please unsquash it. Please design the series so that the patches
>>> work even if it is only partially applied.
>>
>> Acked.
>>
>>>
>>>> - Moved DATABUS_WIDEN bit setting to dsi_ctr_enable() (Marijn)
>>>> - Have DPU check if wide bus is requested by output driver (Dmitry)
>>>> - Introduced bytes_per_pclk variable for dsi_timing_setup() hdisplay
>>>> adjustment (Marijn)
>>>> - Link to v1:
>>>> https://lore.kernel.org/r/20230525-add-widebus-support-v1-0-c7069f2efca1@quicinc.com
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 10 ++++++----
>>>>    .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  4 +++-
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        |  3 +++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  1 +
>>>>    drivers/gpu/drm/msm/dsi/dsi.c                      |  5 +++++
>>>>    drivers/gpu/drm/msm/dsi/dsi.h                      |  1 +
>>>>    drivers/gpu/drm/msm/dsi/dsi.xml.h                  |  1 +
>>>>    drivers/gpu/drm/msm/dsi/dsi_host.c                 | 23
>>>> +++++++++++++++++++++-
>>>>    drivers/gpu/drm/msm/msm_drv.h                      |  6 ++++++
>>>>    9 files changed, 48 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index f0a2a1dca741..6aed63c06c1d 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -2411,6 +2411,7 @@ struct drm_encoder *dpu_encoder_init(struct
>>>> drm_device *dev,
>>>>        struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
>>>>        struct drm_encoder *drm_enc = NULL;
>>>>        struct dpu_encoder_virt *dpu_enc = NULL;
>>>> +    int index = disp_info->h_tile_instance[0];
>>>>        int ret = 0;
>>>>        dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
>>>> @@ -2439,13 +2440,14 @@ struct drm_encoder *dpu_encoder_init(struct
>>>> drm_device *dev,
>>>>        timer_setup(&dpu_enc->frame_done_timer,
>>>>                dpu_encoder_frame_done_timeout, 0);
>>>> -    if (disp_info->intf_type == INTF_DSI)
>>>> +    if (disp_info->intf_type == INTF_DSI) {
>>>>            timer_setup(&dpu_enc->vsync_event_timer,
>>>>                    dpu_encoder_vsync_event_handler,
>>>
>>> While you are touching this part, could you please drop
>>> dpu_encoder_vsync_event_handler() and
>>> dpu_encoder_vsync_event_work_handler(), they are useless?
>>
>> Since these calls aren't related to widebus, I don't think I'll include
>> it in this series. However, I can post this cleanup as a separate patch
>> and add that as a dependency if that's ok.
> 
> Sure, that will work for me. Thank you!
> 
>>
>>>
>>>>                    0);
>>>> -    else if (disp_info->intf_type == INTF_DP)
>>>> -        dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
>>>> -                priv->dp[disp_info->h_tile_instance[0]]);
>>>> +        dpu_enc->wide_bus_en =
>>>> msm_dsi_is_widebus_enabled(priv->dsi[index]);
>>>> +    } else if (disp_info->intf_type == INTF_DP) {
>>>> +        dpu_enc->wide_bus_en =
>>>> msm_dp_wide_bus_available(priv->dp[index]);
>>>> +    }
>>>>        INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>>>                dpu_encoder_off_work);
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> index df88358e7037..dace6168be2d 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>>                    phys_enc->hw_intf,
>>>>                    phys_enc->hw_pp->idx);
>>>> -    if (intf_cfg.dsc != 0)
>>>> +    if (intf_cfg.dsc != 0) {
>>>>            cmd_mode_cfg.data_compress = true;
>>>> +        cmd_mode_cfg.wide_bus_en =
>>>> dpu_encoder_is_widebus_enabled(phys_enc->parent);
>>>> +    }
>>>>        if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>>>>
>>>> phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf,
>>>> &cmd_mode_cfg);
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> index 8ec6505d9e78..dc6f3febb574 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> @@ -521,6 +521,9 @@ static void
>>>> dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>>>>        if (cmd_mode_cfg->data_compress)
>>>>            intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>>> +    if (cmd_mode_cfg->wide_bus_en)
>>>> +        intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>>>> +
>>>>        DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>>>    }
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> index 77f80531782b..c539025c418b 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>>>>    struct dpu_hw_intf_cmd_mode_cfg {
>>>>        u8 data_compress;    /* enable data compress between dpu and dsi */
>>>> +    u8 wide_bus_en;        /* enable databus widen mode */
>>>>    };
>>>>    /**
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c
>>>> b/drivers/gpu/drm/msm/dsi/dsi.c
>>>> index baab79ab6e74..e3cc06c94397 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>>>> @@ -17,6 +17,11 @@ struct drm_dsc_config
>>>> *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>>>>        return msm_dsi_host_get_dsc_config(msm_dsi->host);
>>>>    }
>>>> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
>>>> +{
>>>> +    return msm_dsi_host_is_widebus_supported(msm_dsi->host);
>>>
>>> This is incorrect. It will enable widebus even for non-DSC cases.
>>
>> FWIW, all calls for msm_dsi_is_widebus_enabled() and
>> msm_dsi_host_is_widebus_supported() are guarded by a DSC check.
>>
>> That being said, I also see your point that msm_dsi_is_widebus_enabled()
>> is an incorrect name since this will only check if widebus is supported.
>>
>> Maybe a better change would be to change msm_dsi_is_widebus_enabled to
>> *_is_widebus_supported(), move the setting of dpu_enc->wide_bus_en for
>> both DP and DSI to dpu_encoder_virt_atomic_enable(), then for DSI set
>> wide_bus_en = dpu_enc->dsc && dsi_is_widebus_supported().
> 
> I think we should change msm_dp_wide_bus_available() to
> msm_dp_wide_bus_enabled(). We don't have a way to tell DP (or DSI) if
> widebus really should be enabled or not. So it would be better to make
> DP and DSI drivers provide is_widebus_enabled function.

I think making changes to the DP API is outside the scope of this series 
since these changes are focused on supporting widebus for DSI specifically.

> 
>>
>>>
>>>> +}
>>>> +
>>>>    static int dsi_get_phy(struct msm_dsi *msm_dsi)
>>>>    {
>>>>        struct platform_device *pdev = msm_dsi->pdev;
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h
>>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>>> index bd3763a5d723..219a9f756759 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>>> @@ -134,6 +134,7 @@ int dsi_calc_clk_rate_6g(struct msm_dsi_host
>>>> *msm_host, bool is_bonded_dsi);
>>>>    void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct
>>>> mipi_dsi_host *host);
>>>>    void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
>>>>    struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct
>>>> mipi_dsi_host *host);
>>>> +bool msm_dsi_host_is_widebus_supported(struct mipi_dsi_host *host);
>>>>    /* dsi phy */
>>>>    struct msm_dsi_phy;
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h
>>>> b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>>>> index a4a154601114..2a7d980e12c3 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>>>> @@ -664,6 +664,7 @@ static inline uint32_t
>>>> DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
>>>>        return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) &
>>>> DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
>>>>    }
>>>>    #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE            0x00010000
>>>> +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN            0x00100000
>>>>    #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL            0x000001b8
>>>>    #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK        0x0000003f
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index 645927214871..6ea3476acf0d 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -710,6 +710,14 @@ static void dsi_ctrl_disable(struct msm_dsi_host
>>>> *msm_host)
>>>>        dsi_write(msm_host, REG_DSI_CTRL, 0);
>>>>    }
>>>> +bool msm_dsi_host_is_widebus_supported(struct mipi_dsi_host *host)
>>>> +{
>>>> +    struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>>>> +
>>>> +    return msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
>>>> +            msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
>>>
>>> Would it be better to push it to the config data, like we did for DP?
>>
>> I don't think so -- I think adding it to the config data as a feature
>> flag would bloat dsi_cfg.c. It would be simpler and cleaner (IMO) to
>> keep this as a version check.
> 
> Ok.
> 
>>
>>>
>>>> +}
>>>> +
>>>>    static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
>>>>                struct msm_dsi_phy_shared_timings *phy_shared_timings,
>>>> struct msm_dsi_phy *phy)
>>>>    {
>>>> @@ -757,6 +765,11 @@ static void dsi_ctrl_enable(struct msm_dsi_host
>>>> *msm_host,
>>>>                msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3) {
>>>>                data = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
>>>>                data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
>>>> +
>>>> +            /* TODO: Allow for video-mode support once tested/fixed */
>>>> +            if (msm_host->cfg_hnd->minor >=
>>>> MSM_DSI_6G_VER_MINOR_V2_5_0 && msm_host->dsc)
>>>
>>> msm_dsi_is_widebus_enabled() && msm_host->dsc
>>
>> *_is_widebus_enabled() also checks for major version >= 6G, so calling
>> it here would be a bit redundant as we're already checking for that earlier.
> 
> However now you have different checks in two different places.
> 
> My suggestion is to have the function msm_dsi_is_widebus_supported(),
> which the rest of the code uses to check if wide bus is actually
> enabled in the HW. If at some point DSI 2.11 drops wide bus support,
> I'd like to see a change at a single place, rather than having changes
> all over the code.

Ok, I think this is a fair point. The main reason I just had the minor 
version check here was to avoid the redundant major version check from 
the BURST_MODE conditional [1].

What if we just move the MDP_CTRL2 configuration to a separate function 
and do the BURST_MODE and DATABUS_WIDEN checks separately?

So, something like:

```
bool burst_supported()
{
     return major >= 6g && minor >= 1.3;
}


bool widebus_supported()
{
     return major >= 6g && minor >= 2.5;
}


void program_cmd_mdp_ctrl2()
{
     if (major < 6g)
         return;

     data = read(mdp_ctrl2);
     if (burst_supported)
         data |= burst;
     if (widebus_supported)
         data |= widebus;

     write(mdp_ctrl2, data);
}

void dsi_ctrl_enable()
{

     ...

     program_cmd_mdp_ctrl2();

     ...
}

```

> 
> Likewise I'd like to have the function msm_dsi_is_widebus_enabled(),
> which is used by the rest of the code to check if the widebus should
> be actually enabled.
> 
> Actually I think we can even drop the is_supported at all() and use
> the following code:
> 
> bool msm_dsi_is_widebus_enabled()
> {
>     if (major < 6G || minor < V2_5_0)
>       return false;
> 
>     return !!msm_host->dsc;

I'd rather keep this API as *_is_widebus_supported() or 
*_is_widebus_available() (if you'd like it to match DP) and have the DSC 
check outside.

This is because we already guard for DSC in DSI for the 
dsi_timing_setup() hdisplay adjustments and for the DPU hw_intf widebus 
register configuration. The only place where a DSC check needed to be 
added was for the MDP_CTRL2 configuration.

So, since most places can already use preexisting DSC checks, having 
another DSC check within msm_dsi_is_widebus_enabled() would either be 
redundant or require extensive refactoring to justify adding the check here.

Thanks,

Jessica Zhang

> }
> 
> Then the rest of the code should call this function only.
> 
>>
>> FWIW, I've nested the widebus configuration within the burst mode
>> configuration to keep it so that we only have to read/write the
>> MDP_CTRL2 once.
>>
>>>
>>>> +                data |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
>>>> +
>>>>                dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, data);
>>>>            }
>>>>        }
>>>> @@ -894,6 +907,7 @@ static void dsi_timing_setup(struct msm_dsi_host
>>>> *msm_host, bool is_bonded_dsi)
>>>>        u32 hdisplay = mode->hdisplay;
>>>>        u32 wc;
>>>>        int ret;
>>>> +    bool widebus_supported =
>>>> msm_dsi_host_is_widebus_supported(&msm_host->base);
>>>
>>> s/supported/enabled for this function.
>>
>> I would like to keep the name as *_is_widebus_supported() since it
>> better reflects the functionality of the helper.
>>
>> FWIW, the widebus hdisplay adjustments are already guarded by a DSC check.
> 
> See my previous comment.
> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>>        DBG("");
>>>> @@ -914,6 +928,7 @@ static void dsi_timing_setup(struct msm_dsi_host
>>>> *msm_host, bool is_bonded_dsi)
>>>>        if (msm_host->dsc) {
>>>>            struct drm_dsc_config *dsc = msm_host->dsc;
>>>> +        u32 bytes_per_pclk;
>>>>            /* update dsc params with timing params */
>>>>            if (!dsc || !mode->hdisplay || !mode->vdisplay) {
>>>> @@ -937,7 +952,13 @@ static void dsi_timing_setup(struct msm_dsi_host
>>>> *msm_host, bool is_bonded_dsi)
>>>>             * pulse width same
>>>>             */
>>>>            h_total -= hdisplay;
>>>> -        hdisplay =
>>>> DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>>>> +        if (widebus_supported && !(msm_host->mode_flags &
>>>> MIPI_DSI_MODE_VIDEO))
>>>> +            bytes_per_pclk = 6;
>>>> +        else
>>>> +            bytes_per_pclk = 3;
>>>> +
>>>> +        hdisplay =
>>>> DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
>>>> +
>>>>            h_total += hdisplay;
>>>>            ha_end = ha_start + hdisplay;
>>>>        }
>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h
>>>> b/drivers/gpu/drm/msm/msm_drv.h
>>>> index 9d9d5e009163..7ff56d09014c 100644
>>>> --- a/drivers/gpu/drm/msm/msm_drv.h
>>>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>>>> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state
>>>> *disp_state, struct msm_dsi *msm_dsi
>>>>    bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
>>>>    bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>>>>    bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
>>>> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
>>>>    struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
>>>>    #else
>>>>    static inline void __init msm_dsi_register(void)
>>>> @@ -374,6 +375,11 @@ static inline bool msm_dsi_is_master_dsi(struct
>>>> msm_dsi *msm_dsi)
>>>>        return false;
>>>>    }
>>>> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
>>>> +{
>>>> +    return false;
>>>> +}
>>>> +
>>>>    static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct
>>>> msm_dsi *msm_dsi)
>>>>    {
>>>>        return NULL;
>>>>
>>>> ---
>>>> base-commit: 9445fc2942a890e84c74e170ebd7dfb9566e3357
>>>> change-id: 20230525-add-widebus-support-f785546ee751
>>>>
>>>> Best regards,
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>>
> 
> 
> 
> -- 
> With best wishes
> Dmitry

Powered by blists - more mailing lists