On Wed, Sep 10, 2014 at 09:25:29PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 10, 2014 at 12:04:18PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> >
> > 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)
> >
> > v2: take Ville's comments:
> > - remove unecessary plane check
> > - move mutex lock to inside the conditional
> > - make the pin fail message a debug one
> > - add a fixme for the fastboot hack
> > - call intel_frontbuffer_flip() after FBC update
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 93 ++++++++++++++++++++++++------------
> > 1 file changed, 62 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b78f00a..1a001bf 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11824,12 +11824,23 @@ intel_check_primary_plane(struct drm_plane *plane,
> > 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;
> > +
> > + /* no fb bound */
> > + if (state->visible && !fb) {
> > + DRM_ERROR("No FB bound\n");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > }
> >
> > static int
> > @@ -11841,14 +11852,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;
> > + }
> > +
> > + if (plane->fb != fb) {
> > + mutex_lock(&dev->struct_mutex);
> > + 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_DEBUG_KMS("pin & fence failed\n");
> > + return ret;
> > + }
>
> I'd move this error handling also inside the 'plane->fb != fb' block.
> That avoids the ret=0 initialization, and more importantly it will
> match what you did in the sprite code. Thanks to your efforts I'm
> already dreaming about sharing buffer pinning codes across
> different plane types...
>
> > +
> > /*
> > * If clipping results in a non-visible primary plane, we'll disable
> > * the primary plane. Note that this is a bit different than what
> > @@ -11856,33 +11887,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) {
> > @@ -11902,12 +11909,36 @@ 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);
> > + /* FIXME: kill this fastboot hack */
> > + intel_update_pipe_size(intel_crtc);
> > +
> > + intel_crtc->primary_enabled = true;
> > +
> > + dev_priv->display.update_primary_plane(crtc, fb, src->x1, src->y1);
> > +
> > + if (intel_crtc->active)
> > + intel_frontbuffer_flip(dev,
> > + INTEL_FRONTBUFFER_PRIMARY(pipe));
> > +
> > + mutex_lock(&dev->struct_mutex);
> > + intel_update_fbc(dev);
> > + mutex_unlock(&dev->struct_mutex);
>
> This is still done too early. We need primary->fb to be set before since
> intel_update_fbc() will consult it to determine if it can now enable fbc.
>
> Hmm. Actaully I think we may want to reorganize this code a bit more so
> that it more closely resembles the sprite code. So I'm thinking
> something like this:
>
> crtc->primary->fb = fb;
> crtc->x = ...;
> crtc->y = ...;
>
> intel_plane->crtc_x = ...;
> intel_plane->crtc_y = ...;
> ...
>
> if (crtc->active) {
> if (fbc.plane == plane && ...) {
> disable_fbc();
>
> if (visible)
> update_pipe_size();
>
> primary_enabled = visible;
> .update_primary_plane()
Actually better make that
if (visible) {
update_pipe_size();
intel_enable_primary_hw_plane();
} else {
intel_disable_primary_hw_plane();
}
for now so that we we don't have to duplicate the nasty BDW vblank wait
workaround when enabling the primary plane.
The next step might be to kill off intel_{enable,disable}_primary_hw_plane()
and replace them with intel_primary_plane_setplane() as well, and then we
have to move the vblank hack into intel_primary_plane_setplane() itself.
But let's do baby steps and get this intel_pipe_set_base() killing
finished before we continue too far ahead. Also the sprite plane
covering primary plane case probably needs a bit more thought before
we can do this.
>
> intel_frontbuffer_flip()
>
> update_fbc();
> }
>
>
> > + }
> > +
> > + crtc->primary->fb = fb;
> > + crtc->x = src->x1;
> > + crtc->y = src->y1;
> > +
> > + 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;
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
