On Sat, Jun 8, 2013 at 9:08 AM, Chris Wilson chris@chris-wilson.co.uk wrote:
On Wed, Jun 05, 2013 at 10:59:23PM +0100, Chris Wilson wrote:
It is useful for userspace to know when it may be able to skip a forced detection cycle as the connector maintains an accurate status. It also provides status feedback to the user of the hotplug storm detection, and the ability to override the method used for detecting changes in connection status.
Please review this in the context of the user being able to manually override the hotplug detection method for an individual connector. In that role this makes a lot of sense as it should improve the user experience in quite a few situations.
The problem I see is that userspace _can't_ trust the kernel like this, i.e. the optimization you do in
http://cgit.freedesktop.org/~ickle/xf86-video-intel/commit/?h=hotplug-proper...
to not force the kernel into the full ->fill_modes dance will break systems. We do have a bunch of machines on record where we claim to have full hpd support, but it doesn't seem to do anything useful.
But it's not just machines where hotplug flat-out does nothing, hw hotplug is also pretty racy. We've tried to plug them a bit, but due to the completely broken machines that regressed again.
The other thing that irks me is that we give the user the option to override stuff. Imo stuff _really_ should just work, and it sounds like we have to do the same retraining exercise with "please drop poll=0 from your boot options" as with disabling modeset. So up to a certain catastrophic level I prefer pitchforks in front of my house over people just quirking each and every machine themselves.
Lastly I'm not too happy about how complicated the re-detect avoidance logic works with your two kernel patches + the userspace change. Originally I've thought it doesn't solve the edid re-reading issue, but after carefully re-reading things I stand corrected: - output poll work or hpd handler call just ->detect from the helpers interface - if a change has happened it'll eventually call down to the fb helper (note that we might end up with kms only drivers who punt on this eventually). Thanks to the new force parameter it'll forego the ->detect and only call ->get_modes. - Then userspace makes sure that none of ->fill_modes will get called, which avoids both the ->detect and the ->get_modes.
So it's pretty tricky, and also doesn't avoid re-reading the EDID if both ->detect and ->get_modes does that. Also I don't like that fbdev gets special treatment interface-wise compared to userspace (it can set force=false, which userspace can't). I've very much like to avoid this, our kms interface should be good enough for both userspace and legacy fbdev.
Finally a pure process thing on top: I'd like to confirm that Egbert's hotplug storm stuff is solid before we frob our detect code a bit more. Which means 3.10 should get out the door first and we need to ping the various bug reporters a bit.
Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Sat, Jun 08, 2013 at 02:34:20PM +0200, Daniel Vetter wrote:
On Sat, Jun 8, 2013 at 9:08 AM, Chris Wilson chris@chris-wilson.co.uk wrote:
On Wed, Jun 05, 2013 at 10:59:23PM +0100, Chris Wilson wrote:
It is useful for userspace to know when it may be able to skip a forced detection cycle as the connector maintains an accurate status. It also provides status feedback to the user of the hotplug storm detection, and the ability to override the method used for detecting changes in connection status.
Please review this in the context of the user being able to manually override the hotplug detection method for an individual connector. In that role this makes a lot of sense as it should improve the user experience in quite a few situations.
The problem I see is that userspace _can't_ trust the kernel like this, i.e. the optimization you do in
http://cgit.freedesktop.org/~ickle/xf86-video-intel/commit/?h=hotplug-proper...
to not force the kernel into the full ->fill_modes dance will break systems. We do have a bunch of machines on record where we claim to have full hpd support, but it doesn't seem to do anything useful.
It is these machines that require the per-connector quirk to turn off hotplug detection anyway. And there are users that need to turn off all hotplug detection (hardware and polling) on certain connectors whilst plugged into their kvm.
I'm not happy with the ddx portion as it breaks the RandR expectation of ->detect performing an explicit probe, which is exposed over the protocol. RandR added the ability to query the existing modes, so I am going to have to live with the current interface and keep on hoping that everyone migrates over to the cheap query.
But it's not just machines where hotplug flat-out does nothing, hw hotplug is also pretty racy. We've tried to plug them a bit, but due to the completely broken machines that regressed again.
If the hpd itself is racy, the detect is likely to be as well. It's a no-win situation.
The other thing that irks me is that we give the user the option to override stuff. Imo stuff _really_ should just work, and it sounds like we have to do the same retraining exercise with "please drop poll=0 from your boot options" as with disabling modeset. So up to a certain catastrophic level I prefer pitchforks in front of my house over people just quirking each and every machine themselves.
Those who suffer are already pretty irate. I take the opposite view, in that we should not quirk in the kernel unless there is a user switch as well. What's is good for one machine, is likely required for many more - so making that option available makes it easier to test, and gives users control before we can get a quirked kernel to them (if it is even possible to add a quirk for their particular machine).
Lastly I'm not too happy about how complicated the re-detect avoidance logic works with your two kernel patches + the userspace change.
I think the root cause here is that fill_modes should not be doing a detect, but that detect does the the fill_modes. I.e. the EDID and probed mode list for a connector should be considered static for the lifetime of the connection.
The complication in userspace is a separate issue in trying to avoid doing a forced detection because the RandR interface doesn't distinguish between returning the latest information after a hotplug uevent versus doing an explicit probe. (So yes, it is a repetition of the same problem we have in the kernel code.)
[snip]
Finally a pure process thing on top: I'd like to confirm that Egbert's hotplug storm stuff is solid before we frob our detect code a bit more. Which means 3.10 should get out the door first and we need to ping the various bug reporters a bit.
In hindsight, I would have liked the hotplug property in with Egbert's automatic storm mitigation. -Chris
On Mon, Jun 10, 2013 at 12:27 AM, Chris Wilson chris@chris-wilson.co.uk wrote:
It is these machines that require the per-connector quirk to turn off hotplug detection anyway. And there are users that need to turn off all hotplug detection (hardware and polling) on certain connectors whilst plugged into their kvm.
I think it'd be useful to take one step back and lock at the intended result (i.e. use-cases) a bit first, so cut out all the other discussion.
Pondering this a bit more I see a bunch of things we could aim for: - Giving userspace more rope to make even horribly broken systems useful. We have the hpd storm stuff and the connector state forcing, but atm the story is still imcomplete there I think. I guess it would be useful to expose the connector->force attribute (maybe through sysfs, tends to be easier to set correctly with scripts), so that users with really flaky hw could update the forced state at runtim. Then we could make the EDID firmware loading a bit more flexible (again maybe expose the firmware filename in sysfs per-connector) and users would be able to fake any setup. The last missing puzzle piece would be to add a new FORCE_INTEGRATED mode which also eschews the repeated ->get_modes calls when ->num_modes > 0 or so.
- Allowing some fine-tuning of the hpd storm detection engine. Since we don't yet have much real-world usage data I'm not sure what we could want here, so probably best to postpone for now.
- Disabling polling. Presuming we have the pimped connector forcing infrastructure described above I'm not sure whether we really need more than what the global tunable already affords us. Thanks to locking reworks and a few other things the poll work is really unobtrusive nowadays (especially if all outputs claim to support hotplug).
- Better caching of EDID data and connector state. My favourite approach for this would be some opportunistic caching for e.g. 1 second in the ->fill_modes callback. Hotplug interrupts would invalidate the caching (and since we already filter hotplug interrupts to only touch relevant outputs with Egbert's latest patches, that should be fairly good). Of course caching the edid like this in a reality with horribly unreliable hpd will add new races and inconsistencies between reality and what the kernel reports. But in those cases hotplug reporting is already horribly racy, so not really a real degration. Like the poll intervall we could make that tuneable.
- Full EDID caching when the hotplug interrupt is actually reliable. I think that this is the case for native DP outputs, at least I'm not aware of any bug report. And maybe we'll figure out the few holdouts for HDMI hpd handling. If we have the opportunistic caching infrastructure from the previous point we could simply disable the time-based invalidation for those, and only invalidate the edid/connector state on a hotplug event. But even for DP I think it's better if the kernel is in charge of setting this, since e.g. plugging in a DP->VGA converter means that we need to disable full caching and switch back to opportunistic caching. And then back again if a native DP monitor is plugged in.
Hopefully this dump here is useful to broaden the discussion a bit. Thoughts?
Cheers, Daniel
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter