From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset. Date: Tue, 5 Nov 2013 13:47:04 +0200 Message-ID: <20131105114704.GF13047@intel.com> References: <1379079331-19405-1-git-send-email-sandeepx.ramakutty@intel.com> <20130913142608.GC4531@intel.com> <9FFD648774C2FE4CBFB1FC2607336026095D3C@BGSMSX102.gar.corp.intel.com> <20131031122405.GC13047@intel.com> <9FFD648774C2FE4CBFB1FC260733602609608D@BGSMSX102.gar.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 1AD16EFB2D for ; Tue, 5 Nov 2013 03:47:08 -0800 (PST) Content-Disposition: inline In-Reply-To: <9FFD648774C2FE4CBFB1FC260733602609608D@BGSMSX102.gar.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: "Ramakutty, SandeepX" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Tue, Nov 05, 2013 at 07:00:14AM +0000, Ramakutty, SandeepX wrote: > Hi Ville, > = > Thanks for the information. Here MI_LOAD_REGISTER_IMM is used and not MI_= LOAD_REGISTER_MEM. Is SRM needed after MI_LOAD_REGISTER_IMM? BSpec for MI_L= OAD_REGISTER_IMM does not mention any thing about using SRM, but it is the= re for MI_LOAD_REGISTER_MEM. = Just checked BSpec and it's still there: "Limited LRI cycles to the Display Engine 0x40000-0xBFFFF) are allowed, but must be spaced to allow only one pending at a time. This can be done by issuing an SRM to the same address immediately after each LRI." > = > Regards, > Sandeep > = > -----Original Message----- > From: Ville Syrj=E4l=E4 [mailto:ville.syrjala@linux.intel.com] = > Sent: Thursday, October 31, 2013 5:54 PM > To: Ramakutty, SandeepX > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add support to change pixel fo= rmat, tiling mode, tiling offset. > = > On Thu, Oct 31, 2013 at 12:15:17PM +0000, Ramakutty, SandeepX wrote: > > Hi Ville , > > = > > Thanks for the feedback. > > = > > We verified without updating watermarks and found that the video playba= ck, 3DMark and GLBenchmark plays fine. There was no underrun error too. > > Cases tried out- > > Pixel format changed to 32BPP when water mark calculated based on = > > 16BPP Pixel format changed to 16BPP when water mark calculated based on= 32BPP. > > = > > In both cases, we verified with video playback, 3DMark and GLBenchmark = and did not see any underrun error. Is there any specific situations in wh= ich not setting WM can throw up an issue? = > > = > > With MMIO there may be a possibility of flickering. The pixel format ma= y be updated well before a page flip. With MMIO, the format will be written= to register before a page flip and this can cause unwanted effects. Hence= we used MI commands as this will get updated much nearer to a page flip. > > = > > What is SRM? > = > "store register mem". It reads the register and stores the result to memo= ry. > = > BSpec tells us that there can only be one outstanding LRI targeted at the= display registers at any given. So you need to do LRI+SRM for every displa= y register you want to write w/ LRI. > = > > = > > Regards, > > Sandeep > > = > > -----Original Message----- > > From: Ville Syrj=E4l=E4 [mailto:ville.syrjala@linux.intel.com] > > Sent: Friday, September 13, 2013 7:56 PM > > To: Ramakutty, SandeepX > > Cc: intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add support to change pixel = format, tiling mode, tiling offset. > > = > > On Fri, Sep 13, 2013 at 07:05:31PM +0530, Sandeep Ramankutty wrote: > > > Change: Add support to change the pixel format, base surface = > > > address, tiling mode, tiling offset on the flow during the primary pl= ane flip. > > > = > > > Issue: This support is needed by hardware composer to meet the = > > > performance optimization requirement for 3D benchmark. This patch = > > > enables change in pixel format and tiling params without adding and r= emoving the framebuffer. > > > Adding and removing the framebuffer impacts performance. > > = > > We can't do it quite like this unfortunately. Watermarksm may need to b= e updated carefully if the bpp changes. Also updating multiple plane regist= ers isn't always atomic, so we anyway need the vblank evade tricks to make = it look pretty. I suppose it might be possible to make all that work via th= e ring, but my plan is to go for MMIO, and later investigate if we can opti= mize it by using the ring. > > = > > As far as the lowlevel details go, you supposedly need a SRM after each= LRI aimed at the display registers. Otherwise I guess you could even blast= all the registers with a single LRI. > > = > > > Change Details- > > > drm: Defined function pointer set_pixelformat in drm_crtc_funcs. = > > > drm_crtc.c is modified to execute the function to update pixel format. > > > drm/i915: intel_crtc_set_pixel_format() implemented in i915 driver = > > > code (intel_display.c) and set to drm_crtc_funcs->set_pixelformat. > > > haswell_set_pixelformat() implemented to update pixel format. This = > > > is specific to Haswell > > > haswell_update_plane() implemented to update changes in tiling = > > > offset, surface base address and stride-fb->pitches[0]. Sends MI = > > > command to update the params in primary plane control registers. > > > = > > > Change-Id: I4b0af83f641b2b79ee4f5c3401790d7d90ccd9ef > > > Signed-off-by: Pallavi G > > > Signed-off-by: Sandeep Ramakutty > > > --- > > > drivers/gpu/drm/drm_crtc.c | 15 +++- > > > drivers/gpu/drm/i915/intel_display.c | 156 ++++++++++++++++++++++++= ++++++++++ > > > include/drm/drm_crtc.h | 8 ++ > > > 3 files changed, 176 insertions(+), 3 deletions(-) > > > = > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c = > > > index c9f9f3d..4739b6c 100644 > > > --- a/drivers/gpu/drm/drm_crtc.c > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > @@ -3518,9 +3518,18 @@ int drm_mode_page_flip_ioctl(struct drm_device= *dev, > > > } > > > = > > > if (crtc->fb->pixel_format !=3D fb->pixel_format) { > > > - DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer for= mat.\n"); > > > - ret =3D -EINVAL; > > > - goto out; > > > + if (crtc->funcs->set_pixelformat =3D=3D NULL) { > > > + DRM_DEBUG_KMS("Pixel format change not allowed"); > > > + ret =3D -EINVAL; > > > + goto out; > > > + } > > > + /* supports dynamic change in pixel format */ > > > + ret =3D crtc->funcs->set_pixelformat(crtc, fb->pixel_format); > > > + if (ret) { > > > + DRM_DEBUG_KMS("Pixel format change failed %d", > > > + fb->pixel_format); > > > + goto out; > > > + } > > > } > > > = > > > if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { diff --git = > > > a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 955df91..7bfe088 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -45,6 +45,7 @@ bool intel_pipe_has_type(struct drm_crtc *crtc, = > > > int type); static void intel_increase_pllclock(struct drm_crtc = > > > *crtc); static void intel_crtc_update_cursor(struct drm_crtc *crtc, = > > > bool on); > > > = > > > + > > > typedef struct { > > > int min, max; > > > } intel_range_t; > > > @@ -1959,6 +1960,128 @@ static int i9xx_update_plane(struct drm_crtc = *crtc, struct drm_framebuffer *fb, > > > return 0; > > > } > > > = > > > +/* Set Pixel format for Haswell using MI commands */ static int = > > > +haswell_set_pixelformat(struct drm_crtc *crtc, u32 pixel_format) { > > > + u32 dspcntr, reg; > > > + 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 intel_ring_buffer *ring =3D &dev_priv->ring[BCS]; > > > + int ret =3D 0; > > > + > > > + reg =3D DSPCNTR(intel_crtc->pipe); > > > + dspcntr =3D I915_READ(reg); > > > + DRM_DEBUG_DRIVER("pixel format =3D %d\n", pixel_format); > > > + /* Mask out pixel format bits in case we change it */ > > > + dspcntr &=3D ~DISPPLANE_PIXFORMAT_MASK; > > > + switch (pixel_format) { > > > + case DRM_FORMAT_C8: > > > + dspcntr |=3D DISPPLANE_8BPP; > > > + break; > > > + case DRM_FORMAT_XRGB1555: > > > + case DRM_FORMAT_ARGB1555: > > > + dspcntr |=3D DISPPLANE_BGRX555; > > > + break; > > > + case DRM_FORMAT_RGB565: > > > + dspcntr |=3D DISPPLANE_BGRX565; > > > + break; > > > + case DRM_FORMAT_XRGB8888: > > > + case DRM_FORMAT_ARGB8888: > > > + dspcntr |=3D DISPPLANE_BGRX888; > > > + break; > > > + case DRM_FORMAT_XBGR8888: > > > + case DRM_FORMAT_ABGR8888: > > > + dspcntr |=3D DISPPLANE_RGBX888; > > > + break; > > > + case DRM_FORMAT_XRGB2101010: > > > + case DRM_FORMAT_ARGB2101010: > > > + dspcntr |=3D DISPPLANE_BGRX101010; > > > + break; > > > + case DRM_FORMAT_XBGR2101010: > > > + case DRM_FORMAT_ABGR2101010: > > > + dspcntr |=3D DISPPLANE_RGBX101010; > > > + break; > > > + default: > > > + DRM_ERROR("Unsupported pixel format 0x%08x\n", pixel_format); > > > + return -EINVAL; > > > + } > > > + > > > + /* Write pixel format update command to ring */ > > > + ret =3D intel_ring_begin(ring, 4); > > > + if (ret) { > > > + DRM_ERROR("MI Command emit failed.\n"); > > > + return ret; > > > + } > > > + intel_ring_emit(ring, MI_NOOP); > > > + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1)); > > > + intel_ring_emit(ring, reg); > > > + intel_ring_emit(ring, dspcntr); > > > + intel_ring_advance(ring); > > > + return 0; > > > +} > > > + > > > +/* Update the primary plane registers. Uses MI commands */ static = > > > +int haswell_update_plane(struct drm_crtc *crtc, > > > + struct drm_framebuffer *fb, int x, int y) { > > > + int ret; > > > + 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 intel_ring_buffer *ring =3D &dev_priv->ring[BCS]; > > > + struct drm_i915_gem_object *obj; > > > + int plane =3D intel_crtc->plane; > > > + > > > + switch (plane) { > > > + case 0: > > > + case 1: > > > + case 2: > > > + break; > > > + default: > > > + DRM_ERROR("Can't update plane %c\n", plane_name(plane)); > > > + return -EINVAL; > > > + } > > > + obj =3D to_intel_framebuffer(fb)->obj; > > > + > > > + /* Set pixel format */ > > > + haswell_set_pixelformat(crtc, fb->pixel_format); > > > + > > > + /* Set tiling offsets. Tiling mode is not set here as * > > > + * it is set from intel_gen7_queue_flip. Send MI Command * > > > + * to change - * > > > + * 1. Tiling offset * > > > + * 2. stride - fb->pitches[0] * > > > + * 2. surface base address * > > > + * Linear offset and tile offset is same for Haswell */ > > > + intel_crtc->dspaddr_offset =3D > > > + intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode, > > > + fb->bits_per_pixel / 8, > > > + fb->pitches[0]); > > > + > > > + DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n", > > > + obj->gtt_offset, intel_crtc->dspaddr_offset, > > > + x, y, fb->pitches[0]); > > > + > > > + /* Emit MI commands here */ > > > + ret =3D intel_ring_begin(ring, 10); > > > + if (ret) > > > + return ret; > > > + intel_ring_emit(ring, MI_NOOP); > > > + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1)); > > > + intel_ring_emit(ring, DSPOFFSET(plane)); > > > + intel_ring_emit(ring, (y << 16) | x); > > > + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1)); > > > + intel_ring_emit(ring, DSPSTRIDE(plane)); > > > + intel_ring_emit(ring, fb->pitches[0]); > > > + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1)); > > > + intel_ring_emit(ring, DSPSURF(plane)); > > > + intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset); > > > + intel_ring_advance(ring); > > > + > > > + return 0; > > > +} > > > + > > > static int ironlake_update_plane(struct drm_crtc *crtc, > > > struct drm_framebuffer *fb, int x, int y) { @@ -7494,9 = > > > +7617,26 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > > > struct drm_framebuffer *old_fb =3D crtc->fb; > > > struct drm_i915_gem_object *obj =3D to_intel_framebuffer(fb)->obj; > > > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > > > + struct intel_framebuffer *intel_fb; > > > struct intel_unpin_work *work; > > > unsigned long flags; > > > int ret; > > > + /* > > > + * TILEOFF registers can't be changed via MI display flips. > > > + * Changes in pitches[0] and offsets[0] updates the primary plane > > > + * control registers. > > > + */ > > > + intel_fb =3D to_intel_framebuffer(crtc->fb); > > > + if ((IS_HASWELL(dev)) && > > > + ((obj->tiling_mode !=3D intel_fb->obj->tiling_mode) || > > > + (fb->offsets[0] !=3D crtc->fb->offsets[0]) || > > > + (fb->pitches[0] !=3D crtc->fb->pitches[0]))) { > > > + DRM_DEBUG_DRIVER(" crtc fb: pitch =3D %d offset =3D %d\n", > > > + crtc->fb->pitches[0], crtc->fb->offsets[0]); > > > + DRM_DEBUG_DRIVER(" input fb: pitch =3D %d offset =3D %d\n", > > > + fb->pitches[0], fb->offsets[0]); > > > + haswell_update_plane(crtc, fb, 0, 0); > > > + } > > > = > > > work =3D kzalloc(sizeof *work, GFP_KERNEL); > > > if (work =3D=3D NULL) > > > @@ -8766,6 +8906,20 @@ out_config: > > > return ret; > > > } > > > = > > > +/* Callback function - Called if change in pixel format is detected. > > > +* Sends MI command to update change in pixel format. > > > +*/ > > > +static int intel_crtc_set_pixel_format(struct drm_crtc *crtc, u32 > > > +pixel_format) { > > > + struct drm_device *dev =3D crtc->dev; > > > + if (IS_HASWELL(dev)) { > > > + return haswell_set_pixelformat(crtc, pixel_format); > > > + } else { > > > + DRM_ERROR("Pixel format change not allowed.\n"); > > > + return -EINVAL; > > > + } > > > +} > > > + > > > static const struct drm_crtc_funcs intel_crtc_funcs =3D { > > > .cursor_set =3D intel_crtc_cursor_set, > > > .cursor_move =3D intel_crtc_cursor_move, @@ -8773,6 +8927,7 @@ = > > > static const struct drm_crtc_funcs intel_crtc_funcs =3D { > > > .set_config =3D intel_crtc_set_config, > > > .destroy =3D intel_crtc_destroy, > > > .page_flip =3D intel_crtc_page_flip, > > > + .set_pixelformat =3D intel_crtc_set_pixel_format, > > > }; > > > = > > > static void intel_cpu_pll_init(struct drm_device *dev) @@ -10180,3 > > > +10335,4 @@ intel_display_print_error_state(struct = > > > +drm_i915_error_state_buf *m, > > > } > > > } > > > #endif > > > + > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index > > > e215bcc..9a0ea92 100644 > > > --- a/include/drm/drm_crtc.h > > > +++ b/include/drm/drm_crtc.h > > > @@ -323,6 +323,7 @@ struct drm_plane; > > > * @set_property: called when a property is changed > > > * @set_config: apply a new CRTC configuration > > > * @page_flip: initiate a page flip > > > + * @set_pixelformat: apply new pixel format to primary plane = > > > + control register > > > * > > > * The drm_crtc_funcs structure is the central CRTC management struc= ture > > > * in the DRM. Each CRTC controls one or more connectors (note = > > > that the name @@ -369,6 +370,13 @@ struct drm_crtc_funcs { > > > = > > > int (*set_property)(struct drm_crtc *crtc, > > > struct drm_property *property, uint64_t val); > > > + /* > > > + * Update the primary plane pixel format register during page flip. > > > + * To support dynamic change in pixel format define the callback > > > + * function for set_pixelformat. > > > + */ > > > + int (*set_pixelformat)(struct drm_crtc *crtc, > > > + uint32_t pixel_format); > > > }; > > > = > > > /** > > > -- > > > 1.7.9.5 > > > = > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > = > > -- > > Ville Syrj=E4l=E4 > > Intel OTC > = > -- > Ville Syrj=E4l=E4 > Intel OTC -- = Ville Syrj=E4l=E4 Intel OTC