| Message ID | 1464546923-13439-9-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, May 29, 2016 at 08:35:05PM +0200, Daniel Vetter wrote: > It's kinda pointless to have 2 separate mallocs for these. And when we > add more per-connector state in the future it's even more pointless. > > Right now there's no such thing planned, but both Gustavo's per-crtc > fence patches, and some nonblocking commit helpers I'm playing around > with will add more per-crtc stuff. It makes sense to also consolidate > connectors, just for consistency. > > In the future we can use this to store a pointer to the preceeding > state, making an atomic update entirely free-standing. This will be > needed to be able to queue them up with a depth > 1. > > Cc: Gustavo Padovan <gustavo@padovan.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 27 +++++++++------------------ > drivers/gpu/drm/drm_atomic_helper.c | 2 +- > include/drm/drm_atomic.h | 10 +++++----- > include/drm/drm_crtc.h | 11 +++++++---- > 4 files changed, 22 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 3ff1ed7b33db..a6395e9654af 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -44,7 +44,6 @@ > void drm_atomic_state_default_release(struct drm_atomic_state *state) > { > kfree(state->connectors); > - kfree(state->connector_states); > kfree(state->crtcs); > kfree(state->crtc_states); > kfree(state->planes); > @@ -139,15 +138,15 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > DRM_DEBUG_ATOMIC("Clearing atomic state %p\n", state); > > for (i = 0; i < state->num_connector; i++) { > - struct drm_connector *connector = state->connectors[i]; > + struct drm_connector *connector = state->connectors[i].ptr; 'ptr' is not a very good name.
On Mon, May 30, 2016 at 05:59:24PM +0300, Ville Syrjälä wrote: > On Sun, May 29, 2016 at 08:35:05PM +0200, Daniel Vetter wrote: > > It's kinda pointless to have 2 separate mallocs for these. And when we > > add more per-connector state in the future it's even more pointless. > > > > Right now there's no such thing planned, but both Gustavo's per-crtc > > fence patches, and some nonblocking commit helpers I'm playing around > > with will add more per-crtc stuff. It makes sense to also consolidate > > connectors, just for consistency. > > > > In the future we can use this to store a pointer to the preceeding > > state, making an atomic update entirely free-standing. This will be > > needed to be able to queue them up with a depth > 1. > > > > Cc: Gustavo Padovan <gustavo@padovan.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 27 +++++++++------------------ > > drivers/gpu/drm/drm_atomic_helper.c | 2 +- > > include/drm/drm_atomic.h | 10 +++++----- > > include/drm/drm_crtc.h | 11 +++++++---- > > 4 files changed, 22 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 3ff1ed7b33db..a6395e9654af 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -44,7 +44,6 @@ > > void drm_atomic_state_default_release(struct drm_atomic_state *state) > > { > > kfree(state->connectors); > > - kfree(state->connector_states); > > kfree(state->crtcs); > > kfree(state->crtc_states); > > kfree(state->planes); > > @@ -139,15 +138,15 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > DRM_DEBUG_ATOMIC("Clearing atomic state %p\n", state); > > > > for (i = 0; i < state->num_connector; i++) { > > - struct drm_connector *connector = state->connectors[i]; > > + struct drm_connector *connector = state->connectors[i].ptr; > > 'ptr' is not a very good name. Suggestions for something better? I was lacking good inspiration ... -Daniel
On Mon, May 30, 2016 at 05:13:56PM +0200, Daniel Vetter wrote: > On Mon, May 30, 2016 at 05:59:24PM +0300, Ville Syrjälä wrote: > > On Sun, May 29, 2016 at 08:35:05PM +0200, Daniel Vetter wrote: > > > It's kinda pointless to have 2 separate mallocs for these. And when we > > > add more per-connector state in the future it's even more pointless. > > > > > > Right now there's no such thing planned, but both Gustavo's per-crtc > > > fence patches, and some nonblocking commit helpers I'm playing around > > > with will add more per-crtc stuff. It makes sense to also consolidate > > > connectors, just for consistency. > > > > > > In the future we can use this to store a pointer to the preceeding > > > state, making an atomic update entirely free-standing. This will be > > > needed to be able to queue them up with a depth > 1. > > > > > > Cc: Gustavo Padovan <gustavo@padovan.org> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > drivers/gpu/drm/drm_atomic.c | 27 +++++++++------------------ > > > drivers/gpu/drm/drm_atomic_helper.c | 2 +- > > > include/drm/drm_atomic.h | 10 +++++----- > > > include/drm/drm_crtc.h | 11 +++++++---- > > > 4 files changed, 22 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index 3ff1ed7b33db..a6395e9654af 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -44,7 +44,6 @@ > > > void drm_atomic_state_default_release(struct drm_atomic_state *state) > > > { > > > kfree(state->connectors); > > > - kfree(state->connector_states); > > > kfree(state->crtcs); > > > kfree(state->crtc_states); > > > kfree(state->planes); > > > @@ -139,15 +138,15 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > > DRM_DEBUG_ATOMIC("Clearing atomic state %p\n", state); > > > > > > for (i = 0; i < state->num_connector; i++) { > > > - struct drm_connector *connector = state->connectors[i]; > > > + struct drm_connector *connector = state->connectors[i].ptr; > > > > 'ptr' is not a very good name. > > Suggestions for something better? I was lacking good inspiration ... Maybe just 'connector' ?
On Mon, May 30, 2016 at 06:25:50PM +0300, Ville Syrjälä wrote: > On Mon, May 30, 2016 at 05:13:56PM +0200, Daniel Vetter wrote: > > On Mon, May 30, 2016 at 05:59:24PM +0300, Ville Syrjälä wrote: > > > On Sun, May 29, 2016 at 08:35:05PM +0200, Daniel Vetter wrote: > > > > It's kinda pointless to have 2 separate mallocs for these. And when we > > > > add more per-connector state in the future it's even more pointless. > > > > > > > > Right now there's no such thing planned, but both Gustavo's per-crtc > > > > fence patches, and some nonblocking commit helpers I'm playing around > > > > with will add more per-crtc stuff. It makes sense to also consolidate > > > > connectors, just for consistency. > > > > > > > > In the future we can use this to store a pointer to the preceeding > > > > state, making an atomic update entirely free-standing. This will be > > > > needed to be able to queue them up with a depth > 1. > > > > > > > > Cc: Gustavo Padovan <gustavo@padovan.org> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > --- > > > > drivers/gpu/drm/drm_atomic.c | 27 +++++++++------------------ > > > > drivers/gpu/drm/drm_atomic_helper.c | 2 +- > > > > include/drm/drm_atomic.h | 10 +++++----- > > > > include/drm/drm_crtc.h | 11 +++++++---- > > > > 4 files changed, 22 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > > index 3ff1ed7b33db..a6395e9654af 100644 > > > > --- a/drivers/gpu/drm/drm_atomic.c > > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > > @@ -44,7 +44,6 @@ > > > > void drm_atomic_state_default_release(struct drm_atomic_state *state) > > > > { > > > > kfree(state->connectors); > > > > - kfree(state->connector_states); > > > > kfree(state->crtcs); > > > > kfree(state->crtc_states); > > > > kfree(state->planes); > > > > @@ -139,15 +138,15 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > > > DRM_DEBUG_ATOMIC("Clearing atomic state %p\n", state); > > > > > > > > for (i = 0; i < state->num_connector; i++) { > > > > - struct drm_connector *connector = state->connectors[i]; > > > > + struct drm_connector *connector = state->connectors[i].ptr; > > > > > > 'ptr' is not a very good name. > > > > Suggestions for something better? I was lacking good inspiration ... > > Maybe just 'connector' ? state->connectors[i].connector is really long, and makes a lot of code look ugly. "obj" might be a bit better than "ptr" at least. Something else? -Daniel
On Mon, May 30, 2016 at 05:33:56PM +0200, Daniel Vetter wrote: > On Mon, May 30, 2016 at 06:25:50PM +0300, Ville Syrjälä wrote: > > On Mon, May 30, 2016 at 05:13:56PM +0200, Daniel Vetter wrote: > > > On Mon, May 30, 2016 at 05:59:24PM +0300, Ville Syrjälä wrote: > > > > On Sun, May 29, 2016 at 08:35:05PM +0200, Daniel Vetter wrote: > > > > > It's kinda pointless to have 2 separate mallocs for these. And when we > > > > > add more per-connector state in the future it's even more pointless. > > > > > > > > > > Right now there's no such thing planned, but both Gustavo's per-crtc > > > > > fence patches, and some nonblocking commit helpers I'm playing around > > > > > with will add more per-crtc stuff. It makes sense to also consolidate > > > > > connectors, just for consistency. > > > > > > > > > > In the future we can use this to store a pointer to the preceeding > > > > > state, making an atomic update entirely free-standing. This will be > > > > > needed to be able to queue them up with a depth > 1. > > > > > > > > > > Cc: Gustavo Padovan <gustavo@padovan.org> > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > > --- > > > > > drivers/gpu/drm/drm_atomic.c | 27 +++++++++------------------ > > > > > drivers/gpu/drm/drm_atomic_helper.c | 2 +- > > > > > include/drm/drm_atomic.h | 10 +++++----- > > > > > include/drm/drm_crtc.h | 11 +++++++---- > > > > > 4 files changed, 22 insertions(+), 28 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > > > index 3ff1ed7b33db..a6395e9654af 100644 > > > > > --- a/drivers/gpu/drm/drm_atomic.c > > > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > > > @@ -44,7 +44,6 @@ > > > > > void drm_atomic_state_default_release(struct drm_atomic_state *state) > > > > > { > > > > > kfree(state->connectors); > > > > > - kfree(state->connector_states); > > > > > kfree(state->crtcs); > > > > > kfree(state->crtc_states); > > > > > kfree(state->planes); > > > > > @@ -139,15 +138,15 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > > > > DRM_DEBUG_ATOMIC("Clearing atomic state %p\n", state); > > > > > > > > > > for (i = 0; i < state->num_connector; i++) { > > > > > - struct drm_connector *connector = state->connectors[i]; > > > > > + struct drm_connector *connector = state->connectors[i].ptr; > > > > > > > > 'ptr' is not a very good name. > > > > > > Suggestions for something better? I was lacking good inspiration ... > > > > Maybe just 'connector' ? > > state->connectors[i].connector is really long, and makes a lot of code > look ugly. "obj" might be a bit better than "ptr" at least. Something > else? How often are you expecting to have to type this anyway? Using any kind of generic name here will make life harder for cscope users. Atomic is really bad in this regard, escecially with all the identically named function pointers. It's seriosuly hard to navigate that maze these days. Someone should do a bit of renaming of stuff to make things more unique.
On Mon, May 30, 2016 at 5:45 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> state->connectors[i].connector is really long, and makes a lot of code >> look ugly. "obj" might be a bit better than "ptr" at least. Something >> else? > > How often are you expecting to have to type this anyway? Using any > kind of generic name here will make life harder for cscope users. > Atomic is really bad in this regard, escecially with all the identically > named function pointers. It's seriosuly hard to navigate that maze > these days. Someone should do a bit of renaming of stuff to make > things more unique. We have the same aliasing with legacy hooks shared between encoder/bridge&crtc. I just always search for the containing structure since indeed there's no other way to find stuff. Plus big screens ;-) -Daniel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3ff1ed7b33db..a6395e9654af 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -44,7 +44,6 @@ void drm_atomic_state_default_release(struct drm_atomic_state *state) { kfree(state->connectors); - kfree(state->connector_states); kfree(state->crtcs); kfree(state->crtc_states); kfree(state->planes); @@ -139,15 +138,15 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) DRM_DEBUG_ATOMIC("Clearing atomic state %p\n", state); for (i = 0; i < state->num_connector; i++) { - struct drm_connector *connector = state->connectors[i]; + struct drm_connector *connector = state->connectors[i].ptr; if (!connector) continue; connector->funcs->atomic_destroy_state(connector, - state->connector_states[i]); - state->connectors[i] = NULL; - state->connector_states[i] = NULL; + state->connectors[i].state); + state->connectors[i].ptr = NULL; + state->connectors[i].state = NULL; drm_connector_unreference(connector); } @@ -896,8 +895,7 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state, index = drm_connector_index(connector); if (index >= state->num_connector) { - struct drm_connector **c; - struct drm_connector_state **cs; + struct __drm_connnectors_state *c; int alloc = max(index + 1, config->num_connector); c = krealloc(state->connectors, alloc * sizeof(*state->connectors), GFP_KERNEL); @@ -908,26 +906,19 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state, memset(&state->connectors[state->num_connector], 0, sizeof(*state->connectors) * (alloc - state->num_connector)); - cs = krealloc(state->connector_states, alloc * sizeof(*state->connector_states), GFP_KERNEL); - if (!cs) - return ERR_PTR(-ENOMEM); - - state->connector_states = cs; - memset(&state->connector_states[state->num_connector], 0, - sizeof(*state->connector_states) * (alloc - state->num_connector)); state->num_connector = alloc; } - if (state->connector_states[index]) - return state->connector_states[index]; + if (state->connectors[index].state) + return state->connectors[index].state; connector_state = connector->funcs->atomic_duplicate_state(connector); if (!connector_state) return ERR_PTR(-ENOMEM); drm_connector_reference(connector); - state->connector_states[index] = connector_state; - state->connectors[index] = connector; + state->connectors[index].state = connector_state; + state->connectors[index].ptr = connector; connector_state->state = state; DRM_DEBUG_ATOMIC("Added [CONNECTOR:%d] %p state to %p\n", diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 464541c87c40..0fb4868fdad6 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1567,7 +1567,7 @@ void drm_atomic_helper_swap_state(struct drm_device *dev, for_each_connector_in_state(state, connector, conn_state, i) { connector->state->state = state; - swap(state->connector_states[i], connector->state); + swap(state->connectors[i].state, connector->state); connector->state->state = NULL; } diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 4e97186293be..37478adb6a16 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -106,7 +106,7 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state, if (index >= state->num_connector) return NULL; - return state->connector_states[index]; + return state->connectors[index].state; } /** @@ -175,11 +175,11 @@ int __must_check drm_atomic_check_only(struct drm_atomic_state *state); int __must_check drm_atomic_commit(struct drm_atomic_state *state); int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); -#define for_each_connector_in_state(state, connector, connector_state, __i) \ +#define for_each_connector_in_state(__state, connector, connector_state, __i) \ for ((__i) = 0; \ - (__i) < (state)->num_connector && \ - ((connector) = (state)->connectors[__i], \ - (connector_state) = (state)->connector_states[__i], 1); \ + (__i) < (__state)->num_connector && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (connector_state) = (__state)->connectors[__i].state, 1); \ (__i)++) \ for_each_if (connector) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5bffb5c86ea8..d4c46bfe4142 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1694,6 +1694,11 @@ struct drm_bridge { void *driver_private; }; +struct __drm_connnectors_state { + struct drm_connector *ptr; + struct drm_connector_state *state; +}; + /** * struct drm_atomic_state - the global state object for atomic updates * @dev: parent DRM device @@ -1705,8 +1710,7 @@ struct drm_bridge { * @crtcs: pointer to array of CRTC pointers * @crtc_states: pointer to array of CRTC states pointers * @num_connector: size of the @connectors and @connector_states arrays - * @connectors: pointer to array of connector pointers - * @connector_states: pointer to array of connector states pointers + * @connectors: pointer to array of structures with per-connector data * @acquire_ctx: acquire context for this atomic modeset state update */ struct drm_atomic_state { @@ -1719,8 +1723,7 @@ struct drm_atomic_state { struct drm_crtc **crtcs; struct drm_crtc_state **crtc_states; int num_connector; - struct drm_connector **connectors; - struct drm_connector_state **connector_states; + struct __drm_connnectors_state *connectors; struct drm_modeset_acquire_ctx *acquire_ctx; };
It's kinda pointless to have 2 separate mallocs for these. And when we add more per-connector state in the future it's even more pointless. Right now there's no such thing planned, but both Gustavo's per-crtc fence patches, and some nonblocking commit helpers I'm playing around with will add more per-crtc stuff. It makes sense to also consolidate connectors, just for consistency. In the future we can use this to store a pointer to the preceeding state, making an atomic update entirely free-standing. This will be needed to be able to queue them up with a depth > 1. Cc: Gustavo Padovan <gustavo@padovan.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_atomic.c | 27 +++++++++------------------ drivers/gpu/drm/drm_atomic_helper.c | 2 +- include/drm/drm_atomic.h | 10 +++++----- include/drm/drm_crtc.h | 11 +++++++---- 4 files changed, 22 insertions(+), 28 deletions(-)