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 50FF7C433F5 for ; Tue, 26 Apr 2022 18:03:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1353661AbiDZSGw (ORCPT ); Tue, 26 Apr 2022 14:06:52 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:32966 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1349900AbiDZSGt (ORCPT ); Tue, 26 Apr 2022 14:06:49 -0400 Received: from alexa-out.qualcomm.com (alexa-out.qualcomm.com [129.46.98.28]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A1B9127CEC; Tue, 26 Apr 2022 11:03:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; i=@quicinc.com; q=dns/txt; s=qcdkim; t=1650996220; x=1682532220; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=aPIKkpibUxQQR3KJl7UUM0dwN+Ho/AxYVCKHrcAsEQc=; b=eOlUngWArIEVIlR2G1wQ1QJi1kUIiHLBmS7lI9KmRRoscjfA6FMj5Sp1 xAq/0GkajEjO6iH5dUZOhuiTAe77H7jUBix8O8YmMT6enZls4mpGyHqYS ZbfnVEIwTLRLEgYjvR1PjegP4jY2+x6w/N/4Sehm+FnaEb+loyktewZQR 4=; Received: from ironmsg08-lv.qualcomm.com ([10.47.202.152]) by alexa-out.qualcomm.com with ESMTP; 26 Apr 2022 11:03:39 -0700 X-QCInternal: smtphost Received: from nasanex01c.na.qualcomm.com ([10.47.97.222]) by ironmsg08-lv.qualcomm.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Apr 2022 11:03:38 -0700 Received: from nalasex01a.na.qualcomm.com (10.47.209.196) by nasanex01c.na.qualcomm.com (10.47.97.222) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.22; Tue, 26 Apr 2022 11:03:37 -0700 Received: from [10.111.160.161] (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.22; Tue, 26 Apr 2022 11:03:34 -0700 Message-ID: <9425d65b-85b1-6695-2a0b-95a1a46fb752@quicinc.com> Date: Tue, 26 Apr 2022 11:03:32 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.6.2 Subject: Re: [Freedreno] [PATCH] drm/msm/dp: move add fail safe mode to dp_connector_get_mode() Content-Language: en-US To: Doug Anderson , Dmitry Baryshkov CC: Sean Paul , Sankeerth Billakanti , David Airlie , linux-arm-msm , Stephen Boyd , dri-devel , LKML , Vinod Koul , Andy Gross , Bjorn Andersson , "Aravind Venkateswaran (QUIC)" , Kuogee Hsieh , freedreno References: <1650671124-14030-1-git-send-email-quic_khsieh@quicinc.com> <3b9588d2-d9f6-c96f-b316-953b56b59bfe@linaro.org> <73e2a37e-23db-d614-5f5c-8120f1869158@quicinc.com> <9b331b16-8d1b-4e74-8fee-d74c4041f8d7@quicinc.com> <9b4ccdef-c98a-b907-c7ee-a92456dc5bba@quicinc.com> <87a921db-c4bb-eb43-96c5-0bdb757c7df9@linaro.org> From: Abhinav Kumar In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nalasex01a.na.qualcomm.com (10.47.209.196) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 4/26/2022 10:56 AM, Doug Anderson wrote: > Hi, > > On Tue, Apr 26, 2022 at 10:44 AM Dmitry Baryshkov > wrote: >> >> On 26/04/2022 20:11, Doug Anderson wrote: >>> Hi, >>> >>> On Tue, Apr 26, 2022 at 10:01 AM Dmitry Baryshkov >>> wrote: >>>> >>>> On 26/04/2022 18:37, Abhinav Kumar wrote: >>>>> Hi Doug >>>>> >>>>> On 4/26/2022 8:20 AM, Doug Anderson wrote: >>>>>> Hi, >>>>>> >>>>>> On Mon, Apr 25, 2022 at 8:35 PM Abhinav Kumar >>>>>> wrote: >>>>>>> >>>>>>> On 4/25/2022 7:18 PM, Doug Anderson wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On Mon, Apr 25, 2022 at 6:42 PM Abhinav Kumar >>>>>>>> wrote: >>>>>>>>> >>>>>>>>>>> 2) When there was a valid EDID but no 640x480 mode >>>>>>>>>>> >>>>>>>>>>> This is the equipment specific case and the one even I was a bit >>>>>>>>>>> surprised. There is a DP compliance equipment we have in-house >>>>>>>>>>> and while >>>>>>>>>>> validation, it was found that in its list of modes , it did not >>>>>>>>>>> have any >>>>>>>>>>> modes which chromebook supported ( due to 2 lanes ). But my >>>>>>>>>>> understanding was that, all sinks should have atleast 640x480 but >>>>>>>>>>> apparently this one did not have that. So to handle this DP >>>>>>>>>>> compliance >>>>>>>>>>> equipment behavior, we had to do this. >>>>>>>>>> >>>>>>>>>> That doesn't seem right. If there's a valid EDID and the valid EDID >>>>>>>>>> doesn't contain 640x480, are you _sure_ you're supposed to be adding >>>>>>>>>> 640x480? That doesn't sound right to me. I've got a tiny display in >>>>>>>>>> front of me for testing that only has one mode: >>>>>>>>>> >>>>>>>>>> #0 800x480 65.68 800 840 888 928 480 493 496 525 32000 >>>>>>>>>> >>>>>>>>> >>>>>>>>> As I had wrote, DRM core kicks in only when the count of modes is 0. >>>>>>>>> Here what is happening is the count was not 0 but 640x480 was not >>>>>>>>> present in the EDID. So we had to add it explicitly. >>>>>>>>> >>>>>>>>> Your tiny display is a display port display? >>>>>>>>> >>>>>>>>> I am referring to only display port monitors. If your tiny display is >>>>>>>>> DP, it should have had 640x480 in its list of modes. >>>>>>>> >>>>>>>> My tiny display is actually a HDMI display hooked up to a HDMI to DP >>>>>>>> (active) adapter. >>>>>>>> >>>>>>>> ...but this is a legal and common thing to have. I suppose possibly my >>>>>>>> HDMI display is "illegal"? >>>>>>>> >>>>>>>> OK, so reading through the spec more carefully, I do see that the DP >>>>>>>> spec makes numerous mentions of the fact that DP sinks _must_ support >>>>>>>> 640x480. Even going back to DP 1.4, I see section "5.2.1.2 Video >>>>>>>> Timing Format" says that we must support 640x480. It seems like that's >>>>>>>> _intended_ to be used only if the EDID read fails, though or if we >>>>>>>> somehow have to output video without knowledge of the EDID. It seems >>>>>>>> hard to believe that there's a great reason to assume a display will >>>>>>>> support 640x480 if we have more accurate knowledge. >>>>>>>> >>>>>>>> In any case, I guess I would still say that adding this mode belongs >>>>>>>> in the DRM core. The core should notice that it's a DP connection >>>>>>>> (bridge->type == DRM_MODE_CONNECTOR_DisplayPort) and that 640x480 was >>>>>>>> left out and it should add it. We should also make sure it's not >>>>>>>> "preferred" and is last in the list so we never accidentally pick it. >>>>>>>> If DP truly says that we should always give the user 640x480 then >>>>>>>> that's true for everyone, not just Qualcomm. We should add it in the >>>>>>>> core. If, later, someone wants to hide this from the UI it would be >>>>>>>> much easier if they only needed to modify one place. >>>>>>>> >>>>>>> >>>>>>> So I debugged with kuogee just now using the DP compliance equipment. >>>>>>> It turns out, the issue is not that 640x480 mode is not present. >>>>>>> >>>>>>> The issue is that it is not marked as preferred. >>>>>>> >>>>>>> Hence we missed this part during debugging this equipment failure. >>>>>>> >>>>>>> We still have to figure out the best way to either mark 640x480 as >>>>>>> preferred or eliminate other modes during the test-case so that 640x480 >>>>>>> is actually picked by usermode. >>>>>>> >>>>>>> Now that being said, the fix still doesn't belong in the framework. It >>>>>>> has to be in the msm/dp code. >>>>>>> >>>>>>> Different vendors handle this failure case differently looks like. >>>>>>> >>>>>>> Lets take below snippet from i915 as example. >>>>>>> >>>>>>> 3361 if (intel_connector->detect_edid == NULL || >>>>>>> 3362 connector->edid_corrupt || >>>>>>> 3363 intel_dp->aux.i2c_defer_count > 6) { >>>>>>> 3364 /* Check EDID read for NACKs, DEFERs and corruption >>>>>>> 3365 * (DP CTS 1.2 Core r1.1) >>>>>>> 3366 * 4.2.2.4 : Failed EDID read, I2C_NAK >>>>>>> 3367 * 4.2.2.5 : Failed EDID read, I2C_DEFER >>>>>>> 3368 * 4.2.2.6 : EDID corruption detected >>>>>>> 3369 * Use failsafe mode for all cases >>>>>>> 3370 */ >>>>>>> 3371 if (intel_dp->aux.i2c_nack_count > 0 || >>>>>>> 3372 intel_dp->aux.i2c_defer_count > 0) >>>>>>> 3373 drm_dbg_kms(&i915->drm, >>>>>>> 3374 "EDID read had %d NACKs, %d >>>>>>> DEFERs\n", >>>>>>> 3375 intel_dp->aux.i2c_nack_count, >>>>>>> 3376 intel_dp->aux.i2c_defer_count); >>>>>>> 3377 intel_dp->compliance.test_data.edid = >>>>>>> INTEL_DP_RESOLUTION_FAILSAFE; >>>>>> >>>>> >>>>> The reason I pointed to this code is to give an example of how other >>>>> drivers handle this test-case. >>>>> >>>>> We added this patch for 4.2.2.1 and 4.2.2.6 EDID test cases. >>>>> >>>>> The challenge here as found out from our discussion here was to mark a >>>>> particular mode as preferred so that the Chrome usermode can pick it. >>>>> >>>>> Now whats happening with that there was always a possibility of two >>>>> modes being marked as preferred due to this and so-on. >>>>> >>>>> We had a pretty long discussion last night and thought of all possible >>>>> solutions but all of them look like a hack to us in the driver because >>>>> we end up breaking other things due to this. >>>>> >>>>> So we decided that driver is not the place to handle this test case. >>>>> Since we do have IGT support for chromebooks, we will handle both these >>>>> test cases there as other vendors do the same way and it works. >>>>> >>>>> >>>>>> Just because Intel DRM has its own solution for something doesn't mean >>>>>> everyone else should copy them and implement their own solution. Up >>>>>> until recently DP AUX backlights were baked into different DRM >>>>>> drivers. A recent effort was made to pull it out. I think the Intel >>>>>> DRM code was the "first one" to the party and it wasn't clear how >>>>>> things should be broken up to share with other drivers, so mostly it >>>>>> did everything itself, but that's not the long term answer. >>>>>> >>>>>> I'm not saying that we need to block your change on a full re-design >>>>>> or anything, but I'm just saying that: >>>>>> >>>>>> * You're trying to implement a generic DP rule, not something specific >>>>>> to Qualcomm hardware. That implies that, if possible, it shouldn't be >>>>>> in a Qualcomm driver. >>>>>> >>>>>> * It doesn't seem like it would be terrible to handle this in the core. >>>>>> >>>>>> >>>>>>> This marks the fail safe mode and IGT test case reads this to set this >>>>>>> mode and hence the test passes. >>>>>>> >>>>>>> We rely on the chromeOS usermode to output pixel data for this test-case >>>>>>> and not IGT. We use IGT only for video pattern CTS today but this is a >>>>>>> different test-case which is failing. >>>>>>> >>>>>>> ChromeOS usermode will not pick 640x480 unless we mark it as preferred >>>>>>> or other modes are eliminated. >>>>>>> >>>>>>> So we have to come up with the right way for the usermode to pick >>>>>>> 640x480. >>>>>>> >>>>>>> We will discuss this a bit more and come up with a different change. >>>>>> >>>>>> Can you provide the exact EDID from the failing test case? Maybe that >>>>>> will help shed some light on what's going on. I looked at the original >>>>>> commit and it just referred to 4.2.2.1, which I assume is "EDID Read >>>>>> upon HPD Plug Event", but that doesn't give details that seem relevant >>>>>> to the discussion here. >>>>> >>>>> Yes so it is 4.2.2.1 and 4.2.2.6. >>>>> >>>>> That alone wont give the full picture. >>>>> >>>>> So its a combination of things. >>>>> >>>>> While running the test, the test equipment published only one mode. >>>>> But we could not support that mode because of 2 lanes. >>>>> Equipment did not add 640x480 to the list of modes. >>>>> DRM fwk will also not add it because count_modes is not 0 ( there was >>>>> one mode ). >>>>> So we ended up making these changes. >>>> >>>> I think a proper solution might be to rewrite >>>> drm_helper_probe_single_connector_modes() in the following way: >>>> - call get_modes() >>>> - validate the result >>>> - prune invalid >>>> >>>> - if the number of modes is 0, call drm_add_override_edid_modes() >>>> - validate the result >>>> - prune invalid >>>> >>>> - if the number of modes is still 0, call drm_add_modes_noedid() >>>> - validate the result >>>> - prune invalid >>>> >>>> [A separate change might happen here after all the checks: if the number >>>> of modes is still 0 and if it is a DP, enforce adding 640x480 even w/o >>>> validation. But generally I feel that this shouldn't be necessary >>>> because the previous step should have added it.] >>>> >>>> This way we can be sure that all modes are validated, but still to do >>>> our best to add something supported to the list of modes. >>> >>> I'm partway through implementing / testing something similar to this. >>> ;-) My logic is slightly different than yours, though. In the very >>> least I'm not convinced that we want to add the higher resolution >>> modes (like 1024x768) if all the modes fail to validate. The DP spec >>> only claims 640x480 is always supported. The higher resolution modes >>> are for when the EDID fails to read I think. Similarly I'm not >>> convinced that we should do pruning before deciding on >>> drm_add_override_edid_modes(). >> >> >> I think pruning before drm_add_override_edid_modes() would allow one to >> use override if the first read returned some modes which are invalid. > > Yeah, I'm less certain about drm_add_override_edid_modes(), but as per > documented it's only to be used if the EDID failed to read. > > If someone has an actual use case where they need to add the override > modes for this specific case then we can, but I think it can be done > separately once someone has an actual use case. > > >> Regarding 1024 vs 640. For the restructure we shouldn't change this. And >> I'd actually point to the following commit message: >> >> commit 9632b41f00cc2fb2846569cc99dbeef78e5c94a0 >> Author: Adam Jackson >> Date: Mon Nov 23 14:23:07 2009 -0500 >> >> drm/modes: Fall back to 1024x768 instead of 800x600 >> >> This matches the X server's fallback modes when using RANDR 1.2. >> >> See also: http://bugzilla.redhat.com/538761 >> >> Signed-off-by: Adam Jackson >> Signed-off-by: Dave Airlie >> >> >> So I'd say, let's leave 1024 as is and just try them if all other modes >> are invalid. > > I'm pretty strongly against adding 1024x768 when all modes fail to > validate. Specifically: > > If the EDID fully fails to read then adding these higher resolution > modes makes sense. We have no knowledge at all about the display in > this case and so we can guess that some standard higher resolutions > might make sense. In the case we're dealing with here, we have very > specific knowledge about what the display said it could handle and we > can't support any of them. The DP spec _only_ lists 640x480 as a > required mode so that's the only one we should add. > > -Doug So one thing to note Doug, in case you have not already made note of it. I believe you are referring to drm_add_modes_noedid() and NOT drm_add_override_edid_modes()? Because the latter needs an override firware If so, just wanted to mention that drm_add_modes_noedid() operates on the probed_modes() list but after validation and pruning, probed_modes list does not exits. It gets copied over to connector->modes list. So we cannot directly call that.