Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1685464318-25031-1-git-send-email-quic_khsieh@quicinc.com>
 <1685464318-25031-3-git-send-email-quic_khsieh@quicinc.com>
 <dfa12c8b-ccec-261c-9c83-54536e17c02d@linaro.org> <157e8219-7af2-c7ed-6d99-3caa6fbc11ba@quicinc.com>
 <CAA8EJponkEne2vVsNP=2Fxmv=Uc6i_LzAGBSEz9hPPotCEpGzg@mail.gmail.com> <e69f02b7-eba9-5f33-5ca1-eb0638928414@quicinc.com>
In-Reply-To: <e69f02b7-eba9-5f33-5ca1-eb0638928414@quicinc.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Wed, 31 May 2023 20:53:13 +0300
Message-ID: <CAA8EJpr9dfrrEsFf8heOvG3BWRTVCY-q1QYNH_3OBeMAWEwotA@mail.gmail.com>
Subject: Re: [PATCH v1 2/3] drm/msm/dpu: retrieve DSI DSC struct at atomic_check()
To:     Kuogee Hsieh <quic_khsieh@quicinc.com>
Cc:     dri-devel@lists.freedesktop.org, robdclark@gmail.com,
        sean@poorly.run, swboyd@chromium.org, dianders@chromium.org,
        vkoul@kernel.org, daniel@ffwll.ch, airlied@gmail.com,
        agross@kernel.org, andersson@kernel.org, quic_abhinavk@quicinc.com,
        quic_jesszhan@quicinc.com, quic_sbillaka@quicinc.com,
        marijn.suijten@somainline.org, freedreno@lists.freedesktop.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 31 May 2023 at 20:29, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 5/31/2023 10:12 AM, Dmitry Baryshkov wrote:
> > On Wed, 31 May 2023 at 18:41, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >>
> >>
> >>>>    +    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> >>> INTF_DSI
> >>>
> >>>> +        struct drm_bridge *bridge;
> >>>> +
> >>>> +        if (!dpu_enc->dsc) {
> >>> This condition is not correct. We should be updating the DSC even if
> >>> there is one.
> >>>
> >>>> +            bridge = drm_bridge_chain_get_first_bridge(drm_enc);
> >>>> +            dpu_enc->dsc = msm_dsi_bridge_get_dsc_config(bridge);
> >>> This approach will not work for the hot-pluggable outputs. The dpu_enc
> >>> is not a part of the state. It should not be touched before
> >>> atomic_commit actually commits changes.
> >> where can drm_dsc_config be stored?
> > I'd say, get it during atomic_check (and don't store it anywhere).
> > Then get it during atomic_enable (and save in dpu_enc).
> got it.
> >
> >>> Also, I don't think I like the API. It makes it impossible for the
> >>> driver to check that the bridge is the actually our DSI bridge or not.
> >>> Once you add DP here, the code will explode.
> >>>
> >>> I think instead we should extend the drm_bridge API to be able to get
> >>> the DSC configuration from it directly. Additional care should be put
> >>> to design an assymetrical API. Theoretically a drm_bridge can be both
> >>> DSC source and DSC sink. Imagine a DSI-to-DP or DSI-to-HDMI bridge,
> >>> supporting DSC on the DSI side too.
> >> Form my understanding, a bridge contains two interfaces.
> >>
> >> Therefore I would think only one bridge for dsi-to-dp bridge? and this
> >> bridge should represent the bridge chip?
> >>
> >> I am thinking adding an ops function, get_bridge_dsc() to struct
> >> drm_bridge_funcs to retrieve drm_dsc_config.
> > So, for this DSI-to-DP bridge will get_bridge_dsc() return DSC
> > configuration for  the DSI or for the DP side of the bridge?
>
> I would think should be DP side. there is no reason to enable dsc on
> both DSI and DP fro a bridge chip.

Well, there can be. E.g. to lower the clock rates of the DSI link.

>
> drm_bridge_chain_get_first_bridge(drm_enc) should return the bridge of
> the controller.
>
> In DSI-to-DP bridge chip case, this controller will be the bridge chip
> who configured to perform protocol conversion between DSI and DP.
>
> If DSC enabled should be at DP size which connect to panel.

Ok, so it returns the DSC configuration of the bridge's source side.
Now let's consider a panel bridge for the DSC-enabled DSI panel.
Should get_bridge_dsc() return a DSC config in this case?

> >> Do you have other suggestion?
> > Let me think about it for a few days.
-- 
With best wishes
Dmitry