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: <ca9cf8b1-7a2d-dee5-5ce4-2ac338b2ecb5@linaro.org>
Date:   Wed, 19 Jul 2023 01:00:16 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v2] drm/msm/dsi: Enable DATABUS_WIDEN for DSI command mode
Content-Language: en-GB
To:     Jessica Zhang <quic_jesszhan@quicinc.com>
Cc:     freedreno@lists.freedesktop.org, quic_abhinavk@quicinc.com,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org,
        Marijn Suijten <marijn.suijten@somainline.org>,
        Sean Paul <sean@poorly.run>
References: <20230713-add-widebus-support-v2-1-ad0added17b6@quicinc.com>
 <91bc6348-2030-85dd-1492-1609b392793f@linaro.org>
 <05996344-0e43-7f37-c99a-42c04f91dc83@quicinc.com>
 <CAA8EJppFDcrVdegskSD0TJPOdSzVw_50+Bq+u8LKn26jdKE=tw@mail.gmail.com>
 <19ff3766-a73e-761d-8965-015817fa9c0c@quicinc.com>
 <637e4979-cadc-2167-a14f-25363c9da990@linaro.org>
 <2804b7c7-0ed2-6490-3316-7a38eb1262f8@quicinc.com>
 <d7b93f39-e866-899b-578f-17f7bdd5bdd5@linaro.org>
 <b2b044f7-9ddd-1d68-4eae-d5670e70cc8c@quicinc.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
In-Reply-To: <b2b044f7-9ddd-1d68-4eae-d5670e70cc8c@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 18/07/2023 23:44, Jessica Zhang wrote:
> 
> 
> On 7/17/2023 11:04 PM, Dmitry Baryshkov wrote:
>> On 18/07/2023 03:30, Jessica Zhang wrote:
>>>
>>>
>>> In addition, *_is_widebus_enabled() would only work under the 
>>> assumption that DSC (if supported) will always be enabled during 
>>> bootup for DSI. If there ends up being support for a panel that 
>>> allows for DSC to be enabled on the fly after bootup, the proposed 
>>> *_is_widebus_enabled() check would break since the assumption that 
>>> DSC is always enabled on bootup would not be true anymore.
>>
>> Why is that going to break?
> 
> 
> Currently, the msm_host->dsc is initialized in attach() [1] and we do 
> widebus setup during power_on() [2]. So we can assume that if the panel 
> supports DSC, msm_host->dsc will be not NULL during power_on() and the 
> widebus setup will work as expected.
> 
> However, if a panel supports enabling DSC after bootup, then 
> msm_host->dsc will be set later within the commit enable() path meaning 
> the necessary widebus setup during power_on() would *not* happen (as 
> *_is_widebus_enabled() would return false on account of msm_host->dsc == 
> NULL during power_on()).


Minor corrections:
- power_on() happens during pre_enable() stage,
- the dynamic msm_host->dsc can also be set during pre_enable() stage.

Note: there still exists dsi_mgr_bridge_mode_set(), which can also be 
used to set msm_host->dsc (however I would prefer to drop mode_set() 
completely and set the mode from atomic_enable().

I'd say, this is the minor issue. The dsi_timing_setup() is plagued with 
if (dsc) checks, so if we make DSC dynamic, it will have to be audited 
anyway.

> 
> Thanks,
> 
> Jessica Zhang
> 
> [1] 
> https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1587
> 
> [2] 
> https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L2359
> 
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry