| Message ID | 1533683132-21625-2-git-send-email-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Forward Error Correction | expand |
On Tue, 07 Aug 2018, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote: > From: "Srivatsa, Anusha" <anusha.srivatsa@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@linux.intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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];
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@intel.com> wrote: > > From: "Srivatsa, Anusha" <anusha.srivatsa@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@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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
>-----Original Message----- >From: Navare, Manasi D >Sent: Monday, August 20, 2018 12:32 PM >To: Jani Nikula <jani.nikula@linux.intel.com> >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- >gfx@lists.freedesktop.org; Ville Syrjala <ville.syrjala@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@intel.com> wrote: >> > From: "Srivatsa, Anusha" <anusha.srivatsa@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@linux.intel.com> >> > Cc: Jani Nikula <jani.nikula@linux.intel.com> >> > Cc: Manasi Navare <manasi.d.navare@intel.com> >> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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
>-----Original Message----- >From: Navare, Manasi D >Sent: Monday, August 20, 2018 12:32 PM >To: Jani Nikula <jani.nikula@linux.intel.com> >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- >gfx@lists.freedesktop.org; Ville Syrjala <ville.syrjala@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@intel.com> wrote: >> > From: "Srivatsa, Anusha" <anusha.srivatsa@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@linux.intel.com> >> > Cc: Jani Nikula <jani.nikula@linux.intel.com> >> > Cc: Manasi Navare <manasi.d.navare@intel.com> >> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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. Jani, Manasi, Other than FEC_CAPABLE we don't read any other register. We do a bunch of writes, but that's about it. Also The registers are not consecutive. So, In case we decide to go ahead and cache all registers, it has to be three different cache. I think it is good approach to cache the FEC_CAPABLE register and *not* do aux operations everytime. Sound good? Anusha >> 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
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; +} +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];