Patchwork intel/atomic: Stop updating legacy fb parameters

login
register
mail settings
Submitter Daniel Vetter
Date Dec. 7, 2017, 2:32 p.m.
Message ID <20171207143202.6021-1-daniel.vetter@ffwll.ch>
Download mbox | patch
Permalink /patch/10099317/
State New
Headers show

Comments

Daniel Vetter - Dec. 7, 2017, 2:32 p.m.
Even fbc isn't using this stuff anymore, so time to remove it.

Cleaning up one small piece of the atomic conversion cruft at the time
...

Quick explanation on why the plane->fb assignment is ok to delete: The
core code takes care of the refcounting and legacy ->fb pointer
updating, but drivers are allowed to update it ahead of time. Most
legacy modeset drivers did that as part of their set_config callback
(since that's how the legacy/crtc helpers worked). In i915 we only
need that to make the fbc code happy.

v2: don't nuke the assignement of intel_crtc->config, I accidentally
set CI ablaze :-) Spotted by Maarten. And better explain why nuking
the ->fb assignement shouldn't set off alarm bells.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 31 +++----------------------------
 1 file changed, 3 insertions(+), 28 deletions(-)
Maarten Lankhorst - Dec. 7, 2017, 2:49 p.m.
Op 07-12-17 om 15:32 schreef Daniel Vetter:
> Even fbc isn't using this stuff anymore, so time to remove it.
>
> Cleaning up one small piece of the atomic conversion cruft at the time
> ...
>
> Quick explanation on why the plane->fb assignment is ok to delete: The
> core code takes care of the refcounting and legacy ->fb pointer
> updating, but drivers are allowed to update it ahead of time. Most
> legacy modeset drivers did that as part of their set_config callback
> (since that's how the legacy/crtc helpers worked). In i915 we only
> need that to make the fbc code happy.
>
> v2: don't nuke the assignement of intel_crtc->config, I accidentally
> set CI ablaze :-) Spotted by Maarten. And better explain why nuking
> the ->fb assignement shouldn't set off alarm bells.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 31 +++----------------------------
>  1 file changed, 3 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1f7e312d0d0d..4614c7f1eec5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10967,31 +10967,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	return ret;
>  }
>  
> -static void
> -intel_modeset_update_crtc_state(struct drm_atomic_state *state)
> -{
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *new_crtc_state;
> -	int i;
> -
> -	/* Double check state. */
> -	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> -		to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state);
> -
> -		/*
> -		 * Update legacy state to satisfy fbc code. This can
> -		 * be removed when fbc uses the atomic state.
> -		 */
> -		if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
> -			struct drm_plane_state *plane_state = crtc->primary->state;
> -
> -			crtc->primary->fb = plane_state->fb;
> -			crtc->x = plane_state->src_x >> 16;
> -			crtc->y = plane_state->src_y >> 16;
> -		}
> -	}
> -}
> -
>  static bool intel_fuzzy_clock_check(int clock1, int clock2)
>  {
>  	int diff;
> @@ -12364,9 +12339,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		}
>  	}
>  
> -	/* Only after disabling all output pipelines that will be changed can we
> -	 * update the the output configuration. */
> -	intel_modeset_update_crtc_state(state);
> +	/* FIXME: Eventually get rid of our intel_crtc->config pointer */
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> +		to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state);
>  
>  	if (intel_state->modeset) {
>  		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
Daniel Vetter - Dec. 8, 2017, 10:21 a.m.
On Thu, Dec 07, 2017 at 03:49:35PM +0100, Maarten Lankhorst wrote:
> Op 07-12-17 om 15:32 schreef Daniel Vetter:
> > Even fbc isn't using this stuff anymore, so time to remove it.
> >
> > Cleaning up one small piece of the atomic conversion cruft at the time
> > ...
> >
> > Quick explanation on why the plane->fb assignment is ok to delete: The
> > core code takes care of the refcounting and legacy ->fb pointer
> > updating, but drivers are allowed to update it ahead of time. Most
> > legacy modeset drivers did that as part of their set_config callback
> > (since that's how the legacy/crtc helpers worked). In i915 we only
> > need that to make the fbc code happy.
> >
> > v2: don't nuke the assignement of intel_crtc->config, I accidentally
> > set CI ablaze :-) Spotted by Maarten. And better explain why nuking
> > the ->fb assignement shouldn't set off alarm bells.
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Applied, thanks for the review.
-Daniel

> 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 31 +++----------------------------
> >  1 file changed, 3 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1f7e312d0d0d..4614c7f1eec5 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10967,31 +10967,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  	return ret;
> >  }
> >  
> > -static void
> > -intel_modeset_update_crtc_state(struct drm_atomic_state *state)
> > -{
> > -	struct drm_crtc *crtc;
> > -	struct drm_crtc_state *new_crtc_state;
> > -	int i;
> > -
> > -	/* Double check state. */
> > -	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > -		to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state);
> > -
> > -		/*
> > -		 * Update legacy state to satisfy fbc code. This can
> > -		 * be removed when fbc uses the atomic state.
> > -		 */
> > -		if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
> > -			struct drm_plane_state *plane_state = crtc->primary->state;
> > -
> > -			crtc->primary->fb = plane_state->fb;
> > -			crtc->x = plane_state->src_x >> 16;
> > -			crtc->y = plane_state->src_y >> 16;
> > -		}
> > -	}
> > -}
> > -
> >  static bool intel_fuzzy_clock_check(int clock1, int clock2)
> >  {
> >  	int diff;
> > @@ -12364,9 +12339,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >  		}
> >  	}
> >  
> > -	/* Only after disabling all output pipelines that will be changed can we
> > -	 * update the the output configuration. */
> > -	intel_modeset_update_crtc_state(state);
> > +	/* FIXME: Eventually get rid of our intel_crtc->config pointer */
> > +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > +		to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state);
> >  
> >  	if (intel_state->modeset) {
> >  		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
> 
>

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1f7e312d0d0d..4614c7f1eec5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10967,31 +10967,6 @@  intel_modeset_pipe_config(struct drm_crtc *crtc,
 	return ret;
 }
 
-static void
-intel_modeset_update_crtc_state(struct drm_atomic_state *state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *new_crtc_state;
-	int i;
-
-	/* Double check state. */
-	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
-		to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state);
-
-		/*
-		 * Update legacy state to satisfy fbc code. This can
-		 * be removed when fbc uses the atomic state.
-		 */
-		if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
-			struct drm_plane_state *plane_state = crtc->primary->state;
-
-			crtc->primary->fb = plane_state->fb;
-			crtc->x = plane_state->src_x >> 16;
-			crtc->y = plane_state->src_y >> 16;
-		}
-	}
-}
-
 static bool intel_fuzzy_clock_check(int clock1, int clock2)
 {
 	int diff;
@@ -12364,9 +12339,9 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		}
 	}
 
-	/* Only after disabling all output pipelines that will be changed can we
-	 * update the the output configuration. */
-	intel_modeset_update_crtc_state(state);
+	/* FIXME: Eventually get rid of our intel_crtc->config pointer */
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
+		to_intel_crtc(crtc)->config = to_intel_crtc_state(new_crtc_state);
 
 	if (intel_state->modeset) {
 		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);