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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
