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

[Jani Nikula]

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.

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.

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