Re: [PATCH v2 20/22] drm: mxsfb: Merge mxsfb_set_pixel_fmt() and mxsfb_set_bus_fmt()
i Laurent, On Sun, 7 Jun 2020 at 00:02, Laurent Pinchart wrote: > > Hi Emil, > > On Fri, Jun 05, 2020 at 03:58:53PM +0100, Emil Velikov wrote: > > Hi Laurent, > > > > With the previous disclaimer in mind, the series is: > > Reviewed-by: Emil Velikov > > Sorry, which disclaimer is that ? > "I don't know much about the hardware" from earlier. Even then, the refactor that is 1-21 is spot on. > > There's a small suggestion inline, for future work. > > > > On Sat, 30 May 2020 at 04:11, Laurent Pinchart wrote: > > > > > switch (bus_format) { > > > case MEDIA_BUS_FMT_RGB565_1X16: > > > - reg |= CTRL_BUS_WIDTH_16; > > > + ctrl |= CTRL_BUS_WIDTH_16; > > > break; > > > case MEDIA_BUS_FMT_RGB666_1X18: > > > - reg |= CTRL_BUS_WIDTH_18; > > > + ctrl |= CTRL_BUS_WIDTH_18; > > > break; > > > case MEDIA_BUS_FMT_RGB888_1X24: > > > - reg |= CTRL_BUS_WIDTH_24; > > > + ctrl |= CTRL_BUS_WIDTH_24; > > > break; > > > default: > > > dev_err(drm->dev, "Unknown media bus format %d\n", > > > bus_format); > > > break; > > > > Off the top of my head, the default case should be handled in the > > atomic_check() hook. > > That is, unless I'm missing something and one can change the bus > > format in between atomic_check() and atomic_enable(). Which would > > sound like a very odd thing to do. > > I agree, this should go to the atomic check. There are still quite a few > things to improve in this driver, one step at a time :-) > Agreed - simply mentioning for completeness-sake. Doing that at misc later point is perfectly fine. HTH Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 20/22] drm: mxsfb: Merge mxsfb_set_pixel_fmt() and mxsfb_set_bus_fmt()
Hi Emil, On Fri, Jun 05, 2020 at 03:58:53PM +0100, Emil Velikov wrote: > Hi Laurent, > > With the previous disclaimer in mind, the series is: > Reviewed-by: Emil Velikov Sorry, which disclaimer is that ? > There's a small suggestion inline, for future work. > > On Sat, 30 May 2020 at 04:11, Laurent Pinchart wrote: > > > switch (bus_format) { > > case MEDIA_BUS_FMT_RGB565_1X16: > > - reg |= CTRL_BUS_WIDTH_16; > > + ctrl |= CTRL_BUS_WIDTH_16; > > break; > > case MEDIA_BUS_FMT_RGB666_1X18: > > - reg |= CTRL_BUS_WIDTH_18; > > + ctrl |= CTRL_BUS_WIDTH_18; > > break; > > case MEDIA_BUS_FMT_RGB888_1X24: > > - reg |= CTRL_BUS_WIDTH_24; > > + ctrl |= CTRL_BUS_WIDTH_24; > > break; > > default: > > dev_err(drm->dev, "Unknown media bus format %d\n", > > bus_format); > > break; > > Off the top of my head, the default case should be handled in the > atomic_check() hook. > That is, unless I'm missing something and one can change the bus > format in between atomic_check() and atomic_enable(). Which would > sound like a very odd thing to do. I agree, this should go to the atomic check. There are still quite a few things to improve in this driver, one step at a time :-) -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 20/22] drm: mxsfb: Merge mxsfb_set_pixel_fmt() and mxsfb_set_bus_fmt()
Hi Laurent, With the previous disclaimer in mind, the series is: Reviewed-by: Emil Velikov There's a small suggestion inline, for future work. On Sat, 30 May 2020 at 04:11, Laurent Pinchart wrote: > switch (bus_format) { > case MEDIA_BUS_FMT_RGB565_1X16: > - reg |= CTRL_BUS_WIDTH_16; > + ctrl |= CTRL_BUS_WIDTH_16; > break; > case MEDIA_BUS_FMT_RGB666_1X18: > - reg |= CTRL_BUS_WIDTH_18; > + ctrl |= CTRL_BUS_WIDTH_18; > break; > case MEDIA_BUS_FMT_RGB888_1X24: > - reg |= CTRL_BUS_WIDTH_24; > + ctrl |= CTRL_BUS_WIDTH_24; > break; > default: > dev_err(drm->dev, "Unknown media bus format %d\n", > bus_format); > break; Off the top of my head, the default case should be handled in the atomic_check() hook. That is, unless I'm missing something and one can change the bus format in between atomic_check() and atomic_enable(). Which would sound like a very odd thing to do. -Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 20/22] drm: mxsfb: Merge mxsfb_set_pixel_fmt() and mxsfb_set_bus_fmt()
The mxsfb_set_pixel_fmt() and mxsfb_set_bus_fmt() functions both deal with format configuration, are always called in a row from mxsfb_crtc_mode_set_nofb(), and set fields from the LCDC_CTRL register. This requires a read-modify-update cycle in mxsfb_set_bus_fmt(). Make this more efficient by merging them together. Signed-off-by: Laurent Pinchart Reviewed-by: Stefan Agner --- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 47 +-- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 19b937b383cf..f81f8c222c13 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -42,13 +42,23 @@ static u32 set_hsync_pulse_width(struct mxsfb_drm_private *mxsfb, u32 val) mxsfb->devdata->hs_wdth_shift; } -/* Setup the MXSFB registers for decoding the pixels out of the framebuffer */ -static void mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) +/* + * Setup the MXSFB registers for decoding the pixels out of the framebuffer and + * outputting them on the bus. + */ +static void mxsfb_set_formats(struct mxsfb_drm_private *mxsfb) { struct drm_device *drm = mxsfb->drm; const u32 format = mxsfb->crtc.primary->state->fb->format->format; + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; u32 ctrl, ctrl1; + 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); + ctrl = CTRL_BYPASS_COUNT | CTRL_MASTER; /* CTRL1 contains IRQ config and status bits, preserve those. */ @@ -69,40 +79,23 @@ static void mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) break; } - writel(ctrl1, mxsfb->base + LCDC_CTRL1); - writel(ctrl, mxsfb->base + LCDC_CTRL); -} - -static void mxsfb_set_bus_fmt(struct mxsfb_drm_private *mxsfb) -{ - struct drm_device *drm = mxsfb->drm; - u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; - u32 reg; - - reg = readl(mxsfb->base + LCDC_CTRL); - - 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) { case MEDIA_BUS_FMT_RGB565_1X16: - reg |= CTRL_BUS_WIDTH_16; + ctrl |= CTRL_BUS_WIDTH_16; break; case MEDIA_BUS_FMT_RGB666_1X18: - reg |= CTRL_BUS_WIDTH_18; + ctrl |= CTRL_BUS_WIDTH_18; break; case MEDIA_BUS_FMT_RGB888_1X24: - reg |= CTRL_BUS_WIDTH_24; + ctrl |= CTRL_BUS_WIDTH_24; break; default: dev_err(drm->dev, "Unknown media bus format %d\n", bus_format); break; } - writel(reg, mxsfb->base + LCDC_CTRL); + + writel(ctrl1, mxsfb->base + LCDC_CTRL1); + writel(ctrl, mxsfb->base + LCDC_CTRL); } static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) @@ -213,7 +206,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); - mxsfb_set_pixel_fmt(mxsfb); + mxsfb_set_formats(mxsfb); clk_set_rate(mxsfb->clk, m->crtc_clock * 1000); @@ -255,8 +248,6 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0); - mxsfb_set_bus_fmt(mxsfb); - /* Frame length in lines. */ writel(m->crtc_vtotal, mxsfb->base + LCDC_VDCTRL1); -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel