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_LOAD_REGISTER_IMM does not mention any thing about using SRM, but it is > there 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älä [mailto:ville.syrj...@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 > format, 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 playback, > > 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 which > > not setting WM can throw up an issue? > > > > With MMIO there may be a possibility of flickering. The pixel format may 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 memory. > > 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 display > register you want to write w/ LRI. > > > > > Regards, > > Sandeep > > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrj...@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 plane > > > 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 > > > removing the framebuffer. > > > Adding and removing the framebuffer impacts performance. > > > > We can't do it quite like this unfortunately. Watermarksm may need to be > > updated carefully if the bpp changes. Also updating multiple plane > > registers 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 the ring, but my plan is to go for MMIO, and later investigate if we > > can optimize 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 <pallav...@intel.com> > > > Signed-off-by: Sandeep Ramakutty <sandeepx.ramaku...@intel.com> > > > --- > > > 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 != fb->pixel_format) { > > > - DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer > > > format.\n"); > > > - ret = -EINVAL; > > > - goto out; > > > + if (crtc->funcs->set_pixelformat == NULL) { > > > + DRM_DEBUG_KMS("Pixel format change not allowed"); > > > + ret = -EINVAL; > > > + goto out; > > > + } > > > + /* supports dynamic change in pixel format */ > > > + ret = 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 = crtc->dev; > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > + struct intel_ring_buffer *ring = &dev_priv->ring[BCS]; > > > + int ret = 0; > > > + > > > + reg = DSPCNTR(intel_crtc->pipe); > > > + dspcntr = I915_READ(reg); > > > + DRM_DEBUG_DRIVER("pixel format = %d\n", pixel_format); > > > + /* Mask out pixel format bits in case we change it */ > > > + dspcntr &= ~DISPPLANE_PIXFORMAT_MASK; > > > + switch (pixel_format) { > > > + case DRM_FORMAT_C8: > > > + dspcntr |= DISPPLANE_8BPP; > > > + break; > > > + case DRM_FORMAT_XRGB1555: > > > + case DRM_FORMAT_ARGB1555: > > > + dspcntr |= DISPPLANE_BGRX555; > > > + break; > > > + case DRM_FORMAT_RGB565: > > > + dspcntr |= DISPPLANE_BGRX565; > > > + break; > > > + case DRM_FORMAT_XRGB8888: > > > + case DRM_FORMAT_ARGB8888: > > > + dspcntr |= DISPPLANE_BGRX888; > > > + break; > > > + case DRM_FORMAT_XBGR8888: > > > + case DRM_FORMAT_ABGR8888: > > > + dspcntr |= DISPPLANE_RGBX888; > > > + break; > > > + case DRM_FORMAT_XRGB2101010: > > > + case DRM_FORMAT_ARGB2101010: > > > + dspcntr |= DISPPLANE_BGRX101010; > > > + break; > > > + case DRM_FORMAT_XBGR2101010: > > > + case DRM_FORMAT_ABGR2101010: > > > + dspcntr |= DISPPLANE_RGBX101010; > > > + break; > > > + default: > > > + DRM_ERROR("Unsupported pixel format 0x%08x\n", pixel_format); > > > + return -EINVAL; > > > + } > > > + > > > + /* Write pixel format update command to ring */ > > > + ret = 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 = crtc->dev; > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > + struct intel_ring_buffer *ring = &dev_priv->ring[BCS]; > > > + struct drm_i915_gem_object *obj; > > > + int plane = 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 = 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 = > > > + 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 = 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 = crtc->fb; > > > struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj; > > > struct intel_crtc *intel_crtc = 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 = to_intel_framebuffer(crtc->fb); > > > + if ((IS_HASWELL(dev)) && > > > + ((obj->tiling_mode != intel_fb->obj->tiling_mode) || > > > + (fb->offsets[0] != crtc->fb->offsets[0]) || > > > + (fb->pitches[0] != crtc->fb->pitches[0]))) { > > > + DRM_DEBUG_DRIVER(" crtc fb: pitch = %d offset = %d\n", > > > + crtc->fb->pitches[0], crtc->fb->offsets[0]); > > > + DRM_DEBUG_DRIVER(" input fb: pitch = %d offset = %d\n", > > > + fb->pitches[0], fb->offsets[0]); > > > + haswell_update_plane(crtc, fb, 0, 0); > > > + } > > > > > > work = kzalloc(sizeof *work, GFP_KERNEL); > > > if (work == 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 = 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 = { > > > .cursor_set = intel_crtc_cursor_set, > > > .cursor_move = intel_crtc_cursor_move, @@ -8773,6 +8927,7 @@ > > > static const struct drm_crtc_funcs intel_crtc_funcs = { > > > .set_config = intel_crtc_set_config, > > > .destroy = intel_crtc_destroy, > > > .page_flip = intel_crtc_page_flip, > > > + .set_pixelformat = 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 structure > > > * 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älä > > Intel OTC > > -- > Ville Syrjälä > Intel OTC -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
