[RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors
Hi Gustavo, On Wed, Oct 26, 2016 at 07:40:29PM -0200, Gustavo Padovan wrote: >2016-10-26 Brian Starkey : > >> Add the OUT_FENCE_PTR property to writeback connectors, to enable >> userspace to get a fence which will signal once the writeback is >> complete. >> >> A timeline is added to drm_connector for use by the writeback >> out-fences. It is up to drivers to check for a fence in the >> connector_state and signal the it appropriately when their writeback has >> finished. >> >> It is not allowed to request an out-fence without a framebuffer attached >> to the connector. >> >> Signed-off-by: Brian Starkey >> --- >> drivers/gpu/drm/drm_atomic.c| 60 +++--- >> drivers/gpu/drm/drm_atomic_helper.c |5 ++- >> drivers/gpu/drm/drm_writeback.c | 80 >> +++ >> include/drm/drm_connector.h | 14 ++ >> include/drm/drm_writeback.h |2 + >> 5 files changed, 155 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 3f8fc97..061ea13 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -30,6 +30,7 @@ >> #include >> #include >> #include >> +#include >> #include >> >> #include "drm_crtc_internal.h" >> @@ -646,6 +647,12 @@ static int drm_atomic_connector_check(struct >> drm_connector *connector, >> return -EINVAL; >> } >> >> +if (state->out_fence && !state->fb) { >> +DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence >> without framebuffer\n", >> + connector->base.id, connector->name); >> +return -EINVAL; >> +} >> + >> return 0; >> } >> >> @@ -1047,6 +1054,8 @@ int drm_atomic_connector_set_property(struct >> drm_connector *connector, >> drm_atomic_set_fb_for_connector(state, fb); >> if (fb) >> drm_framebuffer_unreference(fb); >> +} else if (property == config->prop_out_fence_ptr) { >> +state->out_fence_ptr = u64_to_user_ptr(val); > >We need to move this out of the conn_state. For my v6 on CRTC out fences >I added drm_atomic_set_out_fence_for_crtc() and >drm_atomic_get_out_fence_for_crtc(). See padovan/fences. > Yep, will do. I was expecting this to change >> } else if (connector->funcs->atomic_set_property) { >> return connector->funcs->atomic_set_property(connector, >> state, property, val); >> @@ -1088,6 +1097,8 @@ drm_atomic_connector_get_property(struct drm_connector >> *connector, >> } else if (property == config->writeback_fb_id_property) { >> /* Writeback framebuffer is one-shot, write and forget */ >> *val = 0; >> +} else if (property == config->prop_out_fence_ptr) { >> +*val = 0; >> } else if (connector->funcs->atomic_get_property) { >> return connector->funcs->atomic_get_property(connector, >> state, property, val); >> @@ -1736,6 +1747,39 @@ static int setup_out_fence(struct drm_out_fence_state >> *fence_state, >> return 0; >> } >> >> +static int setup_connector_out_fences(struct drm_atomic_state *state, >> + struct drm_out_fence_state *fence_state, >> + int *fence_idx) >> +{ >> +struct drm_connector *conn; >> +struct drm_connector_state *conn_state; >> +int i, ret; >> + >> +for_each_connector_in_state(state, conn, conn_state, i) { >> +struct fence *fence; >> + >> +if (!conn_state->out_fence_ptr) >> +continue; >> + >> +fence = drm_writeback_get_out_fence(conn, conn_state); >> +if (!fence) >> +return -ENOMEM; >> + >> +ret = setup_out_fence(_state[(*fence_idx)++], >> + conn_state->out_fence_ptr, >> + fence); >> +if (ret) { >> +fence_put(fence); >> +return ret; >> +} >> + >> +/* One-time usage only */ >> +conn_state->out_fence_ptr = NULL; >> +} >> + >> +return 0; >> +} >> + >> int drm_mode_atomic_ioctl(struct drm_device *dev, >>void *data, struct drm_file *file_priv) >> { >> @@ -1868,8 +1912,8 @@ retry: >> drm_mode_object_unreference(obj); >> } >> >> -fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), >> - GFP_KERNEL); >> +fence_state = kcalloc(dev->mode_config.num_crtc + state->num_connector, >> + sizeof(*fence_state), GFP_KERNEL); >> if (!fence_state) { >> ret = -ENOMEM; >> goto out; >> @@ -1929,10 +1973,16 @@ retry: >> * Below we call drm_atomic_state_free for it. >> */ >> ret =
[RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors
2016-10-26 Brian Starkey : > Add the OUT_FENCE_PTR property to writeback connectors, to enable > userspace to get a fence which will signal once the writeback is > complete. > > A timeline is added to drm_connector for use by the writeback > out-fences. It is up to drivers to check for a fence in the > connector_state and signal the it appropriately when their writeback has > finished. > > It is not allowed to request an out-fence without a framebuffer attached > to the connector. > > Signed-off-by: Brian Starkey > --- > drivers/gpu/drm/drm_atomic.c| 60 +++--- > drivers/gpu/drm/drm_atomic_helper.c |5 ++- > drivers/gpu/drm/drm_writeback.c | 80 > +++ > include/drm/drm_connector.h | 14 ++ > include/drm/drm_writeback.h |2 + > 5 files changed, 155 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 3f8fc97..061ea13 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -30,6 +30,7 @@ > #include > #include > #include > +#include > #include > > #include "drm_crtc_internal.h" > @@ -646,6 +647,12 @@ static int drm_atomic_connector_check(struct > drm_connector *connector, > return -EINVAL; > } > > + if (state->out_fence && !state->fb) { > + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence > without framebuffer\n", > + connector->base.id, connector->name); > + return -EINVAL; > + } > + > return 0; > } > > @@ -1047,6 +1054,8 @@ int drm_atomic_connector_set_property(struct > drm_connector *connector, > drm_atomic_set_fb_for_connector(state, fb); > if (fb) > drm_framebuffer_unreference(fb); > + } else if (property == config->prop_out_fence_ptr) { > + state->out_fence_ptr = u64_to_user_ptr(val); We need to move this out of the conn_state. For my v6 on CRTC out fences I added drm_atomic_set_out_fence_for_crtc() and drm_atomic_get_out_fence_for_crtc(). See padovan/fences. > } else if (connector->funcs->atomic_set_property) { > return connector->funcs->atomic_set_property(connector, > state, property, val); > @@ -1088,6 +1097,8 @@ drm_atomic_connector_get_property(struct drm_connector > *connector, > } else if (property == config->writeback_fb_id_property) { > /* Writeback framebuffer is one-shot, write and forget */ > *val = 0; > + } else if (property == config->prop_out_fence_ptr) { > + *val = 0; > } else if (connector->funcs->atomic_get_property) { > return connector->funcs->atomic_get_property(connector, > state, property, val); > @@ -1736,6 +1747,39 @@ static int setup_out_fence(struct drm_out_fence_state > *fence_state, > return 0; > } > > +static int setup_connector_out_fences(struct drm_atomic_state *state, > + struct drm_out_fence_state *fence_state, > + int *fence_idx) > +{ > + struct drm_connector *conn; > + struct drm_connector_state *conn_state; > + int i, ret; > + > + for_each_connector_in_state(state, conn, conn_state, i) { > + struct fence *fence; > + > + if (!conn_state->out_fence_ptr) > + continue; > + > + fence = drm_writeback_get_out_fence(conn, conn_state); > + if (!fence) > + return -ENOMEM; > + > + ret = setup_out_fence(_state[(*fence_idx)++], > + conn_state->out_fence_ptr, > + fence); > + if (ret) { > + fence_put(fence); > + return ret; > + } > + > + /* One-time usage only */ > + conn_state->out_fence_ptr = NULL; > + } > + > + return 0; > +} > + > int drm_mode_atomic_ioctl(struct drm_device *dev, > void *data, struct drm_file *file_priv) > { > @@ -1868,8 +1912,8 @@ retry: > drm_mode_object_unreference(obj); > } > > - fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), > - GFP_KERNEL); > + fence_state = kcalloc(dev->mode_config.num_crtc + state->num_connector, > + sizeof(*fence_state), GFP_KERNEL); > if (!fence_state) { > ret = -ENOMEM; > goto out; > @@ -1929,10 +1973,16 @@ retry: >* Below we call drm_atomic_state_free for it. >*/ > ret = drm_atomic_check_only(state); > - } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) { > - ret = drm_atomic_nonblocking_commit(state); > } else { > -
[RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors
Hi Gustavo, It would be great if you could cast your eye over this one as-well. My intention was to be as similar to the CRTC out-fences as possible. Thanks, Brian On Wed, Oct 26, 2016 at 09:55:07AM +0100, Brian Starkey wrote: >Add the OUT_FENCE_PTR property to writeback connectors, to enable >userspace to get a fence which will signal once the writeback is >complete. > >A timeline is added to drm_connector for use by the writeback >out-fences. It is up to drivers to check for a fence in the >connector_state and signal the it appropriately when their writeback has >finished. > >It is not allowed to request an out-fence without a framebuffer attached >to the connector. > >Signed-off-by: Brian Starkey >--- > drivers/gpu/drm/drm_atomic.c| 60 +++--- > drivers/gpu/drm/drm_atomic_helper.c |5 ++- > drivers/gpu/drm/drm_writeback.c | 80 +++ > include/drm/drm_connector.h | 14 ++ > include/drm/drm_writeback.h |2 + > 5 files changed, 155 insertions(+), 6 deletions(-) > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >index 3f8fc97..061ea13 100644 >--- a/drivers/gpu/drm/drm_atomic.c >+++ b/drivers/gpu/drm/drm_atomic.c >@@ -30,6 +30,7 @@ > #include > #include > #include >+#include > #include > > #include "drm_crtc_internal.h" >@@ -646,6 +647,12 @@ static int drm_atomic_connector_check(struct >drm_connector *connector, > return -EINVAL; > } > >+ if (state->out_fence && !state->fb) { >+ DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence >without framebuffer\n", >+ connector->base.id, connector->name); >+ return -EINVAL; >+ } >+ > return 0; > } > >@@ -1047,6 +1054,8 @@ int drm_atomic_connector_set_property(struct >drm_connector *connector, > drm_atomic_set_fb_for_connector(state, fb); > if (fb) > drm_framebuffer_unreference(fb); >+ } else if (property == config->prop_out_fence_ptr) { >+ state->out_fence_ptr = u64_to_user_ptr(val); > } else if (connector->funcs->atomic_set_property) { > return connector->funcs->atomic_set_property(connector, > state, property, val); >@@ -1088,6 +1097,8 @@ drm_atomic_connector_get_property(struct drm_connector >*connector, > } else if (property == config->writeback_fb_id_property) { > /* Writeback framebuffer is one-shot, write and forget */ > *val = 0; >+ } else if (property == config->prop_out_fence_ptr) { >+ *val = 0; > } else if (connector->funcs->atomic_get_property) { > return connector->funcs->atomic_get_property(connector, > state, property, val); >@@ -1736,6 +1747,39 @@ static int setup_out_fence(struct drm_out_fence_state >*fence_state, > return 0; > } > >+static int setup_connector_out_fences(struct drm_atomic_state *state, >+struct drm_out_fence_state *fence_state, >+int *fence_idx) >+{ >+ struct drm_connector *conn; >+ struct drm_connector_state *conn_state; >+ int i, ret; >+ >+ for_each_connector_in_state(state, conn, conn_state, i) { >+ struct fence *fence; >+ >+ if (!conn_state->out_fence_ptr) >+ continue; >+ >+ fence = drm_writeback_get_out_fence(conn, conn_state); >+ if (!fence) >+ return -ENOMEM; >+ >+ ret = setup_out_fence(_state[(*fence_idx)++], >+conn_state->out_fence_ptr, >+fence); >+ if (ret) { >+ fence_put(fence); >+ return ret; >+ } >+ >+ /* One-time usage only */ >+ conn_state->out_fence_ptr = NULL; >+ } >+ >+ return 0; >+} >+ > int drm_mode_atomic_ioctl(struct drm_device *dev, > void *data, struct drm_file *file_priv) > { >@@ -1868,8 +1912,8 @@ retry: > drm_mode_object_unreference(obj); > } > >- fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), >-GFP_KERNEL); >+ fence_state = kcalloc(dev->mode_config.num_crtc + state->num_connector, >+sizeof(*fence_state), GFP_KERNEL); > if (!fence_state) { > ret = -ENOMEM; > goto out; >@@ -1929,10 +1973,16 @@ retry: >* Below we call drm_atomic_state_free for it. >*/ > ret = drm_atomic_check_only(state); >- } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) { >- ret = drm_atomic_nonblocking_commit(state); > } else { >- ret = drm_atomic_commit(state); >+
[RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors
On Wed, Oct 26, 2016 at 09:55:07AM +0100, Brian Starkey wrote: > Add the OUT_FENCE_PTR property to writeback connectors, to enable > userspace to get a fence which will signal once the writeback is > complete. > > A timeline is added to drm_connector for use by the writeback > out-fences. It is up to drivers to check for a fence in the > connector_state and signal the it appropriately when their writeback has > finished. > > It is not allowed to request an out-fence without a framebuffer attached > to the connector. > > Signed-off-by: Brian Starkey Ah, here it is, so much for reading patches strictly in-order ;-) One small comment below, otherwise I think this looks good. Again review from Gustavo for the out fences stuff would be really good (so pls cc him). And I think some igt testcases to exercise all the corner-cases in here. > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index a5e3778..7d40537 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -199,6 +199,7 @@ int drm_display_info_set_bus_formats(struct > drm_display_info *info, > * @best_encoder: can be used by helpers and drivers to select the encoder > * @state: backpointer to global drm_atomic_state > * @fb: Writeback framebuffer, for DRM_MODE_CONNECTOR_WRITEBACK > + * @out_fence: Fence which will clear when the framebuffer write has finished > */ > struct drm_connector_state { > struct drm_connector *connector; > @@ -216,6 +217,9 @@ struct drm_connector_state { > struct drm_atomic_state *state; > > struct drm_framebuffer *fb; /* do not write directly, use > drm_atomic_set_fb_for_connector() */ btw if you feel like adding a 2nd comment in-line like above, then that's a clear signal that you should move your kerneldoc struct member comments to the inline style. You can freely mix inline with top-level struct member documentation, so no need to change them all. You also missed the doc for out_fence_ptr, 0day won't like that. > + > + struct fence *out_fence; > + u64 __user *out_fence_ptr; writeback_ prefix for both imo, like in patch 1. > }; > > /** > @@ -546,6 +550,10 @@ struct drm_cmdline_mode { > * @tile_v_loc: vertical location of this tile > * @tile_h_size: horizontal size of this tile. > * @tile_v_size: vertical size of this tile. > + * @fence_context: context for fence signalling > + * @fence_lock: fence lock for the fence context > + * @fence_seqno: seqno variable to create fences > + * @timeline_name: fence timeline name > * > * Each connector may be connected to one or more CRTCs, or may be clonable > by > * another connector if they can share a CRTC. Each connector also has a > specific > @@ -694,6 +702,12 @@ struct drm_connector { > uint8_t num_h_tile, num_v_tile; > uint8_t tile_h_loc, tile_v_loc; > uint16_t tile_h_size, tile_v_size; > + > + /* fence timelines info for DRM out-fences */ > + unsigned int fence_context; > + spinlock_t fence_lock; > + unsigned long fence_seqno; > + char timeline_name[32]; Should all have writeout_ prefix. And at that point I wonder a bit whether we shouldn't just go ahead and create a struct drm_writeout_connector, to keep this stuff nicely separate. Only change visible to drivers would be the type of drm_writeback_connector_init, and they'd need to allocate/embedd a different struct. Worth it imo. -Daniel > }; > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > index afdc2742..01f33bc 100644 > --- a/include/drm/drm_writeback.h > +++ b/include/drm/drm_writeback.h > @@ -16,4 +16,6 @@ int drm_writeback_connector_init(struct drm_device *dev, >const struct drm_connector_funcs *funcs, >u32 *formats, int n_formats); > > +struct fence *drm_writeback_get_out_fence(struct drm_connector *connector, > + struct drm_connector_state > *conn_state); > #endif > -- > 1.7.9.5 > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[RFC PATCH v2 8/9] drm: writeback: Add out-fences for writeback connectors
Add the OUT_FENCE_PTR property to writeback connectors, to enable userspace to get a fence which will signal once the writeback is complete. A timeline is added to drm_connector for use by the writeback out-fences. It is up to drivers to check for a fence in the connector_state and signal the it appropriately when their writeback has finished. It is not allowed to request an out-fence without a framebuffer attached to the connector. Signed-off-by: Brian Starkey --- drivers/gpu/drm/drm_atomic.c| 60 +++--- drivers/gpu/drm/drm_atomic_helper.c |5 ++- drivers/gpu/drm/drm_writeback.c | 80 +++ include/drm/drm_connector.h | 14 ++ include/drm/drm_writeback.h |2 + 5 files changed, 155 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3f8fc97..061ea13 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include "drm_crtc_internal.h" @@ -646,6 +647,12 @@ static int drm_atomic_connector_check(struct drm_connector *connector, return -EINVAL; } + if (state->out_fence && !state->fb) { + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n", +connector->base.id, connector->name); + return -EINVAL; + } + return 0; } @@ -1047,6 +1054,8 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, drm_atomic_set_fb_for_connector(state, fb); if (fb) drm_framebuffer_unreference(fb); + } else if (property == config->prop_out_fence_ptr) { + state->out_fence_ptr = u64_to_user_ptr(val); } else if (connector->funcs->atomic_set_property) { return connector->funcs->atomic_set_property(connector, state, property, val); @@ -1088,6 +1097,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, } else if (property == config->writeback_fb_id_property) { /* Writeback framebuffer is one-shot, write and forget */ *val = 0; + } else if (property == config->prop_out_fence_ptr) { + *val = 0; } else if (connector->funcs->atomic_get_property) { return connector->funcs->atomic_get_property(connector, state, property, val); @@ -1736,6 +1747,39 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state, return 0; } +static int setup_connector_out_fences(struct drm_atomic_state *state, + struct drm_out_fence_state *fence_state, + int *fence_idx) +{ + struct drm_connector *conn; + struct drm_connector_state *conn_state; + int i, ret; + + for_each_connector_in_state(state, conn, conn_state, i) { + struct fence *fence; + + if (!conn_state->out_fence_ptr) + continue; + + fence = drm_writeback_get_out_fence(conn, conn_state); + if (!fence) + return -ENOMEM; + + ret = setup_out_fence(_state[(*fence_idx)++], + conn_state->out_fence_ptr, + fence); + if (ret) { + fence_put(fence); + return ret; + } + + /* One-time usage only */ + conn_state->out_fence_ptr = NULL; + } + + return 0; +} + int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1868,8 +1912,8 @@ retry: drm_mode_object_unreference(obj); } - fence_state = kcalloc(dev->mode_config.num_crtc, sizeof(*fence_state), - GFP_KERNEL); + fence_state = kcalloc(dev->mode_config.num_crtc + state->num_connector, + sizeof(*fence_state), GFP_KERNEL); if (!fence_state) { ret = -ENOMEM; goto out; @@ -1929,10 +1973,16 @@ retry: * Below we call drm_atomic_state_free for it. */ ret = drm_atomic_check_only(state); - } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) { - ret = drm_atomic_nonblocking_commit(state); } else { - ret = drm_atomic_commit(state); + ret = setup_connector_out_fences(state, fence_state, +_idx); + if (ret) + goto out; + + if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) + ret =