| Message ID | 1565779731-1300-1-git-send-email-robert.chiras@nxp.com |
|---|---|
| Headers | show |
| Series | Improvements and fixes for mxsfb DRM driver | expand |
On 2019-08-14 12:48, Robert Chiras wrote: > Currently, the enable of the axi clock return status is ignored, causing > issues when the enable fails then we try to disable it. Therefore, it is > better to check the return status and disable it only when enable > succeeded. Is this actually the case in real world sometimes? Why is it failing? I guess if we do this in one place, we should do it in all places (e.g. also in mxsfb_crtc_enable, mxsfb_plane_atomic_update..) -- Stefan > Also, remove the helper functions around clk_axi, since we can directly > use the clk API function for enable/disable the clock. Those functions > are already checking for NULL clk and returning 0 if that's the case. > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > Acked-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 8 ++++---- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 32 +++++++++++++------------------- > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 3 --- > 3 files changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index a4ba368..e727f5e 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -408,7 +408,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) > { > dma_addr_t paddr; > > - mxsfb_enable_axi_clk(mxsfb); > + clk_prepare_enable(mxsfb->clk_axi); > writel(0, mxsfb->base + LCDC_CTRL); > mxsfb_crtc_mode_set_nofb(mxsfb); > > @@ -425,7 +425,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) > void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) > { > mxsfb_disable_controller(mxsfb); > - mxsfb_disable_axi_clk(mxsfb); > + clk_disable_unprepare(mxsfb->clk_axi); > } > > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > @@ -451,8 +451,8 @@ void mxsfb_plane_atomic_update(struct > mxsfb_drm_private *mxsfb, > > paddr = mxsfb_get_fb_paddr(mxsfb); > if (paddr) { > - mxsfb_enable_axi_clk(mxsfb); > + clk_prepare_enable(mxsfb->clk_axi); > writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > - mxsfb_disable_axi_clk(mxsfb); > + clk_disable_unprepare(mxsfb->clk_axi); > } > } > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index 6dae2bd..694b287 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -97,18 +97,6 @@ drm_pipe_to_mxsfb_drm_private(struct > drm_simple_display_pipe *pipe) > return container_of(pipe, struct mxsfb_drm_private, pipe); > } > > -void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb) > -{ > - if (mxsfb->clk_axi) > - clk_prepare_enable(mxsfb->clk_axi); > -} > - > -void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb) > -{ > - if (mxsfb->clk_axi) > - clk_disable_unprepare(mxsfb->clk_axi); > -} > - > /** > * mxsfb_atomic_helper_check - validate state object > * @dev: DRM device > @@ -229,25 +217,31 @@ static void mxsfb_pipe_update(struct > drm_simple_display_pipe *pipe, > static int mxsfb_pipe_enable_vblank(struct drm_simple_display_pipe *pipe) > { > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > + int ret = 0; > + > + ret = clk_prepare_enable(mxsfb->clk_axi); > + if (ret) > + return ret; > > /* Clear and enable VBLANK IRQ */ > - mxsfb_enable_axi_clk(mxsfb); > writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); > writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_SET); > - mxsfb_disable_axi_clk(mxsfb); > + clk_disable_unprepare(mxsfb->clk_axi); > > - return 0; > + return ret; > } > > static void mxsfb_pipe_disable_vblank(struct drm_simple_display_pipe *pipe) > { > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > > + if (clk_prepare_enable(mxsfb->clk_axi)) > + return; > + > /* Disable and clear VBLANK IRQ */ > - mxsfb_enable_axi_clk(mxsfb); > writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR); > writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); > - mxsfb_disable_axi_clk(mxsfb); > + clk_disable_unprepare(mxsfb->clk_axi); > } > > static struct drm_simple_display_pipe_funcs mxsfb_funcs = { > @@ -413,7 +407,7 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data) > struct mxsfb_drm_private *mxsfb = drm->dev_private; > u32 reg; > > - mxsfb_enable_axi_clk(mxsfb); > + clk_prepare_enable(mxsfb->clk_axi); > > reg = readl(mxsfb->base + LCDC_CTRL1); > > @@ -422,7 +416,7 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data) > > writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); > > - mxsfb_disable_axi_clk(mxsfb); > + clk_disable_unprepare(mxsfb->clk_axi); > > return IRQ_HANDLED; > } > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > index 8fb65d3..d6df8fe 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > @@ -37,9 +37,6 @@ struct mxsfb_drm_private { > int mxsfb_setup_crtc(struct drm_device *dev); > int mxsfb_create_output(struct drm_device *dev); > > -void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb); > -void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb); > - > void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb); > void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb); > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
On 2019-08-14 12:48, Robert Chiras wrote: > Some of the regiters need, like LCDC_CTRL and CTRL2_OUTSTANDING_REQS Typo in registers, and there is a need to many. > needs to be properly cleared and initialized for a better start and stop > routine. > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index b69ace8..5e44f57 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -127,6 +127,10 @@ static void mxsfb_enable_controller(struct > mxsfb_drm_private *mxsfb) > clk_prepare_enable(mxsfb->clk_disp_axi); > clk_prepare_enable(mxsfb->clk); > > + if (mxsfb->devdata->ipversion >= 4) > + writel(CTRL2_OUTSTANDING_REQS(REQ_16), > + mxsfb->base + LCDC_V4_CTRL2 + REG_SET); > + > /* If it was disabled, re-enable the mode again */ > writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); > > @@ -136,12 +140,19 @@ static void mxsfb_enable_controller(struct > mxsfb_drm_private *mxsfb) > writel(reg, mxsfb->base + LCDC_VDCTRL4); > > writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_SET); > + writel(CTRL1_RECOVERY_ON_UNDERFLOW, mxsfb->base + LCDC_CTRL1 + REG_SET); This seems not to be accounted for in the commit message. Can you do this in a separate commit? Also I suggest to introduce CTRL1_RECOVERY_ON_UNDERFLOW in that same commit. -- Stefan > } > > static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb) > { > u32 reg; > > + if (mxsfb->devdata->ipversion >= 4) > + writel(CTRL2_OUTSTANDING_REQS(0x7), > + mxsfb->base + LCDC_V4_CTRL2 + REG_CLR); > + > + writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_CLR); > + > /* > * Even if we disable the controller here, it will still continue > * until its FIFOs are running out of data > @@ -295,6 +306,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb) > dma_addr_t paddr; > > mxsfb_enable_axi_clk(mxsfb); > + writel(0, mxsfb->base + LCDC_CTRL); > mxsfb_crtc_mode_set_nofb(mxsfb); > > /* Write cur_buf as well to avoid an initial corrupt frame */
Hi Stefan, On Mi, 2019-08-14 at 13:06 +0200, Stefan Agner wrote: > On 2019-08-14 12:48, Robert Chiras wrote: > > > > Currently, the enable of the axi clock return status is ignored, > > causing > > issues when the enable fails then we try to disable it. Therefore, > > it is > > better to check the return status and disable it only when enable > > succeeded. > Is this actually the case in real world sometimes? Why is it failing? When I noticed that fail, we had some restrictions in SCU firmware, so that the clock cannot be enabled if the power domain was down. Still, at that time I noticed it is redundant to have functions that checks if a clock is NULL or not, since the clk_* functions are already doing this. > > I guess if we do this in one place, we should do it in all places > (e.g. > also in mxsfb_crtc_enable, mxsfb_plane_atomic_update..) I add specific checks only in the vblank functions, because those functions can be called before calling the pipe enable/disable functions which enables the spefic power domain and, at this point, the clock enable/disable calls should not fail. > > -- > Stefan > > > > > Also, remove the helper functions around clk_axi, since we can > > directly > > use the clk API function for enable/disable the clock. Those > > functions > > are already checking for NULL clk and returning 0 if that's the > > case. > > > > > > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > > Acked-by: Leonard Crestez <leonard.crestez@nxp.com> > > --- > > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 8 ++++---- > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 32 +++++++++++++------------- > > ------ > > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 3 --- > > 3 files changed, 17 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > index a4ba368..e727f5e 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > @@ -408,7 +408,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private > > *mxsfb) > > { > > dma_addr_t paddr; > > > > - mxsfb_enable_axi_clk(mxsfb); > > + clk_prepare_enable(mxsfb->clk_axi); > > writel(0, mxsfb->base + LCDC_CTRL); > > mxsfb_crtc_mode_set_nofb(mxsfb); > > > > @@ -425,7 +425,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private > > *mxsfb) > > void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) > > { > > mxsfb_disable_controller(mxsfb); > > - mxsfb_disable_axi_clk(mxsfb); > > + clk_disable_unprepare(mxsfb->clk_axi); > > } > > > > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > > @@ -451,8 +451,8 @@ void mxsfb_plane_atomic_update(struct > > mxsfb_drm_private *mxsfb, > > > > paddr = mxsfb_get_fb_paddr(mxsfb); > > if (paddr) { > > - mxsfb_enable_axi_clk(mxsfb); > > + clk_prepare_enable(mxsfb->clk_axi); > > writel(paddr, mxsfb->base + mxsfb->devdata- > > >next_buf); > > - mxsfb_disable_axi_clk(mxsfb); > > + clk_disable_unprepare(mxsfb->clk_axi); > > } > > } > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > index 6dae2bd..694b287 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > @@ -97,18 +97,6 @@ drm_pipe_to_mxsfb_drm_private(struct > > drm_simple_display_pipe *pipe) > > return container_of(pipe, struct mxsfb_drm_private, pipe); > > } > > > > -void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb) > > -{ > > - if (mxsfb->clk_axi) > > - clk_prepare_enable(mxsfb->clk_axi); > > -} > > - > > -void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb) > > -{ > > - if (mxsfb->clk_axi) > > - clk_disable_unprepare(mxsfb->clk_axi); > > -} > > - > > /** > > * mxsfb_atomic_helper_check - validate state object > > * @dev: DRM device > > @@ -229,25 +217,31 @@ static void mxsfb_pipe_update(struct > > drm_simple_display_pipe *pipe, > > static int mxsfb_pipe_enable_vblank(struct drm_simple_display_pipe > > *pipe) > > { > > struct mxsfb_drm_private *mxsfb = > > drm_pipe_to_mxsfb_drm_private(pipe); > > + int ret = 0; > > + > > + ret = clk_prepare_enable(mxsfb->clk_axi); > > + if (ret) > > + return ret; > > > > /* Clear and enable VBLANK IRQ */ > > - mxsfb_enable_axi_clk(mxsfb); > > writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + > > REG_CLR); > > writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 > > + REG_SET); > > - mxsfb_disable_axi_clk(mxsfb); > > + clk_disable_unprepare(mxsfb->clk_axi); > > > > - return 0; > > + return ret; > > } > > > > static void mxsfb_pipe_disable_vblank(struct > > drm_simple_display_pipe *pipe) > > { > > struct mxsfb_drm_private *mxsfb = > > drm_pipe_to_mxsfb_drm_private(pipe); > > > > + if (clk_prepare_enable(mxsfb->clk_axi)) > > + return; > > + > > /* Disable and clear VBLANK IRQ */ > > - mxsfb_enable_axi_clk(mxsfb); > > writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 > > + REG_CLR); > > writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + > > REG_CLR); > > - mxsfb_disable_axi_clk(mxsfb); > > + clk_disable_unprepare(mxsfb->clk_axi); > > } > > > > static struct drm_simple_display_pipe_funcs mxsfb_funcs = { > > @@ -413,7 +407,7 @@ static irqreturn_t mxsfb_irq_handler(int irq, > > void *data) > > struct mxsfb_drm_private *mxsfb = drm->dev_private; > > u32 reg; > > > > - mxsfb_enable_axi_clk(mxsfb); > > + clk_prepare_enable(mxsfb->clk_axi); > > > > reg = readl(mxsfb->base + LCDC_CTRL1); > > > > @@ -422,7 +416,7 @@ static irqreturn_t mxsfb_irq_handler(int irq, > > void *data) > > > > writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + > > REG_CLR); > > > > - mxsfb_disable_axi_clk(mxsfb); > > + clk_disable_unprepare(mxsfb->clk_axi); > > > > return IRQ_HANDLED; > > } > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > index 8fb65d3..d6df8fe 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > @@ -37,9 +37,6 @@ struct mxsfb_drm_private { > > int mxsfb_setup_crtc(struct drm_device *dev); > > int mxsfb_create_output(struct drm_device *dev); > > > > -void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb); > > -void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb); > > - > > void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb); > > void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb); > > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, Thanks, Robert
Hi Stefan, On Mi, 2019-08-14 at 13:11 +0200, Stefan Agner wrote: > On 2019-08-14 12:48, Robert Chiras wrote: > > > > Some of the regiters need, like LCDC_CTRL and > > CTRL2_OUTSTANDING_REQS > Typo in registers, and there is a need to many. Thanks, will fix this. > > > > > needs to be properly cleared and initialized for a better start and > > stop > > routine. > > > > > > > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > > --- > > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > index b69ace8..5e44f57 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > @@ -127,6 +127,10 @@ static void mxsfb_enable_controller(struct > > mxsfb_drm_private *mxsfb) > > clk_prepare_enable(mxsfb->clk_disp_axi); > > clk_prepare_enable(mxsfb->clk); > > > > + if (mxsfb->devdata->ipversion >= 4) > > + writel(CTRL2_OUTSTANDING_REQS(REQ_16), > > + mxsfb->base + LCDC_V4_CTRL2 + REG_SET); > > + > > /* If it was disabled, re-enable the mode again */ > > writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); > > > > @@ -136,12 +140,19 @@ static void mxsfb_enable_controller(struct > > mxsfb_drm_private *mxsfb) > > writel(reg, mxsfb->base + LCDC_VDCTRL4); > > > > writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_SET); > > + writel(CTRL1_RECOVERY_ON_UNDERFLOW, mxsfb->base + LCDC_CTRL1 > > + REG_SET); > This seems not to be accounted for in the commit message. Can you do > this in a separate commit? > > Also I suggest to introduce CTRL1_RECOVERY_ON_UNDERFLOW in that same > commit. You are right, I missed this one in the description. I will add this one too. > > -- > Stefan > > > > > } > > > > static void mxsfb_disable_controller(struct mxsfb_drm_private > > *mxsfb) > > { > > u32 reg; > > > > + if (mxsfb->devdata->ipversion >= 4) > > + writel(CTRL2_OUTSTANDING_REQS(0x7), > > + mxsfb->base + LCDC_V4_CTRL2 + REG_CLR); > > + > > + writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_CLR); > > + > > /* > > * Even if we disable the controller here, it will still > > continue > > * until its FIFOs are running out of data > > @@ -295,6 +306,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private > > *mxsfb) > > dma_addr_t paddr; > > > > mxsfb_enable_axi_clk(mxsfb); > > + writel(0, mxsfb->base + LCDC_CTRL); > > mxsfb_crtc_mode_set_nofb(mxsfb); > > > > /* Write cur_buf as well to avoid an initial corrupt frame */ Thanks, Robert
Hi Robert, On Wed, 14 Aug 2019 at 11:49, Robert Chiras <robert.chiras@nxp.com> wrote: > + case DRM_FORMAT_BGR565: /* BG16 */ > + if (mxsfb->devdata->ipversion < 4) > + goto err; > + writel(CTRL2_ODD_LINE_PATTERN(CTRL2_LINE_PATTERN_BGR) | > + CTRL2_EVEN_LINE_PATTERN(CTRL2_LINE_PATTERN_BGR), > + mxsfb->base + LCDC_V4_CTRL2 + REG_SET); > + /* Fall through */ > + case DRM_FORMAT_RGB565: /* RG16 */ > + ctrl |= CTRL_SET_WORD_LENGTH(0); > + ctrl &= ~CTRL_DF16; > + ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); > + break; For non-BGR formats, do you need to write RGB line-pattern back to the CTRL2 register? Otherwise, if you start with BGR565 then switch back to RGB565, presumably CTRL2 would still be programmed for BGR so you would display inverted channels. Same goes for all the other BGR/RGB format pairs below. Cheers, Daniel
Hi Daniel, On Mi, 2019-08-14 at 12:44 +0100, Daniel Stone wrote: > Hi Robert, > > On Wed, 14 Aug 2019 at 11:49, Robert Chiras <robert.chiras@nxp.com> > wrote: > > > > + case DRM_FORMAT_BGR565: /* BG16 */ > > + if (mxsfb->devdata->ipversion < 4) > > + goto err; > > + writel(CTRL2_ODD_LINE_PATTERN(CTRL2_LINE_PATTERN_BG > > R) | > > + CTRL2_EVEN_LINE_PATTERN(CTRL2_LINE_PATTERN_ > > BGR), > > + mxsfb->base + LCDC_V4_CTRL2 + REG_SET); > > + /* Fall through */ > > + case DRM_FORMAT_RGB565: /* RG16 */ > > + ctrl |= CTRL_SET_WORD_LENGTH(0); > > + ctrl &= ~CTRL_DF16; > > + ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); > > + break; > For non-BGR formats, do you need to write RGB line-pattern back to > the > CTRL2 register? Otherwise, if you start with BGR565 then switch back > to RGB565, presumably CTRL2 would still be programmed for BGR so you > would display inverted channels. The LINE_PATTERN from LCDC_V4_CTRL2 register is cleared above, with this code: + if (mxsfb->devdata->ipversion >= 4) + writel(CTRL2_ODD_LINE_PATTERN(CTRL2_LINE_PATTERN_CLR) | + CTRL2_EVEN_LINE_PATTERN(CTRL2_LINE_PATTERN_CLR), + mxsfb->base + LCDC_V4_CTRL2 + REG_CLR); > > Same goes for all the other BGR/RGB format pairs below. > > Cheers, > Daniel Thanks, Robert
On Wed, Aug 14, 2019 at 01:48:43PM +0300, Robert Chiras wrote: > Currently, the vblank support is not correctly implemented in MXSFB_DRM > driver. The call to drm_vblank_init is made with mode_config.num_crtc > which at that time is 0. Because of this, vblank is not activated, so > there won't be any vblank event submitted. > For example, when running modetest with the '-v' parameter will result > in an astronomical refresh rate (10000+ Hz), because of that. Uh, that sounds a bit like a bug somewhere. If vblank doesn't work, we should give userspace an error back. Maybe we need more checks in drm_vblank_init()? Can you pls look into that? > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > --- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index 2743975..829abec 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -38,6 +38,9 @@ > #include "mxsfb_drv.h" > #include "mxsfb_regs.h" > > +/* The eLCDIF max possible CRTCs */ > +#define MAX_CRTCS 1 > + > enum mxsfb_devtype { > MXSFB_V3, > MXSFB_V4, > @@ -138,6 +141,8 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, > mxsfb->connector = &mxsfb->panel_connector; > } > > + drm_crtc_vblank_on(&pipe->crtc); > + > pm_runtime_get_sync(drm->dev); > drm_panel_prepare(mxsfb->panel); > mxsfb_crtc_enable(mxsfb); > @@ -164,6 +169,8 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > } > spin_unlock_irq(&drm->event_lock); > > + drm_crtc_vblank_off(&pipe->crtc); > + > if (mxsfb->connector != &mxsfb->panel_connector) > mxsfb->connector = NULL; > } > @@ -246,7 +253,7 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags) > > pm_runtime_enable(drm->dev); > > - ret = drm_vblank_init(drm, drm->mode_config.num_crtc); > + ret = drm_vblank_init(drm, MAX_CRTCS); > if (ret < 0) { > dev_err(drm->dev, "Failed to initialise vblank\n"); > goto err_vblank; > @@ -269,6 +276,8 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags) > goto err_vblank; > } > > + drm_crtc_vblank_off(&mxsfb->pipe.crtc); Are you sure you need this one here? vblanks should be off after drm_vblank_init() is called. Thanks, Daniel > + > /* > * Attach panel only if there is one. > * If there is no panel attach, it must be a bridge. In this case, we > -- > 2.7.4 >