| Message ID | 83143711-3a30-612e-7f7d-e02e19e1c040@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 23, 2018 at 02:37:23PM +0100, Noralf Trønnes wrote: > > Den 23.03.2018 12.31, skrev Ville Syrjälä: > > On Fri, Mar 23, 2018 at 12:43:58AM +0100, Noralf Trønnes wrote: > >> > >> Den 22.03.2018 21.27, skrev Ville Syrjala: > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> > >>> mipi_dbi_enable_flush() wants to call the fb->dirty() hook from the > >>> bowels of the .atomic_enable() hook. That prevents us from taking the > >>> plane mutex in fb->dirty() unless we also plumb down the acquire > >>> context. > >>> > >>> Instead it seems simpler to split the fb->dirty() into a tinydrm > >>> specific lower level hook that can be called from > >>> mipi_dbi_enable_flush() and from a generic higher level > >>> tinydrm_fb_dirty() helper. As we don't have a tinydrm specific > >>> vfuncs table we'll just stick it into tinydrm_device directly > >>> for now. > >> The long term goal is to try and get rid of tinydrm.ko by moving stuff > >> elsewhere as it's kind of a middle layer. So I'd really like to avoid > >> adding a callback like this. > >> Hopefully we can work out a solution based on my suggestion in the > >> 'drm: Eliminate plane->fb/crtc usage for atomic drivers' thread. > > I don't want to start redesigning the entire architecture at > > this point. That would also cause a bigger risk of regression and > > then we'd potentially have to try and revert the entire plane->fb > > thing, which would not be fun if any significant changes have > > occurred in the meantime. > > > >> If we do need a hook, I prefer that we add it to > >> drm_simple_display_pipe_funcs. > > If you have plans to redesign tinydrm anyway it seems to me that > > a temporary hook in tinydrm is may be a bit less intrusive than > > inflicting it on the simple_kms_helper. > > Yes you're right of course, what was I thinking. > > You missed out on one call site: > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > index 11ae950b0fc9..7924eb59c2e1 100644 > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > @@ -124,11 +124,8 @@ void tinydrm_display_pipe_update(struct > drm_simple_display_pipe *pipe, > struct drm_framebuffer *fb = pipe->plane.state->fb; > struct drm_crtc *crtc = &tdev->pipe.crtc; > > - if (fb && (fb != old_state->fb)) { > - pipe->plane.fb = fb; > - if (fb->funcs->dirty) > - fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0); > - } > + if (fb && (fb != old_state->fb)) > + tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0); > > if (crtc->state->event) { > spin_lock_irq(&crtc->dev->event_lock); > > With that fixed, series is: > > Reviewed-by: Noralf Trønnes <noralf@tronnes.org> Awesome. Thanks. And thanks for catching that extra dirty() call. I'll go and fix it up. > > To be honest I don't understand why it's necessary to wire through the > plane state to the enable hook instead of just looking it up on the pipe > object. You look it up on the pipe in tinydrm_fb_dirty(), and I look up > the new plane state and crtc state on the pipe in the update hook. The use of plane->state etc. isn't really the best practice. What we really should be doing is plumbing down the drm_atomic_state instead, which would allow us to look up the correct new state explicitly. That said, I guess there is something in the helpers to prevent the next swap_state() from overtaking the previous commit, so supposedly using plane->state etc. does actually result in the correct thing currently. In this case since we were already plumbing down the new crtc state I figured I'd just plumb down its friend as well. > > Noralf. > > > > But if you do prefer > > drm_simple_display_pipe_funcs I can move it there of course. > > > >> Noralf. > >> > >>> Cc: "Noralf Trønnes" <noralf@tronnes.org> > >>> Cc: David Lechner <david@lechnology.com> > >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> --- > >>> drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 30 ++++++++++++++++++++++++++ > >>> drivers/gpu/drm/tinydrm/ili9225.c | 23 ++++++-------------- > >>> drivers/gpu/drm/tinydrm/mi0283qt.c | 2 +- > >>> drivers/gpu/drm/tinydrm/mipi-dbi.c | 30 ++++++++++---------------- > >>> drivers/gpu/drm/tinydrm/repaper.c | 28 ++++++++---------------- > >>> drivers/gpu/drm/tinydrm/st7586.c | 23 ++++++-------------- > >>> drivers/gpu/drm/tinydrm/st7735r.c | 2 +- > >>> include/drm/tinydrm/mipi-dbi.h | 4 +++- > >>> include/drm/tinydrm/tinydrm-helpers.h | 5 +++++ > >>> include/drm/tinydrm/tinydrm.h | 4 ++++ > >>> 10 files changed, 76 insertions(+), 75 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > >>> index d1c3ce9ab294..dcd390163a4a 100644 > >>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > >>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c > >>> @@ -78,6 +78,36 @@ bool tinydrm_merge_clips(struct drm_clip_rect *dst, > >>> } > >>> EXPORT_SYMBOL(tinydrm_merge_clips); > >>> > >>> +int tinydrm_fb_dirty(struct drm_framebuffer *fb, > >>> + struct drm_file *file_priv, > >>> + unsigned int flags, unsigned int color, > >>> + struct drm_clip_rect *clips, > >>> + unsigned int num_clips) > >>> +{ > >>> + struct tinydrm_device *tdev = fb->dev->dev_private; > >>> + struct drm_plane *plane = &tdev->pipe.plane; > >>> + int ret = 0; > >>> + > >>> + drm_modeset_lock(&plane->mutex, NULL); > >>> + > >>> + /* fbdev can flush even when we're not interested */ > >>> + if (plane->state->fb == fb) { > >>> + mutex_lock(&tdev->dirty_lock); > >>> + ret = tdev->fb_dirty(fb, file_priv, flags, > >>> + color, clips, num_clips); > >>> + mutex_unlock(&tdev->dirty_lock); > >>> + } > >>> + > >>> + drm_modeset_unlock(&plane->mutex); > >>> + > >>> + if (ret) > >>> + dev_err_once(fb->dev->dev, > >>> + "Failed to update display %d\n", ret); > >>> + > >>> + return ret; > >>> +} > >>> +EXPORT_SYMBOL(tinydrm_fb_dirty); > >>> + > >>> /** > >>> * tinydrm_memcpy - Copy clip buffer > >>> * @dst: Destination buffer > >>> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c > >>> index 089d22798c8b..0874e877b111 100644 > >>> --- a/drivers/gpu/drm/tinydrm/ili9225.c > >>> +++ b/drivers/gpu/drm/tinydrm/ili9225.c > >>> @@ -88,14 +88,8 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb, > >>> bool full; > >>> void *tr; > >>> > >>> - mutex_lock(&tdev->dirty_lock); > >>> - > >>> if (!mipi->enabled) > >>> - goto out_unlock; > >>> - > >>> - /* fbdev can flush even when we're not interested */ > >>> - if (tdev->pipe.plane.fb != fb) > >>> - goto out_unlock; > >>> + return 0; > >>> > >>> full = tinydrm_merge_clips(&clip, clips, num_clips, flags, > >>> fb->width, fb->height); > >>> @@ -108,7 +102,7 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb, > >>> tr = mipi->tx_buf; > >>> ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip, swap); > >>> if (ret) > >>> - goto out_unlock; > >>> + return ret; > >>> } else { > >>> tr = cma_obj->vaddr; > >>> } > >>> @@ -159,20 +153,13 @@ static int ili9225_fb_dirty(struct drm_framebuffer *fb, > >>> ret = mipi_dbi_command_buf(mipi, ILI9225_WRITE_DATA_TO_GRAM, tr, > >>> (clip.x2 - clip.x1) * (clip.y2 - clip.y1) * 2); > >>> > >>> -out_unlock: > >>> - mutex_unlock(&tdev->dirty_lock); > >>> - > >>> - if (ret) > >>> - dev_err_once(fb->dev->dev, "Failed to update display %d\n", > >>> - ret); > >>> - > >>> return ret; > >>> } > >>> > >>> static const struct drm_framebuffer_funcs ili9225_fb_funcs = { > >>> .destroy = drm_gem_fb_destroy, > >>> .create_handle = drm_gem_fb_create_handle, > >>> - .dirty = ili9225_fb_dirty, > >>> + .dirty = tinydrm_fb_dirty, > >>> }; > >>> > >>> static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe, > >>> @@ -269,7 +256,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); > >>> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > >>> } > >>> > >>> static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe) > >>> @@ -342,6 +329,8 @@ static int ili9225_init(struct device *dev, struct mipi_dbi *mipi, > >>> if (ret) > >>> return ret; > >>> > >>> + tdev->fb_dirty = ili9225_fb_dirty; > >>> + > >>> ret = tinydrm_display_pipe_init(tdev, pipe_funcs, > >>> DRM_MODE_CONNECTOR_VIRTUAL, > >>> ili9225_formats, > >>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c > >>> index 82ad9b61898e..4e6d2ee94e55 100644 > >>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c > >>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c > >>> @@ -127,7 +127,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe, > >>> msleep(100); > >>> > >>> out_enable: > >>> - mipi_dbi_enable_flush(mipi); > >>> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > >>> } > >>> > >>> 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 9e903812b573..4d1fb31a781f 100644 > >>> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c > >>> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c > >>> @@ -219,14 +219,8 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb, > >>> bool full; > >>> void *tr; > >>> > >>> - mutex_lock(&tdev->dirty_lock); > >>> - > >>> if (!mipi->enabled) > >>> - goto out_unlock; > >>> - > >>> - /* fbdev can flush even when we're not interested */ > >>> - if (tdev->pipe.plane.fb != fb) > >>> - goto out_unlock; > >>> + return 0; > >>> > >>> full = tinydrm_merge_clips(&clip, clips, num_clips, flags, > >>> fb->width, fb->height); > >>> @@ -239,7 +233,7 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb, > >>> tr = mipi->tx_buf; > >>> ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip, swap); > >>> if (ret) > >>> - goto out_unlock; > >>> + return ret; > >>> } else { > >>> tr = cma_obj->vaddr; > >>> } > >>> @@ -254,20 +248,13 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb, > >>> ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START, tr, > >>> (clip.x2 - clip.x1) * (clip.y2 - clip.y1) * 2); > >>> > >>> -out_unlock: > >>> - mutex_unlock(&tdev->dirty_lock); > >>> - > >>> - if (ret) > >>> - dev_err_once(fb->dev->dev, "Failed to update display %d\n", > >>> - ret); > >>> - > >>> return ret; > >>> } > >>> > >>> static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = { > >>> .destroy = drm_gem_fb_destroy, > >>> .create_handle = drm_gem_fb_create_handle, > >>> - .dirty = mipi_dbi_fb_dirty, > >>> + .dirty = tinydrm_fb_dirty, > >>> }; > >>> > >>> /** > >>> @@ -278,13 +265,16 @@ static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = { > >>> * enables the backlight. Drivers can use this in their > >>> * &drm_simple_display_pipe_funcs->enable callback. > >>> */ > >>> -void mipi_dbi_enable_flush(struct mipi_dbi *mipi) > >>> +void mipi_dbi_enable_flush(struct mipi_dbi *mipi, > >>> + struct drm_crtc_state *crtc_state, > >>> + struct drm_plane_state *plane_state) > >>> { > >>> - struct drm_framebuffer *fb = mipi->tinydrm.pipe.plane.fb; > >>> + struct tinydrm_device *tdev = &mipi->tinydrm; > >>> + struct drm_framebuffer *fb = plane_state->fb; > >>> > >>> mipi->enabled = true; > >>> if (fb) > >>> - fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0); > >>> + tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0); > >>> > >>> backlight_enable(mipi->backlight); > >>> } > >>> @@ -381,6 +371,8 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, > >>> if (ret) > >>> return ret; > >>> > >>> + tdev->fb_dirty = mipi_dbi_fb_dirty; > >>> + > >>> /* TODO: Maybe add DRM_MODE_CONNECTOR_SPI */ > >>> ret = tinydrm_display_pipe_init(tdev, pipe_funcs, > >>> DRM_MODE_CONNECTOR_VIRTUAL, > >>> diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c > >>> index 33b4a71916e4..bb6f80a81899 100644 > >>> --- a/drivers/gpu/drm/tinydrm/repaper.c > >>> +++ b/drivers/gpu/drm/tinydrm/repaper.c > >>> @@ -540,14 +540,8 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb, > >>> clip.y1 = 0; > >>> clip.y2 = fb->height; > >>> > >>> - mutex_lock(&tdev->dirty_lock); > >>> - > >>> if (!epd->enabled) > >>> - goto out_unlock; > >>> - > >>> - /* fbdev can flush even when we're not interested */ > >>> - if (tdev->pipe.plane.fb != fb) > >>> - goto out_unlock; > >>> + return 0; > >>> > >>> repaper_get_temperature(epd); > >>> > >>> @@ -555,16 +549,14 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb, > >>> epd->factored_stage_time); > >>> > >>> buf = kmalloc(fb->width * fb->height, GFP_KERNEL); > >>> - if (!buf) { > >>> - ret = -ENOMEM; > >>> - goto out_unlock; > >>> - } > >>> + if (!buf) > >>> + return -ENOMEM; > >>> > >>> if (import_attach) { > >>> ret = dma_buf_begin_cpu_access(import_attach->dmabuf, > >>> DMA_FROM_DEVICE); > >>> if (ret) > >>> - goto out_unlock; > >>> + goto out_free; > >>> } > >>> > >>> tinydrm_xrgb8888_to_gray8(buf, cma_obj->vaddr, fb, &clip); > >>> @@ -573,7 +565,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb, > >>> ret = dma_buf_end_cpu_access(import_attach->dmabuf, > >>> DMA_FROM_DEVICE); > >>> if (ret) > >>> - goto out_unlock; > >>> + goto out_free; > >>> } > >>> > >>> repaper_gray8_to_mono_reversed(buf, fb->width, fb->height); > >>> @@ -625,11 +617,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb, > >>> } > >>> } > >>> > >>> -out_unlock: > >>> - mutex_unlock(&tdev->dirty_lock); > >>> - > >>> - if (ret) > >>> - DRM_DEV_ERROR(fb->dev->dev, "Failed to update display (%d)\n", ret); > >>> +out_free: > >>> kfree(buf); > >>> > >>> return ret; > >>> @@ -638,7 +626,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb, > >>> static const struct drm_framebuffer_funcs repaper_fb_funcs = { > >>> .destroy = drm_gem_fb_destroy, > >>> .create_handle = drm_gem_fb_create_handle, > >>> - .dirty = repaper_fb_dirty, > >>> + .dirty = tinydrm_fb_dirty, > >>> }; > >>> > >>> static void power_off(struct repaper_epd *epd) > >>> @@ -1070,6 +1058,8 @@ static int repaper_probe(struct spi_device *spi) > >>> if (ret) > >>> return ret; > >>> > >>> + tdev->fb_dirty = repaper_fb_dirty; > >>> + > >>> ret = tinydrm_display_pipe_init(tdev, &repaper_pipe_funcs, > >>> DRM_MODE_CONNECTOR_VIRTUAL, > >>> repaper_formats, > >>> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c > >>> index bb08b293c8ce..22644b88199a 100644 > >>> --- a/drivers/gpu/drm/tinydrm/st7586.c > >>> +++ b/drivers/gpu/drm/tinydrm/st7586.c > >>> @@ -120,14 +120,8 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb, > >>> int start, end; > >>> int ret = 0; > >>> > >>> - mutex_lock(&tdev->dirty_lock); > >>> - > >>> if (!mipi->enabled) > >>> - goto out_unlock; > >>> - > >>> - /* fbdev can flush even when we're not interested */ > >>> - if (tdev->pipe.plane.fb != fb) > >>> - goto out_unlock; > >>> + return 0; > >>> > >>> tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width, > >>> fb->height); > >>> @@ -141,7 +135,7 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb, > >>> > >>> ret = st7586_buf_copy(mipi->tx_buf, fb, &clip); > >>> if (ret) > >>> - goto out_unlock; > >>> + return ret; > >>> > >>> /* Pixels are packed 3 per byte */ > >>> start = clip.x1 / 3; > >>> @@ -158,20 +152,13 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb, > >>> (u8 *)mipi->tx_buf, > >>> (end - start) * (clip.y2 - clip.y1)); > >>> > >>> -out_unlock: > >>> - mutex_unlock(&tdev->dirty_lock); > >>> - > >>> - if (ret) > >>> - dev_err_once(fb->dev->dev, "Failed to update display %d\n", > >>> - ret); > >>> - > >>> return ret; > >>> } > >>> > >>> static const struct drm_framebuffer_funcs st7586_fb_funcs = { > >>> .destroy = drm_gem_fb_destroy, > >>> .create_handle = drm_gem_fb_create_handle, > >>> - .dirty = st7586_fb_dirty, > >>> + .dirty = tinydrm_fb_dirty, > >>> }; > >>> > >>> static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe, > >>> @@ -238,7 +225,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); > >>> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > >>> } > >>> > >>> static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe) > >>> @@ -278,6 +265,8 @@ static int st7586_init(struct device *dev, struct mipi_dbi *mipi, > >>> if (ret) > >>> return ret; > >>> > >>> + tdev->fb_dirty = st7586_fb_dirty; > >>> + > >>> ret = tinydrm_display_pipe_init(tdev, pipe_funcs, > >>> DRM_MODE_CONNECTOR_VIRTUAL, > >>> st7586_formats, > >>> diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c > >>> index 19b28f8c78db..189a07894d36 100644 > >>> --- a/drivers/gpu/drm/tinydrm/st7735r.c > >>> +++ b/drivers/gpu/drm/tinydrm/st7735r.c > >>> @@ -99,7 +99,7 @@ static void jd_t18003_t01_pipe_enable(struct drm_simple_display_pipe *pipe, > >>> > >>> msleep(20); > >>> > >>> - mipi_dbi_enable_flush(mipi); > >>> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state); > >>> } > >>> > >>> 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 44e824af2ef6..b8ba58861986 100644 > >>> --- a/include/drm/tinydrm/mipi-dbi.h > >>> +++ b/include/drm/tinydrm/mipi-dbi.h > >>> @@ -67,7 +67,9 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, > >>> const struct drm_simple_display_pipe_funcs *pipe_funcs, > >>> struct drm_driver *driver, > >>> const struct drm_display_mode *mode, unsigned int rotation); > >>> -void mipi_dbi_enable_flush(struct mipi_dbi *mipi); > >>> +void mipi_dbi_enable_flush(struct mipi_dbi *mipi, > >>> + struct drm_crtc_state *crtc_state, > >>> + struct drm_plane_state *plan_state); > >>> 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); > >>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h > >>> index 0a4ddbc04c60..5b96f0b12c8c 100644 > >>> --- a/include/drm/tinydrm/tinydrm-helpers.h > >>> +++ b/include/drm/tinydrm/tinydrm-helpers.h > >>> @@ -36,6 +36,11 @@ static inline bool tinydrm_machine_little_endian(void) > >>> bool tinydrm_merge_clips(struct drm_clip_rect *dst, > >>> struct drm_clip_rect *src, unsigned int num_clips, > >>> unsigned int flags, u32 max_width, u32 max_height); > >>> +int tinydrm_fb_dirty(struct drm_framebuffer *fb, > >>> + struct drm_file *file_priv, > >>> + unsigned int flags, unsigned int color, > >>> + struct drm_clip_rect *clips, > >>> + unsigned int num_clips); > >>> void tinydrm_memcpy(void *dst, void *vaddr, struct drm_framebuffer *fb, > >>> struct drm_clip_rect *clip); > >>> void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb, > >>> diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h > >>> index 07a9a11fe19d..ea2c71081190 100644 > >>> --- a/include/drm/tinydrm/tinydrm.h > >>> +++ b/include/drm/tinydrm/tinydrm.h > >>> @@ -26,6 +26,10 @@ struct tinydrm_device { > >>> struct drm_simple_display_pipe pipe; > >>> struct mutex dirty_lock; > >>> const struct drm_framebuffer_funcs *fb_funcs; > >>> + int (*fb_dirty)(struct drm_framebuffer *framebuffer, > >>> + struct drm_file *file_priv, unsigned flags, > >>> + unsigned color, struct drm_clip_rect *clips, > >>> + unsigned num_clips); > >>> }; > >>> > >>> static inline struct tinydrm_device *
On Fri, Mar 23, 2018 at 03:58:06PM +0200, Ville Syrjälä wrote: > On Fri, Mar 23, 2018 at 02:37:23PM +0100, Noralf Trønnes wrote: > > > > Den 23.03.2018 12.31, skrev Ville Syrjälä: > > > On Fri, Mar 23, 2018 at 12:43:58AM +0100, Noralf Trønnes wrote: > > >> > > >> Den 22.03.2018 21.27, skrev Ville Syrjala: > > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >>> > > >>> mipi_dbi_enable_flush() wants to call the fb->dirty() hook from the > > >>> bowels of the .atomic_enable() hook. That prevents us from taking the > > >>> plane mutex in fb->dirty() unless we also plumb down the acquire > > >>> context. > > >>> > > >>> Instead it seems simpler to split the fb->dirty() into a tinydrm > > >>> specific lower level hook that can be called from > > >>> mipi_dbi_enable_flush() and from a generic higher level > > >>> tinydrm_fb_dirty() helper. As we don't have a tinydrm specific > > >>> vfuncs table we'll just stick it into tinydrm_device directly > > >>> for now. > > >> The long term goal is to try and get rid of tinydrm.ko by moving stuff > > >> elsewhere as it's kind of a middle layer. So I'd really like to avoid > > >> adding a callback like this. > > >> Hopefully we can work out a solution based on my suggestion in the > > >> 'drm: Eliminate plane->fb/crtc usage for atomic drivers' thread. > > > I don't want to start redesigning the entire architecture at > > > this point. That would also cause a bigger risk of regression and > > > then we'd potentially have to try and revert the entire plane->fb > > > thing, which would not be fun if any significant changes have > > > occurred in the meantime. > > > > > >> If we do need a hook, I prefer that we add it to > > >> drm_simple_display_pipe_funcs. > > > If you have plans to redesign tinydrm anyway it seems to me that > > > a temporary hook in tinydrm is may be a bit less intrusive than > > > inflicting it on the simple_kms_helper. > > > > Yes you're right of course, what was I thinking. > > > > You missed out on one call site: > > > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > > b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > > index 11ae950b0fc9..7924eb59c2e1 100644 > > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c > > @@ -124,11 +124,8 @@ void tinydrm_display_pipe_update(struct > > drm_simple_display_pipe *pipe, > > struct drm_framebuffer *fb = pipe->plane.state->fb; > > struct drm_crtc *crtc = &tdev->pipe.crtc; > > > > - if (fb && (fb != old_state->fb)) { > > - pipe->plane.fb = fb; > > - if (fb->funcs->dirty) > > - fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0); > > - } > > + if (fb && (fb != old_state->fb)) > > + tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0); > > > > if (crtc->state->event) { > > spin_lock_irq(&crtc->dev->event_lock); > > > > With that fixed, series is: > > > > Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > > Awesome. Thanks. And thanks for catching that extra dirty() call. I'll > go and fix it up. OK, I posted the fixed version. Would you be interested in running some kind of smoke test on this before I push it? I'd hate to break things for you, and unfortunately (or maybe fortunately :) I don't have any hardware to test this.
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c index 11ae950b0fc9..7924eb59c2e1 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c @@ -124,11 +124,8 @@ void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_framebuffer *fb = pipe->plane.state->fb; struct drm_crtc *crtc = &tdev->pipe.crtc; - if (fb && (fb != old_state->fb)) { - pipe->plane.fb = fb; - if (fb->funcs->dirty) - fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0); - } + if (fb && (fb != old_state->fb)) + tdev->fb_dirty(fb, NULL, 0, 0, NULL, 0); if (crtc->state->event) {