On Tue, Apr 22, 2014 at 03:47:37PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 11, 2014 at 02:13:42PM -0700, Matt Roper wrote:
...
> > + int ret;
> > +
> > + /*
> > + * At the moment we use the same set of setplane restrictions as the
> > + * DRM primary plane helper, so go ahead and just call the helper if
> > + * the primary plane is already enabled. We only need to take special
> > + * action if the primary plane is disabled (something i915 can do but
> > + * the generic helper can't).
> > + */
> > + if (intel_crtc->primary_enabled)
> > + return drm_primary_helper_update(plane, crtc, fb,
> > + crtc_x, crtc_y,
> > + crtc_w, crtc_h,
> > + src_x, src_y,
> > + src_w, src_h);
>
> Why would we want to call that if we have a custom implementation
> anyway?
This was something Daniel requested on a previous patch iteration; even
though we're stuck duplicating most of the checks here for the !enabled
case, he still wanted to see us call into the helper for the enabled
case (although this will have to change in the future if/when we want to
start relaxing some of the tests that the helper does, such as plane
scaling).
...
> > + /*
> > + * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane
> > + * code that called us expects to handle the framebuffer update and
> > + * reference counting; save and restore the current fb before
> > + * calling it.
> > + */
> > + tmpfb = plane->fb;
> > + ret = intel_pipe_set_base(crtc, src_x, src_y, fb);
> > + if (ret)
> > + return ret;
> > + plane->fb = tmpfb;
> > +
> > + intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> > + intel_crtc->pipe);
>
> Needs a primary_enabled check (+ a visibility check for the fully
> clipped case).
If primary_enabled, we already called directly into the helper and
returned up at the top of the function.
I'll go ahead and add the visibility test farther up the function
though.
...
> > +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->pipe = pipe;
> > + primary->plane = pipe;
>
> We need to handle the pre-gen4 fbc primary plane swapping somewhere. I
> guess we still have intel_crtc->plane as well. That should probably get
> converted to use crtc->primary->plane instead.
Hmm, this is something I'm not terribly familiar with. I'm guessing I
just need to copy the logic from intel_crtc_init()?
I can write a Coccinelle patch to replace the intel_crtc->plane with
crtc->primary->plane as well; thanks for pointing that out.
Thanks for the review; I'll send along a new patch that incorporates
your other feedback shortly (and I think you've pointed out a few things
here that apply to the primary plane helper code too). I'm still
reworking my i-g-t patches for this functionality as well, but I've been
traveling lately and haven't had too much time to work on it.
Matt
--
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
