Den 15.01.2019 05.38, skrev Noralf Trønnes:
>
>
> 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.
'2 lines of code', that sounds like a misleading commercial :-)
It's more like 9 including the declarions. But anyways I prefer it.
>
> 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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
