From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH 4/4] drm/i915: Register primary plane for each CRTC Date: Tue, 4 Mar 2014 15:15:10 +0200 Message-ID: <20140304131510.GI3852@intel.com> References: <1393539283-5901-1-git-send-email-matthew.d.roper@intel.com> <1393539283-5901-5-git-send-email-matthew.d.roper@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <1393539283-5901-5-git-send-email-matthew.d.roper@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Matt Roper Cc: Intel Graphics Development , dri-devel@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Feb 27, 2014 at 02:14:43PM -0800, Matt Roper wrote: > Create a primary plane at CRTC init and hook up handlers for the various > operations that may be performed on it. The DRM core will only > advertise the primary planes to clients that set the appropriate > capability bit. > = > Since we're limited to the legacy plane operations at the moment > (SetPlane and such) this isn't terribly interesting yet; the plane > update handler will perform an MMIO flip of the display plane and the > disable handler will disable the CRTC. Once we migrate more of the > plane and CRTC info over to properties in preparation for atomic/nuclear > operations, primary planes will be more useful. > = > Cc: Intel Graphics Development > Signed-off-by: Matt Roper > --- > drivers/gpu/drm/i915/intel_display.c | 92 ++++++++++++++++++++++++++++++= ++++++ > 1 file changed, 92 insertions(+) > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index 9757010..d9a5cd5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8260,6 +8260,10 @@ static void intel_crtc_destroy(struct drm_crtc *cr= tc) > = > intel_crtc_cursor_set(crtc, NULL, 0, 0, 0); > = > + drm_plane_cleanup(crtc->primary_plane); > + kfree(crtc->primary_plane); > + crtc->primary_plane =3D NULL; > + > drm_crtc_cleanup(crtc); > = > kfree(intel_crtc); > @@ -10272,17 +10276,105 @@ static void intel_shared_dpll_init(struct drm_= device *dev) > BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); > } > = > +static int > +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *c= rtc, > + struct drm_framebuffer *fb, int crtc_x, int crtc_y, > + unsigned int crtc_w, unsigned int crtc_h, > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h) > +{ > + struct drm_device *dev =3D crtc->dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + > + /* setplane API takes shifted source rectangle values; unshift them */ > + src_x >>=3D 16; > + src_y >>=3D 16; > + src_w >>=3D 16; > + src_h >>=3D 16; > + > + if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) { > + DRM_DEBUG_KMS("Unsuitable framebuffer for primary plane\n"); > + return -EINVAL; > + } These aren't quite right for all gens. Actually I think the primary plane limits are genereally more relaxed than the sprite plane limits, so = intel_framebuffer_init() should have already done the necessary checks, at least as far the stride is concerned. So I'd just drop the stride check. In the future I suppose we might need to add some extra checks here too, but for now I think we should fine w/o them. > + > + /* > + * Current hardware can't reposition the primary plane or scale it > + * (although this could change in the future). This means that we > + * don't actually need any of the destination (crtc) rectangle values, > + * or the source rectangle width/height; only the source x/y winds up > + * getting used for panning. Nevertheless, let's sanity check the > + * incoming values to make sure userspace didn't think it could scale > + * or reposition this plane. > + */ > + if (crtc_w !=3D crtc->mode.hdisplay || crtc_h !=3D crtc->mode.vdisplay = || > + crtc_x !=3D 0 || crtc_y !=3D 0) { > + DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n"); > + return -EINVAL; > + } > + if (crtc_w !=3D src_w || crtc_h !=3D src_h) { > + DRM_DEBUG_KMS("Can't scale primary plane\n"); > + return -EINVAL; > + } I'd do the clipping anyway, and then check that the dst size matches the pipe src size post clipping. Otherwise the behaviour is rather inconsistent with the sprite planes. > + > + intel_pipe_set_base(crtc, src_x, src_y, fb); This can return an error. > + dev_priv->display.crtc_enable(crtc); Shouldn't enable the entire pipe, just the plane. > + > + return 0; > +} > + > +static int > +intel_primary_plane_disable(struct drm_plane *plane) > +{ > + struct drm_device *dev =3D plane->dev; > + drm_i915_private_t *dev_priv =3D dev->dev_private; > + > + if (!plane->fb) > + return 0; > + > + if (WARN_ON(!plane->crtc || plane->crtc->primary_plane !=3D plane)) > + return -EINVAL; > + > + dev_priv->display.crtc_disable(plane->crtc); This should just disable the plane, not the entire pipe. > + > + return 0; > +} > + > +static void intel_primary_plane_destroy(struct drm_plane *plane) > +{ > + /* > + * Since primary planes are never put on the mode_config plane list, > + * this entry point should never be called. Primary plane cleanup > + * happens during CRTC destruction. > + */ > + BUG(); Just leave the func pointer as NULL, you get a backtrace either way. > +} > + > +static const struct drm_plane_funcs intel_primary_plane_funcs =3D { > + .update_plane =3D intel_primary_plane_setplane, > + .disable_plane =3D intel_primary_plane_disable, > + .destroy =3D intel_primary_plane_destroy, > +}; > + > static void intel_crtc_init(struct drm_device *dev, int pipe) > { > drm_i915_private_t *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc; > + struct drm_plane *primary_plane; > int i; > = > intel_crtc =3D kzalloc(sizeof(*intel_crtc), GFP_KERNEL); > if (intel_crtc =3D=3D NULL) > return; > = > + primary_plane =3D kzalloc(sizeof(*primary_plane), GFP_KERNEL); > + if (primary_plane =3D=3D NULL) { > + kfree(intel_crtc); > + return; > + } > + > drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs); > + drm_plane_set_primary(dev, primary_plane, &intel_crtc->base, > + &intel_primary_plane_funcs, NULL, 0); > = > drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256); > for (i =3D 0; i < 256; i++) { > -- = > 1.8.5.1 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC