On Thu, May 15, 2014 at 08:00:48PM +0300, Ville Syrjälä wrote:
> 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.
>
Ah, you're right. I was conflating explicit disables (fb=0) with
implicit disables (clipped to invisible). I think the v7 I just sent
should handle this properly...for the implicit disable case we leave the
fb pinned and pointed to by primary->fb. So when we switch to another
fb (or explicitly disable with fb=0), we should unpin it properly.
Matt
>
> >
> >
> > 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
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
