[Intel-gfx] [PATCH 1/5] drm/dp/fec: DRM helper for Forward Error Correction

[Srivatsa, Anusha]

>-----Original Message-----
>From: Navare, Manasi D
>Sent: Monday, August 20, 2018 12:32 PM
>To: Jani Nikula <jani.nikula at linux.intel.com>
>Cc: Srivatsa, Anusha <anusha.srivatsa at intel.com>; intel-
>gfx at lists.freedesktop.org; Ville Syrjala <ville.syrjala at linux.intel.com>
>Subject: Re: [PATCH 1/5] drm/dp/fec: DRM helper for Forward Error Correction
>
>Hi Jani,
>
>Thanks for your feedback. Please see my comments below:
>
>On Fri, Aug 17, 2018 at 12:51:03PM +0300, Jani Nikula wrote:
>> On Tue, 07 Aug 2018, Anusha Srivatsa <anusha.srivatsa at intel.com> wrote:
>> > From: "Srivatsa, Anusha" <anusha.srivatsa at intel.com>
>> >
>> > DP 1.4 has Forward Error Correction Support(FEC).
>> > Add helper function to check if the sink device supports FEC.
>> >
>> > v2: Separate the helper and the code that uses the helper into two
>> > separate patches. (Manasi)
>> >
>> > v3:
>> > - Move the code to drm_dp_helper.c (Manasi)
>> > - change the return type, code style changes (Gaurav)
>> > - Use drm_dp_dpcd_readb instead of drm_dp_dpcd_read. (Jani)
>> >
>> > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
>> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> > Cc: Manasi Navare <manasi.d.navare at intel.com>
>> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> > ---
>> >  drivers/gpu/drm/drm_dp_helper.c | 14 ++++++++++++++
>> >  include/drm/drm_dp_helper.h     |  3 +++
>>
>> Neither of these is i915, thus intel-gfx is not enough.
>>
>> >  2 files changed, 17 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
>> > b/drivers/gpu/drm/drm_dp_helper.c index 7dc61d1..2e127b9 100644
>> > --- a/drivers/gpu/drm/drm_dp_helper.c
>> > +++ b/drivers/gpu/drm/drm_dp_helper.c
>> > @@ -1425,3 +1425,17 @@ u8 drm_dp_dsc_sink_max_color_depth(const u8
>dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
>> >  	return 0;
>> >  }
>> >  EXPORT_SYMBOL(drm_dp_dsc_sink_max_color_depth);
>> > +
>> > +/* Forward Error Correction support for DP 1.4 */ int
>> > +drm_dp_sink_supports_fec(struct drm_dp_aux *aux) {
>> > +	int fec_err;
>> > +	u8 fec_cap;
>> > +
>> > +	fec_err = drm_dp_dpcd_readb(aux, DP_FEC_CAPABILITY, &fec_cap);
>> > +	if (fec_err < 0)
>> > +		return fec_err;
>> > +
>> > +	return fec_cap & DP_FEC_CAPABLE;
>>
>> I can't help but think this function feels wrong in so many ways.
>>
>> First, how does this function fit with the rest of the helpers? Most
>> helpers operate on previously read data. At the very least the
>> function name should reflect the fact that this *reads* DPCD *if* this
>> continues to read the DPCD.
>
>I agree that this does not fit with rest of the helpers. All the other helpers that get
>any information from DPCD registers actually work on the cached set of registers.
>So here to follow the same format, we could cache FEC DPCD registers -
>FEC_SUPPORT, FEC_CONFIGURATION, FEC_STATUS, FEC_ERROR_COUNT.
>Out fo which we really for now need to read FEC_SUPPORT but might need other
>registers in the future.
>One way to correct this would be to cache these at the same time when we
>cache the DSC DPCD registers into say fec_dpcd[].
>That way we dont trigger the aux reads everytime we need to get fec_support()

Sounds about right, Jani,any thougts?

>Jani, does this sound like a good solution?
>
>>
>> Second, what are you going to do when you need to read the other bits
>> in the same register? Read it again in the driver? Add more helpers?
>> But you only want to read the register *once*. This one seems too
>> specialized.
>>
>> Third, the name implies a boolean return, but it's not. An
>> unsuspecting caller will use this and get a "supports fec" return on
>> errors. This is hard to use. Patch 2/5 in this series makes that exact
>> mistake twice, it's the first user, and sets the example.
>
>Yes I think this should follow the same format as drm_dp_sink_supports_dsc()
>where it returns a bool based on the cached value.

Sure. Makes sense.

Thanks Jani, Manasi for the feedback.

Anusha 
>Manasi
>
>
>>
>> I'm not convinced of the usefulness of this particular helper.
>>
>> BR,
>> Jani.
>>
>>
>>
>> > +}
>> > +EXPORT_SYMBOL(drm_dp_sink_supports_fec);
>> > diff --git a/include/drm/drm_dp_helper.h
>> > b/include/drm/drm_dp_helper.h index f178933..d958c7d 100644
>> > --- a/include/drm/drm_dp_helper.h
>> > +++ b/include/drm/drm_dp_helper.h
>> > @@ -1270,6 +1270,9 @@ void drm_dp_aux_unregister(struct drm_dp_aux
>> > *aux);  int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc
>> > *crtc);  int drm_dp_stop_crc(struct drm_dp_aux *aux);
>> >
>> > +/* Forward Error Correction Support on DP 1.4 */ int
>> > +drm_dp_sink_supports_fec(struct drm_dp_aux *aux);
>> > +
>> >  struct drm_dp_dpcd_ident {
>> >  	u8 oui[3];
>> >  	u8 device_id[6];
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center