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

[Manasi Navare]

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()

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.

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