| Message ID | 1413958523-32742-1-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jindal@intel.com wrote: > From: Sonika Jindal <sonika.jindal@intel.com> > > v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh) > > v3: Moving the utility function to drm_dp_helper (Daniel) > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ > include/drm/drm_dp_helper.h | 2 ++ > 2 files changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 08e33b8..a54a760 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) > i2c_del_adapter(&aux->ddc); > } > EXPORT_SYMBOL(drm_dp_aux_unregister); > + > +bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) I'd prefer if this didn't take a dpcd argument but rather directly accessed the DP_EDP_CONFIGURATION_CAP register so that it can be used directly rather than rely on the driver to have read a dpcd block in the appropriate format. > +{ > + uint8_t reg; > + > + if (dpcd[DP_EDP_CONFIGURATION_CAP] & > + DP_DPCD_DISPLAY_CONTROL_CAPABLE) { > + > + if (drm_dp_dpcd_read(aux, DP_EDP_REV, ®, 1)) > + if (reg == 0x03) > + return true; > + } > + return false; > +} > +EXPORT_SYMBOL(drm_dp_is_edp_v1_4); Does it make sense to have a function that checks for a specific version? Why not add one that returns the revision so that it can be compared, something like: u8 value; drm_dp_dpcd_read(aux, DP_EDP_REV, &value, 1); return value; Then we can do something like: #define DP_EDP_REV_1_1 0x00 #define DP_EDP_REV_1_2 0x01 #define DP_EDP_REV_1_3 0x02 #define DP_EDP_REV_1_4 0x03 And code can simply compare against that: drm_dp_get_edp_revision(aux, &rev); if (rev >= DP_EDP_REV_1_4) { ... } The check in your variant will only match v1.4 exactly, but presumably v1.5 will be backwards compatible. Having a direct check on the revision code will allow code to continue to work with future, backwards- compatible revisions. > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 8edeed0..b017e1e 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -102,6 +102,8 @@ > > #define DP_EDP_CONFIGURATION_CAP 0x00d /* XXX 1.2? */ > #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ > +#define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) This seems to be a field in the DP_EDP_CONFIGURATION_CAP register, so it should be sorted below that register, not DP_TRAINING_AUX_RD_INTERVAL. > +#define DP_EDP_REV 0x700 And this belongs further down, so it properly sorts into the list of registers. Thierry
Thanks for your comments Thierry. I agree to all your comments. I will write a general function to return version and repost the patch Thanks, Sonika On Wednesday 29 October 2014 07:12 PM, Thierry Reding wrote: > On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jindal@intel.com wrote: >> From: Sonika Jindal <sonika.jindal@intel.com> >> >> v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh) >> >> v3: Moving the utility function to drm_dp_helper (Daniel) >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> --- >> drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ >> include/drm/drm_dp_helper.h | 2 ++ >> 2 files changed, 17 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c >> index 08e33b8..a54a760 100644 >> --- a/drivers/gpu/drm/drm_dp_helper.c >> +++ b/drivers/gpu/drm/drm_dp_helper.c >> @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) >> i2c_del_adapter(&aux->ddc); >> } >> EXPORT_SYMBOL(drm_dp_aux_unregister); >> + >> +bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) > I'd prefer if this didn't take a dpcd argument but rather directly > accessed the DP_EDP_CONFIGURATION_CAP register so that it can be used > directly rather than rely on the driver to have read a dpcd block in the > appropriate format. > >> +{ >> + uint8_t reg; >> + >> + if (dpcd[DP_EDP_CONFIGURATION_CAP] & >> + DP_DPCD_DISPLAY_CONTROL_CAPABLE) { >> + >> + if (drm_dp_dpcd_read(aux, DP_EDP_REV, ®, 1)) >> + if (reg == 0x03) >> + return true; >> + } >> + return false; >> +} >> +EXPORT_SYMBOL(drm_dp_is_edp_v1_4); > Does it make sense to have a function that checks for a specific > version? Why not add one that returns the revision so that it can be > compared, something like: > > u8 value; > > drm_dp_dpcd_read(aux, DP_EDP_REV, &value, 1); > > return value; > > Then we can do something like: > > #define DP_EDP_REV_1_1 0x00 > #define DP_EDP_REV_1_2 0x01 > #define DP_EDP_REV_1_3 0x02 > #define DP_EDP_REV_1_4 0x03 > > And code can simply compare against that: > > drm_dp_get_edp_revision(aux, &rev); > > if (rev >= DP_EDP_REV_1_4) { > ... > } > > The check in your variant will only match v1.4 exactly, but presumably > v1.5 will be backwards compatible. Having a direct check on the revision > code will allow code to continue to work with future, backwards- > compatible revisions. > >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h >> index 8edeed0..b017e1e 100644 >> --- a/include/drm/drm_dp_helper.h >> +++ b/include/drm/drm_dp_helper.h >> @@ -102,6 +102,8 @@ >> >> #define DP_EDP_CONFIGURATION_CAP 0x00d /* XXX 1.2? */ >> #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ >> +#define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) > This seems to be a field in the DP_EDP_CONFIGURATION_CAP register, so it > should be sorted below that register, not DP_TRAINING_AUX_RD_INTERVAL. > >> +#define DP_EDP_REV 0x700 > And this belongs further down, so it properly sorts into the list of > registers. > > Thierry
On Wed, Oct 29, 2014 at 02:42:29PM +0100, Thierry Reding wrote: > On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jindal@intel.com wrote: > > From: Sonika Jindal <sonika.jindal@intel.com> > > > > v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh) > > > > v3: Moving the utility function to drm_dp_helper (Daniel) > > > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > --- > > drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ > > include/drm/drm_dp_helper.h | 2 ++ > > 2 files changed, 17 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > index 08e33b8..a54a760 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) > > i2c_del_adapter(&aux->ddc); > > } > > EXPORT_SYMBOL(drm_dp_aux_unregister); > > + > > +bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) > > I'd prefer if this didn't take a dpcd argument but rather directly > accessed the DP_EDP_CONFIGURATION_CAP register so that it can be used > directly rather than rely on the driver to have read a dpcd block in the > appropriate format. The idea is that you'd grab the DPCD field anyway since it's needed all over the place. We have a pile of helpers already that take exactly this block and decode parts of it. So I think this makes sense - dp aux is fast but not entirely free, so caching seems useful. It's a bit strange then that this helper reads more code, but I guess you really only care about the edp_rev field if you want the edp revision, and with your interface suggestion I don't think we'd ever need to read this again. So for me the input fields make sense. I agree that for the output of the function an enum with all the edp revisions is better. -Daniel
On Fri, Oct 31, 2014 at 05:06:39PM +0100, Daniel Vetter wrote: > On Wed, Oct 29, 2014 at 02:42:29PM +0100, Thierry Reding wrote: > > On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jindal@intel.com wrote: > > > From: Sonika Jindal <sonika.jindal@intel.com> > > > > > > v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh) > > > > > > v3: Moving the utility function to drm_dp_helper (Daniel) > > > > > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > --- > > > drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ > > > include/drm/drm_dp_helper.h | 2 ++ > > > 2 files changed, 17 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > index 08e33b8..a54a760 100644 > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) > > > i2c_del_adapter(&aux->ddc); > > > } > > > EXPORT_SYMBOL(drm_dp_aux_unregister); > > > + > > > +bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) > > > > I'd prefer if this didn't take a dpcd argument but rather directly > > accessed the DP_EDP_CONFIGURATION_CAP register so that it can be used > > directly rather than rely on the driver to have read a dpcd block in the > > appropriate format. > > The idea is that you'd grab the DPCD field anyway since it's needed all > over the place. We have a pile of helpers already that take exactly this > block and decode parts of it. So I think this makes sense - dp aux is fast > but not entirely free, so caching seems useful. If we want to always cache part of the DPCD wouldn't it be better to add the cache to struct drm_dp_aux instead of having to duplicate this in every driver? Thierry
On Mon, Nov 3, 2014 at 9:25 AM, Thierry Reding <treding@nvidia.com> wrote: >> The idea is that you'd grab the DPCD field anyway since it's needed all >> over the place. We have a pile of helpers already that take exactly this >> block and decode parts of it. So I think this makes sense - dp aux is fast >> but not entirely free, so caching seems useful. > > If we want to always cache part of the DPCD wouldn't it be better to add > the cache to struct drm_dp_aux instead of having to duplicate this in > every driver? Right now we don't (yet) have helper functions to probe SST DP sinks, so there's not really a good place to put it. But yeah longer-term I hope that some shared SST sink probe functions shows up (especially to handle all the various crazy corner-cases with VGA probing, e.g. the apple vga dongle patch Dave sent out many aeons ago), and then putting that cache into the dp aux struct makes sense. But before that happens I don't think it makes a lot of sense really, since then we'd also need to add code to invalidate that cache. Simpler if drivers just take care of that themselves. -Daniel
On Monday 03 November 2014 01:58 PM, Daniel Vetter wrote: > On Mon, Nov 3, 2014 at 9:25 AM, Thierry Reding <treding@nvidia.com> wrote: >>> The idea is that you'd grab the DPCD field anyway since it's needed all >>> over the place. We have a pile of helpers already that take exactly this >>> block and decode parts of it. So I think this makes sense - dp aux is fast >>> but not entirely free, so caching seems useful. >> If we want to always cache part of the DPCD wouldn't it be better to add >> the cache to struct drm_dp_aux instead of having to duplicate this in >> every driver? > Right now we don't (yet) have helper functions to probe SST DP sinks, > so there's not really a good place to put it. But yeah longer-term I > hope that some shared SST sink probe functions shows up (especially to > handle all the various crazy corner-cases with VGA probing, e.g. the > apple vga dongle patch Dave sent out many aeons ago), and then putting > that cache into the dp aux struct makes sense. But before that happens > I don't think it makes a lot of sense really, since then we'd also > need to add code to invalidate that cache. Simpler if drivers just > take care of that themselves. > -Daniel For the edp revision, I am feeling that having a function which simply reads DP_EDP_REV, and returns it doesn't add much value. So, I am thinking to put that part in driver itself. Please let me know if you feel otherwise. -Sonika
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 08e33b8..a54a760 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) i2c_del_adapter(&aux->ddc); } EXPORT_SYMBOL(drm_dp_aux_unregister); + +bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) +{ + uint8_t reg; + + if (dpcd[DP_EDP_CONFIGURATION_CAP] & + DP_DPCD_DISPLAY_CONTROL_CAPABLE) { + + if (drm_dp_dpcd_read(aux, DP_EDP_REV, ®, 1)) + if (reg == 0x03) + return true; + } + return false; +} +EXPORT_SYMBOL(drm_dp_is_edp_v1_4); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 8edeed0..b017e1e 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -102,6 +102,8 @@ #define DP_EDP_CONFIGURATION_CAP 0x00d /* XXX 1.2? */ #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ +#define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) +#define DP_EDP_REV 0x700 /* Multiple stream transport */ #define DP_FAUX_CAP 0x020 /* 1.2 */