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.
Or perhaps we could pass an optional parameter with the custom dirty
function to mipi_dbi_enable_flush() like this:
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
