drm: Exynos: Respect framebuffer pitch for FIMD/Mixer

Commit Message

Daniel Stone March 17, 2015, 1:24 p.m. UTC

When performing a modeset, use the framebuffer pitch value to set FIMD
IMG_SIZE and Mixer SPAN registers. These are both defined as pitch - the
distance between contiguous lines (bytes for FIMD, pixels for mixer).

Fixes display on Snow (1366x768).

Signed-off-by: Daniel Stone <[e-mail]>
Tested-by: Javier Martinez Canillas <[e-mail]>
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c |  8 +++++---
 drivers/gpu/drm/exynos/exynos_mixer.c    | 17 ++++++++++-------
 2 files changed, 15 insertions(+), 10 deletions(-)

The VP parts are untested as I don't have Exynos4 hardware, and the
spec is uselessly vague. It does seem to look like the right fix however,
and in the case where the buffer is naturally aligned (pitch == width*bpp),
there should be no change, as bpp==8 for video buffers.

FIMD/GRP and Mixer behaviour have been manually verified on Snow (5250
with eDP/LVDS bridge and HDMI out).

Comments

Javier Martinez Canillas March 24, 2015, 8:57 a.m. UTC | #1

Hello Inki,

On Tue, Mar 17, 2015 at 2:24 PM, Daniel Stone <[e-mail]> wrote:
> When performing a modeset, use the framebuffer pitch value to set FIMD
> IMG_SIZE and Mixer SPAN registers. These are both defined as pitch - the
> distance between contiguous lines (bytes for FIMD, pixels for mixer).
>
> Fixes display on Snow (1366x768).
>
> Signed-off-by: Daniel Stone <[e-mail]>
> Tested-by: Javier Martinez Canillas <[e-mail]>

Any comments on this patch? It would be great to pick this sooner
rather than later since it fixes (at least) display output on Snow and
HDMI output on Peach Pit/Pi.

Best regards,
Javier

Inki Dae March 26, 2015, 2:32 p.m. UTC | #2

Hello Javier,

Applied.

Could you use right prefix of the subject like below when you post patch?

'drm/exynos: ...', not 'drm: Exynos: ...'

Your email will be filtered from my mailbox if you don't use the right
prefix so I couldn't check and take care of your patch.

Thanks,
Inki Dae

On 2015? 03? 24? 17:57, Javier Martinez Canillas wrote:
> Hello Inki,
> 
> On Tue, Mar 17, 2015 at 2:24 PM, Daniel Stone <[e-mail]> wrote:
>> When performing a modeset, use the framebuffer pitch value to set FIMD
>> IMG_SIZE and Mixer SPAN registers. These are both defined as pitch - the
>> distance between contiguous lines (bytes for FIMD, pixels for mixer).
>>
>> Fixes display on Snow (1366x768).
>>
>> Signed-off-by: Daniel Stone <[e-mail]>
>> Tested-by: Javier Martinez Canillas <[e-mail]>
> 
> Any comments on this patch? It would be great to pick this sooner
> rather than later since it fixes (at least) display output on Snow and
> HDMI output on Peach Pit/Pi.
> 
> Best regards,
> Javier
>

Daniel Stone March 26, 2015, 2:48 p.m. UTC | #3

Hi Inki,

On Thu, 26 Mar, 2015 at 2:32 PM, Inki Dae <[e-mail]> wrote:
> Applied.

Thanks very much.

> Could you use right prefix of the subject like below when you post 
> patch?
> 
> 'drm/exynos: ...', not 'drm: Exynos: ...'
> 
> Your email will be filtered from my mailbox if you don't use the right
> prefix so I couldn't check and take care of your patch.

Ah, I didn't realise this. Maybe it could be good to not filter if the 
patch is also directly addressed/CCed to you, rather than a list? 
Gustavo Padovan is following this convention and I also will in future, 
but I guess it might lead to some patches being dropped from casual 
contributors if they don't know this.

Cheers,
Daniel

> 
> Thanks,
> Inki Dae
> 
> On 2015? 03? 24? 17:57, Javier Martinez Canillas wrote:
>>  Hello Inki,
>> 
>>  On Tue, Mar 17, 2015 at 2:24 PM, Daniel Stone 
>> <[e-mail]> wrote:
>>>  When performing a modeset, use the framebuffer pitch value to set 
>>> FIMD
>>>  IMG_SIZE and Mixer SPAN registers. These are both defined as pitch 
>>> - the
>>>  distance between contiguous lines (bytes for FIMD, pixels for 
>>> mixer).
>>> 
>>>  Fixes display on Snow (1366x768).
>>> 
>>>  Signed-off-by: Daniel Stone <[e-mail]>
>>>  Tested-by: Javier Martinez Canillas 
>>> <[e-mail]>
>> 
>>  Any comments on this patch? It would be great to pick this sooner
>>  rather than later since it fixes (at least) display output on Snow 
>> and
>>  HDMI output on Peach Pit/Pi.
>> 
>>  Best regards,
>>  Javier
>> 
> 
>

Tobias Jakobi March 27, 2015, 12:15 a.m. UTC | #4

Inki Dae wrote:
> Hello Javier,
> 
> Applied.
> 
> Could you use right prefix of the subject like below when you post patch?
> 
> 'drm/exynos: ...', not 'drm: Exynos: ...'
> 
> Your email will be filtered from my mailbox if you don't use the right
> prefix so I couldn't check and take care of your patch.
I would suggest to change (or drop) the filter settings then. I don't
think you can expect people to jump through these hoops to reach a
maintainer of a kernel subsystem. Especially since this not 'documented'
anywhere.



With best wishes,
Tobias

Inki Dae March 27, 2015, 1:47 a.m. UTC | #5

On 2015? 03? 27? 09:15, Tobias Jakobi wrote:
> Inki Dae wrote:
>> Hello Javier,
>>
>> Applied.
>>
>> Could you use right prefix of the subject like below when you post patch?
>>
>> 'drm/exynos: ...', not 'drm: Exynos: ...'
>>
>> Your email will be filtered from my mailbox if you don't use the right
>> prefix so I couldn't check and take care of your patch.
> I would suggest to change (or drop) the filter settings then. I don't

Filters of my mailbox are considered for two cases, one is DRI mailing
list, other is drm/exynos.
So if you don't use the drm/exynos prefix, then I would try to find your
patch in DRI mailing list - I sometimes check the mailing list. However,
the mailing list have so much emails so it'd be not easy to pick up only
exynos relevant patches among the email lists. As a result, you would
get the my review lately.

> think you can expect people to jump through these hoops to reach a
> maintainer of a kernel subsystem. Especially since this not 'documented'
> anywhere.

Right, this is not documented but if you have ever checked exynos drm
driver tree, then I think you could know how we use the prefix. Of
course, I don't like to force the use of this prefix but if you and
other people use prefix in the manner of them, exynos drm tree would be
a little bit messy. i.e., DRM: EXYNOS, drm: exynos, drm: Exynos,
drm/exynos, drm/exynos: fimd, drm: exynos: fimd, DRM: EXYNOS: FIMD, ...
 so many cases....  Do you really want this?

So I will always say "please, use right prefix" Otherwise, I will change
it while merging as is.
And I see that other drm drivers have their own way which is not
documented but just implicitly.

Thanks,
Inki Dae

> 
> 
> 
> With best wishes,
> Tobias
> 
>

Inki Dae March 27, 2015, 1:55 a.m. UTC | #6

Hi Daniel,

On 2015? 03? 26? 23:48, Daniel Stone wrote:
> Hi Inki,
> 
> On Thu, 26 Mar, 2015 at 2:32 PM, Inki Dae <[e-mail]> wrote:
>> Applied.
> 
> Thanks very much.
> 
>> Could you use right prefix of the subject like below when you post patch?
>>
>> 'drm/exynos: ...', not 'drm: Exynos: ...'
>>
>> Your email will be filtered from my mailbox if you don't use the right
>> prefix so I couldn't check and take care of your patch.
> 
> Ah, I didn't realise this. Maybe it could be good to not filter if the
> patch is also directly addressed/CCed to you, rather than a list?
> Gustavo Padovan is following this convention and I also will in future,
> but I guess it might lead to some patches being dropped from casual
> contributors if they don't know this.

Sorry for confusing. I don't really want to drop the patches which don't
use right prefix - undocumented. I have two filters, one is DRI mailing
list, other is drm/exynos. So if you use other prefix, your patch is
filtered and stored in my dri-devel mailbox. But as you may know, the
mailbox would always be full with many emails which contains various
type patches, exynos, omap, sti, radeon, intel and so no. So it wouldn't
be easy to pick up only your patch among them. So I'd like to recommend
you to use drm/exynos prefix so that I could review and pick it up
easily and quickly.

Thanks,
Inki Dae

> 
> Cheers,
> Daniel
> 
>>
>> Thanks,
>> Inki Dae
>>
>> On 2015? 03? 24? 17:57, Javier Martinez Canillas wrote:
>>>  Hello Inki,
>>>
>>>  On Tue, Mar 17, 2015 at 2:24 PM, Daniel Stone
>>> <[e-mail]> wrote:
>>>>  When performing a modeset, use the framebuffer pitch value to set FIMD
>>>>  IMG_SIZE and Mixer SPAN registers. These are both defined as pitch
>>>> - the
>>>>  distance between contiguous lines (bytes for FIMD, pixels for mixer).
>>>>
>>>>  Fixes display on Snow (1366x768).
>>>>
>>>>  Signed-off-by: Daniel Stone <[e-mail]>
>>>>  Tested-by: Javier Martinez Canillas <[e-mail]>
>>>
>>>  Any comments on this patch? It would be great to pick this sooner
>>>  rather than later since it fixes (at least) display output on Snow and
>>>  HDMI output on Peach Pit/Pi.
>>>
>>>  Best regards,
>>>  Javier
>>>
>>
>>
>

Javier Martinez Canillas March 27, 2015, 11:08 a.m. UTC | #7

Hello Inki,

On Fri, Mar 27, 2015 at 2:47 AM, Inki Dae <[e-mail]> wrote:
>
> Right, this is not documented but if you have ever checked exynos drm
> driver tree, then I think you could know how we use the prefix. Of
> course, I don't like to force the use of this prefix but if you and
> other people use prefix in the manner of them, exynos drm tree would be
> a little bit messy. i.e., DRM: EXYNOS, drm: exynos, drm: Exynos,
> drm/exynos, drm/exynos: fimd, drm: exynos: fimd, DRM: EXYNOS: FIMD, ...
>  so many cases....  Do you really want this?
>
> So I will always say "please, use right prefix" Otherwise, I will change
> it while merging as is.
> And I see that other drm drivers have their own way which is not
> documented but just implicitly.
>

I agree with you that people should follow the convention for subject
lines used in a subsystem and that it is tedious to tell them to fix
the subject and resend but I do agree with Tobias that you should
rethink your mail filters.

For example I've a filter for all the emails that are directly
addressed to me or that I'm cc'ed to. It's true that you may get some
emails that are not interesting to you just because people cc'ed you
in random patches but I think is better to have false positives than
to have false negatives. Specially since you are a kernel maintainer
and people expect your feedback / ack for patches to get merged.

> Thanks,
> Inki Dae
>

Best regards,
Javier

Inki Dae March 27, 2015, 4:28 p.m. UTC | #8

Hello Javier,

2015-03-27 20:08 GMT+09:00 Javier Martinez Canillas <[e-mail]>:
> Hello Inki,
>
> On Fri, Mar 27, 2015 at 2:47 AM, Inki Dae <[e-mail]> wrote:
>>
>> Right, this is not documented but if you have ever checked exynos drm
>> driver tree, then I think you could know how we use the prefix. Of
>> course, I don't like to force the use of this prefix but if you and
>> other people use prefix in the manner of them, exynos drm tree would be
>> a little bit messy. i.e., DRM: EXYNOS, drm: exynos, drm: Exynos,
>> drm/exynos, drm/exynos: fimd, drm: exynos: fimd, DRM: EXYNOS: FIMD, ...
>>  so many cases....  Do you really want this?
>>
>> So I will always say "please, use right prefix" Otherwise, I will change
>> it while merging as is.
>> And I see that other drm drivers have their own way which is not
>> documented but just implicitly.
>>
>
> I agree with you that people should follow the convention for subject
> lines used in a subsystem and that it is tedious to tell them to fix
> the subject and resend but I do agree with Tobias that you should
> rethink your mail filters.
>
> For example I've a filter for all the emails that are directly
> addressed to me or that I'm cc'ed to. It's true that you may get some
> emails that are not interesting to you just because people cc'ed you
> in random patches but I think is better to have false positives than
> to have false negatives. Specially since you are a kernel maintainer
> and people expect your feedback / ack for patches to get merged.
>

Added a new filter, which will pick up all emails of CCing me. :)

Thanks,
Inki Dae

>> Thanks,
>> Inki Dae
>>
>
> Best regards,
> Javier
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 925fc69..0d5681f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -147,6 +147,7 @@  struct fimd_win_data {
 	unsigned int		ovl_height;
 	unsigned int		fb_width;
 	unsigned int		fb_height;
+	unsigned int		fb_pitch;
 	unsigned int		bpp;
 	unsigned int		pixel_format;
 	dma_addr_t		dma_addr;
@@ -537,13 +538,14 @@  static void fimd_win_mode_set(struct exynos_drm_crtc *crtc,
 	win_data->offset_y = plane->crtc_y;
 	win_data->ovl_width = plane->crtc_width;
 	win_data->ovl_height = plane->crtc_height;
+	win_data->fb_pitch = plane->pitch;
 	win_data->fb_width = plane->fb_width;
 	win_data->fb_height = plane->fb_height;
 	win_data->dma_addr = plane->dma_addr[0] + offset;
 	win_data->bpp = plane->bpp;
 	win_data->pixel_format = plane->pixel_format;
-	win_data->buf_offsize = (plane->fb_width - plane->crtc_width) *
-				(plane->bpp >> 3);
+	win_data->buf_offsize =
+		plane->pitch - (plane->crtc_width * (plane->bpp >> 3));
 	win_data->line_size = plane->crtc_width * (plane->bpp >> 3);
 
 	DRM_DEBUG_KMS("offset_x = %d, offset_y = %d\n",
@@ -709,7 +711,7 @@  static void fimd_win_commit(struct exynos_drm_crtc *crtc, int zpos)
 	writel(val, ctx->regs + VIDWx_BUF_START(win, 0));
 
 	/* buffer end address */
-	size = win_data->fb_width * win_data->ovl_height * (win_data->bpp >> 3);
+	size = win_data->fb_pitch * win_data->ovl_height * (win_data->bpp >> 3);
 	val = (unsigned long)(win_data->dma_addr + size);
 	writel(val, ctx->regs + VIDWx_BUF_END(win, 0));
 
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 3518bc4..2e3bc57 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -55,6 +55,7 @@  struct hdmi_win_data {
 	unsigned int		fb_x;
 	unsigned int		fb_y;
 	unsigned int		fb_width;
+	unsigned int		fb_pitch;
 	unsigned int		fb_height;
 	unsigned int		src_width;
 	unsigned int		src_height;
@@ -438,7 +439,7 @@  static void vp_video_buffer(struct mixer_context *ctx, int win)
 	} else {
 		luma_addr[0] = win_data->dma_addr;
 		chroma_addr[0] = win_data->dma_addr
-			+ (win_data->fb_width * win_data->fb_height);
+			+ (win_data->fb_pitch * win_data->fb_height);
 	}
 
 	if (win_data->scan_flags & DRM_MODE_FLAG_INTERLACE) {
@@ -447,8 +448,8 @@  static void vp_video_buffer(struct mixer_context *ctx, int win)
 			luma_addr[1] = luma_addr[0] + 0x40;
 			chroma_addr[1] = chroma_addr[0] + 0x40;
 		} else {
-			luma_addr[1] = luma_addr[0] + win_data->fb_width;
-			chroma_addr[1] = chroma_addr[0] + win_data->fb_width;
+			luma_addr[1] = luma_addr[0] + win_data->fb_pitch;
+			chroma_addr[1] = chroma_addr[0] + win_data->fb_pitch;
 		}
 	} else {
 		ctx->interlace = false;
@@ -469,10 +470,10 @@  static void vp_video_buffer(struct mixer_context *ctx, int win)
 	vp_reg_writemask(res, VP_MODE, val, VP_MODE_FMT_MASK);
 
 	/* setting size of input image */
-	vp_reg_write(res, VP_IMG_SIZE_Y, VP_IMG_HSIZE(win_data->fb_width) |
+	vp_reg_write(res, VP_IMG_SIZE_Y, VP_IMG_HSIZE(win_data->fb_pitch) |
 		VP_IMG_VSIZE(win_data->fb_height));
 	/* chroma height has to reduced by 2 to avoid chroma distorions */
-	vp_reg_write(res, VP_IMG_SIZE_C, VP_IMG_HSIZE(win_data->fb_width) |
+	vp_reg_write(res, VP_IMG_SIZE_C, VP_IMG_HSIZE(win_data->fb_pitch) |
 		VP_IMG_VSIZE(win_data->fb_height / 2));
 
 	vp_reg_write(res, VP_SRC_WIDTH, win_data->src_width);
@@ -559,7 +560,7 @@  static void mixer_graph_buffer(struct mixer_context *ctx, int win)
 	/* converting dma address base and source offset */
 	dma_addr = win_data->dma_addr
 		+ (win_data->fb_x * win_data->bpp >> 3)
-		+ (win_data->fb_y * win_data->fb_width * win_data->bpp >> 3);
+		+ (win_data->fb_y * win_data->fb_pitch);
 	src_x_offset = 0;
 	src_y_offset = 0;
 
@@ -576,7 +577,8 @@  static void mixer_graph_buffer(struct mixer_context *ctx, int win)
 		MXR_GRP_CFG_FORMAT_VAL(fmt), MXR_GRP_CFG_FORMAT_MASK);
 
 	/* setup geometry */
-	mixer_reg_write(res, MXR_GRAPHIC_SPAN(win), win_data->fb_width);
+	mixer_reg_write(res, MXR_GRAPHIC_SPAN(win),
+			win_data->fb_pitch / (win_data->bpp >> 3));
 
 	/* setup display size */
 	if (ctx->mxr_ver == MXR_VER_128_0_0_184 &&
@@ -961,6 +963,7 @@  static void mixer_win_mode_set(struct exynos_drm_crtc *crtc,
 	win_data->fb_y = plane->fb_y;
 	win_data->fb_width = plane->fb_width;
 	win_data->fb_height = plane->fb_height;
+	win_data->fb_pitch = plane->pitch;
 	win_data->src_width = plane->src_width;
 	win_data->src_height = plane->src_height;