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@xxxxxxxx> >> --- >> 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); >> _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
