[PATCH] drm: edid: add support for E-DDC

[PATCH] drm: edid: add support for E-DDC

[Shirish S]

This patch adds support in probing 4 block edid data, for E-DDC.
This is the first test case in CTS, for HDMI compliance.

Based on drm-next branch

Shirish S (1):
  drm: edid: add support for E-DDC

 drivers/gpu/drm/drm_edid.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

[PATCH] drm: edid: add support for E-DDC

[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>
---
 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;
 	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

Re: [PATCH] drm: edid: add support for E-DDC

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 721 bytes --]

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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: edid: add support for E-DDC

[Daniel Vetter]

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



-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm: edid: add support for E-DDC

[Shirish S]

[-- Attachment #1.1: Type: text/plain, Size: 4137 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 6742 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: edid: add support for E-DDC

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: edid: add support for E-DDC

[Shirish S]

[-- Attachment #1.1: Type: text/plain, Size: 978 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 1846 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: edid: add support for E-DDC

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: edid: add support for E-DDC

[Shirish S]

[-- Attachment #1.1: Type: text/plain, Size: 3564 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 6204 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: edid: add support for E-DDC

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: edid: add support for E-DDC

[Daniel Vetter]

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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm: edid: add support for E-DDC

[Shirish S]

[-- Attachment #1.1: Type: text/plain, Size: 2983 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 4205 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: edid: add support for E-DDC

[Daniel Vetter]

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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm: edid: add support for E-DDC

[Shirish S]

[-- Attachment #1.1: Type: text/plain, Size: 3952 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 5453 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: edid: add support for E-DDC

[Shirish S]

[-- Attachment #1.1: Type: text/plain, Size: 2116 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 3039 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH V6] drm: edid: add support for E-DDC

[y]

From: Shirish S <s.shirish@samsung.com>

This patch adds support in probing 4 block edid data, for E-DDC.
This is the first test case in CTS, for HDMI compliance.

Changes from V1:
1. Data type of offset adress updated to unsigned short
2. Updated the buf feild of msg[0]

Changes from V2:
Add switch for DDC and E-DDC

Changes from V3:
Remove switch,and avoid sending of segment data for non E-DDC

Changes from V4:
Fix review comments about space and comment indentation.

Changes from V5:
Compacted the code.

Based on drm-next branch

Shirish S (1):
  drm: edid: add support for E-DDC

 drivers/gpu/drm/drm_edid.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

[PATCH] drm: edid: add support for E-DDC

[y]

From: Shirish S <s.shirish@samsung.com>

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>
Reviewed-by: Jean Delvare <jdelvare@suse.de>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Ville Syrjala <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index bcc4725..7f62de5 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 char xfers = segment ? 3 : 2;
 	int ret, retries = 5;
 
 	/* The core i2c driver will automatically retry the transfer if the
@@ -265,6 +267,11 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
 	do {
 		struct i2c_msg msgs[] = {
 			{
+				.addr	= DDC_SEGMENT_ADDR,
+				.flags	= 0,
+				.len	= 1,
+				.buf	= &segment,
+			}, {
 				.addr	= DDC_ADDR,
 				.flags	= 0,
 				.len	= 1,
@@ -276,15 +283,21 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
 				.buf	= buf,
 			}
 		};
-		ret = i2c_transfer(adapter, msgs, 2);
+
+	/*
+	 * Avoid sending the segment addr to not upset non-compliant ddc
+	 * monitors.
+	 */
+		ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers);
+
 		if (ret == -ENXIO) {
 			DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n",
 					adapter->name);
 			break;
 		}
-	} while (ret != 2 && --retries);
+	} while (ret != xfers && --retries);
 
-	return ret == 2 ? 0 : -1;
+	return ret == xfers ? 0 : -1;
 }
 
 static bool drm_edid_is_zero(u8 *in_edid, int length)
-- 
1.7.0.4