On Thu, May 15, 2014 at 09:37:55AM -0700, Matt Roper wrote:
> On Thu, May 15, 2014 at 06:52:28PM +0300, Ville Syrjälä wrote:
> ...
> >
> > > + };
> > > + bool visible;
> > > + int ret;
> > > +
> > > + ret = drm_primary_helper_check_update(plane, crtc, fb,
> > > + &src, &dest, &clip,
> > > + DRM_PLANE_HELPER_NO_SCALING,
> > > + DRM_PLANE_HELPER_NO_SCALING,
> > > + false, true, &visible);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (!visible)
> > > + return intel_primary_plane_disable(plane);
> >
> > Here we unpin the old fb...
> >
> > > +
> > > + /*
> > > + * 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)
> > > + /* Pin and return without programming hardware */
> > > + return intel_pin_and_fence_fb_obj(dev,
> > > + to_intel_framebuffer(fb)->obj,
> > > + NULL);
> >
> > ...but here we just pin the new fb and leave the old also pinned?
> >
> > Something's a bit fishy here. Also a non-enabled crtc but visible primary
> > plane doesn't seem to make sense. We also need to remember that set_base
> > will always unpin the old_fb. So if something can result in set_base
> > being called w/ old_fb!=NULL when it's not pinned, we'll be in deep
> > doodoo.
>
> Right, I guess we need to unpin the old fb, if any, here as well in case
> they perform several setplane() calls while the crtc is disabled.
>
> Eventually the crtc will get reenabled by a drmModeSetCrtc call. If we
> do setcrtc(fb = -1), then it should keep whatever fb we've already
> pinned via the setplane. If they provide a new fb, then the pinning
> we're doing here will get unpinned by the set_base that gets called.
>
> I don't see a way that you can hit set_base with an unpinned
> old_fb!=NULL (since the disable plane case farther up also clears
> primary->fb).
But it doesn't.
>
>
> Matt
>
> >
> > > +
> > > + intel_crtc_wait_for_pending_flips(crtc);
> > > + 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(dev_priv, intel_crtc->plane,
> > > + intel_crtc->pipe);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void intel_primary_plane_destroy(struct drm_plane *plane)
> > > +{
> > > + struct intel_plane *intel_plane = to_intel_plane(plane);
> > > + drm_plane_cleanup(plane);
> > > + kfree(intel_plane);
> > > +}
> > > +
> > > +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> > > + .update_plane = intel_primary_plane_setplane,
> > > + .disable_plane = intel_primary_plane_disable,
> > > + .destroy = intel_primary_plane_destroy,
> > > +};
> > > +
> > > +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > > + int pipe)
> > > +{
> > > + struct intel_plane *primary;
> > > + const uint32_t *intel_primary_formats;
> > > + int num_formats;
> > > +
> > > + primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > > + if (primary == NULL)
> > > + return NULL;
> > > +
> > > + primary->can_scale = false;
> > > + primary->max_downscale = 1;
> > > + primary->pipe = pipe;
> > > + primary->plane = pipe;
> > > + if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > > + primary->plane = !pipe;
> > > +
> > > + if (INTEL_INFO(dev)->gen <= 3) {
> > > + intel_primary_formats = intel_primary_formats_gen3;
> > > + num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
> > > + } else {
> > > + intel_primary_formats = intel_primary_formats_gen4;
> > > + num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> > > + }
> > > +
> > > + drm_plane_init(dev, &primary->base, 0,
> > > + &intel_primary_plane_funcs, intel_primary_formats,
> > > + num_formats, DRM_PLANE_TYPE_PRIMARY);
> > > + return &primary->base;
> > > +}
> > > +
> > > static void intel_crtc_init(struct drm_device *dev, int pipe)
> > > {
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > struct intel_crtc *intel_crtc;
> > > - int i;
> > > + struct drm_plane *primary;
> > > + int i, ret;
> > >
> > > intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> > > if (intel_crtc == NULL)
> > > return;
> > >
> > > - drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> > > + primary = intel_primary_plane_create(dev, pipe);
> > > + ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> > > + NULL, &intel_crtc_funcs);
> > > + if (ret) {
> > > + drm_plane_cleanup(primary);
> > > + kfree(intel_crtc);
> > > + return;
> > > + }
> > >
> > > drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
> > > for (i = 0; i < 256; i++) {
> > > --
> > > 1.8.5.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
