On Wed, Apr 08, 2015 at 06:56:52PM -0700, Matt Roper wrote:
> Various places in the atomic plane code obtain the CRTC via
> plane_state->crtc. But plane_state->crtc is NULL when disabling the
> plane, so the code will fall back to looking at the old CRTC value in
> plane->crtc in that case. This isn't going to work when we start
> supporting non-blocking flips (since the legacy plane->crtc pointer will
> already be updated to its new value before the asynchronous workqueue
> task runs the plane commit function). The code is also fragile in
> general (we have to be careful when doing stuff like updating properties
> on a disabled plane). Since Intel hardware has a fixed plane to pipe
> mapping, let's just lookup the CRTC via that fixed mapping and avoid
> future mistakes.
>
> Cc: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> Reported-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
Because I'm a lazy reviewer ... does cocci agree that you've spotted all
the drm_plane->crtc references?
-Daniel
> ---
> drivers/gpu/drm/i915/intel_atomic_plane.c | 12 ++++++------
> drivers/gpu/drm/i915/intel_display.c | 8 ++++----
> drivers/gpu/drm/i915/intel_drv.h | 7 +++++++
> drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
> 4 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 90c4a82..740d1a6 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -116,19 +116,19 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> struct intel_plane_state *intel_state = to_intel_plane_state(state);
> bool active;
>
> - crtc = crtc ? crtc : plane->crtc;
> + crtc = intel_get_crtc_for_drm_plane(plane);
> intel_crtc = to_intel_crtc(crtc);
>
> intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> active = intel_crtc_state->base.enable;
>
> /*
> - * Both crtc and plane->crtc could be NULL if we're updating a
> - * property while the plane is disabled. We don't actually have
> - * anything driver-specific we need to test in that case, so
> - * just return success.
> + * Both state->crtc and plane->state->crtc could be NULL if we're
> + * updating a property while the plane is disabled. We don't actually
> + * have anything driver-specific we need to test in that case, so just
> + * return success.
> */
> - if (!crtc)
> + if (!state->crtc && !plane->state->crtc)
> return 0;
>
> /*
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 88b0f69..1cf91ad 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12625,7 +12625,7 @@ intel_check_primary_plane(struct drm_plane *plane,
> bool active;
> int ret;
>
> - crtc = crtc ? crtc : plane->crtc;
> + crtc = intel_get_crtc_for_drm_plane(plane);
> intel_crtc = to_intel_crtc(crtc);
>
> intel_crtc_state = intel_crtc_state_for_plane(state);
> @@ -12694,7 +12694,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
> struct drm_rect *src = &state->src;
> bool active;
>
> - crtc = crtc ? crtc : plane->crtc;
> + crtc = intel_get_crtc_for_drm_plane(plane);
> intel_crtc = to_intel_crtc(crtc);
>
> intel_crtc_state = intel_crtc_state_for_plane(state);
> @@ -12916,7 +12916,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> bool active;
> int ret;
>
> - crtc = crtc ? crtc : plane->crtc;
> + crtc = intel_get_crtc_for_drm_plane(plane);
> intel_crtc = to_intel_crtc(crtc);
>
> intel_crtc_state = intel_crtc_state_for_plane(state);
> @@ -12977,7 +12977,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> uint32_t addr;
> bool active;
>
> - crtc = crtc ? crtc : plane->crtc;
> + crtc = intel_get_crtc_for_drm_plane(plane);
> intel_crtc = to_intel_crtc(crtc);
>
> intel_crtc_state = intel_crtc_state_for_plane(state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 71e0152..d5ea24f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -731,6 +731,13 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane)
> return dev_priv->plane_to_crtc_mapping[plane];
> }
>
> +static inline struct drm_crtc *
> +intel_get_crtc_for_drm_plane(struct drm_plane *plane)
> +{
> + struct drm_i915_private *dev_priv = to_i915(plane->dev);
> + return dev_priv->pipe_to_crtc_mapping[to_intel_plane(plane)->pipe];
> +}
> +
> struct intel_unpin_work {
> struct work_struct work;
> struct drm_crtc *crtc;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 2016e78..492abcd 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -878,7 +878,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
> int pixel_size;
> bool active;
>
> - intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> + intel_crtc = to_intel_crtc(intel_get_crtc_for_drm_plane(plane));
>
> intel_crtc_state = intel_crtc_state_for_plane(state);
> active = intel_crtc_state->base.enable;
> @@ -1075,7 +1075,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
> uint32_t src_x, src_y, src_w, src_h;
> bool active;
>
> - crtc = crtc ? crtc : plane->crtc;
> + crtc = intel_get_crtc_for_drm_plane(plane);
> intel_crtc = to_intel_crtc(crtc);
>
> intel_crtc_state = intel_crtc_state_for_plane(state);
> --
> 1.8.5.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
