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 60EC8C83F12 for ; Mon, 28 Aug 2023 08:50:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229734AbjH1IuI (ORCPT ); Mon, 28 Aug 2023 04:50:08 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43798 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229742AbjH1Itr (ORCPT ); Mon, 28 Aug 2023 04:49:47 -0400 Received: from mail-wr1-x436.google.com (mail-wr1-x436.google.com [IPv6:2a00:1450:4864:20::436]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1D41CBF for ; Mon, 28 Aug 2023 01:49:44 -0700 (PDT) Received: by mail-wr1-x436.google.com with SMTP id ffacd0b85a97d-313e742a787so1803025f8f.1 for ; Mon, 28 Aug 2023 01:49:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1693212582; x=1693817382; h=content-transfer-encoding:in-reply-to:organization:autocrypt :references:cc:to:content-language:subject:reply-to:from:user-agent :mime-version:date:message-id:from:to:cc:subject:date:message-id :reply-to; bh=yKPKRFt3ZyH53OPo/QehnO2mWQmnGuhYdCnoCPtQqT8=; b=HWri3WcVTAbMdx2b7sBU50yDwWNYBUAZvSWEiC2Tamlx7k1/66NtShJkD7AjsFN76B 07SEy4y8scVccuL87i8RI1Uq8ImU9ZBPzg6TGGqF27H9jOV0qNs9T7vVPAf1QLobUm20 uLXj7KK/GVQKj7VvB2gZVe+jVn620nx5gg0YCsEpxn7JGSs0LYPNX8asixJuwfCUm0Uj CDBrOC4u6IM1yLKZUD6gpBw5F0NiDwH5x3pkeFjtSFGGD5o59Fd4X9oVmjEuZKKwoDQz V+DJVh9P3ePb2Qz7eORLumPgJLCVygB6CRLePQqECed5eg3wDXAjH179bU4k/0SXJDvy wuUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693212582; x=1693817382; h=content-transfer-encoding:in-reply-to:organization:autocrypt :references:cc:to:content-language:subject:reply-to:from:user-agent :mime-version:date:message-id:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=yKPKRFt3ZyH53OPo/QehnO2mWQmnGuhYdCnoCPtQqT8=; b=K/MEO/UvYYGR80dYMRw2D0LyZ0b1q2Se4+8zqRnES54SNZKDbSyCk1JcNgQdAAPl4M uvnsVoY6NkOLbrt2Hfs+VBibF9Ej7GjZF4lA0HfRsKOMLLlZCzBRt78sjc9KbNYi+XjJ iJ0mUkFxRyIfseE22Fc18f6EdggW1VeV2Uc7g9VauXUqX9i5IiSf9lJy8+XrYpt8MuKM GhauKdGDD6YwyZyVcT4uUpWI29gf2Dxp2VXVCgX50gU8L2XFMffNXB7CXxqFY0nIJvrr Yn1IPOE9PPyMqonB8ADvbDhmxVFMaute3ca8135Y4xPje8wOvXcHBE6LsGzIsrDd/H0p Wljg== X-Gm-Message-State: AOJu0YyEvWwnLYNpjqzC7XNOtD+qS602Ff0YDICGqBu3ftE9KYMXnzlV PkfEh7jnf92rZLjqRhTaBcyOXA== X-Google-Smtp-Source: AGHT+IF/I4tMWHM/TDa+Cw3UZa69daV4EtqQ7nL/k6vgM9WXygXYJlpHEuOTlNM10qFzqZNUJgiRJw== X-Received: by 2002:a5d:6188:0:b0:319:55f9:d50d with SMTP id j8-20020a5d6188000000b0031955f9d50dmr20013359wru.33.1693212582461; Mon, 28 Aug 2023 01:49:42 -0700 (PDT) Received: from ?IPV6:2a01:e0a:982:cbb0:5e59:c27b:747a:3f52? ([2a01:e0a:982:cbb0:5e59:c27b:747a:3f52]) by smtp.gmail.com with ESMTPSA id x16-20020a5d6510000000b003143c9beeaesm9877481wru.44.2023.08.28.01.49.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 28 Aug 2023 01:49:41 -0700 (PDT) Message-ID: <0cb96702-b396-4223-870f-b798d32991ee@linaro.org> Date: Mon, 28 Aug 2023 10:49:40 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: neil.armstrong@linaro.org Reply-To: neil.armstrong@linaro.org Subject: Re: [PATCH] drm/panel: Add prepare_prev_first flag to Visionox VTDR6130 Content-Language: en-US, fr To: Jessica Zhang , Maxime Ripard Cc: Dmitry Baryshkov , Abhinav Kumar , Sam Ravnborg , David Airlie , Daniel Vetter , Douglas Anderson , Rob Clark , Maarten Lankhorst , Thomas Zimmermann , quic_parellan@quicinc.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20230725-visionox-vtdr-prev-first-v1-1-3bc44cec7dc6@quicinc.com> <2f9a9450-438b-257d-759c-22b273a7b35d@quicinc.com> <6c0dd9fd-5d8e-537c-804f-7a03d5899a07@linaro.org> <548b0333-103b-ac66-0fc5-f29e7cc50596@quicinc.com> <6e1f6cb7-7f88-48dc-b494-1a5e990e1a33@linaro.org> <76e76728-974e-46ff-8046-c61c54d07c76@linaro.org> <54b37d60-61b1-e939-c71d-30aecad65598@quicinc.com> Autocrypt: addr=neil.armstrong@linaro.org; keydata= xsBNBE1ZBs8BCAD78xVLsXPwV/2qQx2FaO/7mhWL0Qodw8UcQJnkrWmgTFRobtTWxuRx8WWP GTjuhvbleoQ5Cxjr+v+1ARGCH46MxFP5DwauzPekwJUD5QKZlaw/bURTLmS2id5wWi3lqVH4 BVF2WzvGyyeV1o4RTCYDnZ9VLLylJ9bneEaIs/7cjCEbipGGFlfIML3sfqnIvMAxIMZrvcl9 qPV2k+KQ7q+aXavU5W+yLNn7QtXUB530Zlk/d2ETgzQ5FLYYnUDAaRl+8JUTjc0CNOTpCeik 80TZcE6f8M76Xa6yU8VcNko94Ck7iB4vj70q76P/J7kt98hklrr85/3NU3oti3nrIHmHABEB AAHNKk5laWwgQXJtc3Ryb25nIDxuZWlsLmFybXN0cm9uZ0BsaW5hcm8ub3JnPsLAkQQTAQoA OwIbIwULCQgHAwUVCgkICwUWAgMBAAIeAQIXgBYhBInsPQWERiF0UPIoSBaat7Gkz/iuBQJk Q5wSAhkBAAoJEBaat7Gkz/iuyhMIANiD94qDtUTJRfEW6GwXmtKWwl/mvqQtaTtZID2dos04 YqBbshiJbejgVJjy+HODcNUIKBB3PSLaln4ltdsV73SBcwUNdzebfKspAQunCM22Mn6FBIxQ GizsMLcP/0FX4en9NaKGfK6ZdKK6kN1GR9YffMJd2P08EO8mHowmSRe/ExAODhAs9W7XXExw UNCY4pVJyRPpEhv373vvff60bHxc1k/FF9WaPscMt7hlkbFLUs85kHtQAmr8pV5Hy9ezsSRa GzJmiVclkPc2BY592IGBXRDQ38urXeM4nfhhvqA50b/nAEXc6FzqgXqDkEIwR66/Gbp0t3+r yQzpKRyQif3OwE0ETVkGzwEIALyKDN/OGURaHBVzwjgYq+ZtifvekdrSNl8TIDH8g1xicBYp QTbPn6bbSZbdvfeQPNCcD4/EhXZuhQXMcoJsQQQnO4vwVULmPGgtGf8PVc7dxKOeta+qUh6+ SRh3vIcAUFHDT3f/Zdspz+e2E0hPV2hiSvICLk11qO6cyJE13zeNFoeY3ggrKY+IzbFomIZY 4yG6xI99NIPEVE9lNBXBKIlewIyVlkOaYvJWSV+p5gdJXOvScNN1epm5YHmf9aE2ZjnqZGoM Mtsyw18YoX9BqMFInxqYQQ3j/HpVgTSvmo5ea5qQDDUaCsaTf8UeDcwYOtgI8iL4oHcsGtUX oUk33HEAEQEAAcLAXwQYAQIACQUCTVkGzwIbDAAKCRAWmrexpM/4rrXiB/sGbkQ6itMrAIfn M7IbRuiSZS1unlySUVYu3SD6YBYnNi3G5EpbwfBNuT3H8//rVvtOFK4OD8cRYkxXRQmTvqa3 3eDIHu/zr1HMKErm+2SD6PO9umRef8V82o2oaCLvf4WeIssFjwB0b6a12opuRP7yo3E3gTCS KmbUuLv1CtxKQF+fUV1cVaTPMyT25Od+RC1K+iOR0F54oUJvJeq7fUzbn/KdlhA8XPGzwGRy 4zcsPWvwnXgfe5tk680fEKZVwOZKIEuJC3v+/yZpQzDvGYJvbyix0lHnrCzq43WefRHI5XTT QbM0WUIBIcGmq38+OgUsMYu4NzLu7uZFAcmp6h8g Organization: Linaro Developer Services In-Reply-To: <54b37d60-61b1-e939-c71d-30aecad65598@quicinc.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jessica, On 25/08/2023 20:37, Jessica Zhang wrote: > > > On 8/21/2023 3:01 AM, neil.armstrong@linaro.org wrote: >> Hi Maxime, >> >> On 21/08/2023 10:17, Maxime Ripard wrote: >>> Hi, >>> >>> On Fri, Aug 18, 2023 at 10:25:48AM +0200, neil.armstrong@linaro.org wrote: >>>> On 17/08/2023 20:35, Dmitry Baryshkov wrote: >>>>> On 16/08/2023 10:51, neil.armstrong@linaro.org wrote: >>>>>> Sending HS commands will always work on any controller, it's all >>>>>> about LP commands. The Samsung panels you listed only send HS >>>>>> commands so they can use prepare_prev_first and work on any >>>>>> controllers. >>>>> >>>>> I think there is some misunderstanding there, supported by the >>>>> description of the flag. >>>>> >>>>> If I remember correctly, some hosts (sunxi) can not send DCS >>>>> commands after enabling video stream and switching to HS mode, see >>>>> [1]. Thus, as you know, most of the drivers have all DSI panel setup >>>>> commands in drm_panel_funcs::prepare() / >>>>> drm_bridge_funcs::pre_enable() callbacks, not paying attention >>>>> whether these commands are to be sent in LP or in HS mode. >>>>> >>>>> Previously DSI source drivers could power on the DSI link either in >>>>> mode_set() or in pre_enable() callbacks, with mode_set() being the >>>>> hack to make panel/bridge drivers to be able to send commands from >>>>> their prepare() / pre_enable() callbacks. >>>>> >>>>> With the prev_first flags being introduced, we have established that >>>>> DSI link should be enabled in DSI host's pre_enable() callback and >>>>> switched to HS mode (be it command or video) in the enable() >>>>> callback. >>>>> >>>>> So far so good. >>>> >>>> It seems coherent, I would like first to have a state of all DSI host >>>> drivers and make this would actually work first before adding the >>>> prev_first flag to all the required panels. >>> >>> This is definitely what we should do in an ideal world, but at least for >>> sunxi there's no easy way for it at the moment. There's no documentation >>> for it and the driver provided doesn't allow this to happen. >>> >>> Note that I'm not trying to discourage you or something here, I'm simply >>> pointing out that this will be something that we will have to take into >>> account. And it's possible that other drivers are in a similar >>> situation. >>> >>>>> Unfortunately this change is not fully backwards-compatible. This >>>>> requires that all DSI panels sending commands from prepare() should >>>>> have the prepare_prev_first flag. In some sense, all such patches >>>>> might have Fixes: 5ea6b1702781 ("drm/panel: Add prepare_prev_first >>>>> flag to drm_panel"). >>>> >>>> This kind of migration should be done *before* any possible >>>> regression, not the other way round. >>>> >>>> If all panels sending commands from prepare() should have the >>>> prepare_prev_first flag, then it should be first, check for >>>> regressions then continue. >>>> >>>> >>>> >>>>>> >>>>>> I understand, but this patch doesn't qualify as a fix for >>>>>> 9e15123eca79 and is too late to be merged in drm-misc-next for >>>>>> v6.6, and since 9e15123eca79 actually breaks some support it >>>>>> should be reverted (+ deps) since we are late in the rc cycles. >>>>> >>>>> If we go this way, we can never reapply these patches. There will be >>>>> no guarantee that all panel drivers are completely converted. We >>>>> already have a story without an observable end - >>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >>>> >>>> I don't understand this point, who would block re-applying the patches ? >>>> >>>> The migration to DRM_BRIDGE_ATTACH_NO_CONNECTOR was done over multiple >>>> Linux version and went smoothly because we reverted regressing patches >>>> and restarted when needed, I don't understand why we can't do this >>>> here aswell. >>>> >>>>> I'd consider that the DSI driver is correct here and it is about the >>>>> panel drivers that require fixes patches. If you care about the >>>>> particular Fixes tag, I have provided one several lines above. >>>> >>>> Unfortunately it should be done in the other way round, prepare for >>>> migration, then migrate, >>>> >>>> I mean if it's a required migration, then it should be done and I'll >>>> support it from both bridge and panel PoV. >>>> >>>> So, first this patch has the wrong Fixes tag, and I would like a >>>> better explanation on the commit message in any case. Then I would >>>> like to have an ack from some drm-misc maintainers before applying it >>>> because it fixes a patch that was sent via the msm tree thus per the >>>> drm-misc rules I cannot apply it via the drm-misc-next-fixes tree. >>> >>> Sorry, it's not clear to me what you'd like our feedback on exactly? >> >> So let me resume the situation: >> >> - pre_enable_prev_first was introduced in [1] >> - some panels made use of pre_enable_prev_first >> - Visionox VTDR6130 was enabled on SM8550 systems and works on v6.5 kernels and before >> - patch [2] was introduced on MSM DRM tree, breaking VTDR6130 on SM8550 systems (and probably other Video mode panels on Qcom platforms) >> - this fix was sent late, and is now too late to be merged via drm-misc-next > > Hi Neil and Maxime, > > I agree with Neil that 9e15123eca79 was the commit that introduced the issue (since it changed the MSM DSI host behavior). > > However, I'm not too keen on simply reverting that patch because > > 1) it's not wrong to have the dsi_power_on in pre_enable. Arguably, it actually makes more sense to power on DSI host in pre_enable than in modeset (since modeset is meant for setting the bridge mode), and I never objected that, it's the right path to go. > > 2) I think it would be good practice to keep specific bridge chip checks out of the DSI host driver. We discussed about a plan with Maxime and Dmitry about that, and it would require adding a proper atomic panel API to handle a "negociation" with the host controller. > > > That being said, what do you think about setting the default value of prepare_prev_first to true (possibly in panel_bridge_attach)? As Dmitry pointed, all panels sending LP commands in pre_enable() should have prepare_prev_first to true. > > It seems to me that most panel drivers send DCS commands during pre_enable, so maybe it would make more sense to power on DSI host before panel enable() by default. Any panel that needs DSI host to be powered on later could then explicitly set the flag to false in their respective drivers. A proper migration should be done, yes, but not as a fix on top of v6.5. Neil > > Thanks, > > Jessica Zhang > > >> >> I do not consider it's the right way to fix regression caused by [2] >> I consider [2] should be reverted, panels migrated to pre_enable_prev_first when needed, tested and the [2] applied again >> >> I have no objection about [2] and it should be done widely over the whole DSI controllers >> and DSI Video panels. >> >> I also object about the Fixes tag of this patch, which is wrong, and Dmitry considers [1] >> should be used but it's even more wrong since [2] really caused the regression. >> >> And if [2] was to correct one to use, it was pushed via the MSM tree so it couldn't be >> applied via drm-misc-next-fixes, right ? >> >> [1] 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") >> [2] 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset") >> >> Thanks, >> Neil >> >>> >>> Maxime >>