Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1538940 > unrolled thread

[PATCH] drm/mxsfb: use bus_format to determine LCD bus width

Started byStefan Agner <stefan@agner.ch>
First post2016-12-09 00:00 +0100
Last post2016-12-09 05:30 +0100
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] drm/mxsfb: use bus_format to determine LCD bus width Stefan Agner <stefan@agner.ch> - 2016-12-09 00:00 +0100
    Re: [PATCH] drm/mxsfb: use bus_format to determine LCD bus width Marek Vasut <marex@denx.de> - 2016-12-09 02:00 +0100
      Re: [PATCH] drm/mxsfb: use bus_format to determine LCD bus width Stefan Agner <stefan@agner.ch> - 2016-12-09 05:00 +0100
        Re: [PATCH] drm/mxsfb: use bus_format to determine LCD bus width Marek Vasut <marex@denx.de> - 2016-12-09 05:30 +0100

#1538940 — [PATCH] drm/mxsfb: use bus_format to determine LCD bus width

FromStefan Agner <stefan@agner.ch>
Date2016-12-09 00:00 +0100
Subject[PATCH] drm/mxsfb: use bus_format to determine LCD bus width
Message-ID<sMfPr-3Bd-9@gated-at.bofh.it>
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)
 	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)
-- 
2.10.2

[toc] | [next] | [standalone]

#1538991

FromMarek Vasut <marex@denx.de>
Date2016-12-09 02:00 +0100
Message-ID<sMhHA-4Iu-1@gated-at.bofh.it>
In reply to#1538940
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>

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1539055

FromStefan Agner <stefan@agner.ch>
Date2016-12-09 05:00 +0100
Message-ID<sMkvQ-6CI-3@gated-at.bofh.it>
In reply to#1538991
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

[toc] | [prev] | [next] | [standalone]

#1539063

FromMarek Vasut <marex@denx.de>
Date2016-12-09 05:30 +0100
Message-ID<sMkYN-75X-11@gated-at.bofh.it>
In reply to#1539055
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 :)

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web