From: Gustavo Padovan gustavo.padovan@collabora.co.uk
The !crtc->enabled case will now be handled by the !visible code, since the handling is basically the same.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/i915/intel_display.c | 26 -------------------------- 1 file changed, 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5279b99..2ccf7c0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11837,32 +11837,6 @@ intel_commit_primary_plane(struct drm_plane *plane, struct drm_rect *src = &state->src; int ret;
- /* - * If the CRTC isn't enabled, we're just pinning the framebuffer, - * updating the fb pointer, and returning without touching the - * hardware. This allows us to later do a drmModeSetCrtc with fb=-1 to - * turn on the display with all planes setup as desired. - */ - if (!crtc->enabled) { - mutex_lock(&dev->struct_mutex); - - /* - * If we already called setplane while the crtc was disabled, - * we may have an fb pinned; unpin it. - */ - if (plane->fb) - intel_unpin_fb_obj(old_obj); - - i915_gem_track_fb(old_obj, obj, - INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe)); - - /* Pin and return without programming hardware */ - ret = intel_pin_and_fence_fb_obj(dev, obj, NULL); - mutex_unlock(&dev->struct_mutex); - - return ret; - } - intel_crtc_wait_for_pending_flips(crtc);
/*
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Factor out a piece of code from intel_pipe_set_base() that updates the pipe size and adjust fitter.
This will help refactor the update primary plane path.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/i915/intel_display.c | 71 +++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ccf7c0..e7e7184 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2779,6 +2779,46 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) return pending; }
+static void intel_update_pipe_size(struct drm_crtc *crtc) +{ + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + const struct drm_display_mode *adjusted_mode; + + if (!i915.fastboot) + return; + + /* + * Update pipe size and adjust fitter if needed: the reason for this is + * that in compute_mode_changes we check the native mode (not the pfit + * mode) to see if we can flip rather than do a full mode set. In the + * fastboot case, we'll flip, but if we don't update the pipesrc and + * pfit state, we'll end up with a big fb scanned out into the wrong + * sized surface. + * + * To fix this properly, we need to hoist the checks up into + * compute_mode_changes (or above), check the actual pfit state and + * whether the platform allows pfit disable with pipe active, and only + * then update the pipesrc and pfit state, even on the flip path. + */ + + adjusted_mode = &intel_crtc->config.adjusted_mode; + + I915_WRITE(PIPESRC(intel_crtc->pipe), + ((adjusted_mode->crtc_hdisplay - 1) << 16) | + (adjusted_mode->crtc_vdisplay - 1)); + if (!intel_crtc->config.pch_pfit.enabled && + (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) || + intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) { + I915_WRITE(PF_CTL(intel_crtc->pipe), 0); + I915_WRITE(PF_WIN_POS(intel_crtc->pipe), 0); + I915_WRITE(PF_WIN_SZ(intel_crtc->pipe), 0); + } + intel_crtc->config.pipe_src_w = adjusted_mode->crtc_hdisplay; + intel_crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay; +} + static int intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *fb) @@ -2821,36 +2861,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, return ret; }
- /* - * Update pipe size and adjust fitter if needed: the reason for this is - * that in compute_mode_changes we check the native mode (not the pfit - * mode) to see if we can flip rather than do a full mode set. In the - * fastboot case, we'll flip, but if we don't update the pipesrc and - * pfit state, we'll end up with a big fb scanned out into the wrong - * sized surface. - * - * To fix this properly, we need to hoist the checks up into - * compute_mode_changes (or above), check the actual pfit state and - * whether the platform allows pfit disable with pipe active, and only - * then update the pipesrc and pfit state, even on the flip path. - */ - if (i915.fastboot) { - const struct drm_display_mode *adjusted_mode = - &intel_crtc->config.adjusted_mode; - - I915_WRITE(PIPESRC(intel_crtc->pipe), - ((adjusted_mode->crtc_hdisplay - 1) << 16) | - (adjusted_mode->crtc_vdisplay - 1)); - if (!intel_crtc->config.pch_pfit.enabled && - (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) || - intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) { - I915_WRITE(PF_CTL(intel_crtc->pipe), 0); - I915_WRITE(PF_WIN_POS(intel_crtc->pipe), 0); - I915_WRITE(PF_WIN_SZ(intel_crtc->pipe), 0); - } - intel_crtc->config.pipe_src_w = adjusted_mode->crtc_hdisplay; - intel_crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay; - } + intel_update_pipe_size(crtc);
dev_priv->display.update_primary_plane(crtc, fb, x, y);
On Tue, Sep 09, 2014 at 11:43:20AM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Factor out a piece of code from intel_pipe_set_base() that updates the pipe size and adjust fitter.
This will help refactor the update primary plane path.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/i915/intel_display.c | 71 +++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ccf7c0..e7e7184 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2779,6 +2779,46 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) return pending; }
+static void intel_update_pipe_size(struct drm_crtc *crtc)
These days we usually prefer to pass intel_crtc instead of drm_crtc. You can still call it 'crtc' since that's shorter and because we don't need anything from drm_crtc in this function there won't be any confusion between the two.
Otherwise the patch looks good.
+{
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_device *dev = crtc->dev;
- struct drm_i915_private *dev_priv = dev->dev_private;
- const struct drm_display_mode *adjusted_mode;
- if (!i915.fastboot)
return;- /*
* Update pipe size and adjust fitter if needed: the reason for this is* that in compute_mode_changes we check the native mode (not the pfit* mode) to see if we can flip rather than do a full mode set. In the* fastboot case, we'll flip, but if we don't update the pipesrc and* pfit state, we'll end up with a big fb scanned out into the wrong* sized surface.** To fix this properly, we need to hoist the checks up into* compute_mode_changes (or above), check the actual pfit state and* whether the platform allows pfit disable with pipe active, and only* then update the pipesrc and pfit state, even on the flip path.*/- adjusted_mode = &intel_crtc->config.adjusted_mode;
- I915_WRITE(PIPESRC(intel_crtc->pipe),
((adjusted_mode->crtc_hdisplay - 1) << 16) |(adjusted_mode->crtc_vdisplay - 1));- if (!intel_crtc->config.pch_pfit.enabled &&
(intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {I915_WRITE(PF_CTL(intel_crtc->pipe), 0);I915_WRITE(PF_WIN_POS(intel_crtc->pipe), 0);I915_WRITE(PF_WIN_SZ(intel_crtc->pipe), 0);- }
- intel_crtc->config.pipe_src_w = adjusted_mode->crtc_hdisplay;
- intel_crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay;
+}
static int intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *fb) @@ -2821,36 +2861,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, return ret; }
- /*
* Update pipe size and adjust fitter if needed: the reason for this is* that in compute_mode_changes we check the native mode (not the pfit* mode) to see if we can flip rather than do a full mode set. In the* fastboot case, we'll flip, but if we don't update the pipesrc and* pfit state, we'll end up with a big fb scanned out into the wrong* sized surface.** To fix this properly, we need to hoist the checks up into* compute_mode_changes (or above), check the actual pfit state and* whether the platform allows pfit disable with pipe active, and only* then update the pipesrc and pfit state, even on the flip path.*/- if (i915.fastboot) {
const struct drm_display_mode *adjusted_mode =&intel_crtc->config.adjusted_mode;I915_WRITE(PIPESRC(intel_crtc->pipe),((adjusted_mode->crtc_hdisplay - 1) << 16) |(adjusted_mode->crtc_vdisplay - 1));if (!intel_crtc->config.pch_pfit.enabled &&(intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {I915_WRITE(PF_CTL(intel_crtc->pipe), 0);I915_WRITE(PF_WIN_POS(intel_crtc->pipe), 0);I915_WRITE(PF_WIN_SZ(intel_crtc->pipe), 0);}intel_crtc->config.pipe_src_w = adjusted_mode->crtc_hdisplay;intel_crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay;- }
intel_update_pipe_size(crtc);
dev_priv->display.update_primary_plane(crtc, fb, x, y);
-- 1.9.3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
2014-09-09 Ville Syrjälä ville.syrjala@linux.intel.com:
On Tue, Sep 09, 2014 at 11:43:20AM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Factor out a piece of code from intel_pipe_set_base() that updates the pipe size and adjust fitter.
This will help refactor the update primary plane path.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/i915/intel_display.c | 71 +++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ccf7c0..e7e7184 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2779,6 +2779,46 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) return pending; }
+static void intel_update_pipe_size(struct drm_crtc *crtc)
These days we usually prefer to pass intel_crtc instead of drm_crtc. You can still call it 'crtc' since that's shorter and because we don't need anything from drm_crtc in this function there won't be any confusion between the two.
Actually we need the drm_crtc 3 times in this function, that is why I left it as an argument. We could just do the other way around and get it from &intel_crtc->base.
Gustavo
On Tue, Sep 09, 2014 at 02:43:14PM -0300, Gustavo Padovan wrote:
2014-09-09 Ville Syrjälä ville.syrjala@linux.intel.com:
On Tue, Sep 09, 2014 at 11:43:20AM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Factor out a piece of code from intel_pipe_set_base() that updates the pipe size and adjust fitter.
This will help refactor the update primary plane path.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/i915/intel_display.c | 71 +++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ccf7c0..e7e7184 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2779,6 +2779,46 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) return pending; }
+static void intel_update_pipe_size(struct drm_crtc *crtc)
These days we usually prefer to pass intel_crtc instead of drm_crtc. You can still call it 'crtc' since that's shorter and because we don't need anything from drm_crtc in this function there won't be any confusion between the two.
Actually we need the drm_crtc 3 times in this function, that is why I left it as an argument. We could just do the other way around and get it from &intel_crtc->base.
Yeah I prefer we use the intel_foo types internally without an intel_ prefix in the local variable name. Adding the foo->base. prefix isn't really much longer than just foo.
Imo upcasting should only be done in interface hooks where we have a reason for the paramater to have the more generic type, everywhere else it just looks a bit brittle. And yes I know that we're not there at all. -Daniel
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Fold intel_pipe_set_base() in the update primary plane path merging pieces of code that are common to both paths.
Basically the the pin/unpin procedures are the same for both paths and some checks can also be shared (some of the were moved to the check() stage)
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk --- drivers/gpu/drm/i915/intel_display.c | 100 ++++++++++++++++++++++++----------- 1 file changed, 69 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e7e7184..175a284 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11821,16 +11821,36 @@ intel_check_primary_plane(struct drm_plane *plane, struct intel_plane_state *state) { struct drm_crtc *crtc = state->crtc; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_device *dev = crtc->dev; struct drm_framebuffer *fb = state->fb; struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip; + int ret;
- return drm_plane_helper_check_update(plane, crtc, fb, + ret = drm_plane_helper_check_update(plane, crtc, fb, src, dest, clip, DRM_PLANE_HELPER_NO_SCALING, DRM_PLANE_HELPER_NO_SCALING, false, true, &state->visible); + if (ret) + return ret; + + if (intel_crtc->plane > INTEL_INFO(dev)->num_pipes) { + DRM_ERROR("no plane for crtc: plane %c, num_pipes %d\n", + plane_name(intel_crtc->plane), + INTEL_INFO(dev)->num_pipes); + return -EINVAL; + } + + /* no fb bound */ + if (state->visible && !fb) { + DRM_ERROR("No FB bound\n"); + return -EINVAL; + } + + return 0; }
static int @@ -11842,14 +11862,34 @@ intel_commit_primary_plane(struct drm_plane *plane, struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + enum pipe pipe = intel_crtc->pipe; + struct drm_framebuffer *old_fb = plane->fb; struct drm_i915_gem_object *obj = intel_fb_obj(fb); struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_rect *src = &state->src; - int ret; + int ret = 0;
intel_crtc_wait_for_pending_flips(crtc);
+ if (intel_crtc_has_pending_flip(crtc)) { + DRM_ERROR("pipe is still busy with an old pageflip\n"); + return -EBUSY; + } + + mutex_lock(&dev->struct_mutex); + if (plane->fb != fb) { + ret = intel_pin_and_fence_fb_obj(dev, obj, NULL); + if (ret == 0) + i915_gem_track_fb(old_obj, obj, + INTEL_FRONTBUFFER_PRIMARY(pipe)); + } + mutex_unlock(&dev->struct_mutex); + if (ret != 0) { + DRM_ERROR("pin & fence failed\n"); + return ret; + } + /* * If clipping results in a non-visible primary plane, we'll disable * the primary plane. Note that this is a bit different than what @@ -11857,33 +11897,9 @@ intel_commit_primary_plane(struct drm_plane *plane, * because plane->fb still gets set and pinned. */ if (!state->visible) { - mutex_lock(&dev->struct_mutex); - - /* - * Try to pin the new fb first so that we can bail out if we - * fail. - */ - if (plane->fb != fb) { - ret = intel_pin_and_fence_fb_obj(dev, obj, NULL); - if (ret) { - mutex_unlock(&dev->struct_mutex); - return ret; - } - } - - i915_gem_track_fb(old_obj, obj, - INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe)); - if (intel_crtc->primary_enabled) intel_disable_primary_hw_plane(plane, crtc);
- - if (plane->fb != fb) - if (plane->fb) - intel_unpin_fb_obj(old_obj); - - mutex_unlock(&dev->struct_mutex); - } else { if (intel_crtc && intel_crtc->active && intel_crtc->primary_enabled) { @@ -11903,12 +11919,34 @@ intel_commit_primary_plane(struct drm_plane *plane, intel_disable_fbc(dev); } } - ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb); - if (ret) - return ret;
- if (!intel_crtc->primary_enabled) - intel_enable_primary_hw_plane(plane, crtc); + intel_update_pipe_size(crtc); + + intel_crtc->primary_enabled = true; + + dev_priv->display.update_primary_plane(crtc, fb, src->x1, src->y1); + + mutex_lock(&dev->struct_mutex); + intel_update_fbc(dev); + mutex_unlock(&dev->struct_mutex); + } + + crtc->primary->fb = fb; + crtc->x = src->x1; + crtc->y = src->y1; + + if (intel_crtc->active) + intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe)); + + if (old_fb) { + if (intel_crtc->active && old_fb != fb) + intel_wait_for_vblank(dev, intel_crtc->pipe); + + if (old_fb != fb) { + mutex_lock(&dev->struct_mutex); + intel_unpin_fb_obj(old_obj); + mutex_unlock(&dev->struct_mutex); + } }
intel_plane->crtc_x = state->orig_dst.x1;
On Tue, Sep 09, 2014 at 11:43:21AM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Fold intel_pipe_set_base() in the update primary plane path merging pieces of code that are common to both paths.
Basically the the pin/unpin procedures are the same for both paths and some checks can also be shared (some of the were moved to the check() stage)
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/i915/intel_display.c | 100 ++++++++++++++++++++++++----------- 1 file changed, 69 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e7e7184..175a284 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11821,16 +11821,36 @@ intel_check_primary_plane(struct drm_plane *plane, struct intel_plane_state *state) { struct drm_crtc *crtc = state->crtc;
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_device *dev = crtc->dev; struct drm_framebuffer *fb = state->fb; struct drm_rect *dest = &state->dst; struct drm_rect *src = &state->src; const struct drm_rect *clip = &state->clip;
- int ret;
- return drm_plane_helper_check_update(plane, crtc, fb,
- ret = drm_plane_helper_check_update(plane, crtc, fb, src, dest, clip, DRM_PLANE_HELPER_NO_SCALING, DRM_PLANE_HELPER_NO_SCALING, false, true, &state->visible);
- if (ret)
return ret;- if (intel_crtc->plane > INTEL_INFO(dev)->num_pipes) {
DRM_ERROR("no plane for crtc: plane %c, num_pipes %d\n",plane_name(intel_crtc->plane),INTEL_INFO(dev)->num_pipes);return -EINVAL;- }
I think we can just kill this plane check. It serves no purpose IMO.
- /* no fb bound */
- if (state->visible && !fb) {
DRM_ERROR("No FB bound\n");return -EINVAL;- }
Hmm. This one may be needed still in cases where the BIOS fb takeover fails. Otherwise it shouldn't happen.
- return 0;
}
static int @@ -11842,14 +11862,34 @@ intel_commit_primary_plane(struct drm_plane *plane, struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- enum pipe pipe = intel_crtc->pipe;
- struct drm_framebuffer *old_fb = plane->fb; struct drm_i915_gem_object *obj = intel_fb_obj(fb); struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_rect *src = &state->src;
- int ret;
int ret = 0;
intel_crtc_wait_for_pending_flips(crtc);
if (intel_crtc_has_pending_flip(crtc)) {
DRM_ERROR("pipe is still busy with an old pageflip\n");return -EBUSY;}
Yeah I guess we can keep this sanity check here.
- mutex_lock(&dev->struct_mutex);
- if (plane->fb != fb) {
ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);if (ret == 0)i915_gem_track_fb(old_obj, obj,INTEL_FRONTBUFFER_PRIMARY(pipe));- }
- mutex_unlock(&dev->struct_mutex);
Could move the locking within the 'if (plane->fb != fb)' block since the lock just protects the lower level stuff and not the plane->fb pointer.
Hmm. I started to wonder if we can do the pin/unpin conditionally, but it must be fine, otherwise we already have some kind of problem with the pin count going astray.
I guess we could apply this small optimization to the sprite code as well. There we currently do the pin/unpin unconditionally.
- if (ret != 0) {
DRM_ERROR("pin & fence failed\n");return ret;- }
Hmm. I wonder if this error is user triggerable on some platforms. Should probably kill it or make it DRM_DEBUG_KMS or something.
- /*
- If clipping results in a non-visible primary plane, we'll disable
- the primary plane. Note that this is a bit different than what
@@ -11857,33 +11897,9 @@ intel_commit_primary_plane(struct drm_plane *plane, * because plane->fb still gets set and pinned. */ if (!state->visible) {
mutex_lock(&dev->struct_mutex);/** Try to pin the new fb first so that we can bail out if we* fail.*/if (plane->fb != fb) {ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);if (ret) {mutex_unlock(&dev->struct_mutex);return ret;}}i915_gem_track_fb(old_obj, obj,INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));if (intel_crtc->primary_enabled) intel_disable_primary_hw_plane(plane, crtc);
if (plane->fb != fb)if (plane->fb)intel_unpin_fb_obj(old_obj);mutex_unlock(&dev->struct_mutex);} else { if (intel_crtc && intel_crtc->active && intel_crtc->primary_enabled) {
@@ -11903,12 +11919,34 @@ intel_commit_primary_plane(struct drm_plane *plane, intel_disable_fbc(dev); } }
ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb);if (ret)return ret;if (!intel_crtc->primary_enabled)intel_enable_primary_hw_plane(plane, crtc);
intel_update_pipe_size(crtc);
I was going to say we probably don't want to call this here, but we do in fact call it already so this doesn't change the behaviour. Which is good since I don't want to think about this fastboot hack at all ;)
Maybe slap a comment on top of it. Eg.: /* FIXME kill this fastboot hack */
intel_crtc->primary_enabled = true;dev_priv->display.update_primary_plane(crtc, fb, src->x1, src->y1);mutex_lock(&dev->struct_mutex);intel_update_fbc(dev);mutex_unlock(&dev->struct_mutex);
intel_update_fbc() must be called only after primary->fb has been updated. Just after the intel_frontbuffer_flip() call is probably the best place for it.
Otherwise this patch is looking good I think.
- }
- crtc->primary->fb = fb;
- crtc->x = src->x1;
- crtc->y = src->y1;
- if (intel_crtc->active)
intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));- if (old_fb) {
if (intel_crtc->active && old_fb != fb)intel_wait_for_vblank(dev, intel_crtc->pipe);if (old_fb != fb) {mutex_lock(&dev->struct_mutex);intel_unpin_fb_obj(old_obj);mutex_unlock(&dev->struct_mutex);}-- 1.9.3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Sep 09, 2014 at 11:43:19AM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
The !crtc->enabled case will now be handled by the !visible code, since the handling is basically the same.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/i915/intel_display.c | 26 -------------------------- 1 file changed, 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5279b99..2ccf7c0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11837,32 +11837,6 @@ intel_commit_primary_plane(struct drm_plane *plane, struct drm_rect *src = &state->src; int ret;
- /*
* If the CRTC isn't enabled, we're just pinning the framebuffer,* updating the fb pointer, and returning without touching the* hardware. This allows us to later do a drmModeSetCrtc with fb=-1 to* turn on the display with all planes setup as desired.*/- if (!crtc->enabled) {
mutex_lock(&dev->struct_mutex);/** If we already called setplane while the crtc was disabled,* we may have an fb pinned; unpin it.*/if (plane->fb)intel_unpin_fb_obj(old_obj);i915_gem_track_fb(old_obj, obj,INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));/* Pin and return without programming hardware */ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);mutex_unlock(&dev->struct_mutex);return ret;- }
- intel_crtc_wait_for_pending_flips(crtc);
Yeah this should work just fine.
One difference between the code paths is the intel_crtc_wait_for_pending_flips() call, but since the crtc isn't enabled there can't be any pending flip.
The other difference is the pin vs. unpin order, but that shouldn't matter unless there's tons of other stuff pinned as well. It's not worth optimizing setplane calls on disabled CRTCs too much IMO.
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
/*
1.9.3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Sep 09, 2014 at 06:58:47PM +0300, Ville Syrjälä wrote:
On Tue, Sep 09, 2014 at 11:43:19AM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
The !crtc->enabled case will now be handled by the !visible code, since the handling is basically the same.
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/i915/intel_display.c | 26 -------------------------- 1 file changed, 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5279b99..2ccf7c0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11837,32 +11837,6 @@ intel_commit_primary_plane(struct drm_plane *plane, struct drm_rect *src = &state->src; int ret;
- /*
* If the CRTC isn't enabled, we're just pinning the framebuffer,* updating the fb pointer, and returning without touching the* hardware. This allows us to later do a drmModeSetCrtc with fb=-1 to* turn on the display with all planes setup as desired.*/- if (!crtc->enabled) {
mutex_lock(&dev->struct_mutex);/** If we already called setplane while the crtc was disabled,* we may have an fb pinned; unpin it.*/if (plane->fb)intel_unpin_fb_obj(old_obj);i915_gem_track_fb(old_obj, obj,INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));/* Pin and return without programming hardware */ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);mutex_unlock(&dev->struct_mutex);return ret;- }
- intel_crtc_wait_for_pending_flips(crtc);
Yeah this should work just fine.
One difference between the code paths is the intel_crtc_wait_for_pending_flips() call, but since the crtc isn't enabled there can't be any pending flip.
The other difference is the pin vs. unpin order, but that shouldn't matter unless there's tons of other stuff pinned as well. It's not worth optimizing setplane calls on disabled CRTCs too much IMO.
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
Queued for -next, thanks for the patch. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Gustavo Padovan -
Ville Syrjälä