| Message ID | 1421326527-24906-7-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 15, 2015 at 02:55:26PM +0200, Ander Conselvan de Oliveira wrote: > The previous patch changed the config field in intel_crtc to a pointer, > but to keep the mechanical changes (done with spatch) separate from the > new code, the pointer was made to point to a new _config field with type > struct intel_crtc_state added to that struct. This patch improves that > code by getting rid of that field, allocating a state struct in > intel_crtc_init() a keeping it properly updated when a mode set > happens. > > v2: Manual changes split from previous patch. (Matt) > Don't leak the current state when the crtc is destroyed (Matt) > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> I don't see a need to keep patch #7 separate from the changes you're making here; I'd go ahead and just squash it into this patch since it's a trivial change. Although 6+7 look okay, I do question whether we really want to keep intel_crtc->config now that our intel crtc state is a subclass of the DRM state object and can always be accessed through our base pointer. For planes, we just grab the base state through the object's base pointer, then cast that to the i915-specific state object; i.e., foo = to_intel_plane_state(intel_plane->base.state); My inclination would be to handle crtc's the same way for consistency and so that we don't have two pointers per object (one base, one i915-specific) that are essentially pointing at the same thing and have to be kept in sync. Anyway, you can consider this entire series Reviewed-by: Matt Roper <matthew.d.roper@intel.com> whether or not you decide to make further updates to 6+7. The use of Coccinelle to auto-generate most of these patches was great and made the series very easy to review. Matt > --- > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++---- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index acdaed2..002e5a9 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8926,6 +8926,13 @@ out: > intel_runtime_pm_put(dev_priv); > } > > +static void intel_crtc_set_state(struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state) > +{ > + kfree(crtc->config); > + crtc->config = crtc_state; > +} > + > static void intel_crtc_destroy(struct drm_crtc *crtc) > { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > @@ -8944,6 +8951,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) > > drm_crtc_cleanup(crtc); > > + intel_crtc_set_state(intel_crtc, NULL); > kfree(intel_crtc); > } > > @@ -10995,8 +11003,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, > crtc->mode = *mode; > /* mode_set/enable/disable functions rely on a correct pipe > * config. */ > - (*(to_intel_crtc(crtc)->config)) = *pipe_config; > - to_intel_crtc(crtc)->new_config = to_intel_crtc(crtc)->config; > + intel_crtc_set_state(to_intel_crtc(crtc), pipe_config); > > /* > * Calculate and store various constants which > @@ -11040,7 +11047,6 @@ done: > if (ret && crtc->enabled) > crtc->mode = *saved_mode; > > - kfree(pipe_config); > kfree(saved_mode); > return ret; > } > @@ -12187,6 +12193,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc; > + struct intel_crtc_state *crtc_state = NULL; > struct drm_plane *primary = NULL; > struct drm_plane *cursor = NULL; > int i, ret; > @@ -12195,6 +12202,11 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > if (intel_crtc == NULL) > return; > > + crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL); > + if (!crtc_state) > + goto fail; > + intel_crtc_set_state(intel_crtc, crtc_state); > + > primary = intel_primary_plane_create(dev, pipe); > if (!primary) > goto fail; > @@ -12240,7 +12252,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > > WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe); > - intel_crtc->config = &intel_crtc->_config; > return; > > fail: > @@ -12248,6 +12259,7 @@ fail: > drm_plane_cleanup(primary); > if (cursor) > drm_plane_cleanup(cursor); > + kfree(crtc_state); > kfree(intel_crtc); > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 0b59a93..c8c0b7f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -469,7 +469,6 @@ struct intel_crtc { > uint32_t cursor_base; > > struct intel_plane_config plane_config; > - struct intel_crtc_state _config; > struct intel_crtc_state *config; > struct intel_crtc_state *new_config; > bool new_enabled; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 01/15/2015 08:50 PM, Matt Roper wrote: > On Thu, Jan 15, 2015 at 02:55:26PM +0200, Ander Conselvan de Oliveira wrote: >> The previous patch changed the config field in intel_crtc to a pointer, >> but to keep the mechanical changes (done with spatch) separate from the >> new code, the pointer was made to point to a new _config field with type >> struct intel_crtc_state added to that struct. This patch improves that >> code by getting rid of that field, allocating a state struct in >> intel_crtc_init() a keeping it properly updated when a mode set >> happens. >> >> v2: Manual changes split from previous patch. (Matt) >> Don't leak the current state when the crtc is destroyed (Matt) >> >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > I don't see a need to keep patch #7 separate from the changes you're > making here; I'd go ahead and just squash it into this patch since it's > a trivial change. > > Although 6+7 look okay, I do question whether we really want to keep > intel_crtc->config now that our intel crtc state is a subclass of the > DRM state object and can always be accessed through our base pointer. > For planes, we just grab the base state through the > object's base pointer, then cast that to the i915-specific state object; > i.e., > > foo = to_intel_plane_state(intel_plane->base.state); > > My inclination would be to handle crtc's the same way for consistency > and so that we don't have two pointers per object (one base, one > i915-specific) that are essentially pointing at the same thing and have > to be kept in sync. Daniel was anticipating we would still need ->config and ->new_config. For async updates, we would commit drm_crtc->state before it is written to the hardware. In that case, the hardware current state would still be in intel_crtc->config and drm_crtc->state would actually match ->new_config. We'll have to sort this out later, so I just avoided doing too many changes. Ander > Anyway, you can consider this entire series > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > whether or not you decide to make further updates to 6+7. The use of > Coccinelle to auto-generate most of these patches was great and made the > series very easy to review. > > > Matt > > >> --- >> drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++---- >> drivers/gpu/drm/i915/intel_drv.h | 1 - >> 2 files changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index acdaed2..002e5a9 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -8926,6 +8926,13 @@ out: >> intel_runtime_pm_put(dev_priv); >> } >> >> +static void intel_crtc_set_state(struct intel_crtc *crtc, >> + struct intel_crtc_state *crtc_state) >> +{ >> + kfree(crtc->config); >> + crtc->config = crtc_state; >> +} >> + >> static void intel_crtc_destroy(struct drm_crtc *crtc) >> { >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> @@ -8944,6 +8951,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) >> >> drm_crtc_cleanup(crtc); >> >> + intel_crtc_set_state(intel_crtc, NULL); >> kfree(intel_crtc); >> } >> >> @@ -10995,8 +11003,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, >> crtc->mode = *mode; >> /* mode_set/enable/disable functions rely on a correct pipe >> * config. */ >> - (*(to_intel_crtc(crtc)->config)) = *pipe_config; >> - to_intel_crtc(crtc)->new_config = to_intel_crtc(crtc)->config; >> + intel_crtc_set_state(to_intel_crtc(crtc), pipe_config); >> >> /* >> * Calculate and store various constants which >> @@ -11040,7 +11047,6 @@ done: >> if (ret && crtc->enabled) >> crtc->mode = *saved_mode; >> >> - kfree(pipe_config); >> kfree(saved_mode); >> return ret; >> } >> @@ -12187,6 +12193,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *intel_crtc; >> + struct intel_crtc_state *crtc_state = NULL; >> struct drm_plane *primary = NULL; >> struct drm_plane *cursor = NULL; >> int i, ret; >> @@ -12195,6 +12202,11 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) >> if (intel_crtc == NULL) >> return; >> >> + crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL); >> + if (!crtc_state) >> + goto fail; >> + intel_crtc_set_state(intel_crtc, crtc_state); >> + >> primary = intel_primary_plane_create(dev, pipe); >> if (!primary) >> goto fail; >> @@ -12240,7 +12252,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) >> drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); >> >> WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe); >> - intel_crtc->config = &intel_crtc->_config; >> return; >> >> fail: >> @@ -12248,6 +12259,7 @@ fail: >> drm_plane_cleanup(primary); >> if (cursor) >> drm_plane_cleanup(cursor); >> + kfree(crtc_state); >> kfree(intel_crtc); >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 0b59a93..c8c0b7f 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -469,7 +469,6 @@ struct intel_crtc { >> uint32_t cursor_base; >> >> struct intel_plane_config plane_config; >> - struct intel_crtc_state _config; >> struct intel_crtc_state *config; >> struct intel_crtc_state *new_config; >> bool new_enabled; >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Jan 15, 2015 at 02:55:26PM +0200, Ander Conselvan de Oliveira wrote: > The previous patch changed the config field in intel_crtc to a pointer, > but to keep the mechanical changes (done with spatch) separate from the > new code, the pointer was made to point to a new _config field with type > struct intel_crtc_state added to that struct. This patch improves that > code by getting rid of that field, allocating a state struct in > intel_crtc_init() a keeping it properly updated when a mode set > happens. > > v2: Manual changes split from previous patch. (Matt) > Don't leak the current state when the crtc is destroyed (Matt) > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++---- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index acdaed2..002e5a9 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8926,6 +8926,13 @@ out: > intel_runtime_pm_put(dev_priv); > } > > +static void intel_crtc_set_state(struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state) > +{ > + kfree(crtc->config); > + crtc->config = crtc_state; > +} > + > static void intel_crtc_destroy(struct drm_crtc *crtc) > { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > @@ -8944,6 +8951,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) > > drm_crtc_cleanup(crtc); > > + intel_crtc_set_state(intel_crtc, NULL); Actually I just looked at this again and I think this is going to cause a non-fatal warning on unload. Given your update of the base state in patch 7, the drm_crtc_cleanup() here is going to see crtc->state as non-NULL and try to clean it up itself. But since you haven't implemented crtc->funcs->atomic_destroy_state(), it will just throw a warning and continue on. So I think you want to do one of the following: * Move the intel_crtc_set_state() call above drm_crtc_cleanup() so that the core won't see a non-NULL state and think it needs to clean up. * Completely drop the intel_crtc_set_state() call and instead implement crtc->funcs->atomic_destroy_state() so that the core can handle the cleanup for us. Matt > kfree(intel_crtc); > } > > @@ -10995,8 +11003,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, > crtc->mode = *mode; > /* mode_set/enable/disable functions rely on a correct pipe > * config. */ > - (*(to_intel_crtc(crtc)->config)) = *pipe_config; > - to_intel_crtc(crtc)->new_config = to_intel_crtc(crtc)->config; > + intel_crtc_set_state(to_intel_crtc(crtc), pipe_config); > > /* > * Calculate and store various constants which > @@ -11040,7 +11047,6 @@ done: > if (ret && crtc->enabled) > crtc->mode = *saved_mode; > > - kfree(pipe_config); > kfree(saved_mode); > return ret; > } > @@ -12187,6 +12193,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc; > + struct intel_crtc_state *crtc_state = NULL; > struct drm_plane *primary = NULL; > struct drm_plane *cursor = NULL; > int i, ret; > @@ -12195,6 +12202,11 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > if (intel_crtc == NULL) > return; > > + crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL); > + if (!crtc_state) > + goto fail; > + intel_crtc_set_state(intel_crtc, crtc_state); > + > primary = intel_primary_plane_create(dev, pipe); > if (!primary) > goto fail; > @@ -12240,7 +12252,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > > WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe); > - intel_crtc->config = &intel_crtc->_config; > return; > > fail: > @@ -12248,6 +12259,7 @@ fail: > drm_plane_cleanup(primary); > if (cursor) > drm_plane_cleanup(cursor); > + kfree(crtc_state); > kfree(intel_crtc); > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 0b59a93..c8c0b7f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -469,7 +469,6 @@ struct intel_crtc { > uint32_t cursor_base; > > struct intel_plane_config plane_config; > - struct intel_crtc_state _config; > struct intel_crtc_state *config; > struct intel_crtc_state *new_config; > bool new_enabled; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 16, 2015 at 03:48:56PM -0800, Matt Roper wrote: > On Thu, Jan 15, 2015 at 02:55:26PM +0200, Ander Conselvan de Oliveira wrote: > > The previous patch changed the config field in intel_crtc to a pointer, > > but to keep the mechanical changes (done with spatch) separate from the > > new code, the pointer was made to point to a new _config field with type > > struct intel_crtc_state added to that struct. This patch improves that > > code by getting rid of that field, allocating a state struct in > > intel_crtc_init() a keeping it properly updated when a mode set > > happens. > > > > v2: Manual changes split from previous patch. (Matt) > > Don't leak the current state when the crtc is destroyed (Matt) > > > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++---- > > drivers/gpu/drm/i915/intel_drv.h | 1 - > > 2 files changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index acdaed2..002e5a9 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -8926,6 +8926,13 @@ out: > > intel_runtime_pm_put(dev_priv); > > } > > > > +static void intel_crtc_set_state(struct intel_crtc *crtc, > > + struct intel_crtc_state *crtc_state) > > +{ > > + kfree(crtc->config); > > + crtc->config = crtc_state; > > +} > > + > > static void intel_crtc_destroy(struct drm_crtc *crtc) > > { > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > @@ -8944,6 +8951,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) > > > > drm_crtc_cleanup(crtc); > > > > + intel_crtc_set_state(intel_crtc, NULL); > > Actually I just looked at this again and I think this is going to cause > a non-fatal warning on unload. Given your update of the base state in > patch 7, the drm_crtc_cleanup() here is going to see crtc->state as non-NULL and > try to clean it up itself. But since you haven't implemented > crtc->funcs->atomic_destroy_state(), it will just throw a warning and > continue on. > > So I think you want to do one of the following: > * Move the intel_crtc_set_state() call above drm_crtc_cleanup() so that > the core won't see a non-NULL state and think it needs to clean up. > * Completely drop the intel_crtc_set_state() call and instead implement > crtc->funcs->atomic_destroy_state() so that the core can handle the > cleanup for us. I figured we can do this in a follow-up patch, so went ahead and merged the entire series. First approach is probably simpler until we have the state handling all wired up, but I'll leave that to you two. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index acdaed2..002e5a9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8926,6 +8926,13 @@ out: intel_runtime_pm_put(dev_priv); } +static void intel_crtc_set_state(struct intel_crtc *crtc, + struct intel_crtc_state *crtc_state) +{ + kfree(crtc->config); + crtc->config = crtc_state; +} + static void intel_crtc_destroy(struct drm_crtc *crtc) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); @@ -8944,6 +8951,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) drm_crtc_cleanup(crtc); + intel_crtc_set_state(intel_crtc, NULL); kfree(intel_crtc); } @@ -10995,8 +11003,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, crtc->mode = *mode; /* mode_set/enable/disable functions rely on a correct pipe * config. */ - (*(to_intel_crtc(crtc)->config)) = *pipe_config; - to_intel_crtc(crtc)->new_config = to_intel_crtc(crtc)->config; + intel_crtc_set_state(to_intel_crtc(crtc), pipe_config); /* * Calculate and store various constants which @@ -11040,7 +11047,6 @@ done: if (ret && crtc->enabled) crtc->mode = *saved_mode; - kfree(pipe_config); kfree(saved_mode); return ret; } @@ -12187,6 +12193,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc; + struct intel_crtc_state *crtc_state = NULL; struct drm_plane *primary = NULL; struct drm_plane *cursor = NULL; int i, ret; @@ -12195,6 +12202,11 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) if (intel_crtc == NULL) return; + crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL); + if (!crtc_state) + goto fail; + intel_crtc_set_state(intel_crtc, crtc_state); + primary = intel_primary_plane_create(dev, pipe); if (!primary) goto fail; @@ -12240,7 +12252,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe); - intel_crtc->config = &intel_crtc->_config; return; fail: @@ -12248,6 +12259,7 @@ fail: drm_plane_cleanup(primary); if (cursor) drm_plane_cleanup(cursor); + kfree(crtc_state); kfree(intel_crtc); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0b59a93..c8c0b7f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -469,7 +469,6 @@ struct intel_crtc { uint32_t cursor_base; struct intel_plane_config plane_config; - struct intel_crtc_state _config; struct intel_crtc_state *config; struct intel_crtc_state *new_config; bool new_enabled;
The previous patch changed the config field in intel_crtc to a pointer, but to keep the mechanical changes (done with spatch) separate from the new code, the pointer was made to point to a new _config field with type struct intel_crtc_state added to that struct. This patch improves that code by getting rid of that field, allocating a state struct in intel_crtc_init() a keeping it properly updated when a mode set happens. v2: Manual changes split from previous patch. (Matt) Don't leak the current state when the crtc is destroyed (Matt) Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++---- drivers/gpu/drm/i915/intel_drv.h | 1 - 2 files changed, 16 insertions(+), 5 deletions(-)