Den 15.01.2019 00.26, skrev David Lechner:
> On 1/14/19 4:53 PM, Noralf Trønnes wrote:
>>
>>
>> Den 14.01.2019 23.33, skrev David Lechner:
>>> On 1/14/19 3:50 PM, David Lechner wrote:
>>>> On 1/14/19 10:13 AM, Noralf Trønnes wrote:
>>>>>
>>>>> I see that you have this call chain:
>>>>> st7586_pipe_enable() -> mipi_dbi_enable_flush() ->
>>>>> mipi_dbi_fb_dirty().
>>>>>
>>>>> That doesn't look safe. The st7586 driver allocates a tx_buf with
>>>>> size:
>>>>> size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
>>>>>
>>>>> whereas mipi_dbi_enable_flush() will trigger a copy to tx_buf with
>>>>> len:
>>>>> fb->width * fb->height * 2
>>>>>
>>>>> It looks like you're writing zeroes way past the end of the buffer.
>>>>>
>>>>> Noralf.
>>>>>
>>>>
>>>> Thanks! That does indeed seem to be the problem. I'll put together
>>>> a patch to fix this. I'm thinking it will be easier to make the
>>>> fix before applying this series so that it will be easier to
>>>> backport.
>>>>
>>>
>>> Well, now that I am looking into it more, I see that the problem
>>> was not preexisting. This patch ("drm/tinydrm: Use damage helper
>>> for dirtyfb") also changes mipi_dbi_enable_flush() from calling
>>> tdev->fb_dirty() to mipi_dbi_fb_dirty().
>>>
>>> Perhaps we should not be dropping tdev->fb_dirty()?
>>
>> I want to get rid of tinydrm_device, to avoid tinydrm being like a
>> mid-layer. My goal is to make tinydrm just a collection of tiny regular
>> DRM drivers.
>>
>
> OK. Perhaps we should add a comment to mipi_dbi_enable_flush() to
> say that it should not be used by drivers with a custom dirty function.
>
I've added a note.
> Or perhaps we could pass an optional parameter with the custom dirty
> function to mipi_dbi_enable_flush() like this:
>
I prefer to open code the function instead since it's only 2 lines of code.
Noralf.
> diff --git a/drivers/gpu/drm/tinydrm/hx8357d.c
> b/drivers/gpu/drm/tinydrm/hx8357d.c
> index 8bbd0beafc6a..4b9981ffe5a4 100644
> --- a/drivers/gpu/drm/tinydrm/hx8357d.c
> +++ b/drivers/gpu/drm/tinydrm/hx8357d.c
> @@ -170,7 +170,7 @@ static void yx240qv29_enable(struct
> drm_simple_display_pipe *pipe,
> break;
> }
> mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
> }
>
> static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c
> b/drivers/gpu/drm/tinydrm/ili9225.c
> index aa1376a22bda..bebfd0d01340 100644
> --- a/drivers/gpu/drm/tinydrm/ili9225.c
> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
> @@ -270,7 +270,7 @@ static void ili9225_pipe_enable(struct
> drm_simple_display_pipe *pipe,
>
> ili9225_command(mipi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
>
> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state,
> ili9225_fb_dirty);
> }
>
> static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
> diff --git a/drivers/gpu/drm/tinydrm/ili9341.c
> b/drivers/gpu/drm/tinydrm/ili9341.c
> index 713bb2dd7e04..ecfb8ff02a40 100644
> --- a/drivers/gpu/drm/tinydrm/ili9341.c
> +++ b/drivers/gpu/drm/tinydrm/ili9341.c
> @@ -126,7 +126,7 @@ static void yx240qv29_enable(struct
> drm_simple_display_pipe *pipe,
> }
> addr_mode |= ILI9341_MADCTL_BGR;
> mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
> }
>
> static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
> b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 82a92ec9ae3c..c81aa1fbc2ad 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -134,7 +134,7 @@ static void mi0283qt_enable(struct
> drm_simple_display_pipe *pipe,
> }
> addr_mode |= ILI9341_MADCTL_BGR;
> mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
> }
>
> static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index 680692a83f94..399c66b3f0d7 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -287,6 +287,7 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update);
> * @mipi: MIPI DBI structure
> * @crtc_state: CRTC state
> * @plane_state: Plane state
> + * @dirty: Optional custom dirty function
> *
> * This function sets &mipi_dbi->enabled, flushes the whole framebuffer
> and
> * enables the backlight. Drivers can use this in their
> @@ -294,7 +295,9 @@ EXPORT_SYMBOL(mipi_dbi_pipe_update);
> */
> void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
> struct drm_crtc_state *crtc_state,
> - struct drm_plane_state *plane_state)
> + struct drm_plane_state *plane_state,
> + void (*dirty)(struct drm_framebuffer *fb,
> + struct drm_rect *rect))
> {
> struct drm_framebuffer *fb = plane_state->fb;
> struct drm_rect rect = {
> @@ -305,7 +308,10 @@ void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
> };
>
> mipi->enabled = true;
> - mipi_dbi_fb_dirty(fb, &rect);
> + if (!dirty) {
> + dirty = mipi_dbi_fb_dirty;
> + }
> + dirty(fb, &rect);
> backlight_enable(mipi->backlight);
> }
> EXPORT_SYMBOL(mipi_dbi_enable_flush);
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c
> b/drivers/gpu/drm/tinydrm/st7586.c
> index 50e370ce5a59..d6ffac64822f 100644
> --- a/drivers/gpu/drm/tinydrm/st7586.c
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -234,7 +234,7 @@ static void st7586_pipe_enable(struct
> drm_simple_display_pipe *pipe,
>
> mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
>
> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state,
> st7586_fb_dirty);
> }
>
> static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
> diff --git a/drivers/gpu/drm/tinydrm/st7735r.c
> b/drivers/gpu/drm/tinydrm/st7735r.c
> index 3bab9a9569a6..dee143007048 100644
> --- a/drivers/gpu/drm/tinydrm/st7735r.c
> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
> @@ -100,7 +100,7 @@ static void jd_t18003_t01_pipe_enable(struct
> drm_simple_display_pipe *pipe,
>
> msleep(20);
>
> - mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state, NULL);
> }
>
> static const struct drm_simple_display_pipe_funcs
> jd_t18003_t01_pipe_funcs = {
> diff --git a/include/drm/tinydrm/mipi-dbi.h
> b/include/drm/tinydrm/mipi-dbi.h
> index f4ec2834bc22..3d9f003fdcc6 100644
> --- a/include/drm/tinydrm/mipi-dbi.h
> +++ b/include/drm/tinydrm/mipi-dbi.h
> @@ -72,7 +72,9 @@ void mipi_dbi_pipe_update(struct
> drm_simple_display_pipe *pipe,
> struct drm_plane_state *old_state);
> void mipi_dbi_enable_flush(struct mipi_dbi *mipi,
> struct drm_crtc_state *crtc_state,
> - struct drm_plane_state *plan_state);
> + struct drm_plane_state *plan_state,
> + void (*dirty)(struct drm_framebuffer *fb,
> + struct drm_rect *rect));
> void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
> void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
> bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
