Re: [PATCH] drm/dp: implement EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT

["Atwood, Matthew S" <matthew.s.atwood@intel.com>]

On Thu, 2018-05-17 at 12:50 +0300, Jani Nikula wrote:
> On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c
> om> wrote:
> > On Wed, 2018-05-16 at 09:33 -0700, matthew.s.atwood@intel.com
> > wrote:
> > > From: Matt Atwood <matthew.s.atwood@intel.com>
> > > 
> > > According to DP spec (2.9.3.1 of DP 1.4) if
> > > EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT is set the addresses
> > > in
> > > DPCD
> > > 02200h through 0220Fh shall contain the DPRX's true capability.
> > > These
> > > values will match 00000h through 0000Fh, except for DPCD_REV,
> > > MAX_LINK_RATE, DOWN_STREAM_PORT_PRESENT.
> > > 
> > > Read from DPCD once for all 3 values as this is an expensive
> > > operation.
> > > Spec mentions that all of address space 02200h through 0220Fh
> > > should
> > > contain the right information however currently only 3 values can
> > > differ.
> > > 
> > > There is no address space in the intel_dp->dpcd struct for
> > > addresses
> > > 02200h through 0220Fh, and since so much of the data is a
> > > identical,
> > > simply overwrite the values stored in 00000h through 0000Fh with
> > > the
> > > values that can be overwritten from addresses 02200h through
> > > 0220Fh
> 
> Without reading the spec, this commit message makes one think there's
> no
> point in any of this. Please mention this is for backward
> compatibility
> with older source devices that trip over because of newer DPCD.
sure thing
> 
> > Why overwrite all values if this is an expensive operation? From
> > what I
> > can see, you'll need to read only 0000h - 00005h
was mostly future proofing, we can get away with only reading 6 values.
the expense is to read 1, any number after that doesnt cost alot. That
being said sure thing.
> > 
> > > 
> > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 14 ++++++++++++++
> > >  include/drm/drm_dp_helper.h     |  5 +++--
> > >  2 files changed, 17 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index dde92e4af5d3..899ebc5cece6 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3738,6 +3738,20 @@ intel_dp_read_dpcd(struct intel_dp
> > > *intel_dp)
> > >  			     sizeof(intel_dp->dpcd)) < 0)
> > >  		return false; /* aux transfer failed */
> > >  
> > > +	if (intel_dp->dpcd[DP_TRAINING_AUX_RD_INTERVAL] &
> > > +	    DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT &&
> > > +	    intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14) {
> > 
> > The same section in the spec also says - "The Extended Receiver
> > Capability registers at DPCD Addresses 02200h through 0220Fh shall
> > contain the DPRX’s true capability, while the original Base
> > Receiver
> > Capability registers at DPCD Addresses 00000h through 0000Fh might
> > indicate DPCD r1.1, a MAX_LINK_RATE of 2.7Gbps/lane, and no DFP to
> > avoid interoperability issues ..."
> > 
> > Which means, intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14 is
> > probably
> > not going to be true for panels you want to read the extended
> > capabilities for. 
> 
> Agreed. Only check DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT.
sure
> 
> > 
> > > +		uint8_t dpcd_ext[16];
> 
> u8.
> 
> > > +
> > > +		if (drm_dp_dpcd_read(&intel_dp->aux,
> > > DP_DP13_DPCD_REV,
> > > +		    &dpcd_ext, sizeof(dpcd_ext)) < 0)
> > > +			return false; /* aux transfer failed */
> 
> Please don't return false here. Just waltz on.
sure
> 
> Like DK said, please read the minimal amount. Please memcmp those six
> bytes against the already read DPCD, and debug log the *old* bytes if
> there's a diff (new bytes debug logged below), and memcpy the new
> parts
> in place.
I really like this idea. thanks!
> 
> > > +
> > > +		intel_dp->dpcd[DP_DPCD_REV] =
> > > dpcd_ext[DP_DPCD_REV];
> > > +		intel_dp->dpcd[DP_MAX_LINK_RATE] =
> > > dpcd_ext[DP_MAX_LINK_RATE];
> > > +		intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] =
> > > +			dpcd_ext[DP_DOWNSTREAMPORT_PRESENT];
> > > +	}
> > >  	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp-
> > > >dpcd),
> > > intel_dp->dpcd);
> > >  
> > >  	return intel_dp->dpcd[DP_DPCD_REV] != 0;
> > > diff --git a/include/drm/drm_dp_helper.h
> > > b/include/drm/drm_dp_helper.h
> > > index c01564991a9f..757bd5913f3d 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > 
> > This should be a separate patch as it's outside i915.
no problem
> 
> Yes.
> 
> > 
> > > @@ -123,8 +123,9 @@
> > >  # define DP_FRAMING_CHANGE_CAP		    (1 << 1)
> > >  # define DP_DPCD_DISPLAY_CONTROL_CAPABLE     (1 << 3) /* edp
> > > v1.2 or
> > > higher */
> > >  
> > > -#define DP_TRAINING_AUX_RD_INTERVAL         0x00e   /* XXX 1.2?
> > > */
> > > -# define DP_TRAINING_AUX_RD_MASK            0x7F    /* XXX 1.2?
> > > */
> > > +#define DP_TRAINING_AUX_RD_INTERVAL             0x00e   /* XXX
> > > 1.2?
> > > */
> > > +# define DP_TRAINING_AUX_RD_MASK                0x7F    /* XXX
> > > 1.2?
> > > */
> > > +# define DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT (1 << 7)/* XXX
> > > 1.2?
> 
> Surely this is not XXX 1.2? ;)
I found it in a dp1.2 spec that Rodrigo had, I had originally found it
as a change with dp1.3. Earlier versions of the patch that added
DP_TRAINING_AUX_RD_MASK has had dp1.3 until he showed me that. If you'd
like Ill change it.
> 
> BR,
> Jani.
> 
> > > */
> > >  
> > >  #define DP_ADAPTER_CAP			    0x00f   /* 1.2
> > > */
> > >  # define DP_FORCE_LOAD_SENSE_CAP	    (1 << 0)
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx