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@xxxxxxxxx>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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
