From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4D5B4C433FE for ; Mon, 29 Nov 2021 18:38:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1352475AbhK2Slo (ORCPT ); Mon, 29 Nov 2021 13:41:44 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36856 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1379514AbhK2Sjj (ORCPT ); Mon, 29 Nov 2021 13:39:39 -0500 Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0148FC12B6B2; Mon, 29 Nov 2021 06:57:03 -0800 (PST) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: kholk11) with ESMTPSA id E7DCC1F44873 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=collabora.com; s=mail; t=1638197821; bh=diE9jAw9lUIQQd/ekZxakvexYtMLMupgXaiQfjW+cc0=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=UO/jvS33cB5wBTB4cnZqRx3JAgkN85LgS7IOfCDwj4kYSxdjrk4D4opmiWmxHmwpe 9V6FyjGLxQnjZhGqB7LAVFNl5PqtMkDUpsjUZykYr0bKNosMbGGIlBthGwXFXPVDeZ 9mitBOEq70eDAdYOj7uvJzbLmy8bTEg2PF/WdQkZBIgfND7dNAd5q2n0qkznOh8kB/ Oli2bJO55R53FmGMtzq55YIhnWPv8ssN9BRruJYhKz1KBoOodEpar2Td1cil4Ml2mJ Ak0Rb0E53FCmTpTDeQA2KU+4QcNH1k2wQc5koF1zcfScyevVnrmkqYUne2QMSCUYaL Xy7HFlYLJNUhA== Subject: Re: [PATCH] drm/msm: Initialize MDSS irq domain at probe time To: Dmitry Baryshkov Cc: robdclark@gmail.com, sean@poorly.run, airlied@linux.ie, daniel@ffwll.ch, maxime@cerno.tech, linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel@collabora.com, konrad.dybcio@somainline.org, marijn.suijten@somainline.org, jami.kettunen@somainline.org References: <20211125150947.354076-1-angelogioacchino.delregno@collabora.com> <9a0158ae-a3b1-21b2-1ba3-82d4901eb873@collabora.com> From: AngeloGioacchino Del Regno Message-ID: <4e395a6f-e174-ca5c-4fce-197dc69cd185@collabora.com> Date: Mon, 29 Nov 2021 15:56:57 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Il 29/11/21 15:53, Dmitry Baryshkov ha scritto: > Hi, > > On Mon, 29 Nov 2021 at 17:15, AngeloGioacchino Del Regno > 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 >>> >>> 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