* [PATCH v2] drm/mxsfb: use bus_format to determine LCD bus width
@ 2016-12-14 1:02 Stefan Agner
2016-12-14 7:52 ` Marek Vasut
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Agner @ 2016-12-14 1:02 UTC (permalink / raw)
To: marex; +Cc: airlied, dri-devel, linux-kernel, Stefan Agner
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>
---
Changes in v2:
- Use seperate function mxsfb_set_bus_fmt
drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 33 +++++++++++++++++++++++++++++++--
drivers/gpu/drm/mxsfb/mxsfb_regs.h | 1 +
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
index 4bcc8a3..956769b 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);
@@ -87,6 +85,35 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb)
return 0;
}
+static void mxsfb_set_bus_fmt(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;
+
+ writel(CTRL_BUS_WIDTH_MASK, mxsfb->base + LCDC_CTRL + REG_CLR);
+
+ if (mxsfb->connector.display_info.num_bus_formats)
+ bus_format = mxsfb->connector.display_info.bus_formats[0];
+
+ 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 + REG_SET);
+}
+
static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
{
u32 reg;
@@ -182,6 +209,8 @@ 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);
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] drm/mxsfb: use bus_format to determine LCD bus width
2016-12-14 1:02 [PATCH v2] drm/mxsfb: use bus_format to determine LCD bus width Stefan Agner
@ 2016-12-14 7:52 ` Marek Vasut
2016-12-14 20:51 ` Stefan Agner
0 siblings, 1 reply; 4+ messages in thread
From: Marek Vasut @ 2016-12-14 7:52 UTC (permalink / raw)
To: Stefan Agner; +Cc: airlied, dri-devel, linux-kernel
On 12/14/2016 02:02 AM, 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>
Minor nits below.
> ---
> Changes in v2:
> - Use seperate function mxsfb_set_bus_fmt
>
> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 33 +++++++++++++++++++++++++++++++--
> drivers/gpu/drm/mxsfb/mxsfb_regs.h | 1 +
> 2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index 4bcc8a3..956769b 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);
> @@ -87,6 +85,35 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb)
> return 0;
> }
>
> +static void mxsfb_set_bus_fmt(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;
^^^ this is uninitialized
> + writel(CTRL_BUS_WIDTH_MASK, mxsfb->base + LCDC_CTRL + REG_CLR);
> +
> + if (mxsfb->connector.display_info.num_bus_formats)
> + bus_format = mxsfb->connector.display_info.bus_formats[0];
> +
> + switch (bus_format) {
> + case MEDIA_BUS_FMT_RGB565_1X16:
> + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT);
^^^
here you're doing operation on uninited value.
> + 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 + REG_SET);
What I was thinking might be better is to save the calculated LCD_CRTC
value in mxsfb_drm_private , but maybe R-M-W is good enough after all?
Two writes are IMO not great because it unnecessarily reconfigures the
controller twice.
> +}
> +
> static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
> {
> u32 reg;
> @@ -182,6 +209,8 @@ 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);
>
> 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)
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] drm/mxsfb: use bus_format to determine LCD bus width
2016-12-14 7:52 ` Marek Vasut
@ 2016-12-14 20:51 ` Stefan Agner
2016-12-14 21:28 ` Marek Vasut
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Agner @ 2016-12-14 20:51 UTC (permalink / raw)
To: Marek Vasut; +Cc: airlied, dri-devel, linux-kernel
On 2016-12-13 23:52, Marek Vasut wrote:
> On 12/14/2016 02:02 AM, 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>
>
> Minor nits below.
>
>> ---
>> Changes in v2:
>> - Use seperate function mxsfb_set_bus_fmt
>>
>> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 33 +++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/mxsfb/mxsfb_regs.h | 1 +
>> 2 files changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
>> index 4bcc8a3..956769b 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);
>> @@ -87,6 +85,35 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb)
>> return 0;
>> }
>>
>> +static void mxsfb_set_bus_fmt(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;
> ^^^ this is uninitialized
>
>> + writel(CTRL_BUS_WIDTH_MASK, mxsfb->base + LCDC_CTRL + REG_CLR);
>> +
>> + if (mxsfb->connector.display_info.num_bus_formats)
>> + bus_format = mxsfb->connector.display_info.bus_formats[0];
>> +
>> + switch (bus_format) {
>> + case MEDIA_BUS_FMT_RGB565_1X16:
>> + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT);
> ^^^
> here you're doing operation on uninited value.
>
>> + 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 + REG_SET);
>
> What I was thinking might be better is to save the calculated LCD_CRTC
> value in mxsfb_drm_private , but maybe R-M-W is good enough after all?
Yeah I see what you are saying, however I would prefer to not cache if
there is no real reason to...
It is now a write to the clear and one write to the set variant of the
register, not sure if that counts as "two writes".
> Two writes are IMO not great because it unnecessarily reconfigures the
> controller twice.
Does that matter? At this point, the RUN bit is guaranteed not to be
set...
--
Stefan
>
>> +}
>> +
>> static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
>> {
>> u32 reg;
>> @@ -182,6 +209,8 @@ 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);
>>
>> 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)
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] drm/mxsfb: use bus_format to determine LCD bus width
2016-12-14 20:51 ` Stefan Agner
@ 2016-12-14 21:28 ` Marek Vasut
0 siblings, 0 replies; 4+ messages in thread
From: Marek Vasut @ 2016-12-14 21:28 UTC (permalink / raw)
To: Stefan Agner; +Cc: airlied, dri-devel, linux-kernel
On 12/14/2016 09:51 PM, Stefan Agner wrote:
> On 2016-12-13 23:52, Marek Vasut wrote:
>> On 12/14/2016 02:02 AM, 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>
>>
>> Minor nits below.
>>
>>> ---
>>> Changes in v2:
>>> - Use seperate function mxsfb_set_bus_fmt
>>>
>>> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 33 +++++++++++++++++++++++++++++++--
>>> drivers/gpu/drm/mxsfb/mxsfb_regs.h | 1 +
>>> 2 files changed, 32 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
>>> index 4bcc8a3..956769b 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);
>>> @@ -87,6 +85,35 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private *mxsfb)
>>> return 0;
>>> }
>>>
>>> +static void mxsfb_set_bus_fmt(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;
>> ^^^ this is uninitialized
>>
>>> + writel(CTRL_BUS_WIDTH_MASK, mxsfb->base + LCDC_CTRL + REG_CLR);
>>> +
>>> + if (mxsfb->connector.display_info.num_bus_formats)
>>> + bus_format = mxsfb->connector.display_info.bus_formats[0];
>>> +
>>> + switch (bus_format) {
>>> + case MEDIA_BUS_FMT_RGB565_1X16:
>>> + reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT);
>> ^^^
>> here you're doing operation on uninited value.
>>
>>> + 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 + REG_SET);
>>
>> What I was thinking might be better is to save the calculated LCD_CRTC
>> value in mxsfb_drm_private , but maybe R-M-W is good enough after all?
>
> Yeah I see what you are saying, however I would prefer to not cache if
> there is no real reason to...
OK
> It is now a write to the clear and one write to the set variant of the
> register, not sure if that counts as "two writes".
It's two writel() calls, it does.
>> Two writes are IMO not great because it unnecessarily reconfigures the
>> controller twice.
>
> Does that matter? At this point, the RUN bit is guaranteed not to be
> set...
It's probably a matter of taste, although I'd be more comfortable with
RMW here.
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-14 21:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-14 1:02 [PATCH v2] drm/mxsfb: use bus_format to determine LCD bus width Stefan Agner
2016-12-14 7:52 ` Marek Vasut
2016-12-14 20:51 ` Stefan Agner
2016-12-14 21:28 ` Marek Vasut
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).