Hi, I'm back from vacation, so I'll be looking at this again.
On Thu, Aug 20, 2015 at 04:26:42PM -0700, Rafael Antognolli wrote:
On Mon, Aug 17, 2015 at 10:02:04AM +0300, Jani Nikula wrote:
On Fri, 14 Aug 2015, Rafael Antognolli rafael.antognolli@intel.com wrote:
On Fri, Aug 14, 2015 at 02:56:55PM +0300, Jani Nikula wrote:
On Wed, 12 Aug 2015, Thierry Reding thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
The new drm_dp_dpcd_dump() helper dumps the contents of a DPCD to a seq_file and can be used to make the DPCD available via debugfs for example.
See i915/i915_debugfs.c for one DPCD dump implementation.
Around the time that was added, there was also some discussion (and patches [1]) to expose a read/write debugfs interface to DPCD, letting userspace access arbitrary DPCD registers.
Just this week there was some discussion about revisiting that. It was about accessing some proprietary panel features, but there's also the ease of debugging without having to keep updating the kernel to dump more.
I think it would be great to agree on a common debugfs interface to access DPCD arbitrarily. Last time I checked, the blocker to that was access to the aux channel from generic code; it's always driver specific. SMOP. ;)
Do you mean it would require the generic code/interface to somehow route this to the driver specific code? I am not sure yet how this works (if there's something like it around), but I'll take a look.
Drivers can choose to support DP AUX any way they like, and they don't have to use the common helpers to do so. It's pretty much an implementation detail. I think we could require the use of the common helpers to support generic DPCD access from debugfs, but we'd still have to come up with a way to get hold of struct drm_dp_aux (again, driver internals) given a drm_connector in generic debugfs code.
I was under the assumption they always used the helpers, but I understand that's not always the case.
Anyway, I was going to propose a new helper to "store" the drm_dp_aux inside the drm_connector. And just expose something on debugfs if it was used. Something like:
int drm_dp_aux_store(struct drm_dp_aux *aux, struct drm_connector *connector);
The helpers don't seem to know about drm_connector's though, so I'm not sure this is a good approach.
Thierry, do you see any problems with having a connector function to get hold of its drm_dp_aux?
Would this be a new function pointer inside struct drm_connector_funcs? If so, I'm fine with this approach too.
Regarding the interface, you mentioned that you didn't like it because it had a state. What about just dumping the content of the register into dmesg when one tries to read it, and use a different sintaxe for writing, passing both the register addr and the value?
Daniel had another suggestion, regarding an ioctl in debugfs, but I'm not sure I clearly understand that yet.
Regarding the interface, this is the comment Daniel did a while ago:
"As mentioned in another thread I think the right approach here is to expose the dp aux interface through some ioctls in debugfs or dev somewhere, and then add simple tools on top of that. Similar to what we have with i2c. That would allow us to implement additional things on top like mst inspection or using the i2c-over-dp-aux sidechannel."
If exposing through an ioctl, shouldn't it be yet another DRM ioctl? And if not, then I should create another /dev node/file specifically for this purpose, right?
And if we are going with ioctls, I still don't understand exactly how it relates to debugfs.
Thanks, Rafael