| Message ID | 1567078215-31601-1-git-send-email-robert.chiras@nxp.com |
|---|---|
| Headers | show |
| Series | Improvements and fixes for mxsfb DRM driver | expand |
Hi Robert, Sorry it took me so long to have a closer look at this patchset. I will definitely merge part of it, but this particular patch actually breaks i.MX 7. I have vertical stripes on my display with this patch applied (using Weston with DRM backend). Not sure why this exactly happens, from what I understand this should only affect IP Version 4. Some notes below: On 2019-08-29 13:30, Robert Chiras wrote: > Besides the eLCDIF block, there is another IP block, used in the past > for EPDC panels. Since the iMX.8mq doesn't have an EPDC connector, this > block is not documented, but we can use it to do additional operations > on the frame buffer. Hm, but this block is part of the ELCDIF block, in terms of clock, power domain etc? > In this case, we can use the pigeon registers from this IP block in > order to do horizontal crop on the frame buffer processed by the eLCDIF > block. > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > Tested-by: Guido Günther <agx@sigxcpu.org> > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 79 ++++++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 1 + > drivers/gpu/drm/mxsfb/mxsfb_regs.h | 16 ++++++++ > 3 files changed, 92 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index a12f53d..317575e 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -15,6 +15,7 @@ > > #include <video/videomode.h> > > +#include <drm/drm_atomic.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_crtc.h> > #include <drm/drm_fb_cma_helper.h> > @@ -435,13 +436,66 @@ void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) > clk_disable_unprepare(mxsfb->clk_axi); > } > > +void mxsfb_set_fb_hcrop(struct mxsfb_drm_private *mxsfb, u32 src_w, u32 fb_w) > +{ > + u32 mask_cnt, htotal, hcount; > + u32 vdctrl2, vdctrl3, vdctrl4, transfer_count; > + u32 pigeon_12_0, pigeon_12_1, pigeon_12_2; > + > + if (src_w == fb_w) { > + writel(0x0, mxsfb->base + HW_EPDC_PIGEON_12_0); > + writel(0x0, mxsfb->base + HW_EPDC_PIGEON_12_1); > + > + return; > + } > + > + transfer_count = readl(mxsfb->base + LCDC_V4_TRANSFER_COUNT); > + hcount = TRANSFER_COUNT_GET_HCOUNT(transfer_count); > + > + transfer_count &= ~TRANSFER_COUNT_SET_HCOUNT(0xffff); > + transfer_count |= TRANSFER_COUNT_SET_HCOUNT(fb_w); > + writel(transfer_count, mxsfb->base + LCDC_V4_TRANSFER_COUNT); > + > + vdctrl2 = readl(mxsfb->base + LCDC_VDCTRL2); > + htotal = VDCTRL2_GET_HSYNC_PERIOD(vdctrl2); > + htotal += fb_w - hcount; > + vdctrl2 &= ~VDCTRL2_SET_HSYNC_PERIOD(0x3ffff); > + vdctrl2 |= VDCTRL2_SET_HSYNC_PERIOD(htotal); > + writel(vdctrl2, mxsfb->base + LCDC_VDCTRL2); > + > + vdctrl4 = readl(mxsfb->base + LCDC_VDCTRL4); > + vdctrl4 &= ~SET_DOTCLK_H_VALID_DATA_CNT(0x3ffff); > + vdctrl4 |= SET_DOTCLK_H_VALID_DATA_CNT(fb_w); > + writel(vdctrl4, mxsfb->base + LCDC_VDCTRL4); > + > + /* configure related pigeon registers */ > + vdctrl3 = readl(mxsfb->base + LCDC_VDCTRL3); > + mask_cnt = GET_HOR_WAIT_CNT(vdctrl3) - 5; > + > + pigeon_12_0 = PIGEON_12_0_SET_STATE_MASK(0x24) | > + PIGEON_12_0_SET_MASK_CNT(mask_cnt) | > + PIGEON_12_0_SET_MASK_CNT_SEL(0x6) | > + PIGEON_12_0_POL_ACTIVE_LOW | > + PIGEON_12_0_EN; > + writel(pigeon_12_0, mxsfb->base + HW_EPDC_PIGEON_12_0); > + > + pigeon_12_1 = PIGEON_12_1_SET_CLR_CNT(src_w) | > + PIGEON_12_1_SET_SET_CNT(0x0); > + writel(pigeon_12_1, mxsfb->base + HW_EPDC_PIGEON_12_1); > + > + pigeon_12_2 = 0x0; > + writel(pigeon_12_2, mxsfb->base + HW_EPDC_PIGEON_12_2); > +} > + > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > struct drm_plane_state *state) > { > struct drm_simple_display_pipe *pipe = &mxsfb->pipe; > struct drm_crtc *crtc = &pipe->crtc; > + struct drm_plane_state *new_state = pipe->plane.state; > + struct drm_framebuffer *fb = pipe->plane.state->fb; > struct drm_pending_vblank_event *event; > - dma_addr_t paddr; > + u32 fb_addr, src_off, src_w, stride, cpp = 0; dma_addr_t seems to be the better type here, why change? > > spin_lock_irq(&crtc->dev->event_lock); > event = crtc->state->event; > @@ -456,10 +510,27 @@ void mxsfb_plane_atomic_update(struct > mxsfb_drm_private *mxsfb, > } > spin_unlock_irq(&crtc->dev->event_lock); > > - paddr = mxsfb_get_fb_paddr(mxsfb); > - if (paddr) { > + if (!fb) > + return; > + > + fb_addr = mxsfb_get_fb_paddr(mxsfb); > + if (mxsfb->devdata->ipversion >= 4) { > + cpp = fb->format->cpp[0]; > + src_off = (new_state->src_y >> 16) * fb->pitches[0] + > + (new_state->src_x >> 16) * cpp; > + fb_addr += fb->offsets[0] + src_off; > + } > + > + if (fb_addr) { > clk_prepare_enable(mxsfb->clk_axi); > - writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > + writel(fb_addr, mxsfb->base + mxsfb->devdata->next_buf); > clk_disable_unprepare(mxsfb->clk_axi); > } > + > + if (mxsfb->devdata->ipversion >= 4 && > + unlikely(drm_atomic_crtc_needs_modeset(new_state->crtc->state))) { > + stride = DIV_ROUND_UP(fb->pitches[0], cpp); > + src_w = new_state->src_w >> 16; > + mxsfb_set_fb_hcrop(mxsfb, src_w, stride); > + } > } > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index 888b520..06d3bf0 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -133,6 +133,7 @@ static int mxsfb_atomic_helper_check(struct drm_device *dev, > if (old_bpp != new_bpp) > new_state->mode_changed = true; > } > + Can you also drop this unrelated change? -- Stefan > return ret; > } > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h > b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > index 0f63ba1..df3279b 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > @@ -145,6 +145,22 @@ > #define DEBUG0_HSYNC BIT(26) > #define DEBUG0_VSYNC BIT(25) > > +/* pigeon registers for crop */ > +#define HW_EPDC_PIGEON_12_0 0xb00 > +#define HW_EPDC_PIGEON_12_1 0xb10 > +#define HW_EPDC_PIGEON_12_2 0xb20 > + > +#define PIGEON_12_0_SET_STATE_MASK(x) REG_PUT((x), 31, 24) > +#define PIGEON_12_0_SET_MASK_CNT(x) REG_PUT((x), 23, 12) > +#define PIGEON_12_0_SET_MASK_CNT_SEL(x) REG_PUT((x), 11, 8) > +#define PIGEON_12_0_SET_OFFSET(x) REG_PUT((x), 7, 4) > +#define PIGEON_12_0_SET_INC_SEL(x) REG_PUT((x), 3, 2) > +#define PIGEON_12_0_POL_ACTIVE_LOW BIT(1) > +#define PIGEON_12_0_EN BIT(0) > + > +#define PIGEON_12_1_SET_CLR_CNT(x) REG_PUT((x), 31, 16) > +#define PIGEON_12_1_SET_SET_CNT(x) REG_PUT((x), 15, 0) > + > #define MXSFB_MIN_XRES 120 > #define MXSFB_MIN_YRES 120 > #define MXSFB_MAX_XRES 0xffff
On 2019-08-29 13:30, Robert Chiras wrote: > Some of the registers, like LCDC_CTRL, CTRL2_OUTSTANDING_REQS and > CTRL1_RECOVERY_ON_UNDERFLOW needs to be properly cleared/initialized > for a better start and stop routine. This patch uses CTRL2_OUTSTANDING_REQS which is only introduced in the next patch. This breaks bisectability. -- Stefan > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > Tested-by: Guido Günther <agx@sigxcpu.org> > --- > 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); > } > > 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 */
On 2019-08-29 13:30, Robert Chiras wrote: > Use BIT(x) and GEN_MASK(h, l) for better representation the inside of > various registers. > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > Tested-by: Guido Günther <agx@sigxcpu.org> > --- > drivers/gpu/drm/mxsfb/mxsfb_regs.h | 151 ++++++++++++++++++++++--------------- > 1 file changed, 89 insertions(+), 62 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h > b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > index 71426aa..9fcb1db 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > @@ -40,66 +40,93 @@ > #define LCDC_AS_BUF 0x220 > #define LCDC_AS_NEXT_BUF 0x230 > > -#define CTRL_SFTRST (1 << 31) > -#define CTRL_CLKGATE (1 << 30) > -#define CTRL_BYPASS_COUNT (1 << 19) > -#define CTRL_VSYNC_MODE (1 << 18) > -#define CTRL_DOTCLK_MODE (1 << 17) > -#define CTRL_DATA_SELECT (1 << 16) > -#define CTRL_SET_BUS_WIDTH(x) (((x) & 0x3) << 10) > -#define CTRL_GET_BUS_WIDTH(x) (((x) >> 10) & 0x3) > -#define CTRL_BUS_WIDTH_MASK (0x3 << 10) > -#define CTRL_SET_WORD_LENGTH(x) (((x) & 0x3) << 8) > -#define CTRL_GET_WORD_LENGTH(x) (((x) >> 8) & 0x3) > -#define CTRL_MASTER (1 << 5) > -#define CTRL_DF16 (1 << 3) > -#define CTRL_DF18 (1 << 2) > -#define CTRL_DF24 (1 << 1) > -#define CTRL_RUN (1 << 0) > - > -#define CTRL1_RECOVERY_ON_UNDERFLOW (1 << 24) > -#define CTRL1_FIFO_CLEAR (1 << 21) > -#define CTRL1_SET_BYTE_PACKAGING(x) (((x) & 0xf) << 16) > -#define CTRL1_GET_BYTE_PACKAGING(x) (((x) >> 16) & 0xf) > -#define CTRL1_CUR_FRAME_DONE_IRQ_EN (1 << 13) > -#define CTRL1_CUR_FRAME_DONE_IRQ (1 << 9) > - > -#define CTRL2_OUTSTANDING_REQS__REQ_16 (4 << 21) > - This is introduced two patches earlier just to be removed here. I suggest to reorder this patch in front of "drm/mxsfb: Add defines for the rest of registers", basically convert first to using BIT/GENMASK etc, and then introduce the new request. > -#define TRANSFER_COUNT_SET_VCOUNT(x) (((x) & 0xffff) << 16) > -#define TRANSFER_COUNT_GET_VCOUNT(x) (((x) >> 16) & 0xffff) > -#define TRANSFER_COUNT_SET_HCOUNT(x) ((x) & 0xffff) > -#define TRANSFER_COUNT_GET_HCOUNT(x) ((x) & 0xffff) > - > -#define VDCTRL0_ENABLE_PRESENT (1 << 28) > -#define VDCTRL0_VSYNC_ACT_HIGH (1 << 27) > -#define VDCTRL0_HSYNC_ACT_HIGH (1 << 26) > -#define VDCTRL0_DOTCLK_ACT_FALLING (1 << 25) > -#define VDCTRL0_ENABLE_ACT_HIGH (1 << 24) > -#define VDCTRL0_VSYNC_PERIOD_UNIT (1 << 21) > -#define VDCTRL0_VSYNC_PULSE_WIDTH_UNIT (1 << 20) > -#define VDCTRL0_HALF_LINE (1 << 19) > -#define VDCTRL0_HALF_LINE_MODE (1 << 18) > -#define VDCTRL0_SET_VSYNC_PULSE_WIDTH(x) ((x) & 0x3ffff) > -#define VDCTRL0_GET_VSYNC_PULSE_WIDTH(x) ((x) & 0x3ffff) > - > -#define VDCTRL2_SET_HSYNC_PERIOD(x) ((x) & 0x3ffff) > -#define VDCTRL2_GET_HSYNC_PERIOD(x) ((x) & 0x3ffff) > - > -#define VDCTRL3_MUX_SYNC_SIGNALS (1 << 29) > -#define VDCTRL3_VSYNC_ONLY (1 << 28) > -#define SET_HOR_WAIT_CNT(x) (((x) & 0xfff) << 16) > -#define GET_HOR_WAIT_CNT(x) (((x) >> 16) & 0xfff) > -#define SET_VERT_WAIT_CNT(x) ((x) & 0xffff) > -#define GET_VERT_WAIT_CNT(x) ((x) & 0xffff) > - > -#define VDCTRL4_SET_DOTCLK_DLY(x) (((x) & 0x7) << 29) /* v4 only */ > -#define VDCTRL4_GET_DOTCLK_DLY(x) (((x) >> 29) & 0x7) /* v4 only */ > -#define VDCTRL4_SYNC_SIGNALS_ON (1 << 18) > -#define SET_DOTCLK_H_VALID_DATA_CNT(x) ((x) & 0x3ffff) > - > -#define DEBUG0_HSYNC (1 < 26) > -#define DEBUG0_VSYNC (1 < 25) > +/* reg bit manipulation */ > +#define REG_PUT(x, h, l) (((x) << (l)) & GENMASK(h, l)) > +#define REG_GET(x, h, l) (((x) & GENMASK(h, l)) >> (l)) > + > +#define CTRL_SFTRST BIT(31) > +#define CTRL_CLKGATE BIT(30) > +#define CTRL_SHIFT_DIR(x) REG_PUT((x), 26, 26) > +#define CTRL_SHIFT_NUM(x) REG_PUT((x), 25, 21) > +#define CTRL_BYPASS_COUNT BIT(19) > +#define CTRL_VSYNC_MODE BIT(18) > +#define CTRL_DOTCLK_MODE BIT(17) > +#define CTRL_DATA_SELECT BIT(16) > +#define CTRL_INPUT_SWIZZLE(x) REG_PUT((x), 15, 14) > +#define CTRL_CSC_SWIZZLE(x) REG_PUT((x), 13, 12) > +#define CTRL_SET_BUS_WIDTH(x) REG_PUT((x), 11, 10) > +#define CTRL_GET_BUS_WIDTH(x) REG_GET((x), 11, 10) > +#define CTRL_BUS_WIDTH_MASK REG_PUT((0x3), 11, 10) > +#define CTRL_SET_WORD_LENGTH(x) REG_PUT((x), 9, 8) > +#define CTRL_GET_WORD_LENGTH(x) REG_GET((x), 9, 8) > +#define CTRL_MASTER BIT(5) > +#define CTRL_DF16 BIT(3) > +#define CTRL_DF18 BIT(2) > +#define CTRL_DF24 BIT(1) > +#define CTRL_RUN BIT(0) > + > +#define CTRL1_RECOVERY_ON_UNDERFLOW BIT(24) > +#define CTRL1_FIFO_CLEAR BIT(21) > + > +/* > + * BYTE_PACKAGING > + * > + * This bitfield is used to show which data bytes in a 32-bit word area valid. > + * Default value 0xf indicates that all bytes are valid. For 8-bit transfers, > + * any combination in this bitfield will mean valid data is present in the > + * corresponding bytes. In the 16-bit mode, a 16-bit half-word is valid only if > + * adjacent bits [1:0] or [3:2] or both are 1. A value of 0x0 will mean that > + * none of the bytes are valid and should not be used. For example, set the bit > + * field value to 0x7 if the display data is arranged in the 24-bit unpacked > + * format (A-R-G-B where A value does not have be transmitted). > + */ > +#define CTRL1_SET_BYTE_PACKAGING(x) REG_PUT((x), 19, 16) > +#define CTRL1_GET_BYTE_PACKAGING(x) REG_GET((x), 19, 16) > + > +#define CTRL1_CUR_FRAME_DONE_IRQ_EN BIT(13) > +#define CTRL1_CUR_FRAME_DONE_IRQ BIT(9) > + > +#define CTRL2_OUTSTANDING_REQS(x) REG_PUT((x), 23, 21) > +#define REQ_1 0 > +#define REQ_2 1 > +#define REQ_4 2 > +#define REQ_8 3 > +#define REQ_16 4 Can you prefix them with CTRL2_? I think it is more in line with other defines. -- Stefan > + > +#define TRANSFER_COUNT_SET_VCOUNT(x) REG_PUT((x), 31, 16) > +#define TRANSFER_COUNT_GET_VCOUNT(x) REG_GET((x), 31, 16) > +#define TRANSFER_COUNT_SET_HCOUNT(x) REG_PUT((x), 15, 0) > +#define TRANSFER_COUNT_GET_HCOUNT(x) REG_GET((x), 15, 0) > + > +#define VDCTRL0_ENABLE_PRESENT BIT(28) > +#define VDCTRL0_VSYNC_ACT_HIGH BIT(27) > +#define VDCTRL0_HSYNC_ACT_HIGH BIT(26) > +#define VDCTRL0_DOTCLK_ACT_FALLING BIT(25) > +#define VDCTRL0_ENABLE_ACT_HIGH BIT(24) > +#define VDCTRL0_VSYNC_PERIOD_UNIT BIT(21) > +#define VDCTRL0_VSYNC_PULSE_WIDTH_UNIT BIT(20) > +#define VDCTRL0_HALF_LINE BIT(19) > +#define VDCTRL0_HALF_LINE_MODE BIT(18) > +#define VDCTRL0_SET_VSYNC_PULSE_WIDTH(x) REG_PUT((x), 17, 0) > +#define VDCTRL0_GET_VSYNC_PULSE_WIDTH(x) REG_GET((x), 17, 0) > + > +#define VDCTRL2_SET_HSYNC_PERIOD(x) REG_PUT((x), 15, 0) > +#define VDCTRL2_GET_HSYNC_PERIOD(x) REG_GET((x), 15, 0) > + > +#define VDCTRL3_MUX_SYNC_SIGNALS BIT(29) > +#define VDCTRL3_VSYNC_ONLY BIT(28) > +#define SET_HOR_WAIT_CNT(x) REG_PUT((x), 27, 16) > +#define GET_HOR_WAIT_CNT(x) REG_GET((x), 27, 16) > +#define SET_VERT_WAIT_CNT(x) REG_PUT((x), 15, 0) > +#define GET_VERT_WAIT_CNT(x) REG_GET((x), 15, 0) > + > +#define VDCTRL4_SET_DOTCLK_DLY(x) REG_PUT((x), 31, 29) /* v4 only */ > +#define VDCTRL4_GET_DOTCLK_DLY(x) REG_GET((x), 31, 29) /* v4 only */ > +#define VDCTRL4_SYNC_SIGNALS_ON BIT(18) > +#define SET_DOTCLK_H_VALID_DATA_CNT(x) REG_PUT((x), 17, 0) > + > +#define DEBUG0_HSYNC BIT(26) > +#define DEBUG0_VSYNC BIT(25) > > #define MXSFB_MIN_XRES 120 > #define MXSFB_MIN_YRES 120 > @@ -116,7 +143,7 @@ > #define STMLCDIF_18BIT 2 /* pixel data bus to the display is of 18 bit width */ > #define STMLCDIF_24BIT 3 /* pixel data bus to the display is of 24 bit width */ > > -#define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT (1 << 6) > -#define MXSFB_SYNC_DOTCLK_FALLING_ACT (1 << 7) /* negative edge sampling */ > +#define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT BIT(6) > +#define MXSFB_SYNC_DOTCLK_FALLING_ACT BIT(7) /* negative edge sampling */ > > #endif /* __MXSFB_REGS_H__ */
On 2019-08-29 13:30, Robert Chiras wrote: > Currently, the MXSFB DRM driver only supports a panel. But, its output > display signal can also be redirected to another encoder, like a DSI > controller. In this case, that DSI controller may act like a drm_bridge. > In order support this use-case too, this patch adds support for drm_bridge > in mxsfb. > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > Tested-by: Guido Günther <agx@sigxcpu.org> Applied to the drm-misc-next branch. -- Stefan > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 17 +++++++++++--- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 46 +++++++++++++++++++++++++++++++++----- > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 4 +++- > drivers/gpu/drm/mxsfb/mxsfb_out.c | 26 +++++++++++---------- > 4 files changed, 72 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index 1242156..de09b93 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -95,8 +95,11 @@ static void mxsfb_set_bus_fmt(struct > mxsfb_drm_private *mxsfb) > > reg = readl(mxsfb->base + LCDC_CTRL); > > - if (mxsfb->connector.display_info.num_bus_formats) > - bus_format = mxsfb->connector.display_info.bus_formats[0]; > + if (mxsfb->connector->display_info.num_bus_formats) > + bus_format = mxsfb->connector->display_info.bus_formats[0]; > + > + DRM_DEV_DEBUG_DRIVER(drm->dev, "Using bus_format: 0x%08X\n", > + bus_format); > > reg &= ~CTRL_BUS_WIDTH_MASK; > switch (bus_format) { > @@ -204,8 +207,9 @@ static dma_addr_t mxsfb_get_fb_paddr(struct > mxsfb_drm_private *mxsfb) > > static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) > { > + struct drm_device *drm = mxsfb->pipe.crtc.dev; > struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode; > - const u32 bus_flags = mxsfb->connector.display_info.bus_flags; > + const u32 bus_flags = mxsfb->connector->display_info.bus_flags; > u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; > int err; > > @@ -229,6 +233,13 @@ static void mxsfb_crtc_mode_set_nofb(struct > mxsfb_drm_private *mxsfb) > > clk_set_rate(mxsfb->clk, m->crtc_clock * 1000); > > + DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock: %dkHz (actual: %dkHz)\n", > + m->crtc_clock, > + (int)(clk_get_rate(mxsfb->clk) / 1000)); > + DRM_DEV_DEBUG_DRIVER(drm->dev, "Connector bus_flags: 0x%08X\n", > + bus_flags); > + DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags); > + > writel(TRANSFER_COUNT_SET_VCOUNT(m->crtc_vdisplay) | > TRANSFER_COUNT_SET_HCOUNT(m->crtc_hdisplay), > mxsfb->base + mxsfb->devdata->transfer_count); > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index e850633..497cf44 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -101,9 +101,25 @@ static void mxsfb_pipe_enable(struct > drm_simple_display_pipe *pipe, > struct drm_crtc_state *crtc_state, > struct drm_plane_state *plane_state) > { > + struct drm_connector *connector; > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > struct drm_device *drm = pipe->plane.dev; > > + if (!mxsfb->connector) { > + list_for_each_entry(connector, > + &drm->mode_config.connector_list, > + head) > + if (connector->encoder == &mxsfb->pipe.encoder) { > + mxsfb->connector = connector; > + break; > + } > + } > + > + if (!mxsfb->connector) { > + dev_warn(drm->dev, "No connector attached, using default\n"); > + mxsfb->connector = &mxsfb->panel_connector; > + } > + > pm_runtime_get_sync(drm->dev); > drm_panel_prepare(mxsfb->panel); > mxsfb_crtc_enable(mxsfb); > @@ -129,6 +145,9 @@ static void mxsfb_pipe_disable(struct > drm_simple_display_pipe *pipe) > drm_crtc_send_vblank_event(crtc, event); > } > spin_unlock_irq(&drm->event_lock); > + > + if (mxsfb->connector != &mxsfb->panel_connector) > + mxsfb->connector = NULL; > } > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > @@ -226,16 +245,33 @@ static int mxsfb_load(struct drm_device *drm, > unsigned long flags) > > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > - &mxsfb->connector); > + mxsfb->connector); > if (ret < 0) { > dev_err(drm->dev, "Cannot setup simple display pipe\n"); > goto err_vblank; > } > > - ret = drm_panel_attach(mxsfb->panel, &mxsfb->connector); > - if (ret) { > - dev_err(drm->dev, "Cannot connect panel\n"); > - goto err_vblank; > + /* > + * Attach panel only if there is one. > + * If there is no panel attach, it must be a bridge. In this case, we > + * need a reference to its connector for a proper initialization. > + * We will do this check in pipe->enable(), since the connector won't > + * be attached to an encoder until then. > + */ > + > + if (mxsfb->panel) { > + ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); > + if (ret) { > + dev_err(drm->dev, "Cannot connect panel: %d\n", ret); > + goto err_vblank; > + } > + } else if (mxsfb->bridge) { > + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, > + mxsfb->bridge); > + if (ret) { > + dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > + goto err_vblank; > + } > } > > drm->mode_config.min_width = MXSFB_MIN_XRES; > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > index d975300..0b65b51 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > @@ -27,8 +27,10 @@ struct mxsfb_drm_private { > struct clk *clk_disp_axi; > > struct drm_simple_display_pipe pipe; > - struct drm_connector connector; > + struct drm_connector panel_connector; > + struct drm_connector *connector; > struct drm_panel *panel; > + struct drm_bridge *bridge; > }; > > int mxsfb_setup_crtc(struct drm_device *dev); > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c > b/drivers/gpu/drm/mxsfb/mxsfb_out.c > index be36f4d..4eb9474 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_out.c > @@ -21,7 +21,8 @@ > static struct mxsfb_drm_private * > drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) > { > - return container_of(connector, struct mxsfb_drm_private, connector); > + return container_of(connector, struct mxsfb_drm_private, > + panel_connector); > } > > static int mxsfb_panel_get_modes(struct drm_connector *connector) > @@ -76,22 +77,23 @@ static const struct drm_connector_funcs > mxsfb_panel_connector_funcs = { > int mxsfb_create_output(struct drm_device *drm) > { > struct mxsfb_drm_private *mxsfb = drm->dev_private; > - struct drm_panel *panel; > int ret; > > - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, NULL); > + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, > + &mxsfb->panel, &mxsfb->bridge); > if (ret) > return ret; > > - mxsfb->connector.dpms = DRM_MODE_DPMS_OFF; > - mxsfb->connector.polled = 0; > - drm_connector_helper_add(&mxsfb->connector, > - &mxsfb_panel_connector_helper_funcs); > - ret = drm_connector_init(drm, &mxsfb->connector, > - &mxsfb_panel_connector_funcs, > - DRM_MODE_CONNECTOR_Unknown); > - if (!ret) > - mxsfb->panel = panel; > + if (mxsfb->panel) { > + mxsfb->connector = &mxsfb->panel_connector; > + mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; > + mxsfb->connector->polled = 0; > + drm_connector_helper_add(mxsfb->connector, > + &mxsfb_panel_connector_helper_funcs); > + ret = drm_connector_init(drm, mxsfb->connector, > + &mxsfb_panel_connector_funcs, > + DRM_MODE_CONNECTOR_Unknown); > + } > > return ret; > }
On 2019-08-29 13:30, Robert Chiras wrote: > From: Guido Günther <agx@sigxcpu.org> > > The bridge might have special requirmentes on the input bus. This > is e.g. used by the imx-nwl bridge. > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > Reviewed-by: Stefan Agner <stefan@agner.ch> Applied to the drm-misc-next branch. I decided to apply those two since they are independent from the rest. You can drop them in the next spin of the rest. -- Stefan > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index de09b93..b69ace8 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -209,7 +209,7 @@ static void mxsfb_crtc_mode_set_nofb(struct > mxsfb_drm_private *mxsfb) > { > struct drm_device *drm = mxsfb->pipe.crtc.dev; > struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode; > - const u32 bus_flags = mxsfb->connector->display_info.bus_flags; > + u32 bus_flags = mxsfb->connector->display_info.bus_flags; > u32 vdctrl0, vsync_pulse_len, hsync_pulse_len; > int err; > > @@ -233,6 +233,9 @@ static void mxsfb_crtc_mode_set_nofb(struct > mxsfb_drm_private *mxsfb) > > clk_set_rate(mxsfb->clk, m->crtc_clock * 1000); > > + if (mxsfb->bridge && mxsfb->bridge->timings) > + bus_flags = mxsfb->bridge->timings->input_bus_flags; > + > DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock: %dkHz (actual: %dkHz)\n", > m->crtc_clock, > (int)(clk_get_rate(mxsfb->clk) / 1000));
Hi Robert, Thank you for the patch. On Thu, Aug 29, 2019 at 02:30:07PM +0300, Robert Chiras wrote: > Since version 4 of eLCDIF, there are some registers that can do > transformations on the input data, like re-arranging the pixel > components. By doing that, we can support more pixel formats. > This patch adds support for X/ABGR and RGBX/A. Although, the local alpha > is not supported by eLCDIF, the alpha pixel formats were added to the > supported pixel formats but it will be ignored. This was necessary since > there are systems (like Android) that requires such pixel formats. > > Also, add support for the following pixel formats: > 16 bpp: RG16 ,BG16, XR15, XB15, AR15, AB15 > > Set the bus format based on input from the user and panel > capabilities. > Save the bus format in crtc->mode.private_flags, so the bridge can > use it. > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > Tested-by: Guido Günther <agx@sigxcpu.org> > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 147 ++++++++++++++++++++++++++++++------- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 30 ++++++-- > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 3 +- > drivers/gpu/drm/mxsfb/mxsfb_regs.h | 15 ++++ > 4 files changed, 163 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index 5e44f57..1be29f5 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -43,14 +43,17 @@ static u32 set_hsync_pulse_width(struct mxsfb_drm_private *mxsfb, u32 val) > } > > /* Setup the MXSFB registers for decoding the pixels out of the framebuffer */ > -static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) > +static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb, bool update) > { > struct drm_crtc *crtc = &mxsfb->pipe.crtc; > struct drm_device *drm = crtc->dev; > const u32 format = crtc->primary->state->fb->format->format; > - u32 ctrl, ctrl1; > + u32 ctrl = 0, ctrl1 = 0; > + bool bgr_format = true; > + struct drm_format_name_buf format_name_buf; > > - ctrl = CTRL_BYPASS_COUNT | CTRL_MASTER; > + if (!update) > + ctrl = CTRL_BYPASS_COUNT | CTRL_MASTER; > > /* > * WARNING: The bus width, CTRL_SET_BUS_WIDTH(), is configured to > @@ -59,64 +62,158 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) > * to arbitrary value. This limitation should not pose an issue. > */ > > - /* CTRL1 contains IRQ config and status bits, preserve those. */ > - ctrl1 = readl(mxsfb->base + LCDC_CTRL1); > - ctrl1 &= CTRL1_CUR_FRAME_DONE_IRQ_EN | CTRL1_CUR_FRAME_DONE_IRQ; > + if (!update) { > + /* CTRL1 contains IRQ config and status bits, preserve those. */ > + ctrl1 = readl(mxsfb->base + LCDC_CTRL1); > + ctrl1 &= CTRL1_CUR_FRAME_DONE_IRQ_EN | CTRL1_CUR_FRAME_DONE_IRQ; > + } > + > + DRM_DEV_DEBUG_DRIVER(drm->dev, "Setting up %s mode\n", > + drm_get_format_name(format, &format_name_buf)); > + > + /* Do some clean-up that we might have from a previous mode */ > + ctrl &= ~CTRL_SHIFT_DIR(1); > + ctrl &= ~CTRL_SHIFT_NUM(0x3f); > + 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); > > switch (format) { > - case DRM_FORMAT_RGB565: > - dev_dbg(drm->dev, "Setting up RGB565 mode\n"); > + case DRM_FORMAT_BGR565: /* BG16 */ > + if (mxsfb->devdata->ipversion < 4) > + goto err; This function should not return an error. Invalid pixel formats should be caught at atomic check time. The easiest way to do so is to use two different lists of supported formats when constructing the plane, based on the IP version. By the way, the IP version register has been removed in i.MX6. i.MX6 and newer SoCs support a second plane, which I'm trying to get working. It would help to know what version number to use for the ipversion field for i.MX6, i.MX7 and i.MX8 in order to enable support for the second plane based on the SoC model. On a similar topic, would you happen to have tested the second plane (configured through AS_CTRL, AS_BUF and AS_NEXT_BUF) ? The plane "works" but seems to always use a 32bpp format regardless of how I program it, and the frame buffer start address seems to be shifted for some reason. > + 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; > + case DRM_FORMAT_XBGR1555: /* XB15 */ > + case DRM_FORMAT_ABGR1555: /* AB15 */ > + 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_XRGB1555: /* XR15 */ > + case DRM_FORMAT_ARGB1555: /* AR15 */ > ctrl |= CTRL_SET_WORD_LENGTH(0); > + ctrl |= CTRL_DF16; > ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); > break; > - case DRM_FORMAT_XRGB8888: > - dev_dbg(drm->dev, "Setting up XRGB8888 mode\n"); > + case DRM_FORMAT_RGBX8888: /* RX24 */ > + case DRM_FORMAT_RGBA8888: /* RA24 */ > + /* RGBX - > 0RGB */ > + ctrl |= CTRL_SHIFT_DIR(1); > + ctrl |= CTRL_SHIFT_NUM(8); > + bgr_format = false; > + /* Fall through */ > + case DRM_FORMAT_XBGR8888: /* XB24 */ > + case DRM_FORMAT_ABGR8888: /* AB24 */ > + if (bgr_format) { > + 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_XRGB8888: /* XR24 */ > + case DRM_FORMAT_ARGB8888: /* AR24 */ > ctrl |= CTRL_SET_WORD_LENGTH(3); > /* Do not use packed pixels = one pixel per word instead. */ > ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7); > break; > default: > - dev_err(drm->dev, "Unhandled pixel format %08x\n", format); > - return -EINVAL; > + goto err; > } > > - writel(ctrl1, mxsfb->base + LCDC_CTRL1); > - writel(ctrl, mxsfb->base + LCDC_CTRL); > + if (update) { > + writel(ctrl, mxsfb->base + LCDC_CTRL + REG_SET); > + writel(ctrl1, mxsfb->base + LCDC_CTRL1 + REG_SET); > + } else { > + writel(ctrl, mxsfb->base + LCDC_CTRL); > + writel(ctrl1, mxsfb->base + LCDC_CTRL1); > + } > > return 0; > + > +err: > + DRM_DEV_ERROR(drm->dev, "Unhandled pixel format: %s\n", > + drm_get_format_name(format, &format_name_buf)); > + > + return -EINVAL; > +} > + > +static u32 get_bus_format_from_bpp(u32 bpp) > +{ > + switch (bpp) { > + case 16: > + return MEDIA_BUS_FMT_RGB565_1X16; > + case 18: > + return MEDIA_BUS_FMT_RGB666_1X18; > + case 24: > + return MEDIA_BUS_FMT_RGB888_1X24; > + default: > + return MEDIA_BUS_FMT_RGB888_1X24; > + } > } > > static void mxsfb_set_bus_fmt(struct mxsfb_drm_private *mxsfb) > { > struct drm_crtc *crtc = &mxsfb->pipe.crtc; > + unsigned int bits_per_pixel = crtc->primary->state->fb->format->depth; > struct drm_device *drm = crtc->dev; > u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; > - u32 reg; > - > - reg = readl(mxsfb->base + LCDC_CTRL); > + int num_bus_formats = mxsfb->connector->display_info.num_bus_formats; > + const u32 *bus_formats = mxsfb->connector->display_info.bus_formats; > + u32 reg = 0; > + int i = 0; > + > + /* match the user requested bus_format to one supported by the panel */ > + if (num_bus_formats) { > + u32 user_bus_format = get_bus_format_from_bpp(bits_per_pixel); > + > + bus_format = bus_formats[0]; > + for (i = 0; i < num_bus_formats; i++) { > + if (user_bus_format == bus_formats[i]) { > + bus_format = user_bus_format; > + break; > + } > + } > + } > > - if (mxsfb->connector->display_info.num_bus_formats) > - bus_format = mxsfb->connector->display_info.bus_formats[0]; > + /* > + * CRTC will dictate the bus format via private_flags[16:1] > + * and private_flags[0] will signal a bus format change > + */ > + crtc->mode.private_flags &= ~0x1FFFF; /* clear bus format */ > + crtc->mode.private_flags |= (bus_format << 1); /* set bus format */ > + crtc->mode.private_flags |= 0x1; /* bus format change indication*/ > > DRM_DEV_DEBUG_DRIVER(drm->dev, "Using bus_format: 0x%08X\n", > bus_format); > > - reg &= ~CTRL_BUS_WIDTH_MASK; > switch (bus_format) { > case MEDIA_BUS_FMT_RGB565_1X16: > - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); > + reg = CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); > break; > case MEDIA_BUS_FMT_RGB666_1X18: > - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT); > + reg = CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT); > break; > case MEDIA_BUS_FMT_RGB888_1X24: > - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT); > + reg = CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT); > break; > default: > dev_err(drm->dev, "Unknown media bus format %d\n", bus_format); > break; > } > - writel(reg, mxsfb->base + LCDC_CTRL); > + writel(reg, mxsfb->base + LCDC_CTRL + REG_SET); > } > > static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) > @@ -238,7 +335,7 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) > /* Clear the FIFOs */ > writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET); > > - err = mxsfb_set_pixel_fmt(mxsfb); > + err = mxsfb_set_pixel_fmt(mxsfb, false); > if (err) > return; > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index 497cf44..23027a9 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -43,6 +43,27 @@ enum mxsfb_devtype { > MXSFB_V4, > }; > > +/* > + * When adding new formats, make sure to update the num_formats from > + * mxsfb_devdata below. > + */ > +static const u32 mxsfb_formats[] = { > + /* MXSFB_V3 */ > + DRM_FORMAT_XRGB8888, > + DRM_FORMAT_ARGB8888, > + DRM_FORMAT_RGB565, > + /* MXSFB_V4 */ > + DRM_FORMAT_XBGR8888, > + DRM_FORMAT_ABGR8888, > + DRM_FORMAT_RGBX8888, > + DRM_FORMAT_RGBA8888, > + DRM_FORMAT_ARGB1555, > + DRM_FORMAT_XRGB1555, > + DRM_FORMAT_ABGR1555, > + DRM_FORMAT_XBGR1555, > + DRM_FORMAT_BGR565 > +}; > + > static const struct mxsfb_devdata mxsfb_devdata[] = { > [MXSFB_V3] = { > .transfer_count = LCDC_V3_TRANSFER_COUNT, > @@ -52,6 +73,7 @@ static const struct mxsfb_devdata mxsfb_devdata[] = { > .hs_wdth_mask = 0xff, > .hs_wdth_shift = 24, > .ipversion = 3, > + .num_formats = 3, > }, > [MXSFB_V4] = { > .transfer_count = LCDC_V4_TRANSFER_COUNT, > @@ -61,14 +83,10 @@ static const struct mxsfb_devdata mxsfb_devdata[] = { > .hs_wdth_mask = 0x3fff, > .hs_wdth_shift = 18, > .ipversion = 4, > + .num_formats = ARRAY_SIZE(mxsfb_formats), > }, > }; > > -static const uint32_t mxsfb_formats[] = { > - DRM_FORMAT_XRGB8888, > - DRM_FORMAT_RGB565 > -}; > - > static struct mxsfb_drm_private * > drm_pipe_to_mxsfb_drm_private(struct drm_simple_display_pipe *pipe) > { > @@ -244,7 +262,7 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags) > } > > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > + mxsfb_formats, mxsfb->devdata->num_formats, NULL, > mxsfb->connector); > if (ret < 0) { > dev_err(drm->dev, "Cannot setup simple display pipe\n"); > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > index 0b65b51..8fb65d3 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > @@ -16,6 +16,7 @@ struct mxsfb_devdata { > unsigned int hs_wdth_mask; > unsigned int hs_wdth_shift; > unsigned int ipversion; > + unsigned int num_formats; > }; > > struct mxsfb_drm_private { > @@ -42,6 +43,6 @@ 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, > - struct drm_plane_state *state); > + struct drm_plane_state *old_state); > > #endif /* __MXSFB_DRV_H__ */ > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > index 9fcb1db..dc4daa0 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > @@ -44,6 +44,11 @@ > #define REG_PUT(x, h, l) (((x) << (l)) & GENMASK(h, l)) > #define REG_GET(x, h, l) (((x) & GENMASK(h, l)) >> (l)) > > +#define SWIZZLE_LE 0 /* Little-Endian or No swap */ > +#define SWIZZLE_BE 1 /* Big-Endian or swap all */ > +#define SWIZZLE_HWD 2 /* Swap half-words */ > +#define SWIZZLE_HWD_BYTE 3 /* Swap bytes within each half-word */ > + > #define CTRL_SFTRST BIT(31) > #define CTRL_CLKGATE BIT(30) > #define CTRL_SHIFT_DIR(x) REG_PUT((x), 26, 26) > @@ -93,6 +98,16 @@ > #define REQ_8 3 > #define REQ_16 4 > > +#define CTRL2_ODD_LINE_PATTERN(x) REG_PUT((x), 18, 16) > +#define CTRL2_EVEN_LINE_PATTERN(x) REG_PUT((x), 14, 12) > +#define CTRL2_LINE_PATTERN_RGB 0 > +#define CTRL2_LINE_PATTERN_RBG 1 > +#define CTRL2_LINE_PATTERN_GBR 2 > +#define CTRL2_LINE_PATTERN_GRB 3 > +#define CTRL2_LINE_PATTERN_BRG 4 > +#define CTRL2_LINE_PATTERN_BGR 5 > +#define CTRL2_LINE_PATTERN_CLR 7 > + > #define TRANSFER_COUNT_SET_VCOUNT(x) REG_PUT((x), 31, 16) > #define TRANSFER_COUNT_GET_VCOUNT(x) REG_GET((x), 31, 16) > #define TRANSFER_COUNT_SET_HCOUNT(x) REG_PUT((x), 15, 0)
On Mon, Oct 14, 2019 at 02:40:55PM +0200, Stefan Agner wrote: > Hi Robert, > > Sorry it took me so long to have a closer look at this patchset. > > I will definitely merge part of it, but this particular patch actually > breaks i.MX 7. I have vertical stripes on my display with this patch > applied (using Weston with DRM backend). Not sure why this exactly > happens, from what I understand this should only affect IP Version 4. The code tests for ipversion >= 4, which should match the i.MX7. > Some notes below: > > On 2019-08-29 13:30, Robert Chiras wrote: > > Besides the eLCDIF block, there is another IP block, used in the past > > for EPDC panels. Since the iMX.8mq doesn't have an EPDC connector, this > > block is not documented, but we can use it to do additional operations > > on the frame buffer. > > Hm, but this block is part of the ELCDIF block, in terms of clock, power > domain etc? On i.MX7 the EPDC IP core is present as the SoC has an EPDC output. Can the EPDC be used to control LCDIF stride on the i.MX7 too ? > > In this case, we can use the pigeon registers from this IP block in > > order to do horizontal crop on the frame buffer processed by the eLCDIF > > block. > > > > Signed-off-by: Robert Chiras <robert.chiras@nxp.com> > > Tested-by: Guido Günther <agx@sigxcpu.org> > > --- > > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 79 ++++++++++++++++++++++++++++++++++++-- > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 1 + > > drivers/gpu/drm/mxsfb/mxsfb_regs.h | 16 ++++++++ > > 3 files changed, 92 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > index a12f53d..317575e 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > > @@ -15,6 +15,7 @@ > > > > #include <video/videomode.h> > > > > +#include <drm/drm_atomic.h> > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_crtc.h> > > #include <drm/drm_fb_cma_helper.h> > > @@ -435,13 +436,66 @@ void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb) > > clk_disable_unprepare(mxsfb->clk_axi); > > } > > > > +void mxsfb_set_fb_hcrop(struct mxsfb_drm_private *mxsfb, u32 src_w, u32 fb_w) > > +{ > > + u32 mask_cnt, htotal, hcount; > > + u32 vdctrl2, vdctrl3, vdctrl4, transfer_count; > > + u32 pigeon_12_0, pigeon_12_1, pigeon_12_2; > > + > > + if (src_w == fb_w) { > > + writel(0x0, mxsfb->base + HW_EPDC_PIGEON_12_0); > > + writel(0x0, mxsfb->base + HW_EPDC_PIGEON_12_1); > > + > > + return; > > + } > > + > > + transfer_count = readl(mxsfb->base + LCDC_V4_TRANSFER_COUNT); > > + hcount = TRANSFER_COUNT_GET_HCOUNT(transfer_count); > > + > > + transfer_count &= ~TRANSFER_COUNT_SET_HCOUNT(0xffff); > > + transfer_count |= TRANSFER_COUNT_SET_HCOUNT(fb_w); > > + writel(transfer_count, mxsfb->base + LCDC_V4_TRANSFER_COUNT); > > + > > + vdctrl2 = readl(mxsfb->base + LCDC_VDCTRL2); > > + htotal = VDCTRL2_GET_HSYNC_PERIOD(vdctrl2); > > + htotal += fb_w - hcount; > > + vdctrl2 &= ~VDCTRL2_SET_HSYNC_PERIOD(0x3ffff); > > + vdctrl2 |= VDCTRL2_SET_HSYNC_PERIOD(htotal); > > + writel(vdctrl2, mxsfb->base + LCDC_VDCTRL2); > > + > > + vdctrl4 = readl(mxsfb->base + LCDC_VDCTRL4); > > + vdctrl4 &= ~SET_DOTCLK_H_VALID_DATA_CNT(0x3ffff); > > + vdctrl4 |= SET_DOTCLK_H_VALID_DATA_CNT(fb_w); > > + writel(vdctrl4, mxsfb->base + LCDC_VDCTRL4); > > + > > + /* configure related pigeon registers */ > > + vdctrl3 = readl(mxsfb->base + LCDC_VDCTRL3); > > + mask_cnt = GET_HOR_WAIT_CNT(vdctrl3) - 5; > > + > > + pigeon_12_0 = PIGEON_12_0_SET_STATE_MASK(0x24) | > > + PIGEON_12_0_SET_MASK_CNT(mask_cnt) | > > + PIGEON_12_0_SET_MASK_CNT_SEL(0x6) | > > + PIGEON_12_0_POL_ACTIVE_LOW | > > + PIGEON_12_0_EN; > > + writel(pigeon_12_0, mxsfb->base + HW_EPDC_PIGEON_12_0); > > + > > + pigeon_12_1 = PIGEON_12_1_SET_CLR_CNT(src_w) | > > + PIGEON_12_1_SET_SET_CNT(0x0); > > + writel(pigeon_12_1, mxsfb->base + HW_EPDC_PIGEON_12_1); > > + > > + pigeon_12_2 = 0x0; > > + writel(pigeon_12_2, mxsfb->base + HW_EPDC_PIGEON_12_2); > > +} > > + > > void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb, > > struct drm_plane_state *state) > > { > > struct drm_simple_display_pipe *pipe = &mxsfb->pipe; > > struct drm_crtc *crtc = &pipe->crtc; > > + struct drm_plane_state *new_state = pipe->plane.state; > > + struct drm_framebuffer *fb = pipe->plane.state->fb; > > struct drm_pending_vblank_event *event; > > - dma_addr_t paddr; > > + u32 fb_addr, src_off, src_w, stride, cpp = 0; > > dma_addr_t seems to be the better type here, why change? > > > > > spin_lock_irq(&crtc->dev->event_lock); > > event = crtc->state->event; > > @@ -456,10 +510,27 @@ void mxsfb_plane_atomic_update(struct > > mxsfb_drm_private *mxsfb, > > } > > spin_unlock_irq(&crtc->dev->event_lock); > > > > - paddr = mxsfb_get_fb_paddr(mxsfb); > > - if (paddr) { > > + if (!fb) > > + return; > > + > > + fb_addr = mxsfb_get_fb_paddr(mxsfb); > > + if (mxsfb->devdata->ipversion >= 4) { > > + cpp = fb->format->cpp[0]; > > + src_off = (new_state->src_y >> 16) * fb->pitches[0] + > > + (new_state->src_x >> 16) * cpp; > > + fb_addr += fb->offsets[0] + src_off; > > + } > > + > > + if (fb_addr) { > > clk_prepare_enable(mxsfb->clk_axi); > > - writel(paddr, mxsfb->base + mxsfb->devdata->next_buf); > > + writel(fb_addr, mxsfb->base + mxsfb->devdata->next_buf); > > clk_disable_unprepare(mxsfb->clk_axi); > > } > > + > > + if (mxsfb->devdata->ipversion >= 4 && > > + unlikely(drm_atomic_crtc_needs_modeset(new_state->crtc->state))) { > > + stride = DIV_ROUND_UP(fb->pitches[0], cpp); > > + src_w = new_state->src_w >> 16; > > + mxsfb_set_fb_hcrop(mxsfb, src_w, stride); > > + } I doubt userspace will appreciate the stride being ignored on older SoCs. > > } > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > index 888b520..06d3bf0 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > @@ -133,6 +133,7 @@ static int mxsfb_atomic_helper_check(struct drm_device *dev, > > if (old_bpp != new_bpp) > > new_state->mode_changed = true; > > } > > + > > Can you also drop this unrelated change? > > > return ret; > > } > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h > > b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > > index 0f63ba1..df3279b 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h > > @@ -145,6 +145,22 @@ > > #define DEBUG0_HSYNC BIT(26) > > #define DEBUG0_VSYNC BIT(25) > > > > +/* pigeon registers for crop */ > > +#define HW_EPDC_PIGEON_12_0 0xb00 > > +#define HW_EPDC_PIGEON_12_1 0xb10 > > +#define HW_EPDC_PIGEON_12_2 0xb20 > > + > > +#define PIGEON_12_0_SET_STATE_MASK(x) REG_PUT((x), 31, 24) > > +#define PIGEON_12_0_SET_MASK_CNT(x) REG_PUT((x), 23, 12) > > +#define PIGEON_12_0_SET_MASK_CNT_SEL(x) REG_PUT((x), 11, 8) > > +#define PIGEON_12_0_SET_OFFSET(x) REG_PUT((x), 7, 4) > > +#define PIGEON_12_0_SET_INC_SEL(x) REG_PUT((x), 3, 2) > > +#define PIGEON_12_0_POL_ACTIVE_LOW BIT(1) > > +#define PIGEON_12_0_EN BIT(0) > > + > > +#define PIGEON_12_1_SET_CLR_CNT(x) REG_PUT((x), 31, 16) > > +#define PIGEON_12_1_SET_SET_CNT(x) REG_PUT((x), 15, 0) > > + > > #define MXSFB_MIN_XRES 120 > > #define MXSFB_MIN_YRES 120 > > #define MXSFB_MAX_XRES 0xffff