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;
Message-ID: <392b933f-760c-3c81-1040-c514045df3da@linaro.org>
Date:   Sat, 2 Apr 2022 13:37:44 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Content-Language: en-GB
To:     Doug Anderson <dianders@chromium.org>,
        Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Cc:     dri-devel <dri-devel@lists.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        freedreno <freedreno@lists.freedesktop.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, Rob Clark <robdclark@gmail.com>,
        Sean Paul <seanpaul@chromium.org>,
        Stephen Boyd <swboyd@chromium.org>,
        quic_kalyant <quic_kalyant@quicinc.com>,
        "Abhinav Kumar (QUIC)" <quic_abhinavk@quicinc.com>,
        "Kuogee Hsieh (QUIC)" <quic_khsieh@quicinc.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        quic_vproddut <quic_vproddut@quicinc.com>,
        quic_aravindh@quicinc.com
References: <1648656179-10347-1-git-send-email-quic_sbillaka@quicinc.com>
 <1648656179-10347-2-git-send-email-quic_sbillaka@quicinc.com>
 <CAD=FV=X+QvjwoT2zGP82KW4kD0oMUY6ZgCizSikNX_Uj8dNDqA@mail.gmail.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
In-Reply-To: <CAD=FV=X+QvjwoT2zGP82KW4kD0oMUY6ZgCizSikNX_Uj8dNDqA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 01/04/2022 02:22, Doug Anderson wrote:
> Hi,
> 
> On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
>>
>> @@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>
>>          dp_display->encoder = encoder;
>>
>> +       ret = dp_display_get_next_bridge(dp_display);
>> +       if (ret)
>> +               return ret;
> 
> It feels weird to me that this is in a function called "modeset_init",
> though I certainly don't know the structure of the MSM display code
> well enough to fully comment.

It's called modeset_init() as it initializes KMS objects used by DP 
driver. We have similar functions for dsi and hdmi

> My expectation would have been that
> devm_of_dp_aux_populate_ep_devices() would have been called from your
> probe routine and then you would have returned -EPROBE_DEFER from your
> probe if you were unable to find the panel afterwards.

I don't think it's possible to call it from probe() since 
drm_dp_aux_register() is called only from dp_display_bind().
The PHY also isn't initialized at that moment, so we can not probe AUX 
devices.

The overall semantics of the AUX bus is not clear to me.
Typically the bus is populated (and probed) when devices are accessible. 
But for the display related buses this might not be the case.
For example for the DSI bus we clearly define that DSI transfer are not 
possible before the corresponding bridge's (or panel's) enable call.

Maybe the same approach should be adopted for the AUX bus. This would 
allow us to populate the AUX bus before hardware access is actually 
possible, thus creating all the DRM bridges before the hardware is 
actually up and running.

> Huh, but I guess you _are_ getting called (indirectly) from
> dpu_kms_hw_init() and I can't imagine AUX transfers working before
> that function is called, so maybe I should just accept that it's
> complicated and let those who understand this driver better confirm
> that it's OK. ;-)
> 
> 
>> @@ -140,5 +140,6 @@ struct dp_parser {
>>    * can be parsed using this module.
>>    */
>>   struct dp_parser *dp_parser_get(struct platform_device *pdev);
>> +int dp_parser_find_next_bridge(struct dp_parser *parser);
> 
> Everything else in this file is described w/ kerneldoc. Shouldn't your
> function also have a kerneldoc comment?

-- 
With best wishes
Dmitry