Hi,
On 13.12.2022 11:40, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski
> <m.szyprowski@xxxxxxxxxxx> wrote:
>> On 12.12.2022 16:33, Jagan Teki wrote:
>>
>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
>> <m.szyprowski@xxxxxxxxxxx> wrote:
>>
>> On 12.12.2022 09:43, Marek Szyprowski wrote:
>>
>> On 12.12.2022 09:32, Jagan Teki wrote:
>>
>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski
>> <m.szyprowski@xxxxxxxxxxx> wrote:
>>
>> Hi Jagan,
>>
>> On 09.12.2022 16:23, Jagan Teki wrote:
>>
>> The existing drm panels and bridges in Exynos required host
>> initialization during the first DSI command transfer even though
>> the initialization was done before.
>>
>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED
>> flag and triggers from host transfer.
>>
>> Do this exclusively for Exynos.
>>
>> Initial logic is derived from Marek Szyprowski changes.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>> Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> Changes from v9:
>> - derived from v8
>> - added comments
>>
>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++-
>> include/drm/bridge/samsung-dsim.h | 5 +++--
>> 2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> The following chunk is missing compared to v8:
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index 6e9ad955ebd3..6a9403cb92ae 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim
>> *dsi, unsigned int flag)
>> return 0;
>>
>> samsung_dsim_reset(dsi);
>> - samsung_dsim_enable_irq(dsi);
>> +
>> + if (!(dsi->state & DSIM_STATE_INITIALIZED))
>> + samsung_dsim_enable_irq(dsi);
>>
>> Is this really required? does it make sure that the IRQ does not
>> enable twice?
>>
>> That's what that check does. Without the 'if (!(dsi->state &
>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first
>> from pre_enable, then from the first transfer), what leads to a
>> warning from irq core.
>>
>> I've just noticed that we also would need to clear the
>> DSIM_STATE_REINITIALIZED flag in dsim_suspend.
>>
>> However I've found that a bit simpler patch would keep the current code
>> flow for Exynos instead of this reinitialization hack. This can be
>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host
>> init in pre_enable" patch:
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index 0b2e52585485..acc95c61ae45 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -1291,9 +1291,11 @@ static void samsung_dsim_atomic_pre_enable(struct
>> drm_bridge *bridge,
>>
>> dsi->state |= DSIM_STATE_ENABLED;
>>
>> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
>> - if (ret)
>> - return;
>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
>> + if (ret)
>> + return;
>> + }
>>
>> Sorry, I don't understand this. Does it mean Exynos doesn't need to
>> init host in pre_enable? If I remember correctly even though the host
>> is initialized it has to reinitialize during the first transfer - This
>> is what the Exynos requirement is. Please correct or explain here.
>>
>> This is a matter of enabling power regulator(s) in the right order and doing the host initialization in the right moment. It was never a matter of re-initialization. See the current code for the reference (it uses the same approach as my above change). I've already explained that here:
>>
>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@xxxxxxxxxxx/
>>
>> If you would like to see the exact proper moment of the dsi host initialization on the Exynos see the code here:
>>
>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework and patches adding mipi_dsi_host_init() to panel/bridge drivers.
> As I said before, the downstream bridge needs an explicit call to host
> init via mipi_dsi_host_init - this is indeed not a usual use-case
> scenario. Let's handle this with a REINIT fix and see if we can update
> this later to handle both scenarios.
>
> Would you please test this repo, I have included all.
>
> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10
This doesn't work on TM2e board. Give me some time to find why...
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
