[PATCH] drm: exynos: Fix fb_videomode <-> drm_mode_modeinfo conversion

[PATCH] drm: exynos: Fix fb_videomode <-> drm_mode_modeinfo conversion

[Laurent Pinchart]

The fb_videomode structure stores the front porch and back porch in the
right_margin and left_margin fields respectively. right_margin should
thus be computed with hsync_start - hdisplay, and left_margin with
htotal - hsync_end. The same holds for the vertical direction.

       Active               Front           Sync            Back
       Region               Porch                           Porch
<-------------------><----------------><-------------><---------------->

  //////////////////|
 ////////////////// |
//////////////////  |..................               ..................
                                       _______________

<------ xres -------><- right_margin -><- hsync_len -><- left_margin -->

<---- hdisplay ----->
<------------ hsync_start ------------>
<--------------------- hsync_end -------------------->
<--------------------------------- htotal ----------------------------->

Fix the fb_videomode <-> drm_mode_modeinfo conversion functions
accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/exynos/exynos_drm_connector.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

While trying to understand how the fb_videomode and drm_mode_modeinfo fields
map to each other, I found what might be a bug in the Exynos DRM driver. Could
you please check and confirm that my understanding is correct ?

diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c
index d620b07..7bb1dca 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_connector.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c
@@ -52,14 +52,14 @@ convert_to_display_mode(struct drm_display_mode *mode,
 	mode->vrefresh = timing->refresh;
 
 	mode->hdisplay = timing->xres;
-	mode->hsync_start = mode->hdisplay + timing->left_margin;
+	mode->hsync_start = mode->hdisplay + timing->right_margin;
 	mode->hsync_end = mode->hsync_start + timing->hsync_len;
-	mode->htotal = mode->hsync_end + timing->right_margin;
+	mode->htotal = mode->hsync_end + timing->left_margin;
 
 	mode->vdisplay = timing->yres;
-	mode->vsync_start = mode->vdisplay + timing->upper_margin;
+	mode->vsync_start = mode->vdisplay + timing->lower_margin;
 	mode->vsync_end = mode->vsync_start + timing->vsync_len;
-	mode->vtotal = mode->vsync_end + timing->lower_margin;
+	mode->vtotal = mode->vsync_end + timing->upper_margin;
 
 	if (timing->vmode & FB_VMODE_INTERLACED)
 		mode->flags |= DRM_MODE_FLAG_INTERLACE;
@@ -81,14 +81,14 @@ convert_to_video_timing(struct fb_videomode *timing,
 	timing->refresh = drm_mode_vrefresh(mode);
 
 	timing->xres = mode->hdisplay;
-	timing->left_margin = mode->hsync_start - mode->hdisplay;
+	timing->right_margin = mode->hsync_start - mode->hdisplay;
 	timing->hsync_len = mode->hsync_end - mode->hsync_start;
-	timing->right_margin = mode->htotal - mode->hsync_end;
+	timing->left_margin = mode->htotal - mode->hsync_end;
 
 	timing->yres = mode->vdisplay;
-	timing->upper_margin = mode->vsync_start - mode->vdisplay;
+	timing->lower_margin = mode->vsync_start - mode->vdisplay;
 	timing->vsync_len = mode->vsync_end - mode->vsync_start;
-	timing->lower_margin = mode->vtotal - mode->vsync_end;
+	timing->upper_margin = mode->vtotal - mode->vsync_end;
 
 	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
 		timing->vmode = FB_VMODE_INTERLACED;
-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: exynos: Fix fb_videomode <-> drm_mode_modeinfo conversion

[Joonyoung Shim]

On 03/08/2012 08:34 PM, Laurent Pinchart wrote:
> The fb_videomode structure stores the front porch and back porch in the
> right_margin and left_margin fields respectively. right_margin should
> thus be computed with hsync_start - hdisplay, and left_margin with
> htotal - hsync_end. The same holds for the vertical direction.
>
>         Active               Front           Sync            Back
>         Region               Porch                           Porch
> <-------------------><----------------><-------------><---------------->
>
>    //////////////////|
>   ////////////////// |
> //////////////////  |..................               ..................
>                                         _______________
>
> <------ xres -------><- right_margin -><- hsync_len -><- left_margin -->
>
> <---- hdisplay ----->
> <------------ hsync_start ------------>
> <--------------------- hsync_end -------------------->
> <--------------------------------- htotal ----------------------------->
>
> Fix the fb_videomode<->  drm_mode_modeinfo conversion functions
> accordingly.
>
> Signed-off-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
> ---
>   drivers/gpu/drm/exynos/exynos_drm_connector.c |   16 ++++++++--------
>   1 files changed, 8 insertions(+), 8 deletions(-)
>
> While trying to understand how the fb_videomode and drm_mode_modeinfo fields
> map to each other, I found what might be a bug in the Exynos DRM driver. Could
> you please check and confirm that my understanding is correct ?

Good catch. You can refer Documentation/fb/framebuffer.txt to know in 
detail.

Acked-by: Joonyoung Shim <jy0922.shim@samsung.com>

Thanks.

> diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c
> index d620b07..7bb1dca 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c
> @@ -52,14 +52,14 @@ convert_to_display_mode(struct drm_display_mode *mode,
>   	mode->vrefresh = timing->refresh;
>
>   	mode->hdisplay = timing->xres;
> -	mode->hsync_start = mode->hdisplay + timing->left_margin;
> +	mode->hsync_start = mode->hdisplay + timing->right_margin;
>   	mode->hsync_end = mode->hsync_start + timing->hsync_len;
> -	mode->htotal = mode->hsync_end + timing->right_margin;
> +	mode->htotal = mode->hsync_end + timing->left_margin;
>
>   	mode->vdisplay = timing->yres;
> -	mode->vsync_start = mode->vdisplay + timing->upper_margin;
> +	mode->vsync_start = mode->vdisplay + timing->lower_margin;
>   	mode->vsync_end = mode->vsync_start + timing->vsync_len;
> -	mode->vtotal = mode->vsync_end + timing->lower_margin;
> +	mode->vtotal = mode->vsync_end + timing->upper_margin;
>
>   	if (timing->vmode&  FB_VMODE_INTERLACED)
>   		mode->flags |= DRM_MODE_FLAG_INTERLACE;
> @@ -81,14 +81,14 @@ convert_to_video_timing(struct fb_videomode *timing,
>   	timing->refresh = drm_mode_vrefresh(mode);
>
>   	timing->xres = mode->hdisplay;
> -	timing->left_margin = mode->hsync_start - mode->hdisplay;
> +	timing->right_margin = mode->hsync_start - mode->hdisplay;
>   	timing->hsync_len = mode->hsync_end - mode->hsync_start;
> -	timing->right_margin = mode->htotal - mode->hsync_end;
> +	timing->left_margin = mode->htotal - mode->hsync_end;
>
>   	timing->yres = mode->vdisplay;
> -	timing->upper_margin = mode->vsync_start - mode->vdisplay;
> +	timing->lower_margin = mode->vsync_start - mode->vdisplay;
>   	timing->vsync_len = mode->vsync_end - mode->vsync_start;
> -	timing->lower_margin = mode->vtotal - mode->vsync_end;
> +	timing->upper_margin = mode->vtotal - mode->vsync_end;
>
>   	if (mode->flags&  DRM_MODE_FLAG_INTERLACE)
>   		timing->vmode = FB_VMODE_INTERLACED;

Re: [PATCH] drm: exynos: Fix fb_videomode <-> drm_mode_modeinfo conversion

[Laurent Pinchart]

Hi Joonyoung,

On Thursday 08 March 2012 22:05:50 Joonyoung Shim wrote:
> On 03/08/2012 08:34 PM, Laurent Pinchart wrote:
> > The fb_videomode structure stores the front porch and back porch in the
> > right_margin and left_margin fields respectively. right_margin should
> > thus be computed with hsync_start - hdisplay, and left_margin with
> > htotal - hsync_end. The same holds for the vertical direction.
> > 
> >         Active               Front           Sync            Back
> >         Region               Porch                           Porch
> > 
> > <-------------------><----------------><-------------><---------------->
> > 
> >    //////////////////|
> >   
> >   ////////////////// |
> > 
> > //////////////////  |..................               ..................
> > 
> >                                         _______________
> > 
> > <------ xres -------><- right_margin -><- hsync_len -><- left_margin -->
> > 
> > <---- hdisplay ----->
> > <------------ hsync_start ------------>
> > <--------------------- hsync_end -------------------->
> > <--------------------------------- htotal ----------------------------->
> > 
> > Fix the fb_videomode<->  drm_mode_modeinfo conversion functions
> > accordingly.
> > 
> > Signed-off-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >   drivers/gpu/drm/exynos/exynos_drm_connector.c |   16 ++++++++--------
> >   1 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > While trying to understand how the fb_videomode and drm_mode_modeinfo
> > fields map to each other, I found what might be a bug in the Exynos DRM
> > driver. Could you please check and confirm that my understanding is
> > correct ?
> 
> Good catch. You can refer Documentation/fb/framebuffer.txt to know in
> detail.
> 
> Acked-by: Joonyoung Shim <jy0922.shim@samsung.com>

Thank you. There's a similar issue in exynos_mipi_dsi_set_display_mode() where 
front and back porch are inverted. I'll submit a patch for that as well.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: exynos: Fix fb_videomode <-> drm_mode_modeinfo conversion

[Sascha Hauer]

On Thu, Mar 08, 2012 at 04:31:51PM +0100, Laurent Pinchart wrote:
> Hi Joonyoung,
> 
> On Thursday 08 March 2012 22:05:50 Joonyoung Shim wrote:
> > On 03/08/2012 08:34 PM, Laurent Pinchart wrote:
> > > The fb_videomode structure stores the front porch and back porch in the
> > > right_margin and left_margin fields respectively. right_margin should
> > > thus be computed with hsync_start - hdisplay, and left_margin with
> > > htotal - hsync_end. The same holds for the vertical direction.
> > > 
> > >         Active               Front           Sync            Back
> > >         Region               Porch                           Porch
> > > 
> > > <-------------------><----------------><-------------><---------------->
> > > 
> > >    //////////////////|
> > >   
> > >   ////////////////// |
> > > 
> > > //////////////////  |..................               ..................
> > > 
> > >                                         _______________
> > > 
> > > <------ xres -------><- right_margin -><- hsync_len -><- left_margin -->
> > > 
> > > <---- hdisplay ----->
> > > <------------ hsync_start ------------>
> > > <--------------------- hsync_end -------------------->
> > > <--------------------------------- htotal ----------------------------->
> > > 
> > > Fix the fb_videomode<->  drm_mode_modeinfo conversion functions
> > > accordingly.
> > > 
> > > Signed-off-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >   drivers/gpu/drm/exynos/exynos_drm_connector.c |   16 ++++++++--------
> > >   1 files changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > While trying to understand how the fb_videomode and drm_mode_modeinfo
> > > fields map to each other, I found what might be a bug in the Exynos DRM
> > > driver. Could you please check and confirm that my understanding is
> > > correct ?
> > 
> > Good catch. You can refer Documentation/fb/framebuffer.txt to know in
> > detail.
> > 
> > Acked-by: Joonyoung Shim <jy0922.shim@samsung.com>
> 
> Thank you. There's a similar issue in exynos_mipi_dsi_set_display_mode() where 
> front and back porch are inverted. I'll submit a patch for that as well.

Damned. This seems to be a common misunderstanding. I never really
thought about it and always assumed that 'front porch' and 'back porch'
refers to the picture and not to the sync signal. I was so sure that
I didn't even care to google it before I saw this patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] drm: exynos: Fix fb_videomode <-> drm_mode_modeinfo conversion

[James Simmons]

> The fb_videomode structure stores the front porch and back porch in the
> right_margin and left_margin fields respectively. right_margin should
> thus be computed with hsync_start - hdisplay, and left_margin with
> htotal - hsync_end. The same holds for the vertical direction.
> 
>        Active               Front           Sync            Back
>        Region               Porch                           Porch
> <-------------------><----------------><-------------><---------------->
> 
>   //////////////////|
>  ////////////////// |
> //////////////////  |..................               ..................
>                                        _______________
> 
> <------ xres -------><- right_margin -><- hsync_len -><- left_margin -->
> 
> <---- hdisplay ----->
> <------------ hsync_start ------------>
> <--------------------- hsync_end -------------------->
> <--------------------------------- htotal ----------------------------->
> 
> Fix the fb_videomode <-> drm_mode_modeinfo conversion functions
> accordingly.
> 

Wow I see this has remegered. Some time last year I posted a patch that 
had these routines in a generic format for people to use. I can repost 
them again if people are interested.

Re: [PATCH] drm: exynos: Fix fb_videomode <-> drm_mode_modeinfo conversion

[Corbin Simpson]

On Fri, Mar 9, 2012 at 12:57 AM, James Simmons <jsimmons@infradead.org> wrote:
> Wow I see this has remegered. Some time last year I posted a patch that
> had these routines in a generic format for people to use. I can repost
> them again if people are interested.

Yes please!

-- 
When the facts change, I change my mind. What do you do, sir? ~ Keynes

Corbin Simpson
<MostAwesomeDude@gmail.com>

Re: [PATCH] drm: exynos: Fix fb_videomode <-> drm_mode_modeinfo conversion

[Laurent Pinchart]

Hi James,

On Thursday 08 March 2012 16:57:21 James Simmons wrote:
> > The fb_videomode structure stores the front porch and back porch in the
> > right_margin and left_margin fields respectively. right_margin should
> > thus be computed with hsync_start - hdisplay, and left_margin with
> > htotal - hsync_end. The same holds for the vertical direction.
> > 
> >        Active               Front           Sync            Back
> >        Region               Porch                           Porch
> > 
> > <-------------------><----------------><-------------><---------------->
> > 
> >   //////////////////|
> >  
> >  ////////////////// |
> > 
> > //////////////////  |..................               ..................
> > 
> >                                        _______________
> > 
> > <------ xres -------><- right_margin -><- hsync_len -><- left_margin -->
> > 
> > <---- hdisplay ----->
> > <------------ hsync_start ------------>
> > <--------------------- hsync_end -------------------->
> > <--------------------------------- htotal ----------------------------->
> > 
> > Fix the fb_videomode <-> drm_mode_modeinfo conversion functions
> > accordingly.
> 
> Wow I see this has remegered. Some time last year I posted a patch that
> had these routines in a generic format for people to use. I can repost
> them again if people are interested.

As part of the EDID parser rework, I will introduce a common structure to 
describe video modes. Conversion functions between the common structures and 
struct drm_mode_modeinfo, struct fb_videomode and struct v4l2_bt_timings will 
be provided. Direct conversion between drm_mode_modeinfo and fb_videomode 
should then disappear.

This being said, we could introduce add you conversion functions in the 
meantime. I will then post a patch to remove them when the common video mode 
structure will be pushed to mainline :-)

-- 
Regards,

Laurent Pinchart

RE: [PATCH] drm: exynos: Fix fb_videomode <-> drm_mode_modeinfo conversion

[Inki Dae]

Hi Laurent,

> -----Original Message-----
> From: Joonyoung Shim [mailto:jy0922.shim@samsung.com]
> Sent: Thursday, March 08, 2012 10:06 PM
> To: Laurent Pinchart
> Cc: dri-devel@lists.freedesktop.org; linux-fbdev@vger.kernel.org; Inki
Dae;
> Seung-Woo Kim; Kyungmin Park
> Subject: Re: [PATCH] drm: exynos: Fix fb_videomode <-> drm_mode_modeinfo
> conversion
> 
> On 03/08/2012 08:34 PM, Laurent Pinchart wrote:
> > The fb_videomode structure stores the front porch and back porch in the
> > right_margin and left_margin fields respectively. right_margin should
> > thus be computed with hsync_start - hdisplay, and left_margin with
> > htotal - hsync_end. The same holds for the vertical direction.
> >
> >         Active               Front           Sync            Back
> >         Region               Porch                           Porch
> > <-------------------><----------------><-------------><---------------->
> >
> >    //////////////////|
> >   ////////////////// |
> > //////////////////  |..................               ..................
> >                                         _______________
> >
> > <------ xres -------><- right_margin -><- hsync_len -><- left_margin -->
> >
> > <---- hdisplay ----->
> > <------------ hsync_start ------------>
> > <--------------------- hsync_end -------------------->
> > <--------------------------------- htotal ----------------------------->
> >
> > Fix the fb_videomode<->  drm_mode_modeinfo conversion functions
> > accordingly.
> >
> > Signed-off-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/gpu/drm/exynos/exynos_drm_connector.c |   16 ++++++++--------
> >   1 files changed, 8 insertions(+), 8 deletions(-)
> >
> > While trying to understand how the fb_videomode and drm_mode_modeinfo
> fields
> > map to each other, I found what might be a bug in the Exynos DRM driver.
> Could
> > you please check and confirm that my understanding is correct ?
> 
> Good catch. You can refer Documentation/fb/framebuffer.txt to know in
> detail.
> 
> Acked-by: Joonyoung Shim <jy0922.shim@samsung.com>
> 
> Thanks.
> 

it seems like that you doesn't use latest exynos drm driver so I merged it
manually
for this, you can refer to this link:
http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/exyn
os-drm-fixes

Thanks,
Inki Dae

> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c
> b/drivers/gpu/drm/exynos/exynos_drm_connector.c
> > index d620b07..7bb1dca 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c
> > @@ -52,14 +52,14 @@ convert_to_display_mode(struct drm_display_mode
> *mode,
> >   	mode->vrefresh = timing->refresh;
> >
> >   	mode->hdisplay = timing->xres;
> > -	mode->hsync_start = mode->hdisplay + timing->left_margin;
> > +	mode->hsync_start = mode->hdisplay + timing->right_margin;
> >   	mode->hsync_end = mode->hsync_start + timing->hsync_len;
> > -	mode->htotal = mode->hsync_end + timing->right_margin;
> > +	mode->htotal = mode->hsync_end + timing->left_margin;
> >
> >   	mode->vdisplay = timing->yres;
> > -	mode->vsync_start = mode->vdisplay + timing->upper_margin;
> > +	mode->vsync_start = mode->vdisplay + timing->lower_margin;
> >   	mode->vsync_end = mode->vsync_start + timing->vsync_len;
> > -	mode->vtotal = mode->vsync_end + timing->lower_margin;
> > +	mode->vtotal = mode->vsync_end + timing->upper_margin;
> >
> >   	if (timing->vmode&  FB_VMODE_INTERLACED)
> >   		mode->flags |= DRM_MODE_FLAG_INTERLACE;
> > @@ -81,14 +81,14 @@ convert_to_video_timing(struct fb_videomode *timing,
> >   	timing->refresh = drm_mode_vrefresh(mode);
> >
> >   	timing->xres = mode->hdisplay;
> > -	timing->left_margin = mode->hsync_start - mode->hdisplay;
> > +	timing->right_margin = mode->hsync_start - mode->hdisplay;
> >   	timing->hsync_len = mode->hsync_end - mode->hsync_start;
> > -	timing->right_margin = mode->htotal - mode->hsync_end;
> > +	timing->left_margin = mode->htotal - mode->hsync_end;
> >
> >   	timing->yres = mode->vdisplay;
> > -	timing->upper_margin = mode->vsync_start - mode->vdisplay;
> > +	timing->lower_margin = mode->vsync_start - mode->vdisplay;
> >   	timing->vsync_len = mode->vsync_end - mode->vsync_start;
> > -	timing->lower_margin = mode->vtotal - mode->vsync_end;
> > +	timing->upper_margin = mode->vtotal - mode->vsync_end;
> >
> >   	if (mode->flags&  DRM_MODE_FLAG_INTERLACE)
> >   		timing->vmode = FB_VMODE_INTERLACED;