| Message ID | 1433422077-14907-15-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 04, 2015 at 02:47:44PM +0200, Maarten Lankhorst wrote: > By passing crtc_state to the check_plane functions a lot of duplicated > code can be removed. And now that the transitional helpers are gone the > crtc_state can be reliably obtained. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ++- > drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++--------------------- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_sprite.c | 13 +++------ > 4 files changed, 23 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > index aa2128369a0a..4d8cacbca777 100644 > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > @@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane, > crtc = crtc ? crtc : plane->crtc; > intel_crtc = to_intel_crtc(crtc); > > + intel_state->visible = false; > + What do we need this change for? Primary and cursor check functions immediately overwrite state->visible, so setting this here has no effect. The sprite case where fb==NULL is the only case where this would matter, but moving the assignment from the sprite check function to here doesn't seem like it gains us anything. Here's the only effect I see it having: > @@ -757,14 +758,8 @@ intel_check_sprite_plane(struct drm_plane *plane, <snip> > - if (!fb) { > - state->visible = false; > + if (!fb) > return 0; > - } > > /* Don't modify another pipe's plane */ > if (intel_plane->pipe != intel_crtc->pipe) {
Op 11-06-15 om 03:35 schreef Matt Roper: > On Thu, Jun 04, 2015 at 02:47:44PM +0200, Maarten Lankhorst wrote: >> By passing crtc_state to the check_plane functions a lot of duplicated >> code can be removed. And now that the transitional helpers are gone the >> crtc_state can be reliably obtained. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ++- >> drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++--------------------- >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> drivers/gpu/drm/i915/intel_sprite.c | 13 +++------ >> 4 files changed, 23 insertions(+), 43 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c >> index aa2128369a0a..4d8cacbca777 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c >> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c >> @@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane, >> crtc = crtc ? crtc : plane->crtc; >> intel_crtc = to_intel_crtc(crtc); >> >> + intel_state->visible = false; >> + > What do we need this change for? Primary and cursor check functions > immediately overwrite state->visible, so setting this here has no > effect. The sprite case where fb==NULL is the only case where this > would matter, but moving the assignment from the sprite check function > to here doesn't seem like it gains us anything. > I was using it to clear intel_state->visible even if no crtc is set. In that case state->visible should already be false, but a bit of paranoia never hunts. :-) ~Maarten
Op 15-06-15 om 14:05 schreef Daniel Vetter: > On Thu, Jun 11, 2015 at 06:23:09AM +0200, Maarten Lankhorst wrote: >> Op 11-06-15 om 03:35 schreef Matt Roper: >>> On Thu, Jun 04, 2015 at 02:47:44PM +0200, Maarten Lankhorst wrote: >>>> By passing crtc_state to the check_plane functions a lot of duplicated >>>> code can be removed. And now that the transitional helpers are gone the >>>> crtc_state can be reliably obtained. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ++- >>>> drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++--------------------- >>>> drivers/gpu/drm/i915/intel_drv.h | 1 + >>>> drivers/gpu/drm/i915/intel_sprite.c | 13 +++------ >>>> 4 files changed, 23 insertions(+), 43 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c >>>> index aa2128369a0a..4d8cacbca777 100644 >>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c >>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c >>>> @@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane, >>>> crtc = crtc ? crtc : plane->crtc; >>>> intel_crtc = to_intel_crtc(crtc); >>>> >>>> + intel_state->visible = false; >>>> + >>> What do we need this change for? Primary and cursor check functions >>> immediately overwrite state->visible, so setting this here has no >>> effect. The sprite case where fb==NULL is the only case where this >>> would matter, but moving the assignment from the sprite check function >>> to here doesn't seem like it gains us anything. >>> >> I was using it to clear intel_state->visible even if no crtc is set. In that case state->visible >> should already be false, but a bit of paranoia never hunts. :-) > Resetting of state should be done in the duplicate functions. This makes > sure we never ever forget to compute it correctly ;-) Sprinkling clearing > code all over (and especially over the compute code) is imo fragile and > should be avoided if possible. > -Daniel Good point! ~Maarten
On Thu, Jun 11, 2015 at 06:23:09AM +0200, Maarten Lankhorst wrote: > Op 11-06-15 om 03:35 schreef Matt Roper: > > On Thu, Jun 04, 2015 at 02:47:44PM +0200, Maarten Lankhorst wrote: > >> By passing crtc_state to the check_plane functions a lot of duplicated > >> code can be removed. And now that the transitional helpers are gone the > >> crtc_state can be reliably obtained. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ++- > >> drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++--------------------- > >> drivers/gpu/drm/i915/intel_drv.h | 1 + > >> drivers/gpu/drm/i915/intel_sprite.c | 13 +++------ > >> 4 files changed, 23 insertions(+), 43 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > >> index aa2128369a0a..4d8cacbca777 100644 > >> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > >> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > >> @@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane, > >> crtc = crtc ? crtc : plane->crtc; > >> intel_crtc = to_intel_crtc(crtc); > >> > >> + intel_state->visible = false; > >> + > > What do we need this change for? Primary and cursor check functions > > immediately overwrite state->visible, so setting this here has no > > effect. The sprite case where fb==NULL is the only case where this > > would matter, but moving the assignment from the sprite check function > > to here doesn't seem like it gains us anything. > > > I was using it to clear intel_state->visible even if no crtc is set. In that case state->visible > should already be false, but a bit of paranoia never hunts. :-) Resetting of state should be done in the duplicate functions. This makes sure we never ever forget to compute it correctly ;-) Sprinkling clearing code all over (and especially over the compute code) is imo fragile and should be avoided if possible. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index aa2128369a0a..4d8cacbca777 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane, crtc = crtc ? crtc : plane->crtc; intel_crtc = to_intel_crtc(crtc); + intel_state->visible = false; + /* * Both crtc and plane->crtc could be NULL if we're updating a * property while the plane is disabled. We don't actually have @@ -185,7 +187,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane, } } - ret = intel_plane->check_plane(plane, intel_state); + ret = intel_plane->check_plane(plane, crtc_state, intel_state); if (ret || !state->state) return ret; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 857c00881eff..8841cf5e8f1b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13390,36 +13390,25 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state static int intel_check_primary_plane(struct drm_plane *plane, + struct intel_crtc_state *crtc_state, struct intel_plane_state *state) { - struct drm_device *dev = plane->dev; struct drm_crtc *crtc = state->base.crtc; - struct intel_crtc *intel_crtc; - struct intel_crtc_state *crtc_state; struct drm_framebuffer *fb = state->base.fb; - struct drm_rect *dest = &state->dst; - struct drm_rect *src = &state->src; - const struct drm_rect *clip = &state->clip; - bool can_position = false; - int max_scale = DRM_PLANE_HELPER_NO_SCALING; int min_scale = DRM_PLANE_HELPER_NO_SCALING; + int max_scale = DRM_PLANE_HELPER_NO_SCALING; + bool can_position = false; - crtc = crtc ? crtc : plane->crtc; - intel_crtc = to_intel_crtc(crtc); - crtc_state = state->base.state ? - intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL; - - if (INTEL_INFO(dev)->gen >= 9) { - /* use scaler when colorkey is not required */ - if (to_intel_plane(plane)->ckey.flags == I915_SET_COLORKEY_NONE) { - min_scale = 1; - max_scale = skl_max_scale(intel_crtc, crtc_state); - } + /* use scaler when colorkey is not required */ + if (INTEL_INFO(plane->dev)->gen >= 9 && + to_intel_plane(plane)->ckey.flags == I915_SET_COLORKEY_NONE) { + min_scale = 1; + max_scale = skl_max_scale(to_intel_crtc(crtc), crtc_state); can_position = true; } - return drm_plane_helper_check_update(plane, crtc, fb, - src, dest, clip, + return drm_plane_helper_check_update(plane, crtc, fb, &state->src, + &state->dst, &state->clip, min_scale, max_scale, can_position, true, &state->visible); @@ -13658,24 +13647,17 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane * static int intel_check_cursor_plane(struct drm_plane *plane, + struct intel_crtc_state *crtc_state, struct intel_plane_state *state) { - struct drm_crtc *crtc = state->base.crtc; - struct drm_device *dev = plane->dev; + struct drm_crtc *crtc = crtc_state->base.crtc; struct drm_framebuffer *fb = state->base.fb; - struct drm_rect *dest = &state->dst; - struct drm_rect *src = &state->src; - const struct drm_rect *clip = &state->clip; struct drm_i915_gem_object *obj = intel_fb_obj(fb); - struct intel_crtc *intel_crtc; unsigned stride; int ret; - crtc = crtc ? crtc : plane->crtc; - intel_crtc = to_intel_crtc(crtc); - - ret = drm_plane_helper_check_update(plane, crtc, fb, - src, dest, clip, + ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src, + &state->dst, &state->clip, DRM_PLANE_HELPER_NO_SCALING, DRM_PLANE_HELPER_NO_SCALING, true, true, &state->visible); @@ -13687,7 +13669,7 @@ intel_check_cursor_plane(struct drm_plane *plane, return 0; /* Check for which cursor types we support */ - if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) { + if (!cursor_size_ok(plane->dev, state->base.crtc_w, state->base.crtc_h)) { DRM_DEBUG("Cursor dimension %dx%d not supported\n", state->base.crtc_w, state->base.crtc_h); return -EINVAL; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 802f3e99c849..96e365e74b04 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -601,6 +601,7 @@ struct intel_plane { void (*disable_plane)(struct drm_plane *plane, struct drm_crtc *crtc, bool force); int (*check_plane)(struct drm_plane *plane, + struct intel_crtc_state *crtc_state, struct intel_plane_state *state); void (*commit_plane)(struct drm_plane *plane, struct intel_plane_state *state); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index c909b8b8ce85..c15d54b60105 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -739,11 +739,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc, bool force) static int intel_check_sprite_plane(struct drm_plane *plane, + struct intel_crtc_state *crtc_state, struct intel_plane_state *state) { struct drm_device *dev = plane->dev; - struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc); - struct intel_crtc_state *crtc_state; + struct drm_crtc *crtc = state->base.crtc; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_framebuffer *fb = state->base.fb; int crtc_x, crtc_y; @@ -757,14 +758,8 @@ intel_check_sprite_plane(struct drm_plane *plane, bool can_scale; int pixel_size; - intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc); - crtc_state = state->base.state ? - intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL; - - if (!fb) { - state->visible = false; + if (!fb) return 0; - } /* Don't modify another pipe's plane */ if (intel_plane->pipe != intel_crtc->pipe) {
By passing crtc_state to the check_plane functions a lot of duplicated code can be removed. And now that the transitional helpers are gone the crtc_state can be reliably obtained. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ++- drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++--------------------- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_sprite.c | 13 +++------ 4 files changed, 23 insertions(+), 43 deletions(-)