On Thu, 2010-09-02 at 15:16 +0800, Chris Wilson wrote: > Comments inline.
Thanks for your comments. > > On Thu, 2 Sep 2010 21:46:54 +0800, "Xiang, Haihao" <haihao.xi...@intel.com> > wrote: > > This ring buffer is used for video decoding/encoding on Sandybridge. > > > > Signed-off-by: Xiang, Haihao <haihao.xi...@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/i915_gem.c | 6 ++- > > drivers/gpu/drm/i915/i915_irq.c | 15 +++-- > > drivers/gpu/drm/i915/i915_reg.h | 22 +++++- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 121 > > +++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > > 6 files changed, 158 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 047cd7c..22d098b 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1203,7 +1203,7 @@ extern void intel_overlay_print_error_state(struct > > seq_file *m, struct intel_ove > > (dev)->pci_device == 0x2A42 || \ > > (dev)->pci_device == 0x2E42) > > > > -#define HAS_BSD(dev) (IS_IRONLAKE(dev) || IS_G4X(dev)) > > +#define HAS_BSD(dev) (IS_IRONLAKE(dev) || IS_G4X(dev) || > > IS_GEN6(dev)) > > Convert HAS_BSD(dev) to INTEL_INFO(dev)->has_bsd_ring and update > capabilities accordingly. I will fix it. > > > #define I915_NEED_GFX_HWS(dev) (INTEL_INFO(dev)->need_gfx_hws) > > > > /* With the 945 and later, Y tiling got adjusted so that it was 32 128-byte > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > b/drivers/gpu/drm/i915/i915_gem.c > > index 8ccb55a..d2c825a 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -4504,7 +4504,11 @@ i915_gem_init_ringbuffer(struct drm_device *dev) > > goto cleanup_pipe_control; > > > > if (HAS_BSD(dev)) { > > - dev_priv->bsd_ring = bsd_ring; > > + if (IS_GEN6(dev)) > > + dev_priv->bsd_ring = gen6_bsd_ring; > > + else > > + dev_priv->bsd_ring = bsd_ring; > > + > > Lets stop exporting these struct initialisers before we end up with one > per generation per ring. Introduce intel_init_render_ring_buffer() and > intel_init_bsd_ring_buffer() and similarly tidy up the HWS page > initialisation. I will fix it. > > > ret = intel_init_ring_buffer(dev, &dev_priv->bsd_ring); > > if (ret) > > goto cleanup_render_ring; > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > b/drivers/gpu/drm/i915/i915_irq.c > > index 16861b8..82e708c 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -312,6 +312,10 @@ irqreturn_t ironlake_irq_handler(struct drm_device > > *dev) > > u32 de_iir, gt_iir, de_ier, pch_iir; > > struct drm_i915_master_private *master_priv; > > struct intel_ring_buffer *render_ring = &dev_priv->render_ring; > > + u32 bsd_usr_interrupt = GT_BSD_USER_INTERRUPT; > > + > > + if (IS_GEN6(dev)) > > + bsd_usr_interrupt = GT_GEN6_BSD_USER_INTERRUPT; > > > > /* disable master interrupt before clearing iir */ > > de_ier = I915_READ(DEIER); > > @@ -342,10 +346,9 @@ irqreturn_t ironlake_irq_handler(struct drm_device > > *dev) > > dev_priv->hangcheck_count = 0; > > mod_timer(&dev_priv->hangcheck_timer, jiffies + > > DRM_I915_HANGCHECK_PERIOD); > > } > > - if (gt_iir & GT_BSD_USER_INTERRUPT) > > + if (gt_iir & bsd_usr_interrupt) > > DRM_WAKEUP(&dev_priv->bsd_ring.irq_queue); > > > > - > > if (de_iir & DE_GSE) > > ironlake_opregion_gse_intr(dev); > > > > @@ -1381,17 +1384,19 @@ static int ironlake_irq_postinstall(struct > > drm_device *dev) > > I915_WRITE(DEIER, dev_priv->de_irq_enable_reg); > > (void) I915_READ(DEIER); > > > > - /* Gen6 only needs render pipe_control now */ > > if (IS_GEN6(dev)) > > - render_mask = GT_PIPE_NOTIFY; > > + render_mask = GT_PIPE_NOTIFY | GT_GEN6_BSD_USER_INTERRUPT; > > > > dev_priv->gt_irq_mask_reg = ~render_mask; > > dev_priv->gt_irq_enable_reg = render_mask; > > > > I915_WRITE(GTIIR, I915_READ(GTIIR)); > > I915_WRITE(GTIMR, dev_priv->gt_irq_mask_reg); > > - if (IS_GEN6(dev)) > > + if (IS_GEN6(dev)) { > > I915_WRITE(GEN6_RENDER_IMR, > > ~GEN6_RENDER_PIPE_CONTROL_NOTIFY_INTERRUPT); > > + I915_WRITE(GEN6_BSD_IMR, ~GEN6_BSD_IMR_USER_INTERRUPT); > > + } > > + > > I915_WRITE(GTIER, dev_priv->gt_irq_enable_reg); > > (void) I915_READ(GTIER); > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 67e3ec1..9d57ecc 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -192,11 +192,11 @@ > > #define MI_STORE_DWORD_INDEX MI_INSTR(0x21, 1) > > #define MI_STORE_DWORD_INDEX_SHIFT 2 > > #define MI_LOAD_REGISTER_IMM MI_INSTR(0x22, 1) > > +#define MI_FLUSH_DW MI_INSTR(0x26, 2) /* for GEN6 */ > > Random new abbreviation. Use MI_FLUSH_DWORD for consistency. MI_FLUSH_DW is a MI command and used by the document. > > > #define MI_BATCH_BUFFER MI_INSTR(0x30, 1) > > #define MI_BATCH_NON_SECURE (1) > > #define MI_BATCH_NON_SECURE_I965 (1<<8) > > #define MI_BATCH_BUFFER_START MI_INSTR(0x31, 0) > > - > > /* > > * 3D instructions used by the kernel > > */ > > @@ -476,6 +476,24 @@ > > #define BSD_HWS_PGA 0x04080 > > > > /* > > + * video command stream instruction and interrupt control register defines > > + * for GEN6 > > + */ > > +#define GEN6_BSD_RING_TAIL 0x12030 > > +#define GEN6_BSD_RING_HEAD 0x12034 > > +#define GEN6_BSD_RING_START 0x12038 > > +#define GEN6_BSD_RING_CTL 0x1203c > > +#define GEN6_BSD_RING_ACTHD 0x12074 > > +#define GEN6_BSD_HWS_PGA 0x14080 > > + > > +#define GEN6_BSD_SLEEP_PSMI_CONTROL 0x12050 > > +#define GEN6_BSD_SLEEP_PSMI_CONTROL_IDLE_INDICATOR (1 << 3) > > + > > +#define GEN6_BSD_IMR 0x120a8 > > +#define GEN6_BSD_IMR_USER_INTERRUPT (1 << 12) > > +#define GEN6_BSD_RNCID 0x12198 > > + > > +/* > > * Framebuffer compression (915+ only) > > */ > > > > @@ -2507,7 +2525,7 @@ > > #define GT_SYNC_STATUS (1 << 2) > > #define GT_USER_INTERRUPT (1 << 0) > > #define GT_BSD_USER_INTERRUPT (1 << 5) > > - > > +#define GT_GEN6_BSD_USER_INTERRUPT (1 << 12) > > > > #define GTISR 0x44010 > > #define GTIMR 0x44014 > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > > b/drivers/gpu/drm/i915/intel_ringbuffer.c > > index dda2751..4c0f8c4 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > @@ -32,6 +32,7 @@ > > #include "i915_drv.h" > > #include "i915_drm.h" > > #include "i915_trace.h" > > +#include "intel_drv.h" > > > > static u32 i915_gem_get_seqno(struct drm_device *dev) > > { > > @@ -874,3 +875,123 @@ struct intel_ring_buffer bsd_ring = { > > .status_page = {NULL, 0, NULL}, > > .map = {0,} > > }; > > + > > +static void gen6_bsd_setup_status_page(struct drm_device *dev, > > + struct intel_ring_buffer *ring) > > +{ > > + drm_i915_private_t *dev_priv = dev->dev_private; > > + I915_WRITE(GEN6_BSD_HWS_PGA, ring->status_page.gfx_addr); > > + I915_READ(GEN6_BSD_HWS_PGA); > > +} > > + > > +static inline unsigned int gen6_bsd_ring_get_head(struct drm_device *dev, > > + struct intel_ring_buffer *ring) > > +{ > > + drm_i915_private_t *dev_priv = dev->dev_private; > > + return I915_READ(GEN6_BSD_RING_HEAD) & HEAD_ADDR; > > +} > > + > > +static inline unsigned int gen6_bsd_ring_get_tail(struct drm_device *dev, > > + struct intel_ring_buffer *ring) > > +{ > > + drm_i915_private_t *dev_priv = dev->dev_private; > > + return I915_READ(GEN6_BSD_RING_TAIL) & TAIL_ADDR; > > +} > > + > > +static inline void gen6_bsd_ring_set_tail(struct drm_device *dev, > > + u32 value) > > +{ > > + drm_i915_private_t *dev_priv = dev->dev_private; > > + > > + /* Every tail move must follow the sequence below */ > > + I915_WRITE(GEN6_BSD_SLEEP_PSMI_CONTROL, 0x00010001); > > Magic number. (1 << 16) | (1 << 0) would be better just to distinguish > between the mask and enable bits (guess). Actually naming the bits for > that register would be preferable. I will use some macros instead. > > > + I915_WRITE(GEN6_BSD_RNCID, 0x00000000); > > + > > + if (wait_for((I915_READ(GEN6_BSD_SLEEP_PSMI_CONTROL) & > > + GEN6_BSD_SLEEP_PSMI_CONTROL_IDLE_INDICATOR) == > > 0, > > + 50, 0)) > > + DRM_ERROR("timed out waiting for IDLE Indicator\n"); > > + > > + I915_WRITE(GEN6_BSD_RING_TAIL, value); > > + I915_WRITE(GEN6_BSD_SLEEP_PSMI_CONTROL, 0x00010000); > > Do you need to clear the IDLE_INDICATOR here as well? IDLE_INDICATOR is a read-only field. > > > +} > > + > > +static inline unsigned int gen6_bsd_ring_get_active_head(struct drm_device > > *dev, > > + struct intel_ring_buffer *ring) > > +{ > > + drm_i915_private_t *dev_priv = dev->dev_private; > > + return I915_READ(GEN6_BSD_RING_ACTHD); > > +} > > + > > +static inline void gen6_bsd_ring_advance_ring(struct drm_device *dev, > > + struct intel_ring_buffer *ring) > > +{ > > + gen6_bsd_ring_set_tail(dev, ring->tail); > > +} > > + > > +static void gen6_bsd_ring_flush(struct drm_device *dev, > > + struct intel_ring_buffer *ring, > > + u32 invalidate_domains, > > + u32 flush_domains) > > +{ > > + intel_ring_begin(dev, ring, 4); > > + intel_ring_emit(dev, ring, MI_FLUSH_DW); > > + intel_ring_emit(dev, ring, 0); > > + intel_ring_emit(dev, ring, 0); > > + intel_ring_emit(dev, ring, 0); > > + intel_ring_advance(dev, ring); > > +} > > + > > +static int > > +gen6_bsd_ring_dispatch_gem_execbuffer(struct drm_device *dev, > > + struct intel_ring_buffer *ring, > > + struct drm_i915_gem_execbuffer2 *exec, > > + struct drm_clip_rect *cliprects, > > + uint64_t exec_offset) > > +{ > > + uint32_t exec_start; > > + exec_start = (uint32_t) exec_offset + exec->batch_start_offset; > > + intel_ring_begin(dev, ring, 2); > > + intel_ring_emit(dev, ring, MI_BATCH_BUFFER_START | > > MI_BATCH_NON_SECURE_I965); /* bit0-7 is the length on GEN6+ */ > > + intel_ring_emit(dev, ring, exec_start); > > + intel_ring_advance(dev, ring); > > + return 0; > > +} > > + > > +/* ring buffer for Video Codec for Gen6+ */ > > +struct intel_ring_buffer gen6_bsd_ring = { > > + .name = "gen6 bsd ring", > > + .regs = { > > + .ctl = GEN6_BSD_RING_CTL, > > + .head = GEN6_BSD_RING_HEAD, > > + .tail = GEN6_BSD_RING_TAIL, > > + .start = GEN6_BSD_RING_START > > + }, > > + .ring_flag = I915_EXEC_BSD, > > + .size = 32 * PAGE_SIZE, > > + .alignment = PAGE_SIZE, > > + .virtual_start = NULL, > > + .dev = NULL, > > + .gem_object = NULL, > > + .head = 0, > > + .tail = 0, > > + .space = 0, > > + .user_irq_refcount = 0, > > + .irq_gem_seqno = 0, > > + .waiting_gem_seqno = 0, > > + .setup_status_page = gen6_bsd_setup_status_page, > > + .init = init_bsd_ring, > > + .get_head = gen6_bsd_ring_get_head, > > + .get_tail = gen6_bsd_ring_get_tail, > > + .set_tail = gen6_bsd_ring_set_tail, > > + .get_active_head = gen6_bsd_ring_get_active_head, > > + .advance_ring = gen6_bsd_ring_advance_ring, > > + .flush = gen6_bsd_ring_flush, > > + .add_request = bsd_ring_add_request, > > + .get_gem_seqno = bsd_ring_get_gem_seqno, > > + .user_irq_get = bsd_ring_get_user_irq, > > + .user_irq_put = bsd_ring_put_user_irq, > > + .dispatch_gem_execbuffer = gen6_bsd_ring_dispatch_gem_execbuffer, > > + .status_page = {NULL, 0, NULL}, > > + .map = {0,} > > +}; > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h > > b/drivers/gpu/drm/i915/intel_ringbuffer.h > > index 8245261..9d38c33 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > > @@ -129,5 +129,6 @@ u32 intel_ring_get_seqno(struct drm_device *dev, > > > > extern struct intel_ring_buffer render_ring; > > extern struct intel_ring_buffer bsd_ring; > > +extern struct intel_ring_buffer gen6_bsd_ring; > > > > #endif /* _INTEL_RINGBUFFER_H_ */ > > -- > > 1.7.0.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx