Messages in this thread | | | Date | Tue, 26 Apr 2022 10:24:42 -0700 | Subject | Re: [Freedreno] [PATCH] drm/msm/dp: move add fail safe mode to dp_connector_get_mode() | From | Abhinav Kumar <> |
| |
On 4/26/2022 10:11 AM, Doug Anderson wrote: > Hi, > > On Tue, Apr 26, 2022 at 10:01 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> 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 >>>> <quic_abhinavk@quicinc.com> wrote: >>>>> >>>>> On 4/25/2022 7:18 PM, Doug Anderson wrote: >>>>>> Hi, >>>>>> >>>>>> On Mon, Apr 25, 2022 at 6:42 PM Abhinav Kumar >>>>>> <quic_abhinavk@quicinc.com> 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().
Doug, we will certainly evaluate it once you post it.
Thanks
Abhinav
> > -Doug
| |