Messages in this thread | | | Date | Tue, 26 Apr 2022 20:01:02 +0300 | Subject | Re: [Freedreno] [PATCH] drm/msm/dp: move add fail safe mode to dp_connector_get_mode() | From | Dmitry Baryshkov <> |
| |
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 guess maybe what's happening is that the test case is giving an EDID >> where all the modes are not supportable by the current clock rate / >> lanes? ...and then somehow we're not falling back to 640x480. It's >> always possible that this is a userspace problem. >> >> In any case, would you object to a revert of the patches in the short >> term? > > Not sure, if you saw this change kuogee posted last night. > https://patchwork.freedesktop.org/patch/483415/ > We did decided to remove all the code related to these test cases and > handle them in IGT. > >> >> -Doug
-- With best wishes Dmitry
| |