| Message ID | 1447900970-15936-8-git-send-email-stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dave, Hi Thierry, Not sure how to handle this patch: it contains a little change in panel-simple.c. I think this should be in one patch, since it changes the associated logic in the driver... Can I send that through my tree? -- Stefan On 2015-11-18 18:42, Stefan Agner wrote: > The current default configuration is as follows: > - Display samples data on the falling edge > - Invert VSYNC signal (active LOW) > - Invert HSYNC signal (active LOW) > > The mode flags allow to specify the required polarity per > display. Furthermore, none of the current driver settings is > actually a standard polarity. > > This patch applies the current driver standard polarities as > explicit flags to the display which has been introduced with > the driver (NEC WQVGA "nec,nl4827hc19-05b"). The driver now > also parses the flags field and applies the configuration > accordingly, by using the following values as defaults (e.g. > if no flags are specified): > - Display samples data on the rising edge > - VSYNC signal not inverted (active HIGH) > - HSYNC signal not inverted (active HIGH) > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +++++++++++++--- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 4 ++-- > drivers/gpu/drm/panel/panel-simple.c | 2 ++ > 3 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > index b2f56e4..db69725 100644 > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > @@ -18,6 +18,8 @@ > #include <drm/drm_crtc.h> > #include <drm/drm_crtc_helper.h> > > +#include <video/display_timing.h> > + > #include "fsl_dcu_drm_crtc.h" > #include "fsl_dcu_drm_drv.h" > #include "fsl_dcu_drm_plane.h" > @@ -74,7 +76,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct > drm_crtc *crtc) > struct drm_device *dev = crtc->dev; > struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > struct drm_display_mode *mode = &crtc->state->mode; > - unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index; > + unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index, pol = 0; > unsigned long dcuclk; > > index = drm_crtc_index(crtc); > @@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct > drm_crtc *crtc) > vfp = mode->vsync_start - mode->vdisplay; > vsw = mode->vsync_end - mode->vsync_start; > > + if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)) > + pol |= DCU_SYN_POL_INV_PXCK_FALL; > + > + if (mode->flags & DRM_MODE_FLAG_NHSYNC) > + pol |= DCU_SYN_POL_INV_HS_LOW; > + > + if (mode->flags & DRM_MODE_FLAG_NHSYNC) > + pol |= DCU_SYN_POL_INV_VS_LOW; > + > regmap_write(fsl_dev->regmap, DCU_HSYN_PARA, > DCU_HSYN_PARA_BP(hbp) | > DCU_HSYN_PARA_PW(hsw) | > @@ -101,8 +112,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct > drm_crtc *crtc) > DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) | > DCU_DISP_SIZE_DELTA_X(mode->hdisplay)); > regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div); > - regmap_write(fsl_dev->regmap, DCU_SYN_POL, > - DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW); > + regmap_write(fsl_dev->regmap, DCU_SYN_POL, pol); > regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) | > DCU_BGND_G(0) | DCU_BGND_B(0)); > regmap_write(fsl_dev->regmap, DCU_DCU_MODE, > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > index 6413ac9..2a724f3 100644 > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > @@ -47,8 +47,8 @@ > #define DCU_VSYN_PARA_FP(x) (x) > > #define DCU_SYN_POL 0x0024 > -#define DCU_SYN_POL_INV_PXCK_FALL (0 << 6) > -#define DCU_SYN_POL_NEG_REMAIN (0 << 5) > +#define DCU_SYN_POL_INV_PXCK_FALL BIT(6) > +#define DCU_SYN_POL_NEG_REMAIN BIT(5) > #define DCU_SYN_POL_INV_VS_LOW BIT(1) > #define DCU_SYN_POL_INV_HS_LOW BIT(0) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c > b/drivers/gpu/drm/panel/panel-simple.c > index f97b73e..fa68b56 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -960,6 +960,8 @@ static const struct drm_display_mode > nec_nl4827hc19_05b_mode = { > .vsync_end = 272 + 2 + 4, > .vtotal = 272 + 2 + 4 + 2, > .vrefresh = 74, > + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC | > + DISPLAY_FLAGS_PIXDATA_POSEDGE, > }; > > static const struct panel_desc nec_nl4827hc19_05b = {
On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote: [...] > > diff --git a/drivers/gpu/drm/panel/panel-simple.c > > b/drivers/gpu/drm/panel/panel-simple.c > > index f97b73e..fa68b56 100644 > > --- a/drivers/gpu/drm/panel/panel-simple.c > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > @@ -960,6 +960,8 @@ static const struct drm_display_mode > > nec_nl4827hc19_05b_mode = { > > .vsync_end = 272 + 2 + 4, > > .vtotal = 272 + 2 + 4 + 2, > > .vrefresh = 74, > > + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC | > > + DISPLAY_FLAGS_PIXDATA_POSEDGE, It doesn't seem like these two types of flags should be mixed because they overlap. DISPLAY_FLAGS_PIXDATA_POSEDGE has the same value as the DRM_MODE_FLAG_CSYNC define. The definition of the DISPLAY_FLAGS_PIXDATA_POSEDGE is also not very clear to me. I don't think we have an equivalent DRM_MODE_FLAG_* but we could add one if there's really a need. Thierry
On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote: [...] > On 2015-11-18 18:42, Stefan Agner wrote: [...] > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c [...] > > @@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct > > drm_crtc *crtc) > > vfp = mode->vsync_start - mode->vdisplay; > > vsw = mode->vsync_end - mode->vsync_start; > > > > + if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)) > > + pol |= DCU_SYN_POL_INV_PXCK_FALL; > > + > > + if (mode->flags & DRM_MODE_FLAG_NHSYNC) > > + pol |= DCU_SYN_POL_INV_HS_LOW; > > + > > + if (mode->flags & DRM_MODE_FLAG_NHSYNC) > > + pol |= DCU_SYN_POL_INV_VS_LOW; I suspect that you want to check for DRM_MODE_FLAG_NVSYNC here instead. > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > index 6413ac9..2a724f3 100644 > > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > @@ -47,8 +47,8 @@ > > #define DCU_VSYN_PARA_FP(x) (x) > > > > #define DCU_SYN_POL 0x0024 > > -#define DCU_SYN_POL_INV_PXCK_FALL (0 << 6) > > -#define DCU_SYN_POL_NEG_REMAIN (0 << 5) > > +#define DCU_SYN_POL_INV_PXCK_FALL BIT(6) > > +#define DCU_SYN_POL_NEG_REMAIN BIT(5) I don't understand these changes. You're in fact changing the values for these defines, but it's not mentioned in the commit message. It also seems like it should be a separate patch because it isn't related to the mode flags changes in the remainder of the patch. Thierry
On 2016-02-03 06:00, Thierry Reding wrote: > On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote: > [...] >> > diff --git a/drivers/gpu/drm/panel/panel-simple.c >> > b/drivers/gpu/drm/panel/panel-simple.c >> > index f97b73e..fa68b56 100644 >> > --- a/drivers/gpu/drm/panel/panel-simple.c >> > +++ b/drivers/gpu/drm/panel/panel-simple.c >> > @@ -960,6 +960,8 @@ static const struct drm_display_mode >> > nec_nl4827hc19_05b_mode = { >> > .vsync_end = 272 + 2 + 4, >> > .vtotal = 272 + 2 + 4 + 2, >> > .vrefresh = 74, >> > + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC | >> > + DISPLAY_FLAGS_PIXDATA_POSEDGE, > > It doesn't seem like these two types of flags should be mixed because > they overlap. DISPLAY_FLAGS_PIXDATA_POSEDGE has the same value as the > DRM_MODE_FLAG_CSYNC define. There are other entries such as hannstar_hsd100pxn1_timing which make use of DISPLAY_FLAGS too, hence I did not bother further. Having a conflict is definitely not what we want. > The definition of the DISPLAY_FLAGS_PIXDATA_POSEDGE is also not very > clear to me. I don't think we have an equivalent DRM_MODE_FLAG_* but > we could add one if there's really a need. E.g. assuming a parallel video signal, the pixel data signals could be changing on positive or negative edge relative to the clock signal. Most displays _sample_ the data on rising edge, hence controllers should normally drive data on falling edge. However, as always, there are exceptions to the rule, and one of the exception is Freescales default display for the Tower evaluation board. It samples data on falling edge, hence the controller should drive data on rising edge... Yeah I also did not found an equivalent in DRM_MODE_FLAG_*. I have here also other displays where we would need this flag. The display-timings binds and enum display_flags support it too, hence I guess we should have it here too. I will split out this patch from the patchset (I already applied the rest), add another patch to add the flag, make use of the flag in this patch and resend it as v2. -- Stefan
On 2016-02-03 06:04, Thierry Reding wrote: > On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote: > [...] >> On 2015-11-18 18:42, Stefan Agner wrote: > [...] >> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c > [...] >> > @@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct >> > drm_crtc *crtc) >> > vfp = mode->vsync_start - mode->vdisplay; >> > vsw = mode->vsync_end - mode->vsync_start; >> > >> > + if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)) >> > + pol |= DCU_SYN_POL_INV_PXCK_FALL; >> > + >> > + if (mode->flags & DRM_MODE_FLAG_NHSYNC) >> > + pol |= DCU_SYN_POL_INV_HS_LOW; >> > + >> > + if (mode->flags & DRM_MODE_FLAG_NHSYNC) >> > + pol |= DCU_SYN_POL_INV_VS_LOW; > > I suspect that you want to check for DRM_MODE_FLAG_NVSYNC here instead. > Ups, yes. >> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h >> > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h >> > index 6413ac9..2a724f3 100644 >> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h >> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h >> > @@ -47,8 +47,8 @@ >> > #define DCU_VSYN_PARA_FP(x) (x) >> > >> > #define DCU_SYN_POL 0x0024 >> > -#define DCU_SYN_POL_INV_PXCK_FALL (0 << 6) >> > -#define DCU_SYN_POL_NEG_REMAIN (0 << 5) >> > +#define DCU_SYN_POL_INV_PXCK_FALL BIT(6) >> > +#define DCU_SYN_POL_NEG_REMAIN BIT(5) > > I don't understand these changes. You're in fact changing the values for > these defines, but it's not mentioned in the commit message. It also > seems like it should be a separate patch because it isn't related to the > mode flags changes in the remainder of the patch. Yes, in order to set them, I need them to be positive, there is no use if they are zero... They haven't been used at all so far, so there is no issue in changing their value. Just realized that their naming is actually related to the 0 value, so I probably should rename them to just reflect the field name #define DCU_SYN_POL_INV_PXCK BIT(6) #define DCU_SYN_POL_NEG BIT(5) -- Stefan
On 2016-02-03 15:18, Stefan Agner wrote: > On 2016-02-03 06:00, Thierry Reding wrote: >> On Wed, Jan 27, 2016 at 06:46:50PM -0800, Stefan Agner wrote: >> [...] >>> > diff --git a/drivers/gpu/drm/panel/panel-simple.c >>> > b/drivers/gpu/drm/panel/panel-simple.c >>> > index f97b73e..fa68b56 100644 >>> > --- a/drivers/gpu/drm/panel/panel-simple.c >>> > +++ b/drivers/gpu/drm/panel/panel-simple.c >>> > @@ -960,6 +960,8 @@ static const struct drm_display_mode >>> > nec_nl4827hc19_05b_mode = { >>> > .vsync_end = 272 + 2 + 4, >>> > .vtotal = 272 + 2 + 4 + 2, >>> > .vrefresh = 74, >>> > + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC | >>> > + DISPLAY_FLAGS_PIXDATA_POSEDGE, >> >> It doesn't seem like these two types of flags should be mixed because >> they overlap. DISPLAY_FLAGS_PIXDATA_POSEDGE has the same value as the >> DRM_MODE_FLAG_CSYNC define. > > There are other entries such as hannstar_hsd100pxn1_timing which make > use of DISPLAY_FLAGS too, hence I did not bother further. Having a > conflict is definitely not what we want. Oh, just realized, hannstar_hsd100pxn1_timing is actually a different type of struct, and for this struct the DISPALY_FLAGS make sense... > >> The definition of the DISPLAY_FLAGS_PIXDATA_POSEDGE is also not very >> clear to me. I don't think we have an equivalent DRM_MODE_FLAG_* but >> we could add one if there's really a need. > > E.g. assuming a parallel video signal, the pixel data signals could be > changing on positive or negative edge relative to the clock signal. Most > displays _sample_ the data on rising edge, hence controllers should > normally drive data on falling edge. > > However, as always, there are exceptions to the rule, and one of the > exception is Freescales default display for the Tower evaluation board. > It samples data on falling edge, hence the controller should drive data > on rising edge... > > Yeah I also did not found an equivalent in DRM_MODE_FLAG_*. I have here > also other displays where we would need this flag. The display-timings > binds and enum display_flags support it too, hence I guess we should > have it here too. > > I will split out this patch from the patchset (I already applied the > rest), add another patch to add the flag, make use of the flag in this > patch and resend it as v2. I realized that after this, there are only two flags which are not yet in DRM_MODE_FLAG: DE_LOW/DE_HIGH. Thierry, what do you think, should I add these remaining two flags too? -- Stefan
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c index b2f56e4..db69725 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c @@ -18,6 +18,8 @@ #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> +#include <video/display_timing.h> + #include "fsl_dcu_drm_crtc.h" #include "fsl_dcu_drm_drv.h" #include "fsl_dcu_drm_plane.h" @@ -74,7 +76,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc) struct drm_device *dev = crtc->dev; struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; struct drm_display_mode *mode = &crtc->state->mode; - unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index; + unsigned int hbp, hfp, hsw, vbp, vfp, vsw, div, index, pol = 0; unsigned long dcuclk; index = drm_crtc_index(crtc); @@ -89,6 +91,15 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc) vfp = mode->vsync_start - mode->vdisplay; vsw = mode->vsync_end - mode->vsync_start; + if (!(mode->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)) + pol |= DCU_SYN_POL_INV_PXCK_FALL; + + if (mode->flags & DRM_MODE_FLAG_NHSYNC) + pol |= DCU_SYN_POL_INV_HS_LOW; + + if (mode->flags & DRM_MODE_FLAG_NHSYNC) + pol |= DCU_SYN_POL_INV_VS_LOW; + regmap_write(fsl_dev->regmap, DCU_HSYN_PARA, DCU_HSYN_PARA_BP(hbp) | DCU_HSYN_PARA_PW(hsw) | @@ -101,8 +112,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc) DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) | DCU_DISP_SIZE_DELTA_X(mode->hdisplay)); regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div); - regmap_write(fsl_dev->regmap, DCU_SYN_POL, - DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW); + regmap_write(fsl_dev->regmap, DCU_SYN_POL, pol); regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) | DCU_BGND_G(0) | DCU_BGND_B(0)); regmap_write(fsl_dev->regmap, DCU_DCU_MODE, diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h index 6413ac9..2a724f3 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h @@ -47,8 +47,8 @@ #define DCU_VSYN_PARA_FP(x) (x) #define DCU_SYN_POL 0x0024 -#define DCU_SYN_POL_INV_PXCK_FALL (0 << 6) -#define DCU_SYN_POL_NEG_REMAIN (0 << 5) +#define DCU_SYN_POL_INV_PXCK_FALL BIT(6) +#define DCU_SYN_POL_NEG_REMAIN BIT(5) #define DCU_SYN_POL_INV_VS_LOW BIT(1) #define DCU_SYN_POL_INV_HS_LOW BIT(0) diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index f97b73e..fa68b56 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -960,6 +960,8 @@ static const struct drm_display_mode nec_nl4827hc19_05b_mode = { .vsync_end = 272 + 2 + 4, .vtotal = 272 + 2 + 4 + 2, .vrefresh = 74, + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC | + DISPLAY_FLAGS_PIXDATA_POSEDGE, }; static const struct panel_desc nec_nl4827hc19_05b = {
The current default configuration is as follows: - Display samples data on the falling edge - Invert VSYNC signal (active LOW) - Invert HSYNC signal (active LOW) The mode flags allow to specify the required polarity per display. Furthermore, none of the current driver settings is actually a standard polarity. This patch applies the current driver standard polarities as explicit flags to the display which has been introduced with the driver (NEC WQVGA "nec,nl4827hc19-05b"). The driver now also parses the flags field and applies the configuration accordingly, by using the following values as defaults (e.g. if no flags are specified): - Display samples data on the rising edge - VSYNC signal not inverted (active HIGH) - HSYNC signal not inverted (active HIGH) Signed-off-by: Stefan Agner <stefan@agner.ch> --- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +++++++++++++--- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 4 ++-- drivers/gpu/drm/panel/panel-simple.c | 2 ++ 3 files changed, 17 insertions(+), 5 deletions(-)