| Message ID | 20180720174110.25292-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2018-07-20 at 10:41 -0700, Rodrigo Vivi wrote: > Instead of using a backchannel if some dpcd read failed we > can show that directly on debugfs output. > > We are not returning an error because we might still want > to know if sub-sequent reads works, We can print partial ( and successful) output and return the first error value we get. We should probably get rid of that warn_on too. > but we shouldn't > need to check 2 places to see why reg is not on the list. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 59dc0610ea44..5d8da4e8c444 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -4846,8 +4846,8 @@ static int i915_dpcd_show(struct seq_file *m, > void *data) > > err = drm_dp_dpcd_read(&intel_dp->aux, b->offset, > buf, size); > if (err <= 0) { > - DRM_ERROR("dpcd read (%zu bytes at %u) > failed (%zd)\n", > - size, b->offset, err); > + seq_printf(m, "dpcd read (%zu bytes at %u) > failed (%zd)\n", > + size, b->offset, err); > continue; > } >
Quoting Rodrigo Vivi (2018-07-20 18:41:10) > Instead of using a backchannel if some dpcd read failed we > can show that directly on debugfs output. > > We are not returning an error because we might still want > to know if sub-sequent reads works, but we shouldn't > need to check 2 places to see why reg is not on the list. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106371 > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> -Chris
On Fri, 20 Jul 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > Instead of using a backchannel if some dpcd read failed we > can show that directly on debugfs output. > > We are not returning an error because we might still want > to know if sub-sequent reads works, but we shouldn't > need to check 2 places to see why reg is not on the list. Should we just nuke this debugfs and use the aux chardev interface to dpcd instead? BR, Jani. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 59dc0610ea44..5d8da4e8c444 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -4846,8 +4846,8 @@ static int i915_dpcd_show(struct seq_file *m, void *data) > > err = drm_dp_dpcd_read(&intel_dp->aux, b->offset, buf, size); > if (err <= 0) { > - DRM_ERROR("dpcd read (%zu bytes at %u) failed (%zd)\n", > - size, b->offset, err); > + seq_printf(m, "dpcd read (%zu bytes at %u) failed (%zd)\n", > + size, b->offset, err); > continue; > }
On Mon, 2018-08-20 at 10:15 +0300, Jani Nikula wrote: > On Fri, 20 Jul 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > Instead of using a backchannel if some dpcd read failed we > > can show that directly on debugfs output. > > > > We are not returning an error because we might still want > > to know if sub-sequent reads works, but we shouldn't > > need to check 2 places to see why reg is not on the list. > > Should we just nuke this debugfs and use the aux chardev interface to > dpcd instead? Given that this debugfs does not print decoded information, I think, it offers very little benefit over direct reads. We also print some DPCD's in dmesg, so I'm in favour of killing it. Don't see the file being used in IGTs either. Your comment also reminds about the IGT tool that Tarun started writing for reading DPCD. -DK > > BR, > Jani. > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index 59dc0610ea44..5d8da4e8c444 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -4846,8 +4846,8 @@ static int i915_dpcd_show(struct seq_file *m, > > void *data) > > > > err = drm_dp_dpcd_read(&intel_dp->aux, b->offset, > > buf, size); > > if (err <= 0) { > > - DRM_ERROR("dpcd read (%zu bytes at %u) > > failed (%zd)\n", > > - size, b->offset, err); > > + seq_printf(m, "dpcd read (%zu bytes at %u) > > failed (%zd)\n", > > + size, b->offset, err); > > continue; > > } > >
On Mon, Aug 20, 2018 at 12:10:08PM -0700, Pandiyan, Dhinakaran wrote: > > > On Mon, 2018-08-20 at 10:15 +0300, Jani Nikula wrote: > > On Fri, 20 Jul 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > > Instead of using a backchannel if some dpcd read failed we > > > can show that directly on debugfs output. > > > > > > We are not returning an error because we might still want > > > to know if sub-sequent reads works, but we shouldn't > > > need to check 2 places to see why reg is not on the list. > > > > Should we just nuke this debugfs and use the aux chardev interface to > > dpcd instead? > > Given that this debugfs does not print decoded information, I think, it > offers very little benefit over direct reads. We also print some DPCD's > in dmesg, so I'm in favour of killing it. Don't see the file being used > in IGTs either. > > Your comment also reminds about the IGT tool that Tarun started writing > for reading DPCD. There might be people out there using this for debug. So we should first be able to do this in some place like IGT: https://patchwork.freedesktop.org/series/48470/ ? then we remove. But that discussion might take a while. So I'd like to move with this fix here now and remove dpcd_show completely when we create some infrastructure on IGT. > > -DK > > > > > > BR, > > Jani. > > > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > index 59dc0610ea44..5d8da4e8c444 100644 > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > @@ -4846,8 +4846,8 @@ static int i915_dpcd_show(struct seq_file *m, > > > void *data) > > > > > > err = drm_dp_dpcd_read(&intel_dp->aux, b->offset, > > > buf, size); > > > if (err <= 0) { > > > - DRM_ERROR("dpcd read (%zu bytes at %u) > > > failed (%zd)\n", > > > - size, b->offset, err); > > > + seq_printf(m, "dpcd read (%zu bytes at %u) > > > failed (%zd)\n", > > > + size, b->offset, err); > > > continue; > > > } > > > >
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 59dc0610ea44..5d8da4e8c444 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -4846,8 +4846,8 @@ static int i915_dpcd_show(struct seq_file *m, void *data) err = drm_dp_dpcd_read(&intel_dp->aux, b->offset, buf, size); if (err <= 0) { - DRM_ERROR("dpcd read (%zu bytes at %u) failed (%zd)\n", - size, b->offset, err); + seq_printf(m, "dpcd read (%zu bytes at %u) failed (%zd)\n", + size, b->offset, err); continue; }
Instead of using a backchannel if some dpcd read failed we can show that directly on debugfs output. We are not returning an error because we might still want to know if sub-sequent reads works, but we shouldn't need to check 2 places to see why reg is not on the list. Cc: Jani Nikula <jani.nikula@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)