| Message ID | 20161208002706.10147-1-stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/08/2016 01:27 AM, Stefan Agner wrote: > The DRM subsystem specifies the pixel clock polarity from a > controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means > the controller drives the data on pixel clocks falling edge. > That is the controllers DOTCLK_POL=0 (Default is data launched > at negative edge). > > Also change the data enable logic to be high active by default > and only change if explicitly requested via bus_flags. With > that defaults are: > - Data enable: high active > - Pixel clock polarity: controller drives data on negative edge > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > Hi Marek, Hi, that was quick, thanks for checking this. > I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the > non-standard DE polarity was causing issues. I was using a EDT display > which is part of simple panel driver since a while now and does not > specify any bus_flags currently... Of course I could (and probably should) > add the proper bus_flags there too, but there are several displays > which do not specify any polarity and likely rely on sensible driver > standards (which is afact high active for the DE signal). I actually use a panel which requires correct settings of the flags, see e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch would break things for me. So I wonder whether you should fix the panel driver or whether the mxsfb should be fixed ? > -- > Stefan > > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index 0818903..4bcc8a3 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -168,9 +168,16 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) > vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH; > if (m->flags & DRM_MODE_FLAG_PVSYNC) > vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH; > - if (bus_flags & DRM_BUS_FLAG_DE_HIGH) > + /* Data Enable should be high active by default */ > + if (!(bus_flags & DRM_BUS_FLAG_DE_LOW)) > vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH; > - if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) > + /* > + * Note: DRM_BUS_FLAG_PIXDATA defines are controller centric, > + * controllers VDCTRL0_DOTCLK is display centric. > + * Drive on positive edige -> display samples on falling edge > + * DRM_BUS_FLAG_PIXDATA_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING > + */ > + if (bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE) > vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; > > writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0); >
On 2016-12-07 16:49, Marek Vasut wrote: > On 12/08/2016 01:27 AM, Stefan Agner wrote: >> The DRM subsystem specifies the pixel clock polarity from a >> controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means >> the controller drives the data on pixel clocks falling edge. >> That is the controllers DOTCLK_POL=0 (Default is data launched >> at negative edge). >> >> Also change the data enable logic to be high active by default >> and only change if explicitly requested via bus_flags. With >> that defaults are: >> - Data enable: high active >> - Pixel clock polarity: controller drives data on negative edge >> >> Signed-off-by: Stefan Agner <stefan@agner.ch> >> --- >> Hi Marek, > > Hi, that was quick, thanks for checking this. Yeah, I couldn't wait seeing it flying :-) > >> I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the >> non-standard DE polarity was causing issues. I was using a EDT display >> which is part of simple panel driver since a while now and does not >> specify any bus_flags currently... Of course I could (and probably should) >> add the proper bus_flags there too, but there are several displays >> which do not specify any polarity and likely rely on sensible driver >> standards (which is afact high active for the DE signal). > > I actually use a panel which requires correct settings of the flags, see > e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch > would break things for me. So I wonder whether you should fix the panel > driver or whether the mxsfb should be fixed ? If you ask me, mxsfb. Ok, there are actually two things, one is a bug, one is a default change. The bug: Pixel clock polarity is clearly defined to be controller centric (see comments around DRM_BUS_FLAG_PIXDATA_*EDGE in include/drm/drm_connector.h). The driver does it wrong currently. This might affect your display, and if it does, it is actually wrong also in your display... However, since it is a bug, I think it is not really a debate, it should be fixed... The default change: Data enable should be high by default because that is what most displays require, including yours. This won't break your display, since you request DRM_BUS_FLAG_DE_HIGH anyway... We could debate whether that default change is necessary, but since it also won't affect your display, I think it is a worthwhile change. -- Stefan > >> -- >> Stefan >> >> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >> index 0818903..4bcc8a3 100644 >> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >> @@ -168,9 +168,16 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) >> vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH; >> if (m->flags & DRM_MODE_FLAG_PVSYNC) >> vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH; >> - if (bus_flags & DRM_BUS_FLAG_DE_HIGH) >> + /* Data Enable should be high active by default */ >> + if (!(bus_flags & DRM_BUS_FLAG_DE_LOW)) >> vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH; >> - if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) >> + /* >> + * Note: DRM_BUS_FLAG_PIXDATA defines are controller centric, >> + * controllers VDCTRL0_DOTCLK is display centric. >> + * Drive on positive edige -> display samples on falling edge >> + * DRM_BUS_FLAG_PIXDATA_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING >> + */ >> + if (bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE) >> vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; >> >> writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0); >>
On 2016-12-07 16:59, Stefan Agner wrote: > On 2016-12-07 16:49, Marek Vasut wrote: >> On 12/08/2016 01:27 AM, Stefan Agner wrote: >>> The DRM subsystem specifies the pixel clock polarity from a >>> controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means >>> the controller drives the data on pixel clocks falling edge. >>> That is the controllers DOTCLK_POL=0 (Default is data launched >>> at negative edge). >>> >>> Also change the data enable logic to be high active by default >>> and only change if explicitly requested via bus_flags. With >>> that defaults are: >>> - Data enable: high active >>> - Pixel clock polarity: controller drives data on negative edge >>> >>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>> --- >>> Hi Marek, >> >> Hi, that was quick, thanks for checking this. > > Yeah, I couldn't wait seeing it flying :-) > >> >>> I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the >>> non-standard DE polarity was causing issues. I was using a EDT display >>> which is part of simple panel driver since a while now and does not >>> specify any bus_flags currently... Of course I could (and probably should) >>> add the proper bus_flags there too, but there are several displays >>> which do not specify any polarity and likely rely on sensible driver >>> standards (which is afact high active for the DE signal). >> >> I actually use a panel which requires correct settings of the flags, see >> e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch >> would break things for me. So I wonder whether you should fix the panel >> driver or whether the mxsfb should be fixed ? > > If you ask me, mxsfb. > > Ok, there are actually two things, one is a bug, one is a default > change. > > The bug: Pixel clock polarity is clearly defined to be controller > centric (see comments around DRM_BUS_FLAG_PIXDATA_*EDGE in > include/drm/drm_connector.h). The driver does it wrong currently. > > This might affect your display, and if it does, it is actually wrong > also in your display... However, since it is a bug, I think it is not > really a debate, it should be fixed... FWIW, it seems that Ortustech com43h4m85ulc samples on falling edge, so DRM_BUS_FLAG_PIXDATA_POSEDGE seems right. And it means that DOTCLK_POL should be 1 (inverted), so with this patch the polarity should actually be correct for that panel. -- Stefan > The default change: Data enable should be high by default because that > is what most displays require, including yours. This won't break your > display, since you request DRM_BUS_FLAG_DE_HIGH anyway... > > We could debate whether that default change is necessary, but since it > also won't affect your display, I think it is a worthwhile change. > > -- > Stefan > > >> >>> -- >>> Stefan >>> >>> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>> index 0818903..4bcc8a3 100644 >>> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>> @@ -168,9 +168,16 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) >>> vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH; >>> if (m->flags & DRM_MODE_FLAG_PVSYNC) >>> vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH; >>> - if (bus_flags & DRM_BUS_FLAG_DE_HIGH) >>> + /* Data Enable should be high active by default */ >>> + if (!(bus_flags & DRM_BUS_FLAG_DE_LOW)) >>> vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH; >>> - if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) >>> + /* >>> + * Note: DRM_BUS_FLAG_PIXDATA defines are controller centric, >>> + * controllers VDCTRL0_DOTCLK is display centric. >>> + * Drive on positive edige -> display samples on falling edge >>> + * DRM_BUS_FLAG_PIXDATA_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING >>> + */ >>> + if (bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE) >>> vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; >>> >>> writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0); >>>
On 12/08/2016 02:26 AM, Stefan Agner wrote: > On 2016-12-07 16:59, Stefan Agner wrote: >> On 2016-12-07 16:49, Marek Vasut wrote: >>> On 12/08/2016 01:27 AM, Stefan Agner wrote: >>>> The DRM subsystem specifies the pixel clock polarity from a >>>> controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means >>>> the controller drives the data on pixel clocks falling edge. >>>> That is the controllers DOTCLK_POL=0 (Default is data launched >>>> at negative edge). >>>> >>>> Also change the data enable logic to be high active by default >>>> and only change if explicitly requested via bus_flags. With >>>> that defaults are: >>>> - Data enable: high active >>>> - Pixel clock polarity: controller drives data on negative edge >>>> >>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>> --- >>>> Hi Marek, >>> >>> Hi, that was quick, thanks for checking this. >> >> Yeah, I couldn't wait seeing it flying :-) >> >>> >>>> I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the >>>> non-standard DE polarity was causing issues. I was using a EDT display >>>> which is part of simple panel driver since a while now and does not >>>> specify any bus_flags currently... Of course I could (and probably should) >>>> add the proper bus_flags there too, but there are several displays >>>> which do not specify any polarity and likely rely on sensible driver >>>> standards (which is afact high active for the DE signal). >>> >>> I actually use a panel which requires correct settings of the flags, see >>> e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch >>> would break things for me. So I wonder whether you should fix the panel >>> driver or whether the mxsfb should be fixed ? >> >> If you ask me, mxsfb. >> >> Ok, there are actually two things, one is a bug, one is a default >> change. >> >> The bug: Pixel clock polarity is clearly defined to be controller >> centric (see comments around DRM_BUS_FLAG_PIXDATA_*EDGE in >> include/drm/drm_connector.h). The driver does it wrong currently. >> >> This might affect your display, and if it does, it is actually wrong >> also in your display... However, since it is a bug, I think it is not >> really a debate, it should be fixed... > > FWIW, it seems that Ortustech com43h4m85ulc samples on falling edge, so > DRM_BUS_FLAG_PIXDATA_POSEDGE seems right. And it means that DOTCLK_POL > should be 1 (inverted), so with this patch the polarity should actually > be correct for that panel. Well, if I apply this patch, my image is shifted by 1 px to the left and there is a 1px white bar on the right side, so I think I have some polarity problem now ?
On 2016-12-07 18:37, Marek Vasut wrote: > On 12/08/2016 02:26 AM, Stefan Agner wrote: >> On 2016-12-07 16:59, Stefan Agner wrote: >>> On 2016-12-07 16:49, Marek Vasut wrote: >>>> On 12/08/2016 01:27 AM, Stefan Agner wrote: >>>>> The DRM subsystem specifies the pixel clock polarity from a >>>>> controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means >>>>> the controller drives the data on pixel clocks falling edge. >>>>> That is the controllers DOTCLK_POL=0 (Default is data launched >>>>> at negative edge). >>>>> >>>>> Also change the data enable logic to be high active by default >>>>> and only change if explicitly requested via bus_flags. With >>>>> that defaults are: >>>>> - Data enable: high active >>>>> - Pixel clock polarity: controller drives data on negative edge >>>>> >>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>> --- >>>>> Hi Marek, >>>> >>>> Hi, that was quick, thanks for checking this. >>> >>> Yeah, I couldn't wait seeing it flying :-) >>> >>>> >>>>> I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the >>>>> non-standard DE polarity was causing issues. I was using a EDT display >>>>> which is part of simple panel driver since a while now and does not >>>>> specify any bus_flags currently... Of course I could (and probably should) >>>>> add the proper bus_flags there too, but there are several displays >>>>> which do not specify any polarity and likely rely on sensible driver >>>>> standards (which is afact high active for the DE signal). >>>> >>>> I actually use a panel which requires correct settings of the flags, see >>>> e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch >>>> would break things for me. So I wonder whether you should fix the panel >>>> driver or whether the mxsfb should be fixed ? >>> >>> If you ask me, mxsfb. >>> >>> Ok, there are actually two things, one is a bug, one is a default >>> change. >>> >>> The bug: Pixel clock polarity is clearly defined to be controller >>> centric (see comments around DRM_BUS_FLAG_PIXDATA_*EDGE in >>> include/drm/drm_connector.h). The driver does it wrong currently. >>> >>> This might affect your display, and if it does, it is actually wrong >>> also in your display... However, since it is a bug, I think it is not >>> really a debate, it should be fixed... >> >> FWIW, it seems that Ortustech com43h4m85ulc samples on falling edge, so >> DRM_BUS_FLAG_PIXDATA_POSEDGE seems right. And it means that DOTCLK_POL >> should be 1 (inverted), so with this patch the polarity should actually >> be correct for that panel. > > Well, if I apply this patch, my image is shifted by 1 px to the left and > there is a 1px white bar on the right side, so I think I have some > polarity problem now ? Ok, lets create facts here: 1. SoloX Refrence Manual, Figure 37-13. shows DOTCLK_POL=0, and it shows that the controller drives signals on falling edge of the pixel clock. The i.MX 7 has the same figure. 2. Just to verify, I hooked up an oscilloscope on my i.MX 7: It shows that with DOTCLK_POL=0 the controller drives on falling edge: http://imgur.com/a/2f2Xt So my measurements verify what is in the i.MX data sheets. The current code sets the bit when negative edge (falling edge) is requested, which is wrong: #define VDCTRL0_DOTCLK_ACT_FALLING (1 << 25) ... if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; Not sure what is going on with your display, maybe the datasheet is just wrong (it requires DRM_BUS_FLAG_PIXDATA_NEGEDGE in fact) or it is some other artifact. -- Stefan
On 12/08/2016 09:46 PM, Stefan Agner wrote: > On 2016-12-07 18:37, Marek Vasut wrote: >> On 12/08/2016 02:26 AM, Stefan Agner wrote: >>> On 2016-12-07 16:59, Stefan Agner wrote: >>>> On 2016-12-07 16:49, Marek Vasut wrote: >>>>> On 12/08/2016 01:27 AM, Stefan Agner wrote: >>>>>> The DRM subsystem specifies the pixel clock polarity from a >>>>>> controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means >>>>>> the controller drives the data on pixel clocks falling edge. >>>>>> That is the controllers DOTCLK_POL=0 (Default is data launched >>>>>> at negative edge). >>>>>> >>>>>> Also change the data enable logic to be high active by default >>>>>> and only change if explicitly requested via bus_flags. With >>>>>> that defaults are: >>>>>> - Data enable: high active >>>>>> - Pixel clock polarity: controller drives data on negative edge >>>>>> >>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>>> --- >>>>>> Hi Marek, >>>>> >>>>> Hi, that was quick, thanks for checking this. >>>> >>>> Yeah, I couldn't wait seeing it flying :-) >>>> >>>>> >>>>>> I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the >>>>>> non-standard DE polarity was causing issues. I was using a EDT display >>>>>> which is part of simple panel driver since a while now and does not >>>>>> specify any bus_flags currently... Of course I could (and probably should) >>>>>> add the proper bus_flags there too, but there are several displays >>>>>> which do not specify any polarity and likely rely on sensible driver >>>>>> standards (which is afact high active for the DE signal). >>>>> >>>>> I actually use a panel which requires correct settings of the flags, see >>>>> e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch >>>>> would break things for me. So I wonder whether you should fix the panel >>>>> driver or whether the mxsfb should be fixed ? >>>> >>>> If you ask me, mxsfb. >>>> >>>> Ok, there are actually two things, one is a bug, one is a default >>>> change. >>>> >>>> The bug: Pixel clock polarity is clearly defined to be controller >>>> centric (see comments around DRM_BUS_FLAG_PIXDATA_*EDGE in >>>> include/drm/drm_connector.h). The driver does it wrong currently. >>>> >>>> This might affect your display, and if it does, it is actually wrong >>>> also in your display... However, since it is a bug, I think it is not >>>> really a debate, it should be fixed... >>> >>> FWIW, it seems that Ortustech com43h4m85ulc samples on falling edge, so >>> DRM_BUS_FLAG_PIXDATA_POSEDGE seems right. And it means that DOTCLK_POL >>> should be 1 (inverted), so with this patch the polarity should actually >>> be correct for that panel. >> >> Well, if I apply this patch, my image is shifted by 1 px to the left and >> there is a 1px white bar on the right side, so I think I have some >> polarity problem now ? > > Ok, lets create facts here: > 1. SoloX Refrence Manual, Figure 37-13. shows DOTCLK_POL=0, and it shows > that the controller drives signals on falling edge of the pixel clock. > The i.MX 7 has the same figure. > 2. Just to verify, I hooked up an oscilloscope on my i.MX 7: It shows > that with DOTCLK_POL=0 the controller drives on falling edge: > http://imgur.com/a/2f2Xt > > So my measurements verify what is in the i.MX data sheets. Good > The current code sets the bit when negative edge (falling edge) is > requested, which is wrong: > #define VDCTRL0_DOTCLK_ACT_FALLING (1 << 25) > ... > if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) > vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; > > Not sure what is going on with your display, maybe the datasheet is just > wrong (it requires DRM_BUS_FLAG_PIXDATA_NEGEDGE in fact) or it is some > other artifact. This is probably where the problem crept in [1], droping PIXDATA_POSEDGE actually makes this patch work for me. CCing Philipp. [1] https://patchwork.kernel.org/patch/9301517/
On 2016-12-08 15:38, Marek Vasut wrote: > On 12/08/2016 09:46 PM, Stefan Agner wrote: >> On 2016-12-07 18:37, Marek Vasut wrote: >>> On 12/08/2016 02:26 AM, Stefan Agner wrote: >>>> On 2016-12-07 16:59, Stefan Agner wrote: >>>>> On 2016-12-07 16:49, Marek Vasut wrote: >>>>>> On 12/08/2016 01:27 AM, Stefan Agner wrote: >>>>>>> The DRM subsystem specifies the pixel clock polarity from a >>>>>>> controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means >>>>>>> the controller drives the data on pixel clocks falling edge. >>>>>>> That is the controllers DOTCLK_POL=0 (Default is data launched >>>>>>> at negative edge). >>>>>>> >>>>>>> Also change the data enable logic to be high active by default >>>>>>> and only change if explicitly requested via bus_flags. With >>>>>>> that defaults are: >>>>>>> - Data enable: high active >>>>>>> - Pixel clock polarity: controller drives data on negative edge >>>>>>> >>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>>>> --- >>>>>>> Hi Marek, >>>>>> >>>>>> Hi, that was quick, thanks for checking this. >>>>> >>>>> Yeah, I couldn't wait seeing it flying :-) >>>>> >>>>>> >>>>>>> I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the >>>>>>> non-standard DE polarity was causing issues. I was using a EDT display >>>>>>> which is part of simple panel driver since a while now and does not >>>>>>> specify any bus_flags currently... Of course I could (and probably should) >>>>>>> add the proper bus_flags there too, but there are several displays >>>>>>> which do not specify any polarity and likely rely on sensible driver >>>>>>> standards (which is afact high active for the DE signal). >>>>>> >>>>>> I actually use a panel which requires correct settings of the flags, see >>>>>> e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch >>>>>> would break things for me. So I wonder whether you should fix the panel >>>>>> driver or whether the mxsfb should be fixed ? >>>>> >>>>> If you ask me, mxsfb. >>>>> >>>>> Ok, there are actually two things, one is a bug, one is a default >>>>> change. >>>>> >>>>> The bug: Pixel clock polarity is clearly defined to be controller >>>>> centric (see comments around DRM_BUS_FLAG_PIXDATA_*EDGE in >>>>> include/drm/drm_connector.h). The driver does it wrong currently. >>>>> >>>>> This might affect your display, and if it does, it is actually wrong >>>>> also in your display... However, since it is a bug, I think it is not >>>>> really a debate, it should be fixed... >>>> >>>> FWIW, it seems that Ortustech com43h4m85ulc samples on falling edge, so >>>> DRM_BUS_FLAG_PIXDATA_POSEDGE seems right. And it means that DOTCLK_POL >>>> should be 1 (inverted), so with this patch the polarity should actually >>>> be correct for that panel. >>> >>> Well, if I apply this patch, my image is shifted by 1 px to the left and >>> there is a 1px white bar on the right side, so I think I have some >>> polarity problem now ? >> >> Ok, lets create facts here: >> 1. SoloX Refrence Manual, Figure 37-13. shows DOTCLK_POL=0, and it shows >> that the controller drives signals on falling edge of the pixel clock. >> The i.MX 7 has the same figure. >> 2. Just to verify, I hooked up an oscilloscope on my i.MX 7: It shows >> that with DOTCLK_POL=0 the controller drives on falling edge: >> http://imgur.com/a/2f2Xt >> >> So my measurements verify what is in the i.MX data sheets. > > Good > >> The current code sets the bit when negative edge (falling edge) is >> requested, which is wrong: >> #define VDCTRL0_DOTCLK_ACT_FALLING (1 << 25) >> ... >> if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) >> vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; >> >> Not sure what is going on with your display, maybe the datasheet is just >> wrong (it requires DRM_BUS_FLAG_PIXDATA_NEGEDGE in fact) or it is some >> other artifact. > > This is probably where the problem crept in [1], droping PIXDATA_POSEDGE > actually makes this patch work for me. CCing Philipp. > > [1] https://patchwork.kernel.org/patch/9301517/ I looked at a old data sheet of that display and it seemed that PIXDATA_POSEDGE is the right thing. Panelook.cn lists newer data sheets, but I couldn't find them on the open internet, do you have access to a newer one? http://www.panelook.cn/COM43H4M85ULC_ORTUSTECH_4.3_LCM_overview_17296.html I guess in the end it doesn't matter: Given that it is verified that the controllers data sheet is right, I vote for merging that patch and fix the displays polarity... -- Stefan
On 12/14/2016 01:01 AM, Stefan Agner wrote: > On 2016-12-08 15:38, Marek Vasut wrote: >> On 12/08/2016 09:46 PM, Stefan Agner wrote: >>> On 2016-12-07 18:37, Marek Vasut wrote: >>>> On 12/08/2016 02:26 AM, Stefan Agner wrote: >>>>> On 2016-12-07 16:59, Stefan Agner wrote: >>>>>> On 2016-12-07 16:49, Marek Vasut wrote: >>>>>>> On 12/08/2016 01:27 AM, Stefan Agner wrote: >>>>>>>> The DRM subsystem specifies the pixel clock polarity from a >>>>>>>> controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means >>>>>>>> the controller drives the data on pixel clocks falling edge. >>>>>>>> That is the controllers DOTCLK_POL=0 (Default is data launched >>>>>>>> at negative edge). >>>>>>>> >>>>>>>> Also change the data enable logic to be high active by default >>>>>>>> and only change if explicitly requested via bus_flags. With >>>>>>>> that defaults are: >>>>>>>> - Data enable: high active >>>>>>>> - Pixel clock polarity: controller drives data on negative edge >>>>>>>> >>>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>>>>> --- >>>>>>>> Hi Marek, >>>>>>> >>>>>>> Hi, that was quick, thanks for checking this. >>>>>> >>>>>> Yeah, I couldn't wait seeing it flying :-) >>>>>> >>>>>>> >>>>>>>> I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the >>>>>>>> non-standard DE polarity was causing issues. I was using a EDT display >>>>>>>> which is part of simple panel driver since a while now and does not >>>>>>>> specify any bus_flags currently... Of course I could (and probably should) >>>>>>>> add the proper bus_flags there too, but there are several displays >>>>>>>> which do not specify any polarity and likely rely on sensible driver >>>>>>>> standards (which is afact high active for the DE signal). >>>>>>> >>>>>>> I actually use a panel which requires correct settings of the flags, see >>>>>>> e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch >>>>>>> would break things for me. So I wonder whether you should fix the panel >>>>>>> driver or whether the mxsfb should be fixed ? >>>>>> >>>>>> If you ask me, mxsfb. >>>>>> >>>>>> Ok, there are actually two things, one is a bug, one is a default >>>>>> change. >>>>>> >>>>>> The bug: Pixel clock polarity is clearly defined to be controller >>>>>> centric (see comments around DRM_BUS_FLAG_PIXDATA_*EDGE in >>>>>> include/drm/drm_connector.h). The driver does it wrong currently. >>>>>> >>>>>> This might affect your display, and if it does, it is actually wrong >>>>>> also in your display... However, since it is a bug, I think it is not >>>>>> really a debate, it should be fixed... >>>>> >>>>> FWIW, it seems that Ortustech com43h4m85ulc samples on falling edge, so >>>>> DRM_BUS_FLAG_PIXDATA_POSEDGE seems right. And it means that DOTCLK_POL >>>>> should be 1 (inverted), so with this patch the polarity should actually >>>>> be correct for that panel. >>>> >>>> Well, if I apply this patch, my image is shifted by 1 px to the left and >>>> there is a 1px white bar on the right side, so I think I have some >>>> polarity problem now ? >>> >>> Ok, lets create facts here: >>> 1. SoloX Refrence Manual, Figure 37-13. shows DOTCLK_POL=0, and it shows >>> that the controller drives signals on falling edge of the pixel clock. >>> The i.MX 7 has the same figure. >>> 2. Just to verify, I hooked up an oscilloscope on my i.MX 7: It shows >>> that with DOTCLK_POL=0 the controller drives on falling edge: >>> http://imgur.com/a/2f2Xt >>> >>> So my measurements verify what is in the i.MX data sheets. >> >> Good >> >>> The current code sets the bit when negative edge (falling edge) is >>> requested, which is wrong: >>> #define VDCTRL0_DOTCLK_ACT_FALLING (1 << 25) >>> ... >>> if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) >>> vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; >>> >>> Not sure what is going on with your display, maybe the datasheet is just >>> wrong (it requires DRM_BUS_FLAG_PIXDATA_NEGEDGE in fact) or it is some >>> other artifact. >> >> This is probably where the problem crept in [1], droping PIXDATA_POSEDGE >> actually makes this patch work for me. CCing Philipp. >> >> [1] https://patchwork.kernel.org/patch/9301517/ > > I looked at a old data sheet of that display and it seemed that > PIXDATA_POSEDGE is the right thing. Panelook.cn lists newer data sheets, > but I couldn't find them on the open internet, do you have access to a > newer one? Which "version" do you have ? Probably not though. > http://www.panelook.cn/COM43H4M85ULC_ORTUSTECH_4.3_LCM_overview_17296.html > > I guess in the end it doesn't matter: Given that it is verified that the > controllers data sheet is right, I vote for merging that patch and fix > the displays polarity... Merging which patch ?
On 2016-12-14 00:04, Marek Vasut wrote: > On 12/14/2016 01:01 AM, Stefan Agner wrote: >> On 2016-12-08 15:38, Marek Vasut wrote: >>> On 12/08/2016 09:46 PM, Stefan Agner wrote: >>>> On 2016-12-07 18:37, Marek Vasut wrote: >>>>> On 12/08/2016 02:26 AM, Stefan Agner wrote: >>>>>> On 2016-12-07 16:59, Stefan Agner wrote: >>>>>>> On 2016-12-07 16:49, Marek Vasut wrote: >>>>>>>> On 12/08/2016 01:27 AM, Stefan Agner wrote: >>>>>>>>> The DRM subsystem specifies the pixel clock polarity from a >>>>>>>>> controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means >>>>>>>>> the controller drives the data on pixel clocks falling edge. >>>>>>>>> That is the controllers DOTCLK_POL=0 (Default is data launched >>>>>>>>> at negative edge). >>>>>>>>> >>>>>>>>> Also change the data enable logic to be high active by default >>>>>>>>> and only change if explicitly requested via bus_flags. With >>>>>>>>> that defaults are: >>>>>>>>> - Data enable: high active >>>>>>>>> - Pixel clock polarity: controller drives data on negative edge >>>>>>>>> >>>>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>>>>>> --- >>>>>>>>> Hi Marek, >>>>>>>> >>>>>>>> Hi, that was quick, thanks for checking this. >>>>>>> >>>>>>> Yeah, I couldn't wait seeing it flying :-) >>>>>>> >>>>>>>> >>>>>>>>> I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the >>>>>>>>> non-standard DE polarity was causing issues. I was using a EDT display >>>>>>>>> which is part of simple panel driver since a while now and does not >>>>>>>>> specify any bus_flags currently... Of course I could (and probably should) >>>>>>>>> add the proper bus_flags there too, but there are several displays >>>>>>>>> which do not specify any polarity and likely rely on sensible driver >>>>>>>>> standards (which is afact high active for the DE signal). >>>>>>>> >>>>>>>> I actually use a panel which requires correct settings of the flags, see >>>>>>>> e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch >>>>>>>> would break things for me. So I wonder whether you should fix the panel >>>>>>>> driver or whether the mxsfb should be fixed ? >>>>>>> >>>>>>> If you ask me, mxsfb. >>>>>>> >>>>>>> Ok, there are actually two things, one is a bug, one is a default >>>>>>> change. >>>>>>> >>>>>>> The bug: Pixel clock polarity is clearly defined to be controller >>>>>>> centric (see comments around DRM_BUS_FLAG_PIXDATA_*EDGE in >>>>>>> include/drm/drm_connector.h). The driver does it wrong currently. >>>>>>> >>>>>>> This might affect your display, and if it does, it is actually wrong >>>>>>> also in your display... However, since it is a bug, I think it is not >>>>>>> really a debate, it should be fixed... >>>>>> >>>>>> FWIW, it seems that Ortustech com43h4m85ulc samples on falling edge, so >>>>>> DRM_BUS_FLAG_PIXDATA_POSEDGE seems right. And it means that DOTCLK_POL >>>>>> should be 1 (inverted), so with this patch the polarity should actually >>>>>> be correct for that panel. >>>>> >>>>> Well, if I apply this patch, my image is shifted by 1 px to the left and >>>>> there is a 1px white bar on the right side, so I think I have some >>>>> polarity problem now ? >>>> >>>> Ok, lets create facts here: >>>> 1. SoloX Refrence Manual, Figure 37-13. shows DOTCLK_POL=0, and it shows >>>> that the controller drives signals on falling edge of the pixel clock. >>>> The i.MX 7 has the same figure. >>>> 2. Just to verify, I hooked up an oscilloscope on my i.MX 7: It shows >>>> that with DOTCLK_POL=0 the controller drives on falling edge: >>>> http://imgur.com/a/2f2Xt >>>> >>>> So my measurements verify what is in the i.MX data sheets. >>> >>> Good >>> >>>> The current code sets the bit when negative edge (falling edge) is >>>> requested, which is wrong: >>>> #define VDCTRL0_DOTCLK_ACT_FALLING (1 << 25) >>>> ... >>>> if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) >>>> vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; >>>> >>>> Not sure what is going on with your display, maybe the datasheet is just >>>> wrong (it requires DRM_BUS_FLAG_PIXDATA_NEGEDGE in fact) or it is some >>>> other artifact. >>> >>> This is probably where the problem crept in [1], droping PIXDATA_POSEDGE >>> actually makes this patch work for me. CCing Philipp. >>> >>> [1] https://patchwork.kernel.org/patch/9301517/ >> >> I looked at a old data sheet of that display and it seemed that >> PIXDATA_POSEDGE is the right thing. Panelook.cn lists newer data sheets, >> but I couldn't find them on the open internet, do you have access to a >> newer one? > > Which "version" do you have ? Probably not though. > I couldn't find it anymore, but I think it said Rev. 0 >> http://www.panelook.cn/COM43H4M85ULC_ORTUSTECH_4.3_LCM_overview_17296.html >> >> I guess in the end it doesn't matter: Given that it is verified that the >> controllers data sheet is right, I vote for merging that patch and fix >> the displays polarity... > > Merging which patch ? This patch. And I guess, to keep your display working, a patch which changes the Ortustech pixel clock flag... -- Stefan
On 12/14/2016 09:29 PM, Stefan Agner wrote: > On 2016-12-14 00:04, Marek Vasut wrote: >> On 12/14/2016 01:01 AM, Stefan Agner wrote: >>> On 2016-12-08 15:38, Marek Vasut wrote: >>>> On 12/08/2016 09:46 PM, Stefan Agner wrote: >>>>> On 2016-12-07 18:37, Marek Vasut wrote: >>>>>> On 12/08/2016 02:26 AM, Stefan Agner wrote: >>>>>>> On 2016-12-07 16:59, Stefan Agner wrote: >>>>>>>> On 2016-12-07 16:49, Marek Vasut wrote: >>>>>>>>> On 12/08/2016 01:27 AM, Stefan Agner wrote: >>>>>>>>>> The DRM subsystem specifies the pixel clock polarity from a >>>>>>>>>> controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means >>>>>>>>>> the controller drives the data on pixel clocks falling edge. >>>>>>>>>> That is the controllers DOTCLK_POL=0 (Default is data launched >>>>>>>>>> at negative edge). >>>>>>>>>> >>>>>>>>>> Also change the data enable logic to be high active by default >>>>>>>>>> and only change if explicitly requested via bus_flags. With >>>>>>>>>> that defaults are: >>>>>>>>>> - Data enable: high active >>>>>>>>>> - Pixel clock polarity: controller drives data on negative edge >>>>>>>>>> >>>>>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>>>>>>> --- >>>>>>>>>> Hi Marek, >>>>>>>>> >>>>>>>>> Hi, that was quick, thanks for checking this. >>>>>>>> >>>>>>>> Yeah, I couldn't wait seeing it flying :-) >>>>>>>> >>>>>>>>> >>>>>>>>>> I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the >>>>>>>>>> non-standard DE polarity was causing issues. I was using a EDT display >>>>>>>>>> which is part of simple panel driver since a while now and does not >>>>>>>>>> specify any bus_flags currently... Of course I could (and probably should) >>>>>>>>>> add the proper bus_flags there too, but there are several displays >>>>>>>>>> which do not specify any polarity and likely rely on sensible driver >>>>>>>>>> standards (which is afact high active for the DE signal). >>>>>>>>> >>>>>>>>> I actually use a panel which requires correct settings of the flags, see >>>>>>>>> e0932f9d7ba9a16f99a84943b720f109de8e3e06 in mainline , so this patch >>>>>>>>> would break things for me. So I wonder whether you should fix the panel >>>>>>>>> driver or whether the mxsfb should be fixed ? >>>>>>>> >>>>>>>> If you ask me, mxsfb. >>>>>>>> >>>>>>>> Ok, there are actually two things, one is a bug, one is a default >>>>>>>> change. >>>>>>>> >>>>>>>> The bug: Pixel clock polarity is clearly defined to be controller >>>>>>>> centric (see comments around DRM_BUS_FLAG_PIXDATA_*EDGE in >>>>>>>> include/drm/drm_connector.h). The driver does it wrong currently. >>>>>>>> >>>>>>>> This might affect your display, and if it does, it is actually wrong >>>>>>>> also in your display... However, since it is a bug, I think it is not >>>>>>>> really a debate, it should be fixed... >>>>>>> >>>>>>> FWIW, it seems that Ortustech com43h4m85ulc samples on falling edge, so >>>>>>> DRM_BUS_FLAG_PIXDATA_POSEDGE seems right. And it means that DOTCLK_POL >>>>>>> should be 1 (inverted), so with this patch the polarity should actually >>>>>>> be correct for that panel. >>>>>> >>>>>> Well, if I apply this patch, my image is shifted by 1 px to the left and >>>>>> there is a 1px white bar on the right side, so I think I have some >>>>>> polarity problem now ? >>>>> >>>>> Ok, lets create facts here: >>>>> 1. SoloX Refrence Manual, Figure 37-13. shows DOTCLK_POL=0, and it shows >>>>> that the controller drives signals on falling edge of the pixel clock. >>>>> The i.MX 7 has the same figure. >>>>> 2. Just to verify, I hooked up an oscilloscope on my i.MX 7: It shows >>>>> that with DOTCLK_POL=0 the controller drives on falling edge: >>>>> http://imgur.com/a/2f2Xt >>>>> >>>>> So my measurements verify what is in the i.MX data sheets. >>>> >>>> Good >>>> >>>>> The current code sets the bit when negative edge (falling edge) is >>>>> requested, which is wrong: >>>>> #define VDCTRL0_DOTCLK_ACT_FALLING (1 << 25) >>>>> ... >>>>> if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) >>>>> vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; >>>>> >>>>> Not sure what is going on with your display, maybe the datasheet is just >>>>> wrong (it requires DRM_BUS_FLAG_PIXDATA_NEGEDGE in fact) or it is some >>>>> other artifact. >>>> >>>> This is probably where the problem crept in [1], droping PIXDATA_POSEDGE >>>> actually makes this patch work for me. CCing Philipp. >>>> >>>> [1] https://patchwork.kernel.org/patch/9301517/ >>> >>> I looked at a old data sheet of that display and it seemed that >>> PIXDATA_POSEDGE is the right thing. Panelook.cn lists newer data sheets, >>> but I couldn't find them on the open internet, do you have access to a >>> newer one? >> >> Which "version" do you have ? Probably not though. >> > > I couldn't find it anymore, but I think it said Rev. 0 > >>> http://www.panelook.cn/COM43H4M85ULC_ORTUSTECH_4.3_LCM_overview_17296.html >>> >>> I guess in the end it doesn't matter: Given that it is verified that the >>> controllers data sheet is right, I vote for merging that patch and fix >>> the displays polarity... >> >> Merging which patch ? > > This patch. > > And I guess, to keep your display working, a patch which changes the > Ortustech pixel clock flag... Well, let's do that. Btw can you send a V2 with s/edige/edge/ ? ;-) Thanks!
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c index 0818903..4bcc8a3 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c @@ -168,9 +168,16 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH; if (m->flags & DRM_MODE_FLAG_PVSYNC) vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH; - if (bus_flags & DRM_BUS_FLAG_DE_HIGH) + /* Data Enable should be high active by default */ + if (!(bus_flags & DRM_BUS_FLAG_DE_LOW)) vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH; - if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) + /* + * Note: DRM_BUS_FLAG_PIXDATA defines are controller centric, + * controllers VDCTRL0_DOTCLK is display centric. + * Drive on positive edige -> display samples on falling edge + * DRM_BUS_FLAG_PIXDATA_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING + */ + if (bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE) vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0);
The DRM subsystem specifies the pixel clock polarity from a controllers perspective: DRM_BUS_FLAG_PIXDATA_NEGEDGE means the controller drives the data on pixel clocks falling edge. That is the controllers DOTCLK_POL=0 (Default is data launched at negative edge). Also change the data enable logic to be high active by default and only change if explicitly requested via bus_flags. With that defaults are: - Data enable: high active - Pixel clock polarity: controller drives data on negative edge Signed-off-by: Stefan Agner <stefan@agner.ch> --- Hi Marek, I discovered this while testing on a i.MX 7 eLCDIF IP. Particularly the non-standard DE polarity was causing issues. I was using a EDT display which is part of simple panel driver since a while now and does not specify any bus_flags currently... Of course I could (and probably should) add the proper bus_flags there too, but there are several displays which do not specify any polarity and likely rely on sensible driver standards (which is afact high active for the DE signal). -- Stefan drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)