| Message ID | 1523955333-8587-2-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 17, 2018 at 02:25:32PM +0530, Ramalingam C wrote: > >From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is > 511Bytes instead of previous 256Bytes used in SW. > > This change allows the max bytes per op upto 511Bytes from Gen9 onwards. > > v2: > No Change. > v3: > Inline function for max_xfer_size and renaming of the macro.[Jani] > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++-- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 475cac07d3e6..be6114a0e8ab 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3013,6 +3013,7 @@ enum i915_power_well_id { > #define GMBUS_CYCLE_STOP (4<<25) > #define GMBUS_BYTE_COUNT_SHIFT 16 > #define GMBUS_BYTE_COUNT_MAX 256U > +#define GEN9_GMBUS_BYTE_COUNT_MAX 511U > #define GMBUS_SLAVE_INDEX_SHIFT 8 > #define GMBUS_SLAVE_ADDR_SHIFT 1 > #define GMBUS_SLAVE_READ (1<<0) > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > index e6875509bcd9..4367827d7661 100644 > --- a/drivers/gpu/drm/i915/intel_i2c.c > +++ b/drivers/gpu/drm/i915/intel_i2c.c > @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv) > return ret; > } > > +static inline > +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv) > +{ > + return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX : > + GMBUS_BYTE_COUNT_MAX; Hmm. You sure about this 256 limit on older HW? The spec does sort of say that 0-256 is the valid range, but the SPT+ docs still have that same text, and the register has always had 9 bits for byte count. I don't see any statements saying that they changed this in any way for SPT. It only talks about >511 bytes needing the special treatment. If we do this the I think you should just drop the defines and put the raw numbers into this function. The extra indirection just makes life harder. Also pointless parens around the GEN>9 check. > +} > + > static int > gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv, > unsigned short addr, u8 *buf, unsigned int len, > @@ -400,7 +407,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, > int ret; > > do { > - len = min(rx_size, GMBUS_BYTE_COUNT_MAX); > + len = min(rx_size, gmbus_max_xfer_size(dev_priv)); > > ret = gmbus_xfer_read_chunk(dev_priv, msg->addr, > buf, len, gmbus1_index); > @@ -462,7 +469,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg, > int ret; > > do { > - len = min(tx_size, GMBUS_BYTE_COUNT_MAX); > + len = min(tx_size, gmbus_max_xfer_size(dev_priv)); > > ret = gmbus_xfer_write_chunk(dev_priv, msg->addr, buf, len, > gmbus1_index); > -- > 2.7.4
On Tuesday 17 April 2018 11:39 PM, Ville Syrjälä wrote: > On Tue, Apr 17, 2018 at 02:25:32PM +0530, Ramalingam C wrote: >> >From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is >> 511Bytes instead of previous 256Bytes used in SW. >> >> This change allows the max bytes per op upto 511Bytes from Gen9 onwards. >> >> v2: >> No Change. >> v3: >> Inline function for max_xfer_size and renaming of the macro.[Jani] >> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++-- >> 2 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 475cac07d3e6..be6114a0e8ab 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -3013,6 +3013,7 @@ enum i915_power_well_id { >> #define GMBUS_CYCLE_STOP (4<<25) >> #define GMBUS_BYTE_COUNT_SHIFT 16 >> #define GMBUS_BYTE_COUNT_MAX 256U >> +#define GEN9_GMBUS_BYTE_COUNT_MAX 511U >> #define GMBUS_SLAVE_INDEX_SHIFT 8 >> #define GMBUS_SLAVE_ADDR_SHIFT 1 >> #define GMBUS_SLAVE_READ (1<<0) >> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c >> index e6875509bcd9..4367827d7661 100644 >> --- a/drivers/gpu/drm/i915/intel_i2c.c >> +++ b/drivers/gpu/drm/i915/intel_i2c.c >> @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv) >> return ret; >> } >> >> +static inline >> +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv) >> +{ >> + return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX : >> + GMBUS_BYTE_COUNT_MAX; > Hmm. You sure about this 256 limit on older HW? The spec does sort of > say that 0-256 is the valid range, but the SPT+ docs still have that > same text, and the register has always had 9 bits for byte count. I > don't see any statements saying that they changed this in any way for > SPT. It only talks about >511 bytes needing the special treatment. > > If we do this the I think you should just drop the defines and put the > raw numbers into this function. The extra indirection just makes life > harder. Also pointless parens around the GEN>9 check. Even I couldn't get any place where BSpec says 256Bytes is the limit for any platform. Everywhere 9bits are used. And when I cross verified with other OS usage 511Bytes is used as limit across all platforms. Just to be cautious for not breaking any older platforms out in linux world, I limited the extension of the limit to the known and easily testable platforms at my desk (Gen9+) Do you suggest we should apply 511Bytes as max limit for all platforms? Do we have any means to test this new limit on all supported legacy platforms? Except enabling the full potential of the HW in SW, I dont see any ROI here as most of the GMBUS reqs are <256Bytes. Only in case of HDCP2.2 we need single read cycle for 538Bytes. we have couple of options here: Please share your opinion to choose one of them. 1. Just dont change the upper limit for RD/WR. Keep it as it is at 256Bytes. Anyway no user demands it. 2. As per HW capability, Change the upper limit for RD/WR to 511Bytes for all platforms. This is needs the functional verification on all legacy plat supported. 3. Change the upper limit for RD/WR to 511Bytes for newer platforms, say Gen9+. --Ram > >> +} >> + >> static int >> gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv, >> unsigned short addr, u8 *buf, unsigned int len, >> @@ -400,7 +407,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, >> int ret; >> >> do { >> - len = min(rx_size, GMBUS_BYTE_COUNT_MAX); >> + len = min(rx_size, gmbus_max_xfer_size(dev_priv)); >> >> ret = gmbus_xfer_read_chunk(dev_priv, msg->addr, >> buf, len, gmbus1_index); >> @@ -462,7 +469,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg, >> int ret; >> >> do { >> - len = min(tx_size, GMBUS_BYTE_COUNT_MAX); >> + len = min(tx_size, gmbus_max_xfer_size(dev_priv)); >> >> ret = gmbus_xfer_write_chunk(dev_priv, msg->addr, buf, len, >> gmbus1_index); >> -- >> 2.7.4
On Wed, 18 Apr 2018, Ramalingam C <ramalingam.c@intel.com> wrote: > On Tuesday 17 April 2018 11:39 PM, Ville Syrjälä wrote: >> On Tue, Apr 17, 2018 at 02:25:32PM +0530, Ramalingam C wrote: >>> >From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is >>> 511Bytes instead of previous 256Bytes used in SW. >>> >>> This change allows the max bytes per op upto 511Bytes from Gen9 onwards. >>> >>> v2: >>> No Change. >>> v3: >>> Inline function for max_xfer_size and renaming of the macro.[Jani] >>> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_reg.h | 1 + >>> drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++-- >>> 2 files changed, 10 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>> index 475cac07d3e6..be6114a0e8ab 100644 >>> --- a/drivers/gpu/drm/i915/i915_reg.h >>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>> @@ -3013,6 +3013,7 @@ enum i915_power_well_id { >>> #define GMBUS_CYCLE_STOP (4<<25) >>> #define GMBUS_BYTE_COUNT_SHIFT 16 >>> #define GMBUS_BYTE_COUNT_MAX 256U >>> +#define GEN9_GMBUS_BYTE_COUNT_MAX 511U >>> #define GMBUS_SLAVE_INDEX_SHIFT 8 >>> #define GMBUS_SLAVE_ADDR_SHIFT 1 >>> #define GMBUS_SLAVE_READ (1<<0) >>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c >>> index e6875509bcd9..4367827d7661 100644 >>> --- a/drivers/gpu/drm/i915/intel_i2c.c >>> +++ b/drivers/gpu/drm/i915/intel_i2c.c >>> @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv) >>> return ret; >>> } >>> >>> +static inline >>> +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv) >>> +{ >>> + return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX : >>> + GMBUS_BYTE_COUNT_MAX; >> Hmm. You sure about this 256 limit on older HW? The spec does sort of >> say that 0-256 is the valid range, but the SPT+ docs still have that >> same text, and the register has always had 9 bits for byte count. I >> don't see any statements saying that they changed this in any way for >> SPT. It only talks about >511 bytes needing the special treatment. >> >> If we do this the I think you should just drop the defines and put the >> raw numbers into this function. The extra indirection just makes life >> harder. Also pointless parens around the GEN>9 check. > Even I couldn't get any place where BSpec says 256Bytes is the limit for > any platform. Everywhere 9bits are used. > And when I cross verified with other OS usage 511Bytes is used as limit > across all platforms. > > Just to be cautious for not breaking any older platforms out in linux > world, I limited the extension of the limit to the known > and easily testable platforms at my desk (Gen9+) > > Do you suggest we should apply 511Bytes as max limit for all platforms? > Do we have any means to test this new limit on all supported legacy > platforms? > > Except enabling the full potential of the HW in SW, I dont see any ROI > here as most of the GMBUS reqs are <256Bytes. > Only in case of HDCP2.2 we need single read cycle for 538Bytes. > > we have couple of options here: Please share your opinion to choose one > of them. > 1. Just dont change the upper limit for RD/WR. Keep it as it is at > 256Bytes. Anyway no user demands it. > 2. As per HW capability, Change the upper limit for RD/WR to 511Bytes > for all platforms. This is needs the functional verification on all > legacy plat supported. > 3. Change the upper limit for RD/WR to 511Bytes for newer platforms, say > Gen9+. Please let's not change the limit for old platforms for absolutely no gain. And if Ville insists anyway, let's leave that as a separate follow-up change that can easily be reverted later. I might consider using the 511 limit only for platforms that HAS_GMBUS_BURST_READ too. The original limit seems to have been added in 9535c4757b88 ("drm/i915: cope with large i2c transfers") citing "the specs". Any recollection anyone? Chris? BR, Jani. > > --Ram >> >>> +} >>> + >>> static int >>> gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv, >>> unsigned short addr, u8 *buf, unsigned int len, >>> @@ -400,7 +407,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, >>> int ret; >>> >>> do { >>> - len = min(rx_size, GMBUS_BYTE_COUNT_MAX); >>> + len = min(rx_size, gmbus_max_xfer_size(dev_priv)); >>> >>> ret = gmbus_xfer_read_chunk(dev_priv, msg->addr, >>> buf, len, gmbus1_index); >>> @@ -462,7 +469,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg, >>> int ret; >>> >>> do { >>> - len = min(tx_size, GMBUS_BYTE_COUNT_MAX); >>> + len = min(tx_size, gmbus_max_xfer_size(dev_priv)); >>> >>> ret = gmbus_xfer_write_chunk(dev_priv, msg->addr, buf, len, >>> gmbus1_index); >>> -- >>> 2.7.4 >
On Wed, Apr 18, 2018 at 09:20:23AM +0300, Jani Nikula wrote: > On Wed, 18 Apr 2018, Ramalingam C <ramalingam.c@intel.com> wrote: > > On Tuesday 17 April 2018 11:39 PM, Ville Syrjälä wrote: > >> On Tue, Apr 17, 2018 at 02:25:32PM +0530, Ramalingam C wrote: > >>> >From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is > >>> 511Bytes instead of previous 256Bytes used in SW. > >>> > >>> This change allows the max bytes per op upto 511Bytes from Gen9 onwards. > >>> > >>> v2: > >>> No Change. > >>> v3: > >>> Inline function for max_xfer_size and renaming of the macro.[Jani] > >>> > >>> Cc: Jani Nikula <jani.nikula@intel.com> > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > >>> --- > >>> drivers/gpu/drm/i915/i915_reg.h | 1 + > >>> drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++-- > >>> 2 files changed, 10 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >>> index 475cac07d3e6..be6114a0e8ab 100644 > >>> --- a/drivers/gpu/drm/i915/i915_reg.h > >>> +++ b/drivers/gpu/drm/i915/i915_reg.h > >>> @@ -3013,6 +3013,7 @@ enum i915_power_well_id { > >>> #define GMBUS_CYCLE_STOP (4<<25) > >>> #define GMBUS_BYTE_COUNT_SHIFT 16 > >>> #define GMBUS_BYTE_COUNT_MAX 256U > >>> +#define GEN9_GMBUS_BYTE_COUNT_MAX 511U > >>> #define GMBUS_SLAVE_INDEX_SHIFT 8 > >>> #define GMBUS_SLAVE_ADDR_SHIFT 1 > >>> #define GMBUS_SLAVE_READ (1<<0) > >>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > >>> index e6875509bcd9..4367827d7661 100644 > >>> --- a/drivers/gpu/drm/i915/intel_i2c.c > >>> +++ b/drivers/gpu/drm/i915/intel_i2c.c > >>> @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv) > >>> return ret; > >>> } > >>> > >>> +static inline > >>> +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv) > >>> +{ > >>> + return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX : > >>> + GMBUS_BYTE_COUNT_MAX; > >> Hmm. You sure about this 256 limit on older HW? The spec does sort of > >> say that 0-256 is the valid range, but the SPT+ docs still have that > >> same text, and the register has always had 9 bits for byte count. I > >> don't see any statements saying that they changed this in any way for > >> SPT. It only talks about >511 bytes needing the special treatment. > >> > >> If we do this the I think you should just drop the defines and put the > >> raw numbers into this function. The extra indirection just makes life > >> harder. Also pointless parens around the GEN>9 check. > > Even I couldn't get any place where BSpec says 256Bytes is the limit for > > any platform. Everywhere 9bits are used. > > And when I cross verified with other OS usage 511Bytes is used as limit > > across all platforms. > > > > Just to be cautious for not breaking any older platforms out in linux > > world, I limited the extension of the limit to the known > > and easily testable platforms at my desk (Gen9+) > > > > Do you suggest we should apply 511Bytes as max limit for all platforms? > > Do we have any means to test this new limit on all supported legacy > > platforms? > > > > Except enabling the full potential of the HW in SW, I dont see any ROI > > here as most of the GMBUS reqs are <256Bytes. > > Only in case of HDCP2.2 we need single read cycle for 538Bytes. > > > > we have couple of options here: Please share your opinion to choose one > > of them. > > 1. Just dont change the upper limit for RD/WR. Keep it as it is at > > 256Bytes. Anyway no user demands it. > > 2. As per HW capability, Change the upper limit for RD/WR to 511Bytes > > for all platforms. This is needs the functional verification on all > > legacy plat supported. > > 3. Change the upper limit for RD/WR to 511Bytes for newer platforms, say > > Gen9+. > > Please let's not change the limit for old platforms for absolutely no > gain. And if Ville insists anyway, let's leave that as a separate > follow-up change that can easily be reverted later. > > I might consider using the 511 limit only for platforms that > HAS_GMBUS_BURST_READ too. > > The original limit seems to have been added in 9535c4757b88 ("drm/i915: > cope with large i2c transfers") citing "the specs". Any recollection > anyone? Chris? "Duble buffered data register and a 9 bit counter support 0 byte to 256 Byte transfers." has always been in the spec (and still is for SPT+).
On Wednesday 18 April 2018 08:47 PM, Ville Syrjälä wrote: > On Wed, Apr 18, 2018 at 09:20:23AM +0300, Jani Nikula wrote: >> On Wed, 18 Apr 2018, Ramalingam C <ramalingam.c@intel.com> wrote: >>> On Tuesday 17 April 2018 11:39 PM, Ville Syrjälä wrote: >>>> On Tue, Apr 17, 2018 at 02:25:32PM +0530, Ramalingam C wrote: >>>>> >From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is >>>>> 511Bytes instead of previous 256Bytes used in SW. >>>>> >>>>> This change allows the max bytes per op upto 511Bytes from Gen9 onwards. >>>>> >>>>> v2: >>>>> No Change. >>>>> v3: >>>>> Inline function for max_xfer_size and renaming of the macro.[Jani] >>>>> >>>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/i915_reg.h | 1 + >>>>> drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++-- >>>>> 2 files changed, 10 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>>> index 475cac07d3e6..be6114a0e8ab 100644 >>>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>>> @@ -3013,6 +3013,7 @@ enum i915_power_well_id { >>>>> #define GMBUS_CYCLE_STOP (4<<25) >>>>> #define GMBUS_BYTE_COUNT_SHIFT 16 >>>>> #define GMBUS_BYTE_COUNT_MAX 256U >>>>> +#define GEN9_GMBUS_BYTE_COUNT_MAX 511U >>>>> #define GMBUS_SLAVE_INDEX_SHIFT 8 >>>>> #define GMBUS_SLAVE_ADDR_SHIFT 1 >>>>> #define GMBUS_SLAVE_READ (1<<0) >>>>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c >>>>> index e6875509bcd9..4367827d7661 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_i2c.c >>>>> +++ b/drivers/gpu/drm/i915/intel_i2c.c >>>>> @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv) >>>>> return ret; >>>>> } >>>>> >>>>> +static inline >>>>> +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv) >>>>> +{ >>>>> + return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX : >>>>> + GMBUS_BYTE_COUNT_MAX; >>>> Hmm. You sure about this 256 limit on older HW? The spec does sort of >>>> say that 0-256 is the valid range, but the SPT+ docs still have that >>>> same text, and the register has always had 9 bits for byte count. I >>>> don't see any statements saying that they changed this in any way for >>>> SPT. It only talks about >511 bytes needing the special treatment. >>>> >>>> If we do this the I think you should just drop the defines and put the >>>> raw numbers into this function. The extra indirection just makes life >>>> harder. Also pointless parens around the GEN>9 check. >>> Even I couldn't get any place where BSpec says 256Bytes is the limit for >>> any platform. Everywhere 9bits are used. >>> And when I cross verified with other OS usage 511Bytes is used as limit >>> across all platforms. >>> >>> Just to be cautious for not breaking any older platforms out in linux >>> world, I limited the extension of the limit to the known >>> and easily testable platforms at my desk (Gen9+) >>> >>> Do you suggest we should apply 511Bytes as max limit for all platforms? >>> Do we have any means to test this new limit on all supported legacy >>> platforms? >>> >>> Except enabling the full potential of the HW in SW, I dont see any ROI >>> here as most of the GMBUS reqs are <256Bytes. >>> Only in case of HDCP2.2 we need single read cycle for 538Bytes. >>> >>> we have couple of options here: Please share your opinion to choose one >>> of them. >>> 1. Just dont change the upper limit for RD/WR. Keep it as it is at >>> 256Bytes. Anyway no user demands it. >>> 2. As per HW capability, Change the upper limit for RD/WR to 511Bytes >>> for all platforms. This is needs the functional verification on all >>> legacy plat supported. >>> 3. Change the upper limit for RD/WR to 511Bytes for newer platforms, say >>> Gen9+. >> Please let's not change the limit for old platforms for absolutely no >> gain. And if Ville insists anyway, let's leave that as a separate >> follow-up change that can easily be reverted later. >> >> I might consider using the 511 limit only for platforms that >> HAS_GMBUS_BURST_READ too. >> >> The original limit seems to have been added in 9535c4757b88 ("drm/i915: >> cope with large i2c transfers") citing "the specs". Any recollection >> anyone? Chris? > "Duble buffered data register and a 9 bit counter support 0 byte to 256 > Byte transfers." > has always been in the spec (and still is for SPT+). Seems like gap in Bspec. Just today HW team has clarified that all platforms support 511Bytes for single Gmbus read and write cycle. --Ram >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 475cac07d3e6..be6114a0e8ab 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3013,6 +3013,7 @@ enum i915_power_well_id { #define GMBUS_CYCLE_STOP (4<<25) #define GMBUS_BYTE_COUNT_SHIFT 16 #define GMBUS_BYTE_COUNT_MAX 256U +#define GEN9_GMBUS_BYTE_COUNT_MAX 511U #define GMBUS_SLAVE_INDEX_SHIFT 8 #define GMBUS_SLAVE_ADDR_SHIFT 1 #define GMBUS_SLAVE_READ (1<<0) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index e6875509bcd9..4367827d7661 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv) return ret; } +static inline +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv) +{ + return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX : + GMBUS_BYTE_COUNT_MAX; +} + static int gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv, unsigned short addr, u8 *buf, unsigned int len, @@ -400,7 +407,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, int ret; do { - len = min(rx_size, GMBUS_BYTE_COUNT_MAX); + len = min(rx_size, gmbus_max_xfer_size(dev_priv)); ret = gmbus_xfer_read_chunk(dev_priv, msg->addr, buf, len, gmbus1_index); @@ -462,7 +469,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg, int ret; do { - len = min(tx_size, GMBUS_BYTE_COUNT_MAX); + len = min(tx_size, gmbus_max_xfer_size(dev_priv)); ret = gmbus_xfer_write_chunk(dev_priv, msg->addr, buf, len, gmbus1_index);
From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is 511Bytes instead of previous 256Bytes used in SW. This change allows the max bytes per op upto 511Bytes from Gen9 onwards. v2: No Change. v3: Inline function for max_xfer_size and renaming of the macro.[Jani] Cc: Jani Nikula <jani.nikula@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-)