Re: [Freedreno] [PATCH] drm/msm/dp: move add fail safe mode to dp_connector_get_mode()
From: Abhinav Kumar
Date: Tue Apr 26 2022 - 14:03:47 EST
On 4/26/2022 10:56 AM, Doug Anderson wrote:
Hi,
On Tue, Apr 26, 2022 at 10:44 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
On 26/04/2022 20:11, Doug Anderson wrote:
Hi,
On Tue, Apr 26, 2022 at 10:01 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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 <ajax@xxxxxxxxxx>
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 <ajax@xxxxxxxxxx>
Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx>
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.