| Message ID | 20161208225243.19792-1-stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/08/2016 11:52 PM, Stefan Agner wrote: > The LCD bus width does not need to align with the pixel format. The > LCDIF controller automatically converts between pixel formats and > bus width by padding or dropping LSBs. > > The DRM subsystem has the notion of bus_format which allows to > determine what bus_formats are supported by the display. Choose the > first available or fallback to 24 bit if none are available. > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 28 +++++++++++++++++++++++++--- > drivers/gpu/drm/mxsfb/mxsfb_regs.h | 1 + > 2 files changed, 26 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > index 4bcc8a3..00fa244 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c > @@ -65,13 +65,11 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) You should probably drop the WARNING: comment in this function too. > switch (format) { > case DRM_FORMAT_RGB565: > dev_dbg(drm->dev, "Setting up RGB565 mode\n"); > - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); > ctrl |= CTRL_SET_WORD_LENGTH(0); > ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); > break; > case DRM_FORMAT_XRGB8888: > dev_dbg(drm->dev, "Setting up XRGB8888 mode\n"); > - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT); > ctrl |= CTRL_SET_WORD_LENGTH(3); > /* Do not use packed pixels = one pixel per word instead. */ > ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7); > @@ -89,6 +87,9 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) > > static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) > { > + struct drm_crtc *crtc = &mxsfb->pipe.crtc; > + struct drm_device *drm = crtc->dev; > + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; So why do you move the bus width configuration from mxsfb_set_pixel_fmt() to here ? Is there any reason for it? > u32 reg; > > if (mxsfb->clk_disp_axi) > @@ -97,7 +98,28 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) > mxsfb_enable_axi_clk(mxsfb); > > /* If it was disabled, re-enable the mode again */ > - writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); > + reg = readl(mxsfb->base + LCDC_CTRL); Wouldn't it make more sense to cache the LCDC_CTRL content rather than doing R-M-W on it ? > + reg |= CTRL_DOTCLK_MODE; > + > + if (mxsfb->connector.display_info.num_bus_formats) > + bus_format = mxsfb->connector.display_info.bus_formats[0]; > + > + reg &= ~CTRL_BUS_WIDTH_MASK; > + switch (bus_format) { > + case MEDIA_BUS_FMT_RGB565_1X16: > + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); > + break; > + case MEDIA_BUS_FMT_RGB666_1X18: > + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT); > + break; > + case MEDIA_BUS_FMT_RGB888_1X24: > + 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); On MX6SX: Tested-by: Marek Vasut <marex@denx.de>
On 2016-12-08 15:33, Marek Vasut wrote: > On 12/08/2016 11:52 PM, Stefan Agner wrote: >> The LCD bus width does not need to align with the pixel format. The >> LCDIF controller automatically converts between pixel formats and >> bus width by padding or dropping LSBs. >> >> The DRM subsystem has the notion of bus_format which allows to >> determine what bus_formats are supported by the display. Choose the >> first available or fallback to 24 bit if none are available. >> >> Signed-off-by: Stefan Agner <stefan@agner.ch> >> --- >> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 28 +++++++++++++++++++++++++--- >> drivers/gpu/drm/mxsfb/mxsfb_regs.h | 1 + >> 2 files changed, 26 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >> index 4bcc8a3..00fa244 100644 >> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >> @@ -65,13 +65,11 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) > > You should probably drop the WARNING: comment in this function too. > >> switch (format) { >> case DRM_FORMAT_RGB565: >> dev_dbg(drm->dev, "Setting up RGB565 mode\n"); >> - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); >> ctrl |= CTRL_SET_WORD_LENGTH(0); >> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); >> break; >> case DRM_FORMAT_XRGB8888: >> dev_dbg(drm->dev, "Setting up XRGB8888 mode\n"); >> - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT); >> ctrl |= CTRL_SET_WORD_LENGTH(3); >> /* Do not use packed pixels = one pixel per word instead. */ >> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7); >> @@ -89,6 +87,9 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) >> >> static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) >> { >> + struct drm_crtc *crtc = &mxsfb->pipe.crtc; >> + struct drm_device *drm = crtc->dev; >> + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > So why do you move the bus width configuration from > mxsfb_set_pixel_fmt() to here ? Is there any reason > for it? > To emphasize that it is not related to pixel format. Also, if you have a controller with multiple framebuffers/layers, mxsfb_set_pixel_fmt would get called per layer... In a full DRM driver it probably would be part of a encoder function, I feel here it seems to map best to enable controller. It could probably also be in mxsfb_crtc_enable (or even mxsfb_crtc_mode_set_nofb I guess), but we do not touch LCDC_CTRL there... >> u32 reg; >> >> if (mxsfb->clk_disp_axi) >> @@ -97,7 +98,28 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) >> mxsfb_enable_axi_clk(mxsfb); >> >> /* If it was disabled, re-enable the mode again */ >> - writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); >> + reg = readl(mxsfb->base + LCDC_CTRL); > > Wouldn't it make more sense to cache the LCDC_CTRL content rather than > doing R-M-W on it ? > Not sure what you mean by cache? Isn't the variable reg counting as a cache? >> + reg |= CTRL_DOTCLK_MODE; >> + >> + if (mxsfb->connector.display_info.num_bus_formats) >> + bus_format = mxsfb->connector.display_info.bus_formats[0]; >> + >> + reg &= ~CTRL_BUS_WIDTH_MASK; >> + switch (bus_format) { >> + case MEDIA_BUS_FMT_RGB565_1X16: >> + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); >> + break; >> + case MEDIA_BUS_FMT_RGB666_1X18: >> + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT); >> + break; >> + case MEDIA_BUS_FMT_RGB888_1X24: >> + 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); > > On MX6SX: > Tested-by: Marek Vasut <marex@denx.de> Thx! -- Stefan
On 12/09/2016 04:44 AM, Stefan Agner wrote: > On 2016-12-08 15:33, Marek Vasut wrote: >> On 12/08/2016 11:52 PM, Stefan Agner wrote: >>> The LCD bus width does not need to align with the pixel format. The >>> LCDIF controller automatically converts between pixel formats and >>> bus width by padding or dropping LSBs. >>> >>> The DRM subsystem has the notion of bus_format which allows to >>> determine what bus_formats are supported by the display. Choose the >>> first available or fallback to 24 bit if none are available. >>> >>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>> --- >>> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 28 +++++++++++++++++++++++++--- >>> drivers/gpu/drm/mxsfb/mxsfb_regs.h | 1 + >>> 2 files changed, 26 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>> index 4bcc8a3..00fa244 100644 >>> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>> @@ -65,13 +65,11 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) >> >> You should probably drop the WARNING: comment in this function too. >> >>> switch (format) { >>> case DRM_FORMAT_RGB565: >>> dev_dbg(drm->dev, "Setting up RGB565 mode\n"); >>> - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); >>> ctrl |= CTRL_SET_WORD_LENGTH(0); >>> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); >>> break; >>> case DRM_FORMAT_XRGB8888: >>> dev_dbg(drm->dev, "Setting up XRGB8888 mode\n"); >>> - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT); >>> ctrl |= CTRL_SET_WORD_LENGTH(3); >>> /* Do not use packed pixels = one pixel per word instead. */ >>> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7); >>> @@ -89,6 +87,9 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) >>> >>> static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) >>> { >>> + struct drm_crtc *crtc = &mxsfb->pipe.crtc; >>> + struct drm_device *drm = crtc->dev; >>> + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; >> >> So why do you move the bus width configuration from >> mxsfb_set_pixel_fmt() to here ? Is there any reason >> for it? >> > > To emphasize that it is not related to pixel format. Ah, can we then create some function, something like mxsfb_set_bus_fmt() to be explicit about this bus (the wires coming out of the CPU) and pixel (memory layout of the framebuffer) format difference ? I think that'd help with readability. > Also, if you have a > controller with multiple framebuffers/layers, mxsfb_set_pixel_fmt would > get called per layer... Which in this case, you don't and cannot have, right ? > In a full DRM driver it probably would be part of a encoder function, I > feel here it seems to map best to enable controller. It could probably > also be in mxsfb_crtc_enable (or even mxsfb_crtc_mode_set_nofb I guess), > but we do not touch LCDC_CTRL there... My feeling is it should be part of the mode_set_nofb, because we're setting all the controller parameters there, so that should be all kept in one place, which for a simple controller like this might be the approach to take. >>> u32 reg; >>> >>> if (mxsfb->clk_disp_axi) >>> @@ -97,7 +98,28 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) >>> mxsfb_enable_axi_clk(mxsfb); >>> >>> /* If it was disabled, re-enable the mode again */ >>> - writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); >>> + reg = readl(mxsfb->base + LCDC_CTRL); >> >> Wouldn't it make more sense to cache the LCDC_CTRL content rather than >> doing R-M-W on it ? >> > > Not sure what you mean by cache? Isn't the variable reg counting as a > cache? I mean caching the content of the register in struct mxsfb_drm_private, so you always program content which you have under control into the register. Heck, maybe I should've used regmap for this driver ... >>> + reg |= CTRL_DOTCLK_MODE; >>> + >>> + if (mxsfb->connector.display_info.num_bus_formats) >>> + bus_format = mxsfb->connector.display_info.bus_formats[0]; >>> + >>> + reg &= ~CTRL_BUS_WIDTH_MASK; >>> + switch (bus_format) { >>> + case MEDIA_BUS_FMT_RGB565_1X16: >>> + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); >>> + break; >>> + case MEDIA_BUS_FMT_RGB666_1X18: >>> + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT); >>> + break; >>> + case MEDIA_BUS_FMT_RGB888_1X24: >>> + 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); >> >> On MX6SX: >> Tested-by: Marek Vasut <marex@denx.de> > > Thx! NP, thanks for the fixed and keeping up with my ranting :)
On 2016-12-08 20:24, Marek Vasut wrote: > On 12/09/2016 04:44 AM, Stefan Agner wrote: >> On 2016-12-08 15:33, Marek Vasut wrote: >>> On 12/08/2016 11:52 PM, Stefan Agner wrote: >>>> The LCD bus width does not need to align with the pixel format. The >>>> LCDIF controller automatically converts between pixel formats and >>>> bus width by padding or dropping LSBs. >>>> >>>> The DRM subsystem has the notion of bus_format which allows to >>>> determine what bus_formats are supported by the display. Choose the >>>> first available or fallback to 24 bit if none are available. >>>> >>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>> --- >>>> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 28 +++++++++++++++++++++++++--- >>>> drivers/gpu/drm/mxsfb/mxsfb_regs.h | 1 + >>>> 2 files changed, 26 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>>> index 4bcc8a3..00fa244 100644 >>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>>> @@ -65,13 +65,11 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) >>> >>> You should probably drop the WARNING: comment in this function too. >>> >>>> switch (format) { >>>> case DRM_FORMAT_RGB565: >>>> dev_dbg(drm->dev, "Setting up RGB565 mode\n"); >>>> - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); >>>> ctrl |= CTRL_SET_WORD_LENGTH(0); >>>> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); >>>> break; >>>> case DRM_FORMAT_XRGB8888: >>>> dev_dbg(drm->dev, "Setting up XRGB8888 mode\n"); >>>> - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT); >>>> ctrl |= CTRL_SET_WORD_LENGTH(3); >>>> /* Do not use packed pixels = one pixel per word instead. */ >>>> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7); >>>> @@ -89,6 +87,9 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) >>>> >>>> static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) >>>> { >>>> + struct drm_crtc *crtc = &mxsfb->pipe.crtc; >>>> + struct drm_device *drm = crtc->dev; >>>> + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; >>> >>> So why do you move the bus width configuration from >>> mxsfb_set_pixel_fmt() to here ? Is there any reason >>> for it? >>> >> >> To emphasize that it is not related to pixel format. > > Ah, can we then create some function, something like mxsfb_set_bus_fmt() > to be explicit about this bus (the wires coming out of the CPU) and > pixel (memory layout of the framebuffer) format difference ? I think > that'd help with readability. Sure we can, we just have to read the value of the register once more... > >> Also, if you have a >> controller with multiple framebuffers/layers, mxsfb_set_pixel_fmt would >> get called per layer... > > Which in this case, you don't and cannot have, right ? > I think the controller has basically support for one additional surface. They call it LCDIFx_AS (Alpha Surface). But those register have a slightly different layout, so I guess we can't really reuse mxsfb_set_pixel_fmt... >> In a full DRM driver it probably would be part of a encoder function, I >> feel here it seems to map best to enable controller. It could probably >> also be in mxsfb_crtc_enable (or even mxsfb_crtc_mode_set_nofb I guess), >> but we do not touch LCDC_CTRL there... > > My feeling is it should be part of the mode_set_nofb, because we're > setting all the controller parameters there, so that should be all > kept in one place, which for a simple controller like this might be > the approach to take. > It kind of feels wrong in any CRTC callback, since it is a property of an encoder. But since we only have one CRTC and one encoder it really does not matter. My point was that we don't touch that register in mode_set_nofb. But when we anyway move the code in a separate function anyway, than we might as well call it from mxsfb_crtc_mode_set_nofb. >>>> u32 reg; >>>> >>>> if (mxsfb->clk_disp_axi) >>>> @@ -97,7 +98,28 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) >>>> mxsfb_enable_axi_clk(mxsfb); >>>> >>>> /* If it was disabled, re-enable the mode again */ >>>> - writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); >>>> + reg = readl(mxsfb->base + LCDC_CTRL); >>> >>> Wouldn't it make more sense to cache the LCDC_CTRL content rather than >>> doing R-M-W on it ? >>> >> >> Not sure what you mean by cache? Isn't the variable reg counting as a >> cache? > > I mean caching the content of the register in struct mxsfb_drm_private, > so you always program content which you have under control into the > register. Heck, maybe I should've used regmap for this driver ... > The FSL DCU driver (used for Vybrid/LS1021a) which I currently maintain uses regmap. But it doesn't feel quite right either, the DRM subsystem actually holds current state already, just on a higher level. I first tried to use it for suspend and resume (before the DRM suspend resume helper were available), but it was messy, stuff got async between DRM/actual controller settings or have been written twice (unnecessarily). Quite often you actually have to touch a register from one subsystem only too (e.g. in DCU, the layer registers maps nicely with the DRM plane support). I guess this one register is a bit a catch all configuration register... Thinking about it, IMHO reading/writing that one register multiple times is really not a big deal... -- Stefan >>>> + reg |= CTRL_DOTCLK_MODE; >>>> + >>>> + if (mxsfb->connector.display_info.num_bus_formats) >>>> + bus_format = mxsfb->connector.display_info.bus_formats[0]; >>>> + >>>> + reg &= ~CTRL_BUS_WIDTH_MASK; >>>> + switch (bus_format) { >>>> + case MEDIA_BUS_FMT_RGB565_1X16: >>>> + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); >>>> + break; >>>> + case MEDIA_BUS_FMT_RGB666_1X18: >>>> + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT); >>>> + break; >>>> + case MEDIA_BUS_FMT_RGB888_1X24: >>>> + 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); >>> >>> On MX6SX: >>> Tested-by: Marek Vasut <marex@denx.de> >> >> Thx! > > NP, thanks for the fixed and keeping up with my ranting :)
On 12/14/2016 12:56 AM, Stefan Agner wrote: > On 2016-12-08 20:24, Marek Vasut wrote: >> On 12/09/2016 04:44 AM, Stefan Agner wrote: >>> On 2016-12-08 15:33, Marek Vasut wrote: >>>> On 12/08/2016 11:52 PM, Stefan Agner wrote: >>>>> The LCD bus width does not need to align with the pixel format. The >>>>> LCDIF controller automatically converts between pixel formats and >>>>> bus width by padding or dropping LSBs. >>>>> >>>>> The DRM subsystem has the notion of bus_format which allows to >>>>> determine what bus_formats are supported by the display. Choose the >>>>> first available or fallback to 24 bit if none are available. >>>>> >>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>> --- >>>>> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 28 +++++++++++++++++++++++++--- >>>>> drivers/gpu/drm/mxsfb/mxsfb_regs.h | 1 + >>>>> 2 files changed, 26 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>>>> index 4bcc8a3..00fa244 100644 >>>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>>>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c >>>>> @@ -65,13 +65,11 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) >>>> >>>> You should probably drop the WARNING: comment in this function too. >>>> >>>>> switch (format) { >>>>> case DRM_FORMAT_RGB565: >>>>> dev_dbg(drm->dev, "Setting up RGB565 mode\n"); >>>>> - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); >>>>> ctrl |= CTRL_SET_WORD_LENGTH(0); >>>>> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); >>>>> break; >>>>> case DRM_FORMAT_XRGB8888: >>>>> dev_dbg(drm->dev, "Setting up XRGB8888 mode\n"); >>>>> - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT); >>>>> ctrl |= CTRL_SET_WORD_LENGTH(3); >>>>> /* Do not use packed pixels = one pixel per word instead. */ >>>>> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7); >>>>> @@ -89,6 +87,9 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) >>>>> >>>>> static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) >>>>> { >>>>> + struct drm_crtc *crtc = &mxsfb->pipe.crtc; >>>>> + struct drm_device *drm = crtc->dev; >>>>> + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; >>>> >>>> So why do you move the bus width configuration from >>>> mxsfb_set_pixel_fmt() to here ? Is there any reason >>>> for it? >>>> >>> >>> To emphasize that it is not related to pixel format. >> >> Ah, can we then create some function, something like mxsfb_set_bus_fmt() >> to be explicit about this bus (the wires coming out of the CPU) and >> pixel (memory layout of the framebuffer) format difference ? I think >> that'd help with readability. > > Sure we can, we just have to read the value of the register once more... > >> >>> Also, if you have a >>> controller with multiple framebuffers/layers, mxsfb_set_pixel_fmt would >>> get called per layer... >> >> Which in this case, you don't and cannot have, right ? >> > > I think the controller has basically support for one additional surface. > They call it LCDIFx_AS (Alpha Surface). But those register have a > slightly different layout, so I guess we can't really reuse > mxsfb_set_pixel_fmt... Oh, this is new since MX6 :) All right, good point. >>> In a full DRM driver it probably would be part of a encoder function, I >>> feel here it seems to map best to enable controller. It could probably >>> also be in mxsfb_crtc_enable (or even mxsfb_crtc_mode_set_nofb I guess), >>> but we do not touch LCDC_CTRL there... >> >> My feeling is it should be part of the mode_set_nofb, because we're >> setting all the controller parameters there, so that should be all >> kept in one place, which for a simple controller like this might be >> the approach to take. >> > > It kind of feels wrong in any CRTC callback, since it is a property of > an encoder. But since we only have one CRTC and one encoder it really > does not matter. > > My point was that we don't touch that register in mode_set_nofb. But > when we anyway move the code in a separate function anyway, than we > might as well call it from mxsfb_crtc_mode_set_nofb. Given the above information, I kinda get your concern now. Either way works for me then. >>>>> u32 reg; >>>>> >>>>> if (mxsfb->clk_disp_axi) >>>>> @@ -97,7 +98,28 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) >>>>> mxsfb_enable_axi_clk(mxsfb); >>>>> >>>>> /* If it was disabled, re-enable the mode again */ >>>>> - writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); >>>>> + reg = readl(mxsfb->base + LCDC_CTRL); >>>> >>>> Wouldn't it make more sense to cache the LCDC_CTRL content rather than >>>> doing R-M-W on it ? >>>> >>> >>> Not sure what you mean by cache? Isn't the variable reg counting as a >>> cache? >> >> I mean caching the content of the register in struct mxsfb_drm_private, >> so you always program content which you have under control into the >> register. Heck, maybe I should've used regmap for this driver ... >> > > The FSL DCU driver (used for Vybrid/LS1021a) which I currently maintain > uses regmap. But it doesn't feel quite right either, the DRM subsystem > actually holds current state already, just on a higher level. I first > tried to use it for suspend and resume (before the DRM suspend resume > helper were available), but it was messy, stuff got async between > DRM/actual controller settings or have been written twice > (unnecessarily). > > > Quite often you actually have to touch a register from one subsystem > only too (e.g. in DCU, the layer registers maps nicely with the DRM > plane support). I guess this one register is a bit a catch all > configuration register... Thinking about it, IMHO reading/writing that > one register multiple times is really not a big deal... Well, then we should probably go for RMW afterall.
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c index 4bcc8a3..00fa244 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c @@ -65,13 +65,11 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) switch (format) { case DRM_FORMAT_RGB565: dev_dbg(drm->dev, "Setting up RGB565 mode\n"); - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); ctrl |= CTRL_SET_WORD_LENGTH(0); ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf); break; case DRM_FORMAT_XRGB8888: dev_dbg(drm->dev, "Setting up XRGB8888 mode\n"); - ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT); ctrl |= CTRL_SET_WORD_LENGTH(3); /* Do not use packed pixels = one pixel per word instead. */ ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7); @@ -89,6 +87,9 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb) static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) { + struct drm_crtc *crtc = &mxsfb->pipe.crtc; + struct drm_device *drm = crtc->dev; + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; u32 reg; if (mxsfb->clk_disp_axi) @@ -97,7 +98,28 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb) mxsfb_enable_axi_clk(mxsfb); /* If it was disabled, re-enable the mode again */ - writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET); + reg = readl(mxsfb->base + LCDC_CTRL); + reg |= CTRL_DOTCLK_MODE; + + if (mxsfb->connector.display_info.num_bus_formats) + bus_format = mxsfb->connector.display_info.bus_formats[0]; + + reg &= ~CTRL_BUS_WIDTH_MASK; + switch (bus_format) { + case MEDIA_BUS_FMT_RGB565_1X16: + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT); + break; + case MEDIA_BUS_FMT_RGB666_1X18: + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT); + break; + case MEDIA_BUS_FMT_RGB888_1X24: + 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); /* Enable the SYNC signals first, then the DMA engine */ reg = readl(mxsfb->base + LCDC_VDCTRL4); diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h b/drivers/gpu/drm/mxsfb/mxsfb_regs.h index 31d62cd..66a6ba9 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h @@ -44,6 +44,7 @@ #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)
The LCD bus width does not need to align with the pixel format. The LCDIF controller automatically converts between pixel formats and bus width by padding or dropping LSBs. The DRM subsystem has the notion of bus_format which allows to determine what bus_formats are supported by the display. Choose the first available or fallback to 24 bit if none are available. Signed-off-by: Stefan Agner <stefan@agner.ch> --- drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 28 +++++++++++++++++++++++++--- drivers/gpu/drm/mxsfb/mxsfb_regs.h | 1 + 2 files changed, 26 insertions(+), 3 deletions(-)