[RFC 11/13] drm/dp: Add helper to dump DPCD

[Daniel Vetter]

On Thu, Sep 03, 2015 at 10:49:34AM -0700, Rafael Antognolli wrote:
> 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 at 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 at gmail.com> wrote:
> > > >> > From: Thierry Reding <treding at 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.

ioctl on some file, either a /dev chardev or just something in debugfs.
And imo it doesn't make sense to have it as an ioctl on the drm device
node since then we'd need to add some form of multiplexing to select the
right dp aux bus (and with dp mst there's new ones for each port on a
downstream hub). I'd just go with copying the i2c design of a chardev for
each master.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch