From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6) Date: Thu, 15 May 2014 18:52:28 +0300 Message-ID: <20140515155228.GF27580@intel.com> References: <1398877623-16930-1-git-send-email-matthew.d.roper@intel.com> <1398877623-16930-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: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 2CB016EE9C for ; Thu, 15 May 2014 08:52:32 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1398877623-16930-5-git-send-email-matthew.d.roper@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Matt Roper Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Apr 30, 2014 at 10:07:03AM -0700, Matt Roper wrote: > Intel hardware allows the primary plane to be disabled independently of > the CRTC. Provide custom primary plane handling to allow this. > = > v6: > - Pass rectangles to primary helper check function and get plane > visibility back. > - Wait for pending pageflips on primary plane update/disable. > - Allow primary plane to be updated while the crtc is disabled (changes > will take effect when the crtc is re-enabled if modeset passes -1 > for the fb id). > - Drop WARN() if we try to disable the primary plane when it's > already been disabled. This will happen if the crtc gets disabled > after the primary plane has already been disabled independently. > v5: > - Use new drm_primary_helper_check_update() helper function to check > setplane parameter validity. > - Swap primary plane's pipe for pre-gen4 FBC (caught by Ville Syrj=E4l= =E4) > - Cleanup primary plane properly on crtc init failure > v4: > - Don't add a primary_plane field to intel_crtc; that was left over > from a much earlier iteration of this patch series, but is no longer > needed/used now that the DRM core primary plane support has been > merged. > v3: > - Provide gen-specific primary plane format lists (suggested by Daniel > Vetter). > - If the primary plane is already enabled, go ahead and just call the > primary plane helper to do the update (suggested by Daniel Vetter). > - Don't try to disable the primary plane on destruction; the DRM layer > should have already taken care of this for us. > v2: > - Unpin fb properly on primary plane disable > - Provide an Intel-specific set of primary plane formats > - Additional sanity checks on setplane (in line with the checks > currently being done by the DRM core primary plane helper) > = > Signed-off-by: Matt Roper > --- > drivers/gpu/drm/i915/intel_display.c | 194 +++++++++++++++++++++++++++++= +++++- > 1 file changed, 191 insertions(+), 3 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index 04bd821..33ed52d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -39,8 +39,35 @@ > #include "i915_trace.h" > #include > #include > +#include > +#include > #include > = > +/* Primary plane formats supported by all gen */ > +#define COMMON_PRIMARY_FORMATS \ > + DRM_FORMAT_C8, \ > + DRM_FORMAT_RGB565, \ > + DRM_FORMAT_XRGB8888, \ > + DRM_FORMAT_ARGB8888 > + > +/* Primary plane formats for gen <=3D 3 */ > +static const uint32_t intel_primary_formats_gen3[] =3D { nit: I'd call it _gen2 since we usually name things based on the oldest thing that supports it. > + COMMON_PRIMARY_FORMATS, > + DRM_FORMAT_XRGB1555, > + DRM_FORMAT_ARGB1555, > +}; > + > +/* Primary plane formats for gen >=3D 4 */ > +static const uint32_t intel_primary_formats_gen4[] =3D { > + COMMON_PRIMARY_FORMATS, \ > + DRM_FORMAT_XBGR8888, > + DRM_FORMAT_ABGR8888, > + DRM_FORMAT_XRGB2101010, > + DRM_FORMAT_ARGB2101010, > + DRM_FORMAT_XBGR2101010, > + DRM_FORMAT_ABGR2101010, > +}; > + > static void intel_increase_pllclock(struct drm_crtc *crtc); > static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on); > = > @@ -1909,7 +1936,8 @@ static void intel_disable_primary_hw_plane(struct d= rm_i915_private *dev_priv, > int reg; > u32 val; > = > - WARN(!intel_crtc->primary_enabled, "Primary plane already disabled\n"); > + if (!intel_crtc->primary_enabled) > + return; > = > intel_crtc->primary_enabled =3D false; > = > @@ -10551,17 +10579,177 @@ 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_disable(struct drm_plane *plane) > +{ > + struct drm_device *dev =3D plane->dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + struct intel_plane *intel_plane =3D to_intel_plane(plane); > + struct intel_crtc *intel_crtc; > + > + if (!plane->fb) > + return 0; > + > + BUG_ON(!plane->crtc); > + > + intel_crtc =3D to_intel_crtc(plane->crtc); > + > + /* > + * Even though we checked plane->fb above, it's still possible that > + * the primary plane has been implicitly disabled because we detected > + * that it was 100% covered by a sprite plane. Or, the CRTC may be > + * off and we've set a fb, but haven't actually turned on the CRTC yet. > + * In either case, we need to unpin the FB and let the fb pointer get > + * updated, but otherwise we don't need to touch the hardware. > + */ > + if (!intel_crtc->primary_enabled) > + goto disable_unpin; > + > + intel_crtc_wait_for_pending_flips(plane->crtc); > + intel_disable_primary_hw_plane(dev_priv, intel_plane->plane, > + intel_plane->pipe); > + > +disable_unpin: > + /* > + * N.B. The DRM setplane code will update the plane->fb pointer after > + * we finish here. > + */ > + intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj); > + > + return 0; > +} > + > +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; > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > + struct drm_rect dest =3D { > + /* integer pixels */ > + .x1 =3D crtc_x, > + .y1 =3D crtc_y, > + .x2 =3D crtc_x + crtc_w, > + .y2 =3D crtc_y + crtc_h, > + }; > + struct drm_rect src =3D { > + /* 16.16 fixed point */ > + .x1 =3D src_x, > + .y1 =3D src_y, > + .x2 =3D src_x + src_w, > + .y2 =3D src_y + src_h, > + }; > + const struct drm_rect clip =3D { > + /* integer pixels */ > + .x2 =3D intel_crtc->config.pipe_src_w, > + .y2 =3D intel_crtc->config.pipe_src_h, These might contain garbage if the pipe isn't enabled. The sprite code initializes the clip size to 0 in that case, which neatly results in visible=3D=3Dfalse always. > + }; > + bool visible; > + int ret; > + > + ret =3D 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=3D-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!=3DNULL when it's not pinned, we'll be in deep doodoo. > + > + intel_crtc_wait_for_pending_flips(crtc); > + ret =3D 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 =3D to_intel_plane(plane); > + drm_plane_cleanup(plane); > + kfree(intel_plane); > +} > + > +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 struct drm_plane *intel_primary_plane_create(struct drm_device *d= ev, > + int pipe) > +{ > + struct intel_plane *primary; > + const uint32_t *intel_primary_formats; > + int num_formats; > + > + primary =3D kzalloc(sizeof(*primary), GFP_KERNEL); > + if (primary =3D=3D NULL) > + return NULL; > + > + primary->can_scale =3D false; > + primary->max_downscale =3D 1; > + primary->pipe =3D pipe; > + primary->plane =3D pipe; > + if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > + primary->plane =3D !pipe; > + > + if (INTEL_INFO(dev)->gen <=3D 3) { > + intel_primary_formats =3D intel_primary_formats_gen3; > + num_formats =3D ARRAY_SIZE(intel_primary_formats_gen3); > + } else { > + intel_primary_formats =3D intel_primary_formats_gen4; > + num_formats =3D 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 =3D dev->dev_private; > struct intel_crtc *intel_crtc; > - int i; > + struct drm_plane *primary; > + int i, ret; > = > intel_crtc =3D kzalloc(sizeof(*intel_crtc), GFP_KERNEL); > if (intel_crtc =3D=3D NULL) > return; > = > - drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs); > + primary =3D intel_primary_plane_create(dev, pipe); > + ret =3D 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 =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