[Freedreno] [PATCH] drm/msm/dp: move add fail safe mode to dp_connector_get_mode()

[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 at 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 at 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 at 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