Re: [PATCH 1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Oct 20, 2016 at 09:06:48PM +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > Extend the branch/sink descriptor info with the missing device ID
> > field and print this info for eDP and LSPCON connectors too.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c     | 83 +++++++++++++++----------------------
> >  drivers/gpu/drm/i915/intel_drv.h    | 13 ++++++
> >  drivers/gpu/drm/i915/intel_lspcon.c |  7 ++++
> >  3 files changed, 53 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 88f3b74..e90211e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1442,42 +1442,34 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
> >  	DRM_DEBUG_KMS("common rates: %s\n", str);
> >  }
> >  
> > -static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
> > +static bool intel_dp_is_branch(struct intel_dp *intel_dp)
> 
> Belongs in drm dp helpers.

So totally hit reply to say the same. Imo all of this debug stuff belongs
into helpers (but we're probably hitting the limit of what's possible
without further unification with the slightly-different helper world of
msm/tegra).

> > @@ -3519,6 +3511,13 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> >  	if (!intel_dp_read_dpcd(intel_dp))
> >  		return false;
> >  
> > +	if (drm_debug & DRM_UT_KMS) {
> > +		struct intel_dp_desc desc;
> > +
> > +		if (intel_dp_read_desc(intel_dp, &desc))
> > +			intel_dp_print_desc(intel_dp, &desc);
> > +	}
> 
> I *really* don't think we should do dpcd access conditional to drm
> debugs. I smell heisenbugs just thinking about it.

+1

> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index c06a33e..a35e241 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -883,6 +883,14 @@ enum link_m_n_set {
> >  	M2_N2
> >  };
> >  
> > +struct intel_dp_desc {
> > +	u8 oui[3];
> > +	u8 device_id[6];
> > +	u8 hw_rev;
> > +	u8 sw_major_rev;
> > +	u8 sw_minor_rev;
> > +} __packed;
> > +
> 
> I understand this, but makes me wonder about dp helpers vs. i915. I
> guess we could add this for now.

It already exists, in the form of slightly-different dp helpers used by
msm and tegra. Compared to our helpers they have some data structure stuff
to cache things. I guess it's time to port i915 over to that world,
respectively unify things a _lot_.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx