| Message ID | 1345533048-8898-2-git-send-email-s.shirish@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Dienstag, den 21.08.2012, 12:40 +0530 schrieb Shirish S: > The current logic for probing ddc is limited to > 2 blocks (256 bytes), this patch adds support > for the 4 block (512) data. > > To do this, a single 8-bit segment index is > passed to the display via the I2C address 30h. > Data from the selected segment is then immediately > read via the regular DDC2 address using a repeated > I2C 'START' signal. > > Signed-off-by: Shirish S <s.shirish@samsung.com> Please add your full last name, if there is no reason not to, and resend as [PATCH v2]. > --- > drivers/gpu/drm/drm_edid.c | 17 ++++++++++++----- > 1 files changed, 12 insertions(+), 5 deletions(-) […] Thanks, Paul
On Tue, Aug 21, 2012 at 9:10 AM, Shirish S <s.shirish@samsung.com> wrote: > The current logic for probing ddc is limited to > 2 blocks (256 bytes), this patch adds support > for the 4 block (512) data. > > To do this, a single 8-bit segment index is > passed to the display via the I2C address 30h. > Data from the selected segment is then immediately > read via the regular DDC2 address using a repeated > I2C 'START' signal. > > Signed-off-by: Shirish S <s.shirish@samsung.com> > --- > drivers/gpu/drm/drm_edid.c | 17 ++++++++++++----- > 1 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index a8743c3..2c2996e 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -254,6 +254,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, > int block, int len) > { > unsigned char start = block * EDID_LENGTH; > + unsigned char segment = block >> 1; > + unsigned short segFlags = segment ? 0 : I2C_M_IGNORE_NAK; Have you tested this on i915 with gmbus enabled? I'm asking since we don't implement the IGNORE_NAK flag and hence I'd expect spurious failures on displays that don't support E-DDC ... Cheers, Daniel > int ret, retries = 5; > > /* The core i2c driver will automatically retry the transfer if the > @@ -264,27 +266,32 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, > */ > do { > struct i2c_msg msgs[] = { > - { > + { /*set segment pointer */ > + .addr = DDC_SEGMENT_ADDR, > + .flags = segFlags, > + .len = 1, > + .buf = &start, > + }, { /*set offset */ > .addr = DDC_ADDR, > .flags = 0, > .len = 1, > .buf = &start, > - }, { > + }, { /*set data */ > .addr = DDC_ADDR, > .flags = I2C_M_RD, > .len = len, > .buf = buf, > } > }; > - ret = i2c_transfer(adapter, msgs, 2); > + ret = i2c_transfer(adapter, msgs, 3); > if (ret == -ENXIO) { > DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", > adapter->name); > break; > } > - } while (ret != 2 && --retries); > + } while (ret != 3 && --retries); > > - return ret == 2 ? 0 : -1; > + return ret == 3 ? 0 : -1; > } > > static bool drm_edid_is_zero(u8 *in_edid, int length) > -- > 1.7.0.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Aug 21, 2012 at 12:40:48PM +0530, Shirish S wrote: > The current logic for probing ddc is limited to > 2 blocks (256 bytes), this patch adds support > for the 4 block (512) data. A patch for this was already sent a long time ago: http://lists.freedesktop.org/archives/dri-devel/2011-December/017246.html
Hello Daniel, On Tue, Aug 21, 2012 at 4:18 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Aug 21, 2012 at 9:10 AM, Shirish S <s.shirish@samsung.com> wrote: > > The current logic for probing ddc is limited to > > 2 blocks (256 bytes), this patch adds support > > for the 4 block (512) data. > > > > To do this, a single 8-bit segment index is > > passed to the display via the I2C address 30h. > > Data from the selected segment is then immediately > > read via the regular DDC2 address using a repeated > > I2C 'START' signal. > > > > Signed-off-by: Shirish S <s.shirish@samsung.com> > > --- > > drivers/gpu/drm/drm_edid.c | 17 ++++++++++++----- > > 1 files changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index a8743c3..2c2996e 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -254,6 +254,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, > unsigned char *buf, > > int block, int len) > > { > > unsigned char start = block * EDID_LENGTH; > > + unsigned char segment = block >> 1; > > + unsigned short segFlags = segment ? 0 : I2C_M_IGNORE_NAK; > > Have you tested this on i915 with gmbus enabled? I'm asking since we > don't implement the IGNORE_NAK flag and hence I'd expect spurious > failures on displays that don't support E-DDC ... > > I have verified this on samsung exynos5 platform, and it passed the HDMI compliance test for the same. I also verified this on HDMI analyser- Agilent N5988A , this analyser does not support 4 block EDID data(EDDC), it passed in this analyser as well. Is there any specific reason why you dont implement IGNORE_NAK? Infact if i think for EDDC, if one does not pass IGNORE_NAK flag it might give errors. > Cheers, Daniel > > > int ret, retries = 5; > > > > /* The core i2c driver will automatically retry the transfer if > the > > @@ -264,27 +266,32 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, > unsigned char *buf, > > */ > > do { > > struct i2c_msg msgs[] = { > > - { > > + { /*set segment pointer */ > > + .addr = DDC_SEGMENT_ADDR, > > + .flags = segFlags, > > + .len = 1, > > + .buf = &start, > > + }, { /*set offset */ > > .addr = DDC_ADDR, > > .flags = 0, > > .len = 1, > > .buf = &start, > > - }, { > > + }, { /*set data */ > > .addr = DDC_ADDR, > > .flags = I2C_M_RD, > > .len = len, > > .buf = buf, > > } > > }; > > - ret = i2c_transfer(adapter, msgs, 2); > > + ret = i2c_transfer(adapter, msgs, 3); > > if (ret == -ENXIO) { > > DRM_DEBUG_KMS("drm: skipping non-existent > adapter %s\n", > > adapter->name); > > break; > > } > > - } while (ret != 2 && --retries); > > + } while (ret != 3 && --retries); > > > > - return ret == 2 ? 0 : -1; > > + return ret == 3 ? 0 : -1; > > } > > > > static bool drm_edid_is_zero(u8 *in_edid, int length) > > -- > > 1.7.0.4 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Daniel Vetter > daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > Regards, Shirish S
Hello Ville Syrjälä, On Tue, Aug 21, 2012 at 4:26 AM, Ville Syrjälä < ville.syrjala@linux.intel.com> wrote: > On Tue, Aug 21, 2012 at 12:40:48PM +0530, Shirish S wrote: > > The current logic for probing ddc is limited to > > 2 blocks (256 bytes), this patch adds support > > for the 4 block (512) data. > > A patch for this was already sent a long time ago: > http://lists.freedesktop.org/archives/dri-devel/2011-December/017246.html > > I tried the patch you have mentioned,but its not working in my setup. Also did anyone else test this!! I find that although the author asks the i2c to read for 3 msgs, it verifies only for 2.Kindly correct me if i am wrong. My patch i have verified on the analyser for exynos5 platform. > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > Regards, Shirish S
On Tue, Aug 21, 2012 at 06:55:53AM -0700, Shirish S wrote: > Hello Ville Syrjälä, > > On Tue, Aug 21, 2012 at 4:26 AM, Ville Syrjälä < > ville.syrjala@linux.intel.com> wrote: > > > On Tue, Aug 21, 2012 at 12:40:48PM +0530, Shirish S wrote: > > > The current logic for probing ddc is limited to > > > 2 blocks (256 bytes), this patch adds support > > > for the 4 block (512) data. > > > > A patch for this was already sent a long time ago: > > http://lists.freedesktop.org/archives/dri-devel/2011-December/017246.html > > > > I tried the patch you have mentioned,but its not working in my setup. > Also did anyone else test this!! Not that I know. > I find that although the author asks the i2c to read for 3 msgs, it > verifies only for 2.Kindly correct me if i am wrong. Yeah, it looks like the return value isn't checked correctly. > My patch i have verified on the analyser for exynos5 platform. Your patch always sends the segment which seems a bit pointless, and possibly questionable in case a non-EDDC display gets confused by the segment information. Jean's patch only sends the segment when needed, which feels like the safer option, and it would also avoid increasing the amount of i2c traffic. Here are my earlier comments on Jean's patch: http://lists.freedesktop.org/archives/dri-devel/2012-February/019069.html
On Tue, Aug 21, 2012 at 7:56 AM, Ville Syrjälä < ville.syrjala@linux.intel.com> wrote: > On Tue, Aug 21, 2012 at 06:55:53AM -0700, Shirish S wrote: > > Hello Ville Syrjälä, > > > > On Tue, Aug 21, 2012 at 4:26 AM, Ville Syrjälä < > > ville.syrjala@linux.intel.com> wrote: > > > > > On Tue, Aug 21, 2012 at 12:40:48PM +0530, Shirish S wrote: > > > > The current logic for probing ddc is limited to > > > > 2 blocks (256 bytes), this patch adds support > > > > for the 4 block (512) data. > > > > > > A patch for this was already sent a long time ago: > > > > http://lists.freedesktop.org/archives/dri-devel/2011-December/017246.html > > > > > > I tried the patch you have mentioned,but its not working in my setup. > > Also did anyone else test this!! > > Not that I know. > > > I find that although the author asks the i2c to read for 3 msgs, it > > verifies only for 2.Kindly correct me if i am wrong. > > Yeah, it looks like the return value isn't checked correctly. > > > My patch i have verified on the analyser for exynos5 platform. > > Your patch always sends the segment which seems a bit pointless, and > possibly questionable in case a non-EDDC display gets confused by the > segment information. Jean's patch only sends the segment when needed, > which feels like the safer option, and it would also avoid increasing > the amount of i2c traffic. > > As per the spec we need to pass a single 8-bit segment index to the display via the I2C address 30h which is DDC_SEGMENT_ADDR, and the ACK is mandatory on the same for block 2 and 3. (the first of i2c_msg). Although the verification of this is not required or mandatory for non-EDDC(block 0 and 1), i have verified that its presence does not affect non-EDDC displays.(Using HDMI analyser- Agilent N5988A) Another option to avoid i2c traffic would be to add another function to probe EDDC blocks,but beg to differ i dont think current method would affect i2c traffic to an alarming extent. Here are my earlier comments on Jean's patch: > http://lists.freedesktop.org/archives/dri-devel/2012-February/019069.html > > If i am not wrong am doing exactly what you have said in you comments. This seems a bit wrong to me. The spec says that the ack for the segment address is "don't care", but for the segment pointer the ack is required (when segment != 0). The variable segFlags is "dont care for block 0 and 1 wheras". With I2C_M_IGNORE_NAK we would in fact end up reading segment 0 from a non E-DDC display, if we try to read segment != 0 from it. Of course we shouldn't do that unless the display lied to us about what extension blocks it provides. So I'm not sure if it would be better to trust that the display never lies about the extension blocks, or if we should just assume all E-DDC displays ack both segment addr and pointer. The no-ack feature seems to there for backwards compatibility, for cases where the host always sends the segment addr/pointer even when reading segment 0 (which your code doesn't do). To handle it exactly as the spec says, I2C_M_IGNORE_NAK should be split into two flags (one for addr, other for data). Hence i have split the i2c_msg into 3, segment pointer,offset(addr) and data pointer. "a single 8-bit segment index is passed to the display via the I2C address 30h(segment pointer). Data from the selected segment is then immediately read via the regular DDC2 address using a repeated I2C 'START' signal" -- > Ville Syrjälä > Intel OTC > Regards, Shirish S
On Tue, Aug 21, 2012 at 04:28:20PM -0700, Shirish S wrote: > On Tue, Aug 21, 2012 at 7:56 AM, Ville Syrjälä < > ville.syrjala@linux.intel.com> wrote: > > > On Tue, Aug 21, 2012 at 06:55:53AM -0700, Shirish S wrote: > Here are my earlier comments on Jean's patch: > > http://lists.freedesktop.org/archives/dri-devel/2012-February/019069.html > > > > > If i am not wrong am doing exactly what you have said in you comments. > > This seems a bit wrong to me. The spec says that the ack for the > segment address is "don't care", but for the segment pointer the ack is > required (when segment != 0). > The variable segFlags is "dont care for block 0 and 1 wheras". > > With I2C_M_IGNORE_NAK we would in fact end up reading segment 0 from a > non E-DDC display, if we try to read segment != 0 from it. Of course > we shouldn't do that unless the display lied to us about what extension > blocks it provides. > > So I'm not sure if it would be better to trust that the display never > lies about the extension blocks, or if we should just assume all E-DDC > displays ack both segment addr and pointer. The no-ack feature seems > to there for backwards compatibility, for cases where the host always > sends the segment addr/pointer even when reading segment 0 (which your > code doesn't do). > > To handle it exactly as the spec says, I2C_M_IGNORE_NAK should be split > into two flags (one for addr, other for data). > > Hence i have split the i2c_msg into 3, segment pointer,offset(addr) > and data pointer. I was referring to the addr and data phases of the segment pointer. According to the spec the ack for the addr is always optional. But I suppose no sane device would nak the addr, while acking the data.
On Wed, Aug 22, 2012 at 10:52:26AM +0300, Ville Syrjälä wrote: > On Tue, Aug 21, 2012 at 04:28:20PM -0700, Shirish S wrote: > > On Tue, Aug 21, 2012 at 7:56 AM, Ville Syrjälä < > > ville.syrjala@linux.intel.com> wrote: > > > > > On Tue, Aug 21, 2012 at 06:55:53AM -0700, Shirish S wrote: > > Here are my earlier comments on Jean's patch: > > > http://lists.freedesktop.org/archives/dri-devel/2012-February/019069.html > > > > > > > > If i am not wrong am doing exactly what you have said in you comments. > > > > This seems a bit wrong to me. The spec says that the ack for the > > segment address is "don't care", but for the segment pointer the ack is > > required (when segment != 0). > > The variable segFlags is "dont care for block 0 and 1 wheras". > > > > With I2C_M_IGNORE_NAK we would in fact end up reading segment 0 from a > > non E-DDC display, if we try to read segment != 0 from it. Of course > > we shouldn't do that unless the display lied to us about what extension > > blocks it provides. > > > > So I'm not sure if it would be better to trust that the display never > > lies about the extension blocks, or if we should just assume all E-DDC > > displays ack both segment addr and pointer. The no-ack feature seems > > to there for backwards compatibility, for cases where the host always > > sends the segment addr/pointer even when reading segment 0 (which your > > code doesn't do). > > > > To handle it exactly as the spec says, I2C_M_IGNORE_NAK should be split > > into two flags (one for addr, other for data). > > > > Hence i have split the i2c_msg into 3, segment pointer,offset(addr) > > and data pointer. > > I was referring to the addr and data phases of the segment pointer. > According to the spec the ack for the addr is always optional. But I > suppose no sane device would nak the addr, while acking the data. We've seen those. Really. Which is why the current i915 gmbus driver has a hack to never return a NaK on the first i2c transfer. I guess we should fix this by properly supporting the INGNORE_NAK field in our gmbus driver, and setting that on the addr transfer field, too. I concure with Ville that sending the segment i2c message only when we actually need it, and not unconditionally. DDC is way to broken and claiming that the spec says otherwise doesn't fix all the existing bad hw. -Daniel
Hello Ville, On Wed, Aug 22, 2012 at 12:52 AM, Ville Syrjälä < ville.syrjala@linux.intel.com> wrote: > On Tue, Aug 21, 2012 at 04:28:20PM -0700, Shirish S wrote: > > On Tue, Aug 21, 2012 at 7:56 AM, Ville Syrjälä < > > ville.syrjala@linux.intel.com> wrote: > > > > > On Tue, Aug 21, 2012 at 06:55:53AM -0700, Shirish S wrote: > > Here are my earlier comments on Jean's patch: > > > > http://lists.freedesktop.org/archives/dri-devel/2012-February/019069.html > > > > > > > > If i am not wrong am doing exactly what you have said in you comments. > > > > This seems a bit wrong to me. The spec says that the ack for the > > segment address is "don't care", but for the segment pointer the ack is > > required (when segment != 0). > > The variable segFlags is "dont care for block 0 and 1 wheras". > > > > With I2C_M_IGNORE_NAK we would in fact end up reading segment 0 from a > > non E-DDC display, if we try to read segment != 0 from it. Of course > > we shouldn't do that unless the display lied to us about what extension > > blocks it provides. > > > > So I'm not sure if it would be better to trust that the display never > > lies about the extension blocks, or if we should just assume all E-DDC > > displays ack both segment addr and pointer. The no-ack feature seems > > to there for backwards compatibility, for cases where the host always > > sends the segment addr/pointer even when reading segment 0 (which your > > code doesn't do). > > > > To handle it exactly as the spec says, I2C_M_IGNORE_NAK should be split > > into two flags (one for addr, other for data). > > > > Hence i have split the i2c_msg into 3, segment pointer,offset(addr) > > and data pointer. > > I was referring to the addr and data phases of the segment pointer. > According to the spec the ack for the addr is always optional. But I > suppose no sane device would nak the addr, while acking the data. > > Thanks for your comment, actually there was a mistake in my code, i have posted the second set. Kindly have a look. > -- > Ville Syrjälä > Intel OTC > Regards, Shirish S
Hello Daniel, On Thu, Aug 23, 2012 at 1:54 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Aug 22, 2012 at 10:52:26AM +0300, Ville Syrjälä wrote: > > On Tue, Aug 21, 2012 at 04:28:20PM -0700, Shirish S wrote: > > > On Tue, Aug 21, 2012 at 7:56 AM, Ville Syrjälä < > > > ville.syrjala@linux.intel.com> wrote: > > > > > > > On Tue, Aug 21, 2012 at 06:55:53AM -0700, Shirish S wrote: > > > Here are my earlier comments on Jean's patch: > > > > > http://lists.freedesktop.org/archives/dri-devel/2012-February/019069.html > > > > > > > > > > > If i am not wrong am doing exactly what you have said in you comments. > > > > > > This seems a bit wrong to me. The spec says that the ack for the > > > segment address is "don't care", but for the segment pointer the ack is > > > required (when segment != 0). > > > The variable segFlags is "dont care for block 0 and 1 wheras". > > > > > > With I2C_M_IGNORE_NAK we would in fact end up reading segment 0 from a > > > non E-DDC display, if we try to read segment != 0 from it. Of course > > > we shouldn't do that unless the display lied to us about what extension > > > blocks it provides. > > > > > > So I'm not sure if it would be better to trust that the display never > > > lies about the extension blocks, or if we should just assume all E-DDC > > > displays ack both segment addr and pointer. The no-ack feature seems > > > to there for backwards compatibility, for cases where the host always > > > sends the segment addr/pointer even when reading segment 0 (which your > > > code doesn't do). > > > > > > To handle it exactly as the spec says, I2C_M_IGNORE_NAK should be split > > > into two flags (one for addr, other for data). > > > > > > Hence i have split the i2c_msg into 3, segment pointer,offset(addr) > > > and data pointer. > > > > I was referring to the addr and data phases of the segment pointer. > > According to the spec the ack for the addr is always optional. But I > > suppose no sane device would nak the addr, while acking the data. > > We've seen those. Really. > > Which is why the current i915 gmbus driver has a hack to never return a > NaK on the first i2c transfer. I guess we should fix this by properly > supporting the INGNORE_NAK field in our gmbus driver, and setting that on > the addr transfer field, too. > > Thanks for the comment, so are you ok with the current logic? > I concure with Ville that sending the segment i2c message only when we > actually need it, and not unconditionally. DDC is way to broken and > claiming that the spec says otherwise doesn't fix all the existing bad hw. > Agreed, so do you want me to post another patch, in which i add a function only if the number of blocks is more than 2? Also i had some mistake in the patch set 1, hence i updated it. Kindly have a look! > -Daniel > -- > Daniel Vetter > Mail: daniel@ffwll.ch > Mobile: +41 (0)79 365 57 48 > Thanks & Regards, Shirish S
On Thu, Aug 23, 2012 at 07:06:53AM -0700, Shirish S wrote: > Hello Daniel, > > On Thu, Aug 23, 2012 at 1:54 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Wed, Aug 22, 2012 at 10:52:26AM +0300, Ville Syrjälä wrote: > > > On Tue, Aug 21, 2012 at 04:28:20PM -0700, Shirish S wrote: > > > > On Tue, Aug 21, 2012 at 7:56 AM, Ville Syrjälä < > > > > ville.syrjala@linux.intel.com> wrote: > > > > > > > > > On Tue, Aug 21, 2012 at 06:55:53AM -0700, Shirish S wrote: > > > > Here are my earlier comments on Jean's patch: > > > > > > > http://lists.freedesktop.org/archives/dri-devel/2012-February/019069.html > > > > > > > > > > > > > > If i am not wrong am doing exactly what you have said in you comments. > > > > > > > > This seems a bit wrong to me. The spec says that the ack for the > > > > segment address is "don't care", but for the segment pointer the ack is > > > > required (when segment != 0). > > > > The variable segFlags is "dont care for block 0 and 1 wheras". > > > > > > > > With I2C_M_IGNORE_NAK we would in fact end up reading segment 0 from a > > > > non E-DDC display, if we try to read segment != 0 from it. Of course > > > > we shouldn't do that unless the display lied to us about what extension > > > > blocks it provides. > > > > > > > > So I'm not sure if it would be better to trust that the display never > > > > lies about the extension blocks, or if we should just assume all E-DDC > > > > displays ack both segment addr and pointer. The no-ack feature seems > > > > to there for backwards compatibility, for cases where the host always > > > > sends the segment addr/pointer even when reading segment 0 (which your > > > > code doesn't do). > > > > > > > > To handle it exactly as the spec says, I2C_M_IGNORE_NAK should be split > > > > into two flags (one for addr, other for data). > > > > > > > > Hence i have split the i2c_msg into 3, segment pointer,offset(addr) > > > > and data pointer. > > > > > > I was referring to the addr and data phases of the segment pointer. > > > According to the spec the ack for the addr is always optional. But I > > > suppose no sane device would nak the addr, while acking the data. > > > > We've seen those. Really. > > > > Which is why the current i915 gmbus driver has a hack to never return a > > NaK on the first i2c transfer. I guess we should fix this by properly > > supporting the INGNORE_NAK field in our gmbus driver, and setting that on > > the addr transfer field, too. > > > > Thanks for the comment, so are you ok with the current logic? > > > > I concure with Ville that sending the segment i2c message only when we > > actually need it, and not unconditionally. DDC is way to broken and > > claiming that the spec says otherwise doesn't fix all the existing bad hw. > > > > Agreed, so do you want me to post another patch, in which i add a function > only > if the number of blocks is more than 2? > Also i had some mistake in the patch set 1, hence i updated it. I think adding the IGNORE_NAK on the addr i2c transaction would help unconditionally - like I've said we've seen monitors that suggest that this is required. And yeah, I think we should send the E-DDC segment number only if the basic edid block indicates that more than 2 blocks are availble (and again with IGNORE_NAK, just for paranoia's sake). > Kindly have a look! Will do. Yours, Daniel
Small clarification: On Thu, Aug 23, 2012 at 4:23 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Aug 23, 2012 at 07:06:53AM -0700, Shirish S wrote: > > Hello Daniel, > > > > On Thu, Aug 23, 2012 at 1:54 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Wed, Aug 22, 2012 at 10:52:26AM +0300, Ville Syrjälä wrote: > > > > On Tue, Aug 21, 2012 at 04:28:20PM -0700, Shirish S wrote: > > > > > On Tue, Aug 21, 2012 at 7:56 AM, Ville Syrjälä < > > > > > ville.syrjala@linux.intel.com> wrote: > > > > > > > > > > > On Tue, Aug 21, 2012 at 06:55:53AM -0700, Shirish S wrote: > > > > > Here are my earlier comments on Jean's patch: > > > > > > > > > > http://lists.freedesktop.org/archives/dri-devel/2012-February/019069.html > > > > > > > > > > > > > > > > > If i am not wrong am doing exactly what you have said in you > comments. > > > > > > > > > > This seems a bit wrong to me. The spec says that the ack for the > > > > > segment address is "don't care", but for the segment pointer the > ack is > > > > > required (when segment != 0). > > > > > The variable segFlags is "dont care for block 0 and 1 wheras". > > > > > > > > > > With I2C_M_IGNORE_NAK we would in fact end up reading segment 0 > from a > > > > > non E-DDC display, if we try to read segment != 0 from it. Of > course > > > > > we shouldn't do that unless the display lied to us about what > extension > > > > > blocks it provides. > > > > > > > > > > So I'm not sure if it would be better to trust that the display > never > > > > > lies about the extension blocks, or if we should just assume all > E-DDC > > > > > displays ack both segment addr and pointer. The no-ack feature > seems > > > > > to there for backwards compatibility, for cases where the host > always > > > > > sends the segment addr/pointer even when reading segment 0 (which > your > > > > > code doesn't do). > > > > > > > > > > To handle it exactly as the spec says, I2C_M_IGNORE_NAK should be > split > > > > > into two flags (one for addr, other for data). > > > > > > > > > > Hence i have split the i2c_msg into 3, segment pointer,offset(addr) > > > > > and data pointer. > > > > > > > > I was referring to the addr and data phases of the segment pointer. > > > > According to the spec the ack for the addr is always optional. But I > > > > suppose no sane device would nak the addr, while acking the data. > > > > > > We've seen those. Really. > > > > > > Which is why the current i915 gmbus driver has a hack to never return a > > > NaK on the first i2c transfer. I guess we should fix this by properly > > > supporting the INGNORE_NAK field in our gmbus driver, and setting that > on > > > the addr transfer field, too. > > > > > > Thanks for the comment, so are you ok with the current logic? > > > > > > > I concure with Ville that sending the segment i2c message only when we > > > actually need it, and not unconditionally. DDC is way to broken and > > > claiming that the spec says otherwise doesn't fix all the existing bad > hw. > > > > > > > Agreed, so do you want me to post another patch, in which i add a > function > > only > > if the number of blocks is more than 2? > > Also i had some mistake in the patch set 1, hence i updated it. > > I think adding the IGNORE_NAK on the addr i2c transaction would help > unconditionally - like I've said we've seen monitors that suggest that > this is required. And yeah, I think we should send the E-DDC segment > number only if the basic edid block indicates that more than 2 blocks are > availble (and again with IGNORE_NAK, just for paranoia's sake). > > > > Kindly have a look! > > Will do. > > The patch set 2 is based on the 2 comments i received I shall post patch set 3 today,incorporating your comments. Yours, Daniel > -- > Daniel Vetter > Mail: daniel@ffwll.ch > Mobile: +41 (0)79 365 57 48 > Thanks & Regards, Shirish S
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..2c2996e 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -254,6 +254,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, int block, int len) { unsigned char start = block * EDID_LENGTH; + unsigned char segment = block >> 1; + unsigned short segFlags = segment ? 0 : I2C_M_IGNORE_NAK; int ret, retries = 5; /* The core i2c driver will automatically retry the transfer if the @@ -264,27 +266,32 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, */ do { struct i2c_msg msgs[] = { - { + { /*set segment pointer */ + .addr = DDC_SEGMENT_ADDR, + .flags = segFlags, + .len = 1, + .buf = &start, + }, { /*set offset */ .addr = DDC_ADDR, .flags = 0, .len = 1, .buf = &start, - }, { + }, { /*set data */ .addr = DDC_ADDR, .flags = I2C_M_RD, .len = len, .buf = buf, } }; - ret = i2c_transfer(adapter, msgs, 2); + ret = i2c_transfer(adapter, msgs, 3); if (ret == -ENXIO) { DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", adapter->name); break; } - } while (ret != 2 && --retries); + } while (ret != 3 && --retries); - return ret == 2 ? 0 : -1; + return ret == 3 ? 0 : -1; } static bool drm_edid_is_zero(u8 *in_edid, int length)
The current logic for probing ddc is limited to 2 blocks (256 bytes), this patch adds support for the 4 block (512) data. To do this, a single 8-bit segment index is passed to the display via the I2C address 30h. Data from the selected segment is then immediately read via the regular DDC2 address using a repeated I2C 'START' signal. Signed-off-by: Shirish S <s.shirish@samsung.com> --- drivers/gpu/drm/drm_edid.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-)