| Message ID | 1425388937-1247-6-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 03, 2015 at 03:21:59PM +0200, Ander Conselvan de Oliveira wrote: > For the atomic conversion, the mode set paths need to be changed to rely > on an atomic state instead of using the staged config. By using an > atomic state for the legacy code, we will be able to convert the code > base in small chunks. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Two small comments below. -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++++++++++++-------- > 1 file changed, 91 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 798de7b..97d4df5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -37,6 +37,7 @@ > #include <drm/i915_drm.h> > #include "i915_drv.h" > #include "i915_trace.h" > +#include <drm/drm_atomic.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_dp_helper.h> > #include <drm/drm_crtc_helper.h> > @@ -10290,10 +10291,22 @@ static bool check_digital_port_conflicts(struct drm_device *dev) > return true; > } > > -static struct intel_crtc_state * > +static void > +clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > +{ > + struct drm_crtc_state tmp_state; > + > + /* Clear only the intel specific part of the crtc state */ > + tmp_state = crtc_state->base; > + memset(crtc_state, 0, sizeof *crtc_state); > + crtc_state->base = tmp_state; > +} I guess this is to clear out state which we want to recompute, and our compute_config code assumes that it's always kzalloc'ed a new config? I think this should be part of the crtc duplicate_state callback to make sure we're doing this consistently. > + > +static int > intel_modeset_pipe_config(struct drm_crtc *crtc, > struct drm_framebuffer *fb, > - struct drm_display_mode *mode) > + struct drm_display_mode *mode, > + struct drm_atomic_state *state) > { > struct drm_device *dev = crtc->dev; > struct intel_encoder *encoder; > @@ -10303,17 +10316,19 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > > if (!check_encoder_cloning(to_intel_crtc(crtc))) { > DRM_DEBUG_KMS("rejecting invalid cloning configuration\n"); > - return ERR_PTR(-EINVAL); > + return -EINVAL; > } > > if (!check_digital_port_conflicts(dev)) { > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); > - return ERR_PTR(-EINVAL); > + return -EINVAL; > } > > - pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL); > - if (!pipe_config) > - return ERR_PTR(-ENOMEM); > + pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); > + if (IS_ERR(pipe_config)) > + return PTR_ERR(pipe_config); > + > + clear_intel_crtc_state(pipe_config); > > pipe_config->base.crtc = crtc; > drm_mode_copy(&pipe_config->base.adjusted_mode, mode); > @@ -10408,10 +10423,9 @@ encoder_retry: > DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n", > plane_bpp, pipe_config->pipe_bpp, pipe_config->dither); > > - return pipe_config; > + return 0; > fail: > - kfree(pipe_config); > - return ERR_PTR(ret); > + return ret; > } > > /* Computes which crtcs are affected and sets the relevant bits in the mask. For > @@ -11089,17 +11103,19 @@ static struct intel_crtc_state * > intel_modeset_compute_config(struct drm_crtc *crtc, > struct drm_display_mode *mode, > struct drm_framebuffer *fb, > + struct drm_atomic_state *state, > unsigned *modeset_pipes, > unsigned *prepare_pipes, > unsigned *disable_pipes) > { > struct intel_crtc_state *pipe_config = NULL; > + int ret = 0; > > intel_modeset_affected_pipes(crtc, modeset_pipes, > prepare_pipes, disable_pipes); > > if ((*modeset_pipes) == 0) > - goto out; > + return NULL; > > /* > * Note this needs changes when we start tracking multiple modes > @@ -11107,14 +11123,17 @@ intel_modeset_compute_config(struct drm_crtc *crtc, > * (i.e. one pipe_config for each crtc) rather than just the one > * for this crtc. > */ > - pipe_config = intel_modeset_pipe_config(crtc, fb, mode); > - if (IS_ERR(pipe_config)) { > - goto out; > - } > + ret = intel_modeset_pipe_config(crtc, fb, mode, state); > + if (ret) > + return ERR_PTR(ret); > + > + pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); > + if (IS_ERR(pipe_config)) > + return pipe_config; > + > intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, > "[modeset]"); > > -out: > return pipe_config; > } > > @@ -11159,6 +11178,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_display_mode *saved_mode; > + struct intel_crtc_state *crtc_state_copy = NULL; > struct intel_crtc *intel_crtc; > int ret = 0; > > @@ -11166,6 +11186,12 @@ static int __intel_set_mode(struct drm_crtc *crtc, > if (!saved_mode) > return -ENOMEM; > > + crtc_state_copy = kmalloc(sizeof(*crtc_state_copy), GFP_KERNEL); > + if (!crtc_state_copy) { > + ret = -ENOMEM; > + goto done; > + } > + > *saved_mode = crtc->mode; > > if (modeset_pipes) > @@ -11252,6 +11278,19 @@ done: > if (ret && crtc->state->enable) > crtc->mode = *saved_mode; > > + if (ret == 0 && pipe_config) { > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + > + /* The pipe_config will be freed with the atomic state, so > + * make a copy. */ > + memcpy(crtc_state_copy, intel_crtc->config, > + sizeof *crtc_state_copy); > + intel_crtc->config = intel_crtc->new_config = crtc_state_copy; > + intel_crtc->base.state = &crtc_state_copy->base; > + } else { > + kfree(crtc_state_copy); > + } > + > kfree(saved_mode); > return ret; > } > @@ -11279,20 +11318,37 @@ static int intel_set_mode(struct drm_crtc *crtc, > struct drm_display_mode *mode, > int x, int y, struct drm_framebuffer *fb) > { > + struct drm_device *dev = crtc->dev; > + struct drm_atomic_state *state; > struct intel_crtc_state *pipe_config; > unsigned modeset_pipes, prepare_pipes, disable_pipes; > + int ret = 0; > + > + state = drm_atomic_state_alloc(dev); > + if (!state) > + return -ENOMEM; > + > + state->acquire_ctx = dev->mode_config.acquire_ctx; > > - pipe_config = intel_modeset_compute_config(crtc, mode, fb, > + pipe_config = intel_modeset_compute_config(crtc, mode, fb, state, > &modeset_pipes, > &prepare_pipes, > &disable_pipes); > > - if (IS_ERR(pipe_config)) > - return PTR_ERR(pipe_config); > + if (IS_ERR(pipe_config)) { > + ret = PTR_ERR(pipe_config); > + goto out; > + } > + > + ret = intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config, > + modeset_pipes, prepare_pipes, > + disable_pipes); > + if (ret) > + goto out; > > - return intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config, > - modeset_pipes, prepare_pipes, > - disable_pipes); > +out: > + drm_atomic_state_free(state); > + return ret; > } > > void intel_crtc_restore_mode(struct drm_crtc *crtc) > @@ -11622,6 +11678,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > { > struct drm_device *dev; > struct drm_mode_set save_set; > + struct drm_atomic_state *state = NULL; > struct intel_set_config *config; > struct intel_crtc_state *pipe_config; > unsigned modeset_pipes, prepare_pipes, disable_pipes; > @@ -11666,12 +11723,20 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > * such cases. */ > intel_set_config_compute_mode_changes(set, config); > > + state = drm_atomic_state_alloc(dev); > + if (!state) { > + ret = -ENOMEM; > + goto out_config; > + } > + > + state->acquire_ctx = dev->mode_config.acquire_ctx; > + > ret = intel_modeset_stage_output_state(dev, set, config); > if (ret) > goto fail; > > pipe_config = intel_modeset_compute_config(set->crtc, set->mode, > - set->fb, > + set->fb, state, > &modeset_pipes, > &prepare_pipes, > &disable_pipes); > @@ -11691,10 +11756,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > */ > } > > - /* set_mode will free it in the mode_changed case */ > - if (!config->mode_changed) > - kfree(pipe_config); > - > intel_update_pipe_size(to_intel_crtc(set->crtc)); > > if (config->mode_changed) { > @@ -11757,6 +11818,9 @@ fail: > } > > out_config: > + if (state) > + drm_atomic_state_free(state); Right above this we also call set_mode again, which also grabs a global state. Nesting seems tricky, so I thnk you should free up the atomic state before we try the failure code paths to restore the old state. > + > intel_set_config_free(config); > return ret; > } > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Mar 04, 2015 at 04:33:17PM +0100, Daniel Vetter wrote: > On Tue, Mar 03, 2015 at 03:21:59PM +0200, Ander Conselvan de Oliveira wrote: > > For the atomic conversion, the mode set paths need to be changed to rely > > on an atomic state instead of using the staged config. By using an > > atomic state for the legacy code, we will be able to convert the code > > base in small chunks. > > > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > Two small comments below. > -Daniel > > > --- > > drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++++++++++++-------- > > 1 file changed, 91 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 798de7b..97d4df5 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -37,6 +37,7 @@ > > #include <drm/i915_drm.h> > > #include "i915_drv.h" > > #include "i915_trace.h" > > +#include <drm/drm_atomic.h> > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_dp_helper.h> > > #include <drm/drm_crtc_helper.h> > > @@ -10290,10 +10291,22 @@ static bool check_digital_port_conflicts(struct drm_device *dev) > > return true; > > } > > > > -static struct intel_crtc_state * > > +static void > > +clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_crtc_state tmp_state; > > + > > + /* Clear only the intel specific part of the crtc state */ > > + tmp_state = crtc_state->base; > > + memset(crtc_state, 0, sizeof *crtc_state); > > + crtc_state->base = tmp_state; > > +} > > I guess this is to clear out state which we want to recompute, and our > compute_config code assumes that it's always kzalloc'ed a new config? > > I think this should be part of the crtc duplicate_state callback to make > sure we're doing this consistently. If we zero out the config in duplicate_state(), then we're not really "duplicating" anymore are we? Ultimately we want to be able to duplicate the existing state, tweak a couple things, and then pass that state through the atomic pipeline, so it feels like doing a clear in the duplicate function is the wrong move, even if it would work for the frankenstein-style codebase we have at the moment. Matt
On Mon, 2015-03-09 at 16:19 -0700, Matt Roper wrote: > On Wed, Mar 04, 2015 at 04:33:17PM +0100, Daniel Vetter wrote: > > On Tue, Mar 03, 2015 at 03:21:59PM +0200, Ander Conselvan de Oliveira wrote: > > > For the atomic conversion, the mode set paths need to be changed to rely > > > on an atomic state instead of using the staged config. By using an > > > atomic state for the legacy code, we will be able to convert the code > > > base in small chunks. > > > > > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > > > Two small comments below. > > -Daniel > > > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++++++++++++-------- > > > 1 file changed, 91 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 798de7b..97d4df5 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -37,6 +37,7 @@ > > > #include <drm/i915_drm.h> > > > #include "i915_drv.h" > > > #include "i915_trace.h" > > > +#include <drm/drm_atomic.h> > > > #include <drm/drm_atomic_helper.h> > > > #include <drm/drm_dp_helper.h> > > > #include <drm/drm_crtc_helper.h> > > > @@ -10290,10 +10291,22 @@ static bool check_digital_port_conflicts(struct drm_device *dev) > > > return true; > > > } > > > > > > -static struct intel_crtc_state * > > > +static void > > > +clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > > > +{ > > > + struct drm_crtc_state tmp_state; > > > + > > > + /* Clear only the intel specific part of the crtc state */ > > > + tmp_state = crtc_state->base; > > > + memset(crtc_state, 0, sizeof *crtc_state); > > > + crtc_state->base = tmp_state; > > > +} > > > > I guess this is to clear out state which we want to recompute, and our > > compute_config code assumes that it's always kzalloc'ed a new config? > > > > I think this should be part of the crtc duplicate_state callback to make > > sure we're doing this consistently. > > If we zero out the config in duplicate_state(), then we're not really > "duplicating" anymore are we? Ultimately we want to be able to > duplicate the existing state, tweak a couple things, and then pass that > state through the atomic pipeline, so it feels like doing a clear in the > duplicate function is the wrong move, even if it would work for the > frankenstein-style codebase we have at the moment. Yeah, I guess I can't run away from inspecting the code and fixing whatever expects the crtc_state to be zeroed. Ander
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 798de7b..97d4df5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -37,6 +37,7 @@ #include <drm/i915_drm.h> #include "i915_drv.h" #include "i915_trace.h" +#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_dp_helper.h> #include <drm/drm_crtc_helper.h> @@ -10290,10 +10291,22 @@ static bool check_digital_port_conflicts(struct drm_device *dev) return true; } -static struct intel_crtc_state * +static void +clear_intel_crtc_state(struct intel_crtc_state *crtc_state) +{ + struct drm_crtc_state tmp_state; + + /* Clear only the intel specific part of the crtc state */ + tmp_state = crtc_state->base; + memset(crtc_state, 0, sizeof *crtc_state); + crtc_state->base = tmp_state; +} + +static int intel_modeset_pipe_config(struct drm_crtc *crtc, struct drm_framebuffer *fb, - struct drm_display_mode *mode) + struct drm_display_mode *mode, + struct drm_atomic_state *state) { struct drm_device *dev = crtc->dev; struct intel_encoder *encoder; @@ -10303,17 +10316,19 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, if (!check_encoder_cloning(to_intel_crtc(crtc))) { DRM_DEBUG_KMS("rejecting invalid cloning configuration\n"); - return ERR_PTR(-EINVAL); + return -EINVAL; } if (!check_digital_port_conflicts(dev)) { DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); - return ERR_PTR(-EINVAL); + return -EINVAL; } - pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL); - if (!pipe_config) - return ERR_PTR(-ENOMEM); + pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); + if (IS_ERR(pipe_config)) + return PTR_ERR(pipe_config); + + clear_intel_crtc_state(pipe_config); pipe_config->base.crtc = crtc; drm_mode_copy(&pipe_config->base.adjusted_mode, mode); @@ -10408,10 +10423,9 @@ encoder_retry: DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n", plane_bpp, pipe_config->pipe_bpp, pipe_config->dither); - return pipe_config; + return 0; fail: - kfree(pipe_config); - return ERR_PTR(ret); + return ret; } /* Computes which crtcs are affected and sets the relevant bits in the mask. For @@ -11089,17 +11103,19 @@ static struct intel_crtc_state * intel_modeset_compute_config(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_framebuffer *fb, + struct drm_atomic_state *state, unsigned *modeset_pipes, unsigned *prepare_pipes, unsigned *disable_pipes) { struct intel_crtc_state *pipe_config = NULL; + int ret = 0; intel_modeset_affected_pipes(crtc, modeset_pipes, prepare_pipes, disable_pipes); if ((*modeset_pipes) == 0) - goto out; + return NULL; /* * Note this needs changes when we start tracking multiple modes @@ -11107,14 +11123,17 @@ intel_modeset_compute_config(struct drm_crtc *crtc, * (i.e. one pipe_config for each crtc) rather than just the one * for this crtc. */ - pipe_config = intel_modeset_pipe_config(crtc, fb, mode); - if (IS_ERR(pipe_config)) { - goto out; - } + ret = intel_modeset_pipe_config(crtc, fb, mode, state); + if (ret) + return ERR_PTR(ret); + + pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); + if (IS_ERR(pipe_config)) + return pipe_config; + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]"); -out: return pipe_config; } @@ -11159,6 +11178,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct drm_display_mode *saved_mode; + struct intel_crtc_state *crtc_state_copy = NULL; struct intel_crtc *intel_crtc; int ret = 0; @@ -11166,6 +11186,12 @@ static int __intel_set_mode(struct drm_crtc *crtc, if (!saved_mode) return -ENOMEM; + crtc_state_copy = kmalloc(sizeof(*crtc_state_copy), GFP_KERNEL); + if (!crtc_state_copy) { + ret = -ENOMEM; + goto done; + } + *saved_mode = crtc->mode; if (modeset_pipes) @@ -11252,6 +11278,19 @@ done: if (ret && crtc->state->enable) crtc->mode = *saved_mode; + if (ret == 0 && pipe_config) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + + /* The pipe_config will be freed with the atomic state, so + * make a copy. */ + memcpy(crtc_state_copy, intel_crtc->config, + sizeof *crtc_state_copy); + intel_crtc->config = intel_crtc->new_config = crtc_state_copy; + intel_crtc->base.state = &crtc_state_copy->base; + } else { + kfree(crtc_state_copy); + } + kfree(saved_mode); return ret; } @@ -11279,20 +11318,37 @@ static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode, int x, int y, struct drm_framebuffer *fb) { + struct drm_device *dev = crtc->dev; + struct drm_atomic_state *state; struct intel_crtc_state *pipe_config; unsigned modeset_pipes, prepare_pipes, disable_pipes; + int ret = 0; + + state = drm_atomic_state_alloc(dev); + if (!state) + return -ENOMEM; + + state->acquire_ctx = dev->mode_config.acquire_ctx; - pipe_config = intel_modeset_compute_config(crtc, mode, fb, + pipe_config = intel_modeset_compute_config(crtc, mode, fb, state, &modeset_pipes, &prepare_pipes, &disable_pipes); - if (IS_ERR(pipe_config)) - return PTR_ERR(pipe_config); + if (IS_ERR(pipe_config)) { + ret = PTR_ERR(pipe_config); + goto out; + } + + ret = intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config, + modeset_pipes, prepare_pipes, + disable_pipes); + if (ret) + goto out; - return intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config, - modeset_pipes, prepare_pipes, - disable_pipes); +out: + drm_atomic_state_free(state); + return ret; } void intel_crtc_restore_mode(struct drm_crtc *crtc) @@ -11622,6 +11678,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) { struct drm_device *dev; struct drm_mode_set save_set; + struct drm_atomic_state *state = NULL; struct intel_set_config *config; struct intel_crtc_state *pipe_config; unsigned modeset_pipes, prepare_pipes, disable_pipes; @@ -11666,12 +11723,20 @@ static int intel_crtc_set_config(struct drm_mode_set *set) * such cases. */ intel_set_config_compute_mode_changes(set, config); + state = drm_atomic_state_alloc(dev); + if (!state) { + ret = -ENOMEM; + goto out_config; + } + + state->acquire_ctx = dev->mode_config.acquire_ctx; + ret = intel_modeset_stage_output_state(dev, set, config); if (ret) goto fail; pipe_config = intel_modeset_compute_config(set->crtc, set->mode, - set->fb, + set->fb, state, &modeset_pipes, &prepare_pipes, &disable_pipes); @@ -11691,10 +11756,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set) */ } - /* set_mode will free it in the mode_changed case */ - if (!config->mode_changed) - kfree(pipe_config); - intel_update_pipe_size(to_intel_crtc(set->crtc)); if (config->mode_changed) { @@ -11757,6 +11818,9 @@ fail: } out_config: + if (state) + drm_atomic_state_free(state); + intel_set_config_free(config); return ret; }
For the atomic conversion, the mode set paths need to be changed to rely on an atomic state instead of using the staged config. By using an atomic state for the legacy code, we will be able to convert the code base in small chunks. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++++++++++++-------- 1 file changed, 91 insertions(+), 27 deletions(-)