On Tue, Oct 25, 2016 at 06:58:02PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Due to the plane->index not getting readjusted in drm_plane_cleanup(),
> we can't continue initialization of some plane/crtc init fails.
> Well, we sort of could I suppose if we left all initialized planes on
> the list, but that would expose those planes to userspace as well.
>
> But for crtcs the situation is even worse since we assume that
> pipe==crtc index occasionally, so we can't really deal with a partially
> initialize set of crtcs.
>
> So seems safest to just abort the entire thing if anything goes wrong.
> All the failure paths here are kmalloc()s anyway, so it seems unlikely
> we'd get very far if these start failing.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Some bikeshedding in this patch, but I like it. For patches 1-3:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 4 +-
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 101 ++++++++++++++++++++++-------------
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_sprite.c | 8 +--
> 5 files changed, 75 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index af3559d34328..0e393b5a988a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -592,7 +592,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
>
> /* Important: The output setup functions called by modeset_init need
> * working irqs for e.g. gmbus and dp aux transfers. */
> - intel_modeset_init(dev);
> + ret = intel_modeset_init(dev);
> + if (ret)
> + goto cleanup_irq;
>
> intel_guc_init(dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7a621c74254e..a9308aeb2f1f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3700,7 +3700,7 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv);
>
> /* modesetting */
> extern void intel_modeset_init_hw(struct drm_device *dev);
> -extern void intel_modeset_init(struct drm_device *dev);
> +extern int intel_modeset_init(struct drm_device *dev);
> extern void intel_modeset_gem_init(struct drm_device *dev);
> extern void intel_modeset_cleanup(struct drm_device *dev);
> extern int intel_connector_register(struct drm_connector *);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 244a0f59d8f7..d5a49d12dc88 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14975,9 +14975,6 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> */
> void intel_plane_destroy(struct drm_plane *plane)
> {
> - if (!plane)
> - return;
> -
> drm_plane_cleanup(plane);
> kfree(to_intel_plane(plane));
> }
> @@ -14991,11 +14988,10 @@ const struct drm_plane_funcs intel_plane_funcs = {
> .atomic_set_property = intel_plane_atomic_set_property,
> .atomic_duplicate_state = intel_plane_duplicate_state,
> .atomic_destroy_state = intel_plane_destroy_state,
> -
> };
>
> -static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> - int pipe)
> +static struct intel_plane *
> +intel_primary_plane_create(struct drm_device *dev, enum pipe pipe)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_plane *primary = NULL;
> @@ -15006,12 +15002,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> int ret;
>
> primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> - if (!primary)
> + if (!primary) {
> + ret = -ENOMEM;
> goto fail;
> + }
>
> state = intel_create_plane_state(&primary->base);
> - if (!state)
> + if (!state) {
> + ret = -ENOMEM;
> goto fail;
> + }
> +
> primary->base.state = &state->base;
>
> primary->can_scale = false;
> @@ -15092,13 +15093,13 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>
> drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
>
> - return &primary->base;
> + return primary;
>
> fail:
> kfree(state);
> kfree(primary);
>
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> static int
> @@ -15194,8 +15195,8 @@ intel_update_cursor_plane(struct drm_plane *plane,
> intel_crtc_update_cursor(crtc, state);
> }
>
> -static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> - int pipe)
> +static struct intel_plane *
> +intel_cursor_plane_create(struct drm_device *dev, enum pipe pipe)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_plane *cursor = NULL;
> @@ -15203,12 +15204,17 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> int ret;
>
> cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
> - if (!cursor)
> + if (!cursor) {
> + ret = -ENOMEM;
> goto fail;
> + }
>
> state = intel_create_plane_state(&cursor->base);
> - if (!state)
> + if (!state) {
> + ret = -ENOMEM;
> goto fail;
> + }
> +
> cursor->base.state = &state->base;
>
> cursor->can_scale = false;
> @@ -15240,13 +15246,13 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>
> drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
>
> - return &cursor->base;
> + return cursor;
>
> fail:
> kfree(state);
> kfree(cursor);
>
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
> @@ -15265,22 +15271,24 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
> scaler_state->scaler_id = -1;
> }
>
> -static void intel_crtc_init(struct drm_device *dev, int pipe)
> +static int intel_crtc_init(struct drm_device *dev, enum pipe pipe)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc;
> struct intel_crtc_state *crtc_state = NULL;
> - struct drm_plane *primary = NULL;
> - struct drm_plane *cursor = NULL;
> + struct intel_plane *primary = NULL;
> + struct intel_plane *cursor = NULL;
> int sprite, ret;
>
> intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> - if (intel_crtc == NULL)
> - return;
> + if (!intel_crtc)
> + return -ENOMEM;
>
> crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> - if (!crtc_state)
> + if (!crtc_state) {
> + ret = -ENOMEM;
> goto fail;
> + }
> intel_crtc->config = crtc_state;
> intel_crtc->base.state = &crtc_state->base;
> crtc_state->base.crtc = &intel_crtc->base;
> @@ -15296,22 +15304,30 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> }
>
> primary = intel_primary_plane_create(dev, pipe);
> - if (!primary)
> + if (IS_ERR(primary)) {
> + ret = PTR_ERR(primary);
> goto fail;
> + }
>
> for_each_sprite(dev_priv, pipe, sprite) {
> - ret = intel_plane_init(dev, pipe, sprite);
> - if (ret)
> - DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
> - pipe_name(pipe), sprite_name(pipe, sprite), ret);
> + struct intel_plane *plane;
> +
> + plane = intel_sprite_plane_create(dev, pipe, sprite);
> + if (!plane) {
> + ret = PTR_ERR(plane);
> + goto fail;
> + }
> }
>
> cursor = intel_cursor_plane_create(dev, pipe);
> - if (!cursor)
> + if (!cursor) {
> + ret = PTR_ERR(cursor);
> goto fail;
> + }
>
> - ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> - cursor, &intel_crtc_funcs,
> + ret = drm_crtc_init_with_planes(dev, &intel_crtc->base,
> + &primary->base, &cursor->base,
> + &intel_crtc_funcs,
> "pipe %c", pipe_name(pipe));
> if (ret)
> goto fail;
> @@ -15343,13 +15359,18 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> intel_color_init(&intel_crtc->base);
>
> WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> - return;
> +
> + return 0;
>
> fail:
> - intel_plane_destroy(primary);
> - intel_plane_destroy(cursor);
> + /*
> + * drm_mode_config_cleanup() will free up any
> + * crtcs/planes already initialized.
> + */
> kfree(crtc_state);
> kfree(intel_crtc);
> +
> + return ret;
> }
>
> enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> @@ -16426,7 +16447,7 @@ static void sanitize_watermarks(struct drm_device *dev)
> drm_modeset_acquire_fini(&ctx);
> }
>
> -void intel_modeset_init(struct drm_device *dev)
> +int intel_modeset_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct i915_ggtt *ggtt = &dev_priv->ggtt;
> @@ -16450,7 +16471,7 @@ void intel_modeset_init(struct drm_device *dev)
> intel_init_pm(dev);
>
> if (INTEL_INFO(dev)->num_pipes == 0)
> - return;
> + return 0;
>
> /*
> * There may be no VBT; and if the BIOS enabled SSC we can
> @@ -16499,7 +16520,13 @@ void intel_modeset_init(struct drm_device *dev)
> INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
>
> for_each_pipe(dev_priv, pipe) {
> - intel_crtc_init(dev, pipe);
> + int ret;
> +
> + ret = intel_crtc_init(dev, pipe);
> + if (ret) {
> + drm_mode_config_cleanup(dev);
> + return ret;
> + }
> }
>
> intel_update_czclk(dev_priv);
> @@ -16547,6 +16574,8 @@ void intel_modeset_init(struct drm_device *dev)
> * since the watermark calculation done here will use pstate->fb.
> */
> sanitize_watermarks(dev);
> +
> + return 0;
> }
>
> static void intel_enable_pipe_a(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7dda79df55d0..4fd7d74fd6dc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1779,7 +1779,8 @@ bool intel_sdvo_init(struct drm_device *dev,
> /* intel_sprite.c */
> int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
> int usecs);
> -int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
> +struct intel_plane *intel_sprite_plane_create(struct drm_device *dev,
> + enum pipe pipe, int plane);
> int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> void intel_pipe_update_start(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ae1e3d47951b..41ae7f562eec 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1042,8 +1042,8 @@ static uint32_t skl_plane_formats[] = {
> DRM_FORMAT_VYUY,
> };
>
> -int
> -intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> +struct intel_plane *
> +intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_plane *intel_plane = NULL;
> @@ -1160,11 +1160,11 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>
> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>
> - return 0;
> + return intel_plane;
>
> fail:
> kfree(state);
> kfree(intel_plane);
>
> - return ret;
> + return ERR_PTR(ret);
> }
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
