Date:   Mon, 29 Nov 2021 15:56:57 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     robdclark@...il.com, sean@...rly.run, airlied@...ux.ie,
        daniel@...ll.ch, maxime@...no.tech, linux-arm-msm@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, kernel@...labora.com,
        konrad.dybcio@...ainline.org, marijn.suijten@...ainline.org,
        jami.kettunen@...ainline.org
Subject: Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time

Il 29/11/21 15:53, Dmitry Baryshkov ha scritto:
> Hi,
> 
> On Mon, 29 Nov 2021 at 17:15, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@...labora.com> wrote:
>>
>> Il 29/11/21 03:20, Dmitry Baryshkov ha scritto:
>>> Hi,
>>>
>>> On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
>>>> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
>>>> DSI host gets initialized earlier, but this caused unability to probe
>>>> the entire stack of components because they all depend on interrupts
>>>> coming from the main `mdss` node (mdp5, or dpu1).
>>>>
>>>> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
>>>> them at msm_pdev_probe() time: this will make sure that we add the
>>>> required interrupt controller mapping before dsi and/or other components
>>>> try to initialize, finally satisfying the dependency.
>>>>
>>>> While at it, also change the allocation of msm_drm_private to use the
>>>> devm variant of kzalloc().
>>>>
>>>> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>>>
>>> I have been thinking about this. I do not feel that this is the correct approach.
>>> Currently DRM device exists only when all components are bound. If any of the
>>> subdevices is removed, corresponding component is delteted (and thus all components
>>> are unbound), the DRM device is taken down. This results in the state cleanup,
>>> userspace notifications, etc.
>>>
>>> With your changes, DRM device will continue to exist even after one of subdevices
>>> is removed. This is not an expected behaviour, since subdrivers do not perform full
>>> cleanup, delegating that to DRM device takedown.
>>>
>>> I suppose that proper solution would be to split msm_drv.c into into:
>>> - generic components & drm code to be called from mdp4/mdp5/dpu driver (making
>>> mdp4, mdp5 or dpu1 the components master)
>>>
>>> - bare mdss driver, taking care only about IRQs, OF devices population - calling
>>> proper mdss_init/mdss_destroy functions. Most probably we can drop this part
>>> altogether and just make md5_mdss.c/dpu_mdss.c proper platform drivers.
>>>
>>
>>
>> Hmm... getting a better look on how things are structured... yes, I mostly agree
>> with you, though I'm not sure about making MDP{4,5}/DPU1 the component master; that
>> would result in a major change in drm-msm, which may be "a bit too much".
>>
>> Don't misunderstand me here, please, major changes are fine - but I feel urgency
>> to get this bug solved ASAP (since drm-msm is currently broken at least for drm
>> bridges) and, if we do anything major, that would require a very careful slow
>> review process that will leave this driver broken for a lot of time.
> 
> Yep. I wanted to hear your and Rob's opinion, that's why I did not
> rush into implementing it.
> I still think this is the way to go in the long term. What I really
> liked in your patchset was untying the knot between component binds
> returning EPROBE_DEFER and mdss subdevices being in place. This solved
> the "dsi host registration" infinite loop for me.
> 

Thanks. I'm also curious about Rob's opinion on this, as that'd be very valuable.

>>
>> I actually tried something else that "simplifies" the former approach, so here's
>> my proposal:
>> * we introduce {mdp5,dpu}_mdss_early_init(struct device, struct msm_drm_private)
>> * allocate only msm_drm_private in msm_pdev_probe, leaving the drm_dev_alloc call
>>     into msm_drm_init(), so that the drm_dev_put() stays in msm_drm_uninit()
>> * pass msm_drm_private as drvdata instead of drm_device
>> * change all the drvdata users to get drm_device from priv->dev, instead of getting
>>     msm_drm_private from drm_device->dev_private (like many other drm drivers are
>>     currently doing)
> 
> This sounds in a way that it should work. I'm looking forward to
> seeing (and testing) your patches.
> 

Alright then, I'm running some more tests and cleaning up the patches.
Expect a v2 between today and tomorrow at max! :))

>>
>> This way, we keep the current flow of creating the DRM device at msm_drm_init time
>> and tearing it down at msm_drm_unbind time, solving the issue that you are
>> describing.
>>
>> If you're okay with this kind of approach, I have two patches here that are 95%
>> ready, can finish them off and send briefly.
>>
>> Though, something else must be noted here... in the last mail where I'm pasting
>> a crash that happens when running 'rmmod panel_edp ti_sn65dsi86', I have implied
>> that this is happening due to the patch that I've sent: after some more research,
>> I'm not convinced anymore that this is a consequence of that. That crash may not
>> be related to my fix at all, but to something else (perhaps also related to commit
>> 8f59ee9a570c, the one that we're fixing here).
>>
>> Of course, that crash still happens even with the approach that I've just proposed.
>>
>>
>> Looking forward for your opinion!
>>
>> Cheers,
>> - Angelo
> 
> 
> 


-- 
AngeloGioacchino Del Regno
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

Powered by blists - more mailing lists