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

From: Rafael Antognolli <rafael.antognolli@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, dri-devel@lists.freedesktop.org
Subject: Re: [RFC 11/13] drm/dp: Add helper to dump DPCD
Date: Thu, 3 Sep 2015 10:49:34 -0700	[thread overview]
Message-ID: <20150903174934.GA4336@intel.com> (raw)
In-Reply-To: <20150820232640.GA7545@intel.com>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel