Re: [Intel-gfx] looks like a issue in do_switch() and mi_set_context() in i915_gem_context.c?
On Wed, Apr 01, 2015 at 08:01:56PM +0800, Zhi Wang wrote: > Hi Chris: > Thanks for the reply. :) I can understand that the backing > storage is pinned at this time, as the reference counter of context > object should not be zero. But for VMA, is there any chance that the > vma will be unbinded from GGTT at this time by shrinker? I saw that > i915_gem_object_ggtt_unpin() will decrease the VMA reference > counter... In order for the shrinker to evict an active object, it must first wait upon it. (So the shrinker will only do so as a last gasp measure.) Once the vma is unbound, we know that the GPU will have switched contexts away from the vma (because the last request that we waited upon for the vma included the instructions to do the switch away) and so the pages are swappable. This obviously relies on the hardware being correct... As would waiting upon the CCID! -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/vlv: save/restore the power context base reg
On Thu, 02 Apr 2015, Jesse Barnes wrote: > Some BIOSes (e.g. the one on the Minnowboard) don't save/restore this > reg. If it's unlocked, we can just restore the previous value, and if > it's locked (in case the BIOS re-programmed it for us) the write will be > ignored and we'll still have "did it move" sanity check in the PM code to > warn us if something is still amiss. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=89611 Both patches Tested-by: Darren Hart Cc: sta...@vger.kernel.org > Signed-off-by: Jesse Barnes > --- > drivers/gpu/drm/i915/i915_drv.c | 2 ++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index c1a3cdb5..4d6d6f0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1104,6 +1104,7 @@ static void vlv_save_gunit_s0ix_state(struct > drm_i915_private *dev_priv) > /* Gunit-Display CZ domain, 0x182028-0x1821CF */ > s->gu_ctl0 = I915_READ(VLV_GU_CTL0); > s->gu_ctl1 = I915_READ(VLV_GU_CTL1); > + s->pcbr = I915_READ(VLV_PCBR); > s->clock_gate_dis2 = I915_READ(VLV_GUNIT_CLOCK_GATE2); > > /* > @@ -1198,6 +1199,7 @@ static void vlv_restore_gunit_s0ix_state(struct > drm_i915_private *dev_priv) > /* Gunit-Display CZ domain, 0x182028-0x1821CF */ > I915_WRITE(VLV_GU_CTL0, s->gu_ctl0); > I915_WRITE(VLV_GU_CTL1, s->gu_ctl1); > + I915_WRITE(VLV_PCBR,s->pcbr); > I915_WRITE(VLV_GUNIT_CLOCK_GATE2, s->clock_gate_dis2); > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b13c552..f3ac683 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -994,6 +994,7 @@ struct vlv_s0ix_state { > /* Display 2 CZ domain */ > u32 gu_ctl0; > u32 gu_ctl1; > + u32 pcbr; > u32 clock_gate_dis2; > }; > > -- > 1.9.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] community code of conduct for intel-gfx
Code of conducts seem to be in the news a bit recently, and I realized that I've never really documented how we run things. It's different from the kernel's overall CodeOfConflict[1] and also differs from the official Intel/OTC one[2] in small details about handling issues. And for completeness there's also the Xorg Foundation event policy[3]. Anyway, I think this is worth clarifying and here it goes. It's simple: Be respectful, open and excellent to each another. Which doesn't mean we want to sacrifice quality to be nice. Striving for technical excellence very much doesn't exclude being excellent to someone else, and in our experience it tends to go hand in hand. Unfortunately things go south occasionally. So if you feel threatened, personally abused or otherwise uncomfortable, even and especially when you didn't participate in a discussion yourself, then please raise this in private with the drm/i915 maintainers (currently Daniel Vetter and Jani Nikula, see MAINTAINERS[4] for contact information). And the "in private" part is important: Humans screw up, disciplining minor fumbles by tarnishing someones google-able track record forever is out of proportion. Still there are some teeth to this code of conduct: 1. First time around minor issues will be raised in private. 2. On repeat cases a public reply in the discussion will enforce that respectful behavior is expected. 3. We'll ban people who don't get it. And severe cases will be escalated much quicker. This applies to all community communication channels (irc, mailing list and bugzilla). And as mentioned this really just is a public clarification of the rules already in place - you can't see that though since we never had to go further than step 1. Let's keep it at that. And in case you have a problem with an individual drm/i915 maintainer and don't want to raise it with the other one there's the Xorg BoD[5], linux foundation TAB[6] and the drm upstream maintainer Dave Airlie[4]. 1: https://www.kernel.org/doc/Documentation/CodeOfConflict 2: https://01.org/linuxgraphics/community 3: http://www.x.org/wiki/XorgFoundation/Policies/Harassment/ 4: https://www.kernel.org/doc/linux/MAINTAINERS 5: http://www.x.org/wiki/BoardOfDirectors/ 6: http://www.linuxfoundation.org/programs/advisory-councils/tab -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/vlv: save/restore the power context base reg
On Wed, 2015-04-01 at 14:22 -0700, Jesse Barnes wrote: > Some BIOSes (e.g. the one on the Minnowboard) don't save/restore this > reg. If it's unlocked, we can just restore the previous value, and if > it's locked (in case the BIOS re-programmed it for us) the write will be > ignored and we'll still have "did it move" sanity check in the PM code to > warn us if something is still amiss. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=89611 > Signed-off-by: Jesse Barnes Yea, the BIOS is supposed to program this reg according to the Gunit doc, but we can't have any guarantee anyway.. I guess this is a problem only for S3, but I can't see any problem saving/restoring it also during s0ix, so: Reviewed-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_drv.c | 2 ++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index c1a3cdb5..4d6d6f0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1104,6 +1104,7 @@ static void vlv_save_gunit_s0ix_state(struct > drm_i915_private *dev_priv) > /* Gunit-Display CZ domain, 0x182028-0x1821CF */ > s->gu_ctl0 = I915_READ(VLV_GU_CTL0); > s->gu_ctl1 = I915_READ(VLV_GU_CTL1); > + s->pcbr = I915_READ(VLV_PCBR); > s->clock_gate_dis2 = I915_READ(VLV_GUNIT_CLOCK_GATE2); > > /* > @@ -1198,6 +1199,7 @@ static void vlv_restore_gunit_s0ix_state(struct > drm_i915_private *dev_priv) > /* Gunit-Display CZ domain, 0x182028-0x1821CF */ > I915_WRITE(VLV_GU_CTL0, s->gu_ctl0); > I915_WRITE(VLV_GU_CTL1, s->gu_ctl1); > + I915_WRITE(VLV_PCBR,s->pcbr); > I915_WRITE(VLV_GUNIT_CLOCK_GATE2, s->clock_gate_dis2); > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b13c552..f3ac683 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -994,6 +994,7 @@ struct vlv_s0ix_state { > /* Display 2 CZ domain */ > u32 gu_ctl0; > u32 gu_ctl1; > + u32 pcbr; > u32 clock_gate_dis2; > }; > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] topic/drm-fixes
Hi Dave - Here's a single drm core fix, cc: stable, that affects i915 users. Picked it up myself as explained in [1]. I'll still send a separate drm/i915 pull request. BR, Jani. [1] http://mid.gmane.org/874mp6r6bk@intel.com The following changes since commit bc465aa9d045feb0e13b4a8f32cc33c1943f62d6: Linux 4.0-rc5 (2015-03-22 16:50:21 -0700) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/topic/drm-fixes-2015-04-02 for you to fetch changes up to ad692b46dbf122ef90aadce3b389ef64c90e861d: drm/edid: set ELD for firmware and debugfs override EDIDs (2015-03-27 13:27:04 +0200) Jani Nikula (1): drm/edid: set ELD for firmware and debugfs override EDIDs drivers/gpu/drm/drm_edid_load.c| 1 + drivers/gpu/drm/drm_probe_helper.c | 1 + 2 files changed, 2 insertions(+) -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/vlv: remove wait for previous GFX clk disable request
On Wed, 2015-04-01 at 14:22 -0700, Jesse Barnes wrote: > Looks like it was introduced in: > > commit 650ad970a39f8b6164fe8613edc150f585315289 > Author: Imre Deak > Date: Fri Apr 18 16:35:02 2014 +0300 > > drm/i915: vlv: factor out vlv_force_gfx_clock and check for pending > force-of > > but I'm not sure why. It has caused problems for us in the past (see > 85250ddff7a603dfe0ec0503a9e6395f79424f61 and > 8d4eee9cd7a170342dc6fbc2ee19ae77031a8cd5) and doesn't seem to be > required, so let's just drop it. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=89611 > Signed-off-by: Jesse Barnes > --- > drivers/gpu/drm/i915/i915_drv.c | 14 -- > 1 file changed, 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 4d6d6f0..c3fdbb0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1208,21 +1208,7 @@ int vlv_force_gfx_clock(struct drm_i915_private > *dev_priv, bool force_on) > u32 val; > int err; > > - val = I915_READ(VLV_GTLC_SURVIVABILITY_REG); > - > #define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT) > - /* Wait for a previous force-off to settle */ > - if (force_on && !IS_CHERRYVIEW(dev_priv->dev)) { > - /* WARN_ON only for the Valleyview */ > - WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on); > - > - err = wait_for(!COND, 20); > - if (err) { > - DRM_ERROR("timeout waiting for GFX clock force-off > (%08x)\n", > - I915_READ(VLV_GTLC_SURVIVABILITY_REG)); > - return err; > - } > - } The reason I added this is that it's not clear what happens if you try to force the clock on while the previous force-off operation is still pending. That is if Punit will correctly cancel turning off the clock in this case. Since the docs don't clarify this either I thought the above is safer. Is it the WARN that triggers and only during resume (we also call the function from vlv_set_rps_idle)? If so, then it's not a real timeout but BIOS has left the force-on flag set and we could just skip calling vlv_force_gfx_clock(true) in that case. If the HW people can confirm that the above isn't needed then I'm also ok to remove it. > > val = I915_READ(VLV_GTLC_SURVIVABILITY_REG); > val &= ~VLV_GFX_CLK_FORCE_ON_BIT; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Rip out GET_SPRITE_COLORKEY ioctl
On Fri, 27 Mar 2015, Daniel Vetter wrote: > On Fri, Mar 27, 2015 at 09:10:02AM +0100, Daniel Vetter wrote: >> It's completely unused and Tommi noticed that the #define is borked >> since forever. I've done a git search in userspace and only found >> broken definitions and no users anywhere. >> >> Cc: Tommi Rantala >> Signed-off-by: Daniel Vetter > > Hm Tommi discovered oopses in there, so I guess this should be > cherry-picked to -fixes+cc: stable too? Jani? I'm picking up Ville's fix [1] for the oops to fixes, cc: stable, and I think the rest is -next material. BR, Jani. [1] http://mid.gmane.org/1427479180-29894-1-git-send-email-ville.syrj...@linux.intel.com > -Daniel > >> --- >> drivers/gpu/drm/i915/i915_dma.c | 2 +- >> drivers/gpu/drm/i915/intel_drv.h| 2 -- >> drivers/gpu/drm/i915/intel_sprite.c | 24 >> 3 files changed, 1 insertion(+), 27 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_dma.c >> b/drivers/gpu/drm/i915/i915_dma.c >> index d49ed68f041e..68e0c85a17cf 100644 >> --- a/drivers/gpu/drm/i915/i915_dma.c >> +++ b/drivers/gpu/drm/i915/i915_dma.c >> @@ -1199,7 +1199,7 @@ const struct drm_ioctl_desc i915_ioctls[] = { >> DRM_IOCTL_DEF_DRV(I915_OVERLAY_PUT_IMAGE, intel_overlay_put_image, >> DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), >> DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, >> DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), >> DRM_IOCTL_DEF_DRV(I915_SET_SPRITE_COLORKEY, intel_sprite_set_colorkey, >> DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), >> -DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, intel_sprite_get_colorkey, >> DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), >> +DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, drm_noop, >> DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), >> DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, >> DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), >> DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, >> i915_gem_context_create_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), >> DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, >> i915_gem_context_destroy_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), >> diff --git a/drivers/gpu/drm/i915/intel_drv.h >> b/drivers/gpu/drm/i915/intel_drv.h >> index cb57b9c446f3..6036e3b73b7b 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1282,8 +1282,6 @@ void intel_flush_primary_plane(struct drm_i915_private >> *dev_priv, >> int intel_plane_restore(struct drm_plane *plane); >> int intel_sprite_set_colorkey(struct drm_device *dev, void *data, >>struct drm_file *file_priv); >> -int intel_sprite_get_colorkey(struct drm_device *dev, void *data, >> - struct drm_file *file_priv); >> bool intel_pipe_update_start(struct intel_crtc *crtc, >> uint32_t *start_vbl_count); >> void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count); >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c >> b/drivers/gpu/drm/i915/intel_sprite.c >> index f41e872ad858..e9ff6fc61267 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -1134,30 +1134,6 @@ out_unlock: >> return ret; >> } >> >> -int intel_sprite_get_colorkey(struct drm_device *dev, void *data, >> - struct drm_file *file_priv) >> -{ >> -struct drm_intel_sprite_colorkey *get = data; >> -struct drm_plane *plane; >> -struct intel_plane *intel_plane; >> -int ret = 0; >> - >> -drm_modeset_lock_all(dev); >> - >> -plane = drm_plane_find(dev, get->plane_id); >> -if (!plane) { >> -ret = -ENOENT; >> -goto out_unlock; >> -} >> - >> -intel_plane = to_intel_plane(plane); >> -*get = intel_plane->ckey; >> - >> -out_unlock: >> -drm_modeset_unlock_all(dev); >> -return ret; >> -} >> - >> int intel_plane_restore(struct drm_plane *plane) >> { >> if (!plane->crtc || !plane->state->fb) >> -- >> 2.1.4 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/12] xf86drm: Avoid negative array index value
Variable retcode can be negative as well. Put the correct condition on it before using it as array index. Signed-off-by: Praveen Paneri --- xf86drm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xf86drm.c b/xf86drm.c index e73cddd..1e25424 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -661,7 +661,7 @@ static int drmOpenByName(const char *name, int type) if ((fd = open(proc_name, 0, 0)) >= 0) { retcode = read(fd, buf, sizeof(buf)-1); close(fd); - if (retcode) { + if (retcode > 0) { buf[retcode-1] = '\0'; for (driver = pt = buf; *pt && *pt != ' '; ++pt) ; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/12] xf86drmSL: Check function return value
Validate the return value of SLCreateEntry() before using it. Signed-off-by: Praveen Paneri --- xf86drmSL.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/xf86drmSL.c b/xf86drmSL.c index acddb54..45f3906 100644 --- a/xf86drmSL.c +++ b/xf86drmSL.c @@ -139,6 +139,7 @@ void *drmSLCreate(void) list->magic= SL_LIST_MAGIC; list->level= 0; list->head = SLCreateEntry(SL_MAX_LEVEL, 0, NULL); +if (!list->head) return NULL; list->count= 0; for (i = 0; i <= SL_MAX_LEVEL; i++) list->head->forward[i] = NULL; @@ -205,6 +206,7 @@ int drmSLInsert(void *l, unsigned long key, void *value) } entry = SLCreateEntry(level, key, value); +if (!entry) return -ENOMEM; /* Fix up forward pointers */ for (i = 0; i <= level; i++) { -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/12] intel: Validate output of realloc()
realloc will return NULL if failed to allocate the extra memory requested. Return from function if it fails. Signed-off-by: Praveen Paneri --- intel/intel_bufmgr_gem.c | 37 - 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index 5a67f53..2f0ced1 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -433,6 +433,8 @@ drm_intel_add_validate_buffer(drm_intel_bo *bo) drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr; drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo; int index; + struct drm_i915_gem_exec_object *exec_objects; + drm_intel_bo **exec_bos; if (bo_gem->validate_index != -1) return; @@ -444,12 +446,20 @@ drm_intel_add_validate_buffer(drm_intel_bo *bo) if (new_size == 0) new_size = 5; - bufmgr_gem->exec_objects = - realloc(bufmgr_gem->exec_objects, - sizeof(*bufmgr_gem->exec_objects) * new_size); - bufmgr_gem->exec_bos = - realloc(bufmgr_gem->exec_bos, + exec_objects = realloc(bufmgr_gem->exec_objects, + sizeof(*bufmgr_gem->exec_objects) * new_size); + if (!exec_objects) + return; + + bufmgr_gem->exec_objects = exec_objects; + + exec_bos = realloc(bufmgr_gem->exec_bos, sizeof(*bufmgr_gem->exec_bos) * new_size); + if (!exec_bos) + return; + + bufmgr_gem->exec_bos = exec_bos; + bufmgr_gem->exec_size = new_size; } @@ -471,6 +481,8 @@ drm_intel_add_validate_buffer2(drm_intel_bo *bo, int need_fence) drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bo->bufmgr; drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *)bo; int index; + struct drm_i915_gem_exec_object2 *exec2_objects; + drm_intel_bo **exec_bos; if (bo_gem->validate_index != -1) { if (need_fence) @@ -486,12 +498,19 @@ drm_intel_add_validate_buffer2(drm_intel_bo *bo, int need_fence) if (new_size == 0) new_size = 5; - bufmgr_gem->exec2_objects = - realloc(bufmgr_gem->exec2_objects, + exec2_objects = realloc(bufmgr_gem->exec2_objects, sizeof(*bufmgr_gem->exec2_objects) * new_size); - bufmgr_gem->exec_bos = - realloc(bufmgr_gem->exec_bos, + if (!exec2_objects) + return; + + bufmgr_gem->exec2_objects = exec2_objects; + + exec_bos = realloc(bufmgr_gem->exec_bos, sizeof(*bufmgr_gem->exec_bos) * new_size); + if (!exec_bos) + return; + + bufmgr_gem->exec_bos = exec_bos; bufmgr_gem->exec_size = new_size; } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 00/12] drm: Fix various static analysis issues
This patch set fixes various issues reported by a static analysis tool. Praveen Paneri (12): intel: Validate bo_fake before using. intel: Validate output of realloc() intel: Use snprintf instead of sprintf intel: Validate pointer before using xf86drm: Avoid negative array index value xf86drmSL: Check function return value intel: Validate memory allocations drm: Validate memory allocations xf86drmSL: Check memory allocation by SL_RANDOM_INIT() xf86drmHash: Check memory allocation in HashHash() xf86drm: Validate function return value xf86drmSL: Add missing function call to SLLocate() intel/intel_bufmgr_fake.c | 20 intel/intel_bufmgr_gem.c | 40 ++-- intel/intel_decode.c | 14 ++--- xf86drm.c | 77 --- xf86drmHash.c | 9 ++ xf86drmSL.c | 8 + 6 files changed, 131 insertions(+), 37 deletions(-) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/12] xf86drm: Validate function return value
Return value of drmHashCreate() and drmGetEntry() functions can be NULL. It should be validated before being used. Signed-off-by: Praveen Paneri --- xf86drm.c | 24 +++- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/xf86drm.c b/xf86drm.c index 373113b..d3a002a 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -178,20 +178,25 @@ static unsigned long drmGetKeyFromFd(int fd) drmHashEntry *drmGetEntry(int fd) { unsigned long key = drmGetKeyFromFd(fd); -void *value; +void *value = NULL; drmHashEntry *entry; if (!drmHashTable) drmHashTable = drmHashCreate(); -if (drmHashLookup(drmHashTable, key, &value)) { +if (drmHashTable && drmHashLookup(drmHashTable, key, &value)) { entry = drmMalloc(sizeof(*entry)); if (!entry) return NULL; entry->fd = fd; entry->f= NULL; entry->tagTable = drmHashCreate(); - drmHashInsert(drmHashTable, key, entry); + if (entry->tagTable) { + drmHashInsert(drmHashTable, key, entry); + } else { + drmFree(entry); + entry = NULL; + } } else { entry = value; } @@ -1219,6 +1224,8 @@ int drmClose(int fd) { unsigned long key= drmGetKeyFromFd(fd); drmHashEntry *entry = drmGetEntry(fd); +if(!entry) + return -ENOMEM; drmHashDestroy(entry->tagTable); entry->fd = 0; @@ -2258,6 +2265,8 @@ int drmGetInterruptFromBusID(int fd, int busnum, int devnum, int funcnum) int drmAddContextTag(int fd, drm_context_t context, void *tag) { drmHashEntry *entry = drmGetEntry(fd); +if (!entry) +return -ENOMEM; if (drmHashInsert(entry->tagTable, context, tag)) { drmHashDelete(entry->tagTable, context); @@ -2270,13 +2279,18 @@ int drmDelContextTag(int fd, drm_context_t context) { drmHashEntry *entry = drmGetEntry(fd); -return drmHashDelete(entry->tagTable, context); +if (entry) + return drmHashDelete(entry->tagTable, context); +return -ENOMEM; } void *drmGetContextTag(int fd, drm_context_t context) { -drmHashEntry *entry = drmGetEntry(fd); void *value; +drmHashEntry *entry = drmGetEntry(fd); + +if (!entry) +return NULL; if (drmHashLookup(entry->tagTable, context, &value)) return NULL; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/12] intel: Use snprintf instead of sprintf
We must have upper bound on what we are going to write into a fixed size buffer. Signed-off-by: Praveen Paneri --- intel/intel_decode.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/intel/intel_decode.c b/intel/intel_decode.c index 7d5cbe5..b70d949 100644 --- a/intel/intel_decode.c +++ b/intel/intel_decode.c @@ -767,7 +767,7 @@ static void i915_get_instruction_src0(uint32_t *data, int i, char *srcname) char swizzle[100]; i915_get_instruction_src_name((a0 >> 7) & 0x7, src_nr, srcname); - sprintf(swizzle, ".%s%s%s%s", swizzle_x, swizzle_y, swizzle_z, + snprintf(swizzle, sizeof(swizzle), ".%s%s%s%s", swizzle_x, swizzle_y, swizzle_z, swizzle_w); if (strcmp(swizzle, ".xyzw") != 0) strcat(srcname, swizzle); @@ -785,7 +785,7 @@ static void i915_get_instruction_src1(uint32_t *data, int i, char *srcname) char swizzle[100]; i915_get_instruction_src_name((a1 >> 13) & 0x7, src_nr, srcname); - sprintf(swizzle, ".%s%s%s%s", swizzle_x, swizzle_y, swizzle_z, + snprintf(swizzle, sizeof(swizzle), ".%s%s%s%s", swizzle_x, swizzle_y, swizzle_z, swizzle_w); if (strcmp(swizzle, ".xyzw") != 0) strcat(srcname, swizzle); @@ -802,7 +802,7 @@ static void i915_get_instruction_src2(uint32_t *data, int i, char *srcname) char swizzle[100]; i915_get_instruction_src_name((a2 >> 21) & 0x7, src_nr, srcname); - sprintf(swizzle, ".%s%s%s%s", swizzle_x, swizzle_y, swizzle_z, + snprintf(swizzle, sizeof(swizzle), ".%s%s%s%s", swizzle_x, swizzle_y, swizzle_z, swizzle_w); if (strcmp(swizzle, ".xyzw") != 0) strcat(srcname, swizzle); @@ -931,7 +931,7 @@ i915_decode_dcl(struct drm_intel_decode *ctx, int i, char *instr_prefix) switch ((d0 >> 19) & 0x3) { case 1: - sprintf(dcl_mask, ".%s%s%s%s", dcl_x, dcl_y, dcl_z, dcl_w); + snprintf(dcl_mask, sizeof(dcl_mask), ".%s%s%s%s", dcl_x, dcl_y, dcl_z, dcl_w); if (strcmp(dcl_mask, ".") == 0) fprintf(out, "bad (empty) dcl mask\n"); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/12] intel: Validate bo_fake before using.
Check on bo_fake before dereferencing the object in functions evict_lru and evict_mru. Signed-off-by: Praveen Paneri --- intel/intel_bufmgr_fake.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/intel/intel_bufmgr_fake.c b/intel/intel_bufmgr_fake.c index c4828fa..129d344 100644 --- a/intel/intel_bufmgr_fake.c +++ b/intel/intel_bufmgr_fake.c @@ -557,8 +557,10 @@ evict_lru(drm_intel_bufmgr_fake *bufmgr_fake, unsigned int max_fence) max_fence)) return 0; - set_dirty(&bo_fake->bo); - bo_fake->block = NULL; + if (bo_fake) { + set_dirty(&bo_fake->bo); + bo_fake->block = NULL; + } free_block(bufmgr_fake, block, 0); return 1; @@ -577,11 +579,13 @@ evict_mru(drm_intel_bufmgr_fake *bufmgr_fake) DRMLISTFOREACHSAFEREVERSE(block, tmp, &bufmgr_fake->lru) { drm_intel_bo_fake *bo_fake = (drm_intel_bo_fake *) block->bo; - if (bo_fake && (bo_fake->flags & BM_NO_FENCE_SUBDATA)) - continue; + if (bo_fake) { + if (bo_fake->flags & BM_NO_FENCE_SUBDATA) + continue; - set_dirty(&bo_fake->bo); - bo_fake->block = NULL; + set_dirty(&bo_fake->bo); + bo_fake->block = NULL; + } free_block(bufmgr_fake, block, 0); return 1; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/12] xf86drmSL: Check memory allocation by SL_RANDOM_INIT()
If the allocation fails, return -ENOMEM. Handle the return value at the caller funtion drmSLInsert() as well. Signed-off-by: Praveen Paneri --- xf86drmSL.c | 5 + 1 file changed, 5 insertions(+) diff --git a/xf86drmSL.c b/xf86drmSL.c index 45f3906..edafe7b 100644 --- a/xf86drmSL.c +++ b/xf86drmSL.c @@ -40,6 +40,7 @@ #include #include +#include #define SL_MAIN 0 @@ -124,6 +125,7 @@ static int SLRandomLevel(void) SL_RANDOM_DECL; SL_RANDOM_INIT(SL_RANDOM_SEED); +if (!state) return -ENOMEM; while ((SL_RANDOM & 0x01) && level < SL_MAX_LEVEL) ++level; return level; @@ -200,6 +202,9 @@ int drmSLInsert(void *l, unsigned long key, void *value) level = SLRandomLevel(); +if (level < 0) + return level; + if (level > list->level) { level = ++list->level; update[level] = list->head; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/12] drm: Validate memory allocations
This patch adds check on various drmMalloc() calls if they were able to allocate memory as requested or not. Return appropriate error if the allocation fails. Signed-off-by: Praveen Paneri --- xf86drm.c | 51 --- 1 file changed, 40 insertions(+), 11 deletions(-) diff --git a/xf86drm.c b/xf86drm.c index 1e25424..373113b 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -186,6 +186,8 @@ drmHashEntry *drmGetEntry(int fd) if (drmHashLookup(drmHashTable, key, &value)) { entry = drmMalloc(sizeof(*entry)); + if (!entry) + return NULL; entry->fd = fd; entry->f= NULL; entry->tagTable = drmHashCreate(); @@ -837,6 +839,8 @@ drmVersionPtr drmGetVersion(int fd) { drmVersionPtr retval; drm_version_t *version = drmMalloc(sizeof(*version)); +if (!version) + return NULL; memclear(*version); @@ -864,7 +868,9 @@ drmVersionPtr drmGetVersion(int fd) if (version->desc_len) version->desc[version->desc_len] = '\0'; retval = drmMalloc(sizeof(*retval)); -drmCopyVersion(retval, version); +if (retval) +drmCopyVersion(retval, version); + drmFreeKernelVersion(version); return retval; } @@ -886,6 +892,8 @@ drmVersionPtr drmGetVersion(int fd) drmVersionPtr drmGetLibVersion(int fd) { drm_version_t *version = drmMalloc(sizeof(*version)); +if (!version) + return NULL; /* Version history: * NOTE THIS MUST NOT GO ABOVE VERSION 1.X due to drivers needing it @@ -1294,14 +1302,25 @@ drmBufInfoPtr drmGetBufInfo(int fd) } retval = drmMalloc(sizeof(*retval)); + if (!retval) { + drmFree(info.list); + return NULL; + } + retval->count = info.count; retval->list = drmMalloc(info.count * sizeof(*retval->list)); - for (i = 0; i < info.count; i++) { - retval->list[i].count = info.list[i].count; - retval->list[i].size = info.list[i].size; - retval->list[i].low_mark = info.list[i].low_mark; - retval->list[i].high_mark = info.list[i].high_mark; + if (retval->list) { + for (i = 0; i < info.count; i++) { + retval->list[i].count = info.list[i].count; + retval->list[i].size = info.list[i].size; + retval->list[i].low_mark = info.list[i].low_mark; + retval->list[i].high_mark = info.list[i].high_mark; + } + } else { + drmFree(retval); + retval = NULL; } + drmFree(info.list); return retval; } @@ -1345,13 +1364,23 @@ drmBufMapPtr drmMapBufs(int fd) } retval = drmMalloc(sizeof(*retval)); + if (!retval) { + drmFree(bufs.list); +return NULL; + } + retval->count = bufs.count; retval->list = drmMalloc(bufs.count * sizeof(*retval->list)); - for (i = 0; i < bufs.count; i++) { - retval->list[i].idx = bufs.list[i].idx; - retval->list[i].total = bufs.list[i].total; - retval->list[i].used= 0; - retval->list[i].address = bufs.list[i].address; + if (retval->list) { + for (i = 0; i < bufs.count; i++) { + retval->list[i].idx = bufs.list[i].idx; + retval->list[i].total = bufs.list[i].total; + retval->list[i].used= 0; + retval->list[i].address = bufs.list[i].address; + } + } else { + drmFree(retval); + retval = NULL; } drmFree(bufs.list); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/12] xf86drmHash: Check memory allocation in HashHash()
HASH_RANDOM_INIT() can fail to allocate memory. In such case return an invalid hash value (0x) from HashHash() function. Caller functions check the hash value and act accordingly. Signed-off-by: Praveen Paneri --- xf86drmHash.c | 9 + 1 file changed, 9 insertions(+) diff --git a/xf86drmHash.c b/xf86drmHash.c index 82cbc2a..56e9af3 100644 --- a/xf86drmHash.c +++ b/xf86drmHash.c @@ -70,6 +70,7 @@ #include #include +#include #define HASH_MAIN 0 @@ -79,6 +80,7 @@ #define HASH_MAGIC 0xdeadbeef #define HASH_DEBUG 0 +#define HASH_INVALID 0x/* A value that is out of bound */ #define HASH_SIZE 512 /* Good for about 100 entries */ /* If you change this value, you probably have to change the HashHash hashing @@ -137,6 +139,8 @@ static unsigned long HashHash(unsigned long key) if (!init) { HASH_RANDOM_DECL; HASH_RANDOM_INIT(37); + if (!state) + return HASH_INVALID; for (i = 0; i < 256; i++) scatter[i] = HASH_RANDOM; HASH_RANDOM_DESTROY; ++init; @@ -203,6 +207,9 @@ static HashBucketPtr HashFind(HashTablePtr table, if (h) *h = hash; +if (hash == HASH_INVALID) + return NULL; + for (bucket = table->buckets[hash]; bucket; bucket = bucket->next) { if (bucket->key == key) { if (prev) { @@ -244,6 +251,7 @@ int drmHashInsert(void *t, unsigned long key, void *value) if (table->magic != HASH_MAGIC) return -1; /* Bad magic */ if (HashFind(table, key, &hash)) return 1; /* Already in table */ +if (hash == HASH_INVALID) return -1; bucket = HASH_ALLOC(sizeof(*bucket)); if (!bucket) return -1;/* Error */ @@ -267,6 +275,7 @@ int drmHashDelete(void *t, unsigned long key) bucket = HashFind(table, key, &hash); +if (hash == HASH_INVALID) return -1; if (!bucket) return 1; /* Not found */ table->buckets[hash] = bucket->next; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 12/12] xf86drmSL: Add missing function call to SLLocate()
A call to SLLocate() is missing from the function drmSLLookupNeighbors() Adding the same to fix this bug. Signed-off-by: Praveen Paneri --- xf86drmSL.c | 1 + 1 file changed, 1 insertion(+) diff --git a/xf86drmSL.c b/xf86drmSL.c index edafe7b..9c6f65a 100644 --- a/xf86drmSL.c +++ b/xf86drmSL.c @@ -274,6 +274,7 @@ int drmSLLookupNeighbors(void *l, unsigned long key, SLEntryPtrupdate[SL_MAX_LEVEL + 1]; int retcode = 0; +SLLocate(list, key, update); *prev_key = *next_key = key; *prev_value = *next_value = NULL; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 07/12] intel: Validate memory allocations
This patch adds check for various malloc/calloc function if they were able to allocate memory as requested or not. Return appropriate error if the allocation fails. Signed-off-by: Praveen Paneri --- intel/intel_bufmgr_fake.c | 4 intel/intel_bufmgr_gem.c | 3 +++ intel/intel_decode.c | 2 ++ 3 files changed, 9 insertions(+) diff --git a/intel/intel_bufmgr_fake.c b/intel/intel_bufmgr_fake.c index 129d344..e2b25eb 100644 --- a/intel/intel_bufmgr_fake.c +++ b/intel/intel_bufmgr_fake.c @@ -1278,6 +1278,8 @@ drm_intel_fake_emit_reloc(drm_intel_bo *bo, uint32_t offset, if (bo_fake->relocs == NULL) { bo_fake->relocs = malloc(sizeof(struct fake_buffer_reloc) * MAX_RELOCS); + if (!bo_fake->relocs) + return -ENOMEM; } r = &bo_fake->relocs[bo_fake->nr_relocs++]; @@ -1597,6 +1599,8 @@ drm_intel_bufmgr_fake_init(int fd, unsigned long low_offset, drm_intel_bufmgr_fake *bufmgr_fake; bufmgr_fake = calloc(1, sizeof(*bufmgr_fake)); + if (!bufmgr_fake) + return NULL; if (pthread_mutex_init(&bufmgr_fake->lock, NULL) != 0) { free(bufmgr_fake); diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index 2f0ced1..fd6279e 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -2067,6 +2067,9 @@ aub_write_bo_data(drm_intel_bo *bo, uint32_t offset, uint32_t size) unsigned int i; data = malloc(bo->size); + if (!data) + return; + drm_intel_bo_get_subdata(bo, offset, size, data); /* Easy mode: write out bo with no relocations */ diff --git a/intel/intel_decode.c b/intel/intel_decode.c index 5dab9ca..88267fd 100644 --- a/intel/intel_decode.c +++ b/intel/intel_decode.c @@ -3914,6 +3914,8 @@ drm_intel_decode(struct drm_intel_decode *ctx) * checking in statically sized packets. */ temp = malloc(size + 4096); + if (!temp) + return; memcpy(temp, ctx->base_data, size); memset((char *)temp + size, 0xd0, 4096); ctx->data = temp; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/12] intel: Validate pointer before using
Move the dereferencing below the check for valid ctx pointer. Signed-off-by: Praveen Paneri --- intel/intel_decode.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/intel/intel_decode.c b/intel/intel_decode.c index b70d949..5dab9ca 100644 --- a/intel/intel_decode.c +++ b/intel/intel_decode.c @@ -3901,12 +3901,14 @@ drm_intel_decode(struct drm_intel_decode *ctx) int ret; unsigned int index = 0; uint32_t devid; - int size = ctx->base_count * 4; + int size; void *temp; if (!ctx) return; + size = ctx->base_count * 4; + /* Put a scratch page full of obviously undefined data after * the batchbuffer. This lets us avoid a bunch of length * checking in statically sized packets. -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Reject the colorkey ioctls for primary and cursor planes
On Mon, 30 Mar 2015, Daniel Vetter wrote: > On Fri, Mar 27, 2015 at 07:59:40PM +0200, ville.syrj...@linux.intel.com wrote: >> From: Ville Syrjälä >> >> The legcy colorkey ioctls are only implemented for sprite planes, so >> reject the ioctl for primary/cursor planes. If we want to support >> colorkeying with these planes (assuming we have hw support of course) >> we should just move ahead with the colorkey property conversion. >> >> Cc: Tommi Rantala >> Signed-off-by: Ville Syrjälä > > Testcase: kms_legacy_colorkey > Cc: sta...@vger.kernel.org > Reviewed-by: Daniel Vetter Pushed to drm-intel-fixes, with cc: stable and a bunch of other tags added, thanks for the patch, review, and testing. BR, Jani. > >> --- >> drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c >> b/drivers/gpu/drm/i915/intel_sprite.c >> index f41e872..7017384 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -1113,7 +1113,7 @@ int intel_sprite_set_colorkey(struct drm_device *dev, >> void *data, >> drm_modeset_lock_all(dev); >> >> plane = drm_plane_find(dev, set->plane_id); >> -if (!plane) { >> +if (!plane || plane->type != DRM_PLANE_TYPE_OVERLAY) { >> ret = -ENOENT; >> goto out_unlock; >> } >> @@ -1145,7 +1145,7 @@ int intel_sprite_get_colorkey(struct drm_device *dev, >> void *data, >> drm_modeset_lock_all(dev); >> >> plane = drm_plane_find(dev, get->plane_id); >> -if (!plane) { >> +if (!plane || plane->type != DRM_PLANE_TYPE_OVERLAY) { >> ret = -ENOENT; >> goto out_unlock; >> } >> -- >> 2.0.5 >> >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] uxa: Do not use RandR in hotplug if not available
When using Xinerama, RandR is automatically disabled, and calling RR routines will trigger an assert() because the RR keys/resources are not set, leading to an Xserver abort. Hotplug makes little sense without RandR, so it's safer to just return if RandR is not available. Signed-off-by: Olivier Fourdan --- src/uxa/intel_display.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c index a95b3de..e42a59d 100644 --- a/src/uxa/intel_display.c +++ b/src/uxa/intel_display.c @@ -2516,6 +2516,13 @@ intel_mode_hotplug(struct intel_screen_private *intel) Bool found; Bool changed = FALSE; struct intel_mode *mode = intel->modes; + +#ifdef HAS_DIXREGISTERPRIVATEKEY + /* Without RR, nothing we can do here */ + if (!dixPrivateKeyRegistered(rrPrivKey)) + return; +#endif + mode_res = drmModeGetResources(intel->drmSubFD); if (!mode_res) goto out; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] uxa: Do not use RandR in hotplug if not available
On Thu, Apr 02, 2015 at 10:45:39AM +0200, Olivier Fourdan wrote: > When using Xinerama, RandR is automatically disabled, and calling RR > routines will trigger an assert() because the RR keys/resources are > not set, leading to an Xserver abort. > > Hotplug makes little sense without RandR, so it's safer to just return > if RandR is not available. In fact it makes little sense to install a udev monitor without RandR, see 1a489142c8e6a4828348cc9afbd0f430d3b1e2d8 -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation
Hi, On 04/02/2015 05:54 AM, Jindal, Sonika wrote: diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f0bbc22..86ee0f0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, return -EINVAL; } +switch (fb->pixel_format) { +case DRM_FORMAT_XRGB: +case DRM_FORMAT_ARGB: +case DRM_FORMAT_XBGR: +case DRM_FORMAT_ABGR: +case DRM_FORMAT_XRGB2101010: +case DRM_FORMAT_ARGB2101010: +case DRM_FORMAT_XBGR2101010: +case DRM_FORMAT_ABGR2101010: +case DRM_FORMAT_YUYV: +case DRM_FORMAT_YVYU: +case DRM_FORMAT_UYVY: +case DRM_FORMAT_VYUY: +case DRM_FORMAT_NV12: +break; + +default: +DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270 rotation!\n", + fb->pixel_format); +return -EINVAL; +} Shouldn't we be matching against the list of formats the plane supports (which may vary by platform, or by specific plane) rather than this generic list? We already specified what formats the plane can handle at plane init time, so it seems like what you'd really want is just a call to drm_plane_check_pixel_format(plane_state->plane, fb->pixel_format) then follow that up with explicit checks to exclude any formats that we can handle in 0/180, but not in 90/270. I am not sure how it will help. drm_plane_check_pixel_format should be used to check the pixel format of the fb which we should be doing in some -check functions (I don't think we do that right now?) against what is supported by the plane. But to check for the formats which are allowed for 90/270, we would need this kind of explicit check. I'd also move this check to intel_plane_atomic_check(), since the 'check' code path is where I'd usually go looking for these types of checks; the function you've got it in at the moment gets called from the 'prepare' step which works as well, but seems a bit less obvious. Yes, I agree, but this is on top of Tvrtko's patch for secondary buffer mapping where based upon tiling and pixel format we are allowing the rotated gtt. Tvrtko, Can these be moved to the intel_plane_atomic_check() Good point, I think it can and should. I suppose it was just an oversight during endless rebasing, that I put the Y tiling check in there. So you can move that part as well while you are doing it. Also highlights the fact we have no negative testing in kms_rotation_crc for this. I mean, trying to rotate by 90/270 linear or X tiled, or wrong pixel format. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 19/19] drm/i915: Modeset global_pipes() update
On Tue, Mar 31, 2015 at 05:45:56PM +0300, Ville Syrjälä wrote: > On Tue, Mar 31, 2015 at 02:14:23PM +0300, Mika Kahola wrote: > > Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()' > > into one function 'intel_modeset_global_pipes()' > > > > Signed-off-by: Mika Kahola > > --- > > drivers/gpu/drm/i915/intel_display.c | 89 > > +--- > > 1 file changed, 41 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 5ed40df..7180d2b 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5209,38 +5209,6 @@ static int valleyview_calc_cdclk(struct > > drm_i915_private *dev_priv, > > return 20; > > } > > > > -/* compute the max pixel clock for new configuration */ > > -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) > > -{ > > - struct drm_device *dev = dev_priv->dev; > > - struct intel_crtc *intel_crtc; > > - int max_pixclk = 0; > > - > > - for_each_intel_crtc(dev, intel_crtc) { > > - if (intel_crtc->new_enabled) > > - max_pixclk = max(max_pixclk, > > - > > intel_crtc->new_config->base.adjusted_mode.crtc_clock); > > - } > > - > > - return max_pixclk; > > -} > > - > > -static void valleyview_modeset_global_pipes(struct drm_device *dev, > > - unsigned *prepare_pipes) > > -{ > > - struct drm_i915_private *dev_priv = dev->dev_private; > > - struct intel_crtc *intel_crtc; > > - int max_pixclk = intel_mode_max_pixclk(dev_priv); > > - > > - if (valleyview_calc_cdclk(dev_priv, max_pixclk) == dev_priv->cdclk_freq) > > - return; > > - > > - /* disable/enable all currently active pipes while we change cdclk */ > > - for_each_intel_crtc(dev, intel_crtc) > > - if (intel_crtc->base.state->enable) > > - *prepare_pipes |= (1 << intel_crtc->pipe); > > -} > > - > > static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv) > > { > > unsigned int credits, default_credits; > > @@ -5277,6 +5245,22 @@ static void vlv_program_pfi_credits(struct > > drm_i915_private *dev_priv) > > WARN_ON(I915_READ(GCI_CONTROL) & PFI_CREDIT_RESEND); > > } > > > > +/* compute the max pixel clock for new configuration */ > > +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) > > +{ > > + struct drm_device *dev = dev_priv->dev; > > + struct intel_crtc *intel_crtc; > > + int max_pixclk = 0; > > + > > + for_each_intel_crtc(dev, intel_crtc) { > > + if (intel_crtc->new_enabled) > > + max_pixclk = max(max_pixclk, > > + > > intel_crtc->new_config->base.adjusted_mode.crtc_clock); > > + } > > + > > + return max_pixclk; > > +} > > + > > static void valleyview_modeset_global_resources(struct drm_atomic_state > > *state) > > { > > struct drm_device *dev = state->dev; > > @@ -8973,21 +8957,38 @@ static void broadwell_set_cdclk(struct drm_device > > *dev, int cdclk) > > cdclk, dev_priv->cdclk_freq); > > } > > > > -static void haswell_modeset_global_pipes(struct drm_device *dev, > > -unsigned *prepare_pipes) > > +static void intel_modeset_global_pipes(struct drm_device *dev, > > + unsigned *prepare_pipes, > > + unsigned *disable_pipes) > > You don't modify disable_pipes, so no need to pass as pointer. > > I do think passing disable_pipes into intel_modeset_global_pipes() does > make sense however, as it makes it clearer why we need to clear out the > disable_pipes when the code is all in one place like this. That's a good point. > > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct intel_crtc *crtc; > > - int max_pixel_rate = ilk_max_pixel_rate(dev_priv); > > + int max_pixclk; > > > > - if (haswell_calc_cdclk(dev_priv, max_pixel_rate) == > > - dev_priv->cdclk_freq) > > + /* this modeset is valid only for VLV, HSW, and BDW */ > > + if (!IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) && !IS_BROADWELL(dev)) > > return; > > > > + if (IS_VALLEYVIEW(dev)) { > > + max_pixclk = intel_mode_max_pixclk(dev_priv); > > + if (valleyview_calc_cdclk(dev_priv, max_pixclk) == > > + dev_priv->cdclk_freq) > > + return; > > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > > + max_pixclk = ilk_max_pixel_rate(dev_priv); > > + if (haswell_calc_cdclk(dev_priv, max_pixclk) == > > + dev_priv->cdclk_freq) > > + return; > > + > > + } > > Maybe move the current vs. newly computed pixclk comparison out of > the if ladder, so we don't have to duplicate it for each platform? > It should also get rid of
[Intel-gfx] [PATCH] drm/i915: Allow disabling the destination colorkey for overlay
Sometimes userspace wants a true overlay that is never clipped. In such cases, we need to disable the destination colorkey. However, it is currently unconditionally enabled in the overlay with no means of disabling. So rectify that by always default to on, and extending the UPDATE_ATTR ioctl to support explicit disabling of the colorkey. This is contrast to the spite code which requires explicit enabling of either the destination or source colorkey. Handling source colorkey is still todo for the overlay. (Of course it may be worth migrating overlay to sprite before then.) Signed-off-by: Chris Wilson --- The recent discussion over colorkey reminded me about this feature discrepancy in the overlay. 2013! --- drivers/gpu/drm/i915/intel_overlay.c | 30 +++--- include/uapi/drm/i915_drm.h | 1 + 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c index 36400bbbf715..936cf160bb7d 100644 --- a/drivers/gpu/drm/i915/intel_overlay.c +++ b/drivers/gpu/drm/i915/intel_overlay.c @@ -175,7 +175,8 @@ struct intel_overlay { bool active; bool pfit_active; u32 pfit_vscale_ratio; /* shifted-point number, (1<<12) == 1.0 */ - u32 color_key; + u32 color_key:24; + u32 color_key_enabled:1; u32 brightness, contrast, saturation; u32 old_xscale, old_yscale; /* register access */ @@ -630,31 +631,36 @@ static void update_colorkey(struct intel_overlay *overlay, struct overlay_registers __iomem *regs) { u32 key = overlay->color_key; + u32 flags; + + flags = 0; + if (overlay->color_key_enabled) + flags |= DST_KEY_ENABLE; switch (overlay->crtc->base.primary->fb->bits_per_pixel) { case 8: - iowrite32(0, ®s->DCLRKV); - iowrite32(CLK_RGB8I_MASK | DST_KEY_ENABLE, ®s->DCLRKM); + key = 0; + flags |= CLK_RGB8I_MASK; break; case 16: if (overlay->crtc->base.primary->fb->depth == 15) { - iowrite32(RGB15_TO_COLORKEY(key), ®s->DCLRKV); - iowrite32(CLK_RGB15_MASK | DST_KEY_ENABLE, - ®s->DCLRKM); + key = RGB15_TO_COLORKEY(key); + flags |= CLK_RGB15_MASK; } else { - iowrite32(RGB16_TO_COLORKEY(key), ®s->DCLRKV); - iowrite32(CLK_RGB16_MASK | DST_KEY_ENABLE, - ®s->DCLRKM); + key = RGB16_TO_COLORKEY(key); + flags |= CLK_RGB16_MASK; } break; case 24: case 32: - iowrite32(key, ®s->DCLRKV); - iowrite32(CLK_RGB24_MASK | DST_KEY_ENABLE, ®s->DCLRKM); + flags |= CLK_RGB24_MASK; break; } + + iowrite32(key, ®s->DCLRKV); + iowrite32(flags, ®s->DCLRKM); } static u32 overlay_cmd_reg(struct put_image_params *params) @@ -1331,6 +1337,7 @@ int intel_overlay_attrs(struct drm_device *dev, void *data, I915_WRITE(OGAMC5, attrs->gamma5); } } + overlay->color_key_enabled = (attrs->flags & I915_OVERLAY_DISABLE_DEST_COLORKEY) == 0; ret = 0; out_unlock: @@ -1397,6 +1404,7 @@ void intel_setup_overlay(struct drm_device *dev) /* init all values */ overlay->color_key = 0x0101fe; + overlay->color_key_enabled = true; overlay->brightness = -19; overlay->contrast = 75; overlay->saturation = 146; diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 006f026f1ce0..c2e679be8903 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -998,6 +998,7 @@ struct drm_intel_overlay_put_image { /* flags */ #define I915_OVERLAY_UPDATE_ATTRS (1<<0) #define I915_OVERLAY_UPDATE_GAMMA (1<<1) +#define I915_OVERLAY_DISABLE_DEST_COLORKEY (1<<2) struct drm_intel_overlay_attrs { __u32 flags; __u32 color_key; -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 19/19] drm/i915: Modeset global_pipes() update
Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()' into one function 'intel_modeset_global_pipes()' v2: - we don't modify 'disable_pipes', so passing this as a pointer is removed (based on Ville's comment) - introduced a new function 'intel_calc_cdclk()' that combines routines from 'valleyview_calc_cdclk()' and 'haswell_calc_cdclk()' Signed-off-by: Mika Kahola --- drivers/gpu/drm/i915/intel_display.c | 191 --- 1 file changed, 88 insertions(+), 103 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7b97907..18a1262 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5179,66 +5179,62 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk) intel_update_cdclk(dev); } -static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, +static int intel_calc_cdclk(struct drm_i915_private *dev_priv, int max_pixclk) { - int freq_320 = (dev_priv->hpll_freq << 1) % 32 != 0 ? 33 : 32; - int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90; + int cdclk = 20; - /* -* Really only a few cases to deal with, as only 4 CDclks are supported: -* 200MHz -* 267MHz -* 320/333MHz (depends on HPLL freq) -* 400MHz (VLV only) -* So we check to see whether we're above 90% (VLV) or 95% (CHV) -* of the lower bin and adjust if needed. -* -* We seem to get an unstable or solid color picture at 200MHz. -* Not sure what's wrong. For now use 200MHz only when all pipes -* are off. -*/ - if (!IS_CHERRYVIEW(dev_priv) && - max_pixclk > freq_320*limit/100) - return 40; - else if (max_pixclk > 27*limit/100) - return freq_320; - else if (max_pixclk > 0) - return 27; - else - return 20; -} - -/* compute the max pixel clock for new configuration */ -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) -{ - struct drm_device *dev = dev_priv->dev; - struct intel_crtc *intel_crtc; - int max_pixclk = 0; + if (IS_VALLEYVIEW(dev_priv)) { + int freq_320 = (dev_priv->hpll_freq << 1) % 32 != 0 ? 33 : 32; + int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90; + /* +* Really only a few cases to deal with, as only 4 CDclks are supported: +* 200MHz +* 267MHz +* 320/333MHz (depends on HPLL freq) +* 400MHz (VLV only) +* So we check to see whether we're above 90% (VLV) or 95% (CHV) +* of the lower bin and adjust if needed. +* +* We seem to get an unstable or solid color picture at 200MHz. +* Not sure what's wrong. For now use 200MHz only when all pipes +* are off. +*/ + if (!IS_CHERRYVIEW(dev_priv) && + max_pixclk > freq_320*limit/100) + cdclk = 40; + else if (max_pixclk > 27*limit/100) + cdclk = freq_320; + else if (max_pixclk > 0) + cdclk = 27; + else + cdclk = 20; + } else if (IS_HASWELL(dev_priv)) { + /* +* FIXME should also account for plane ratio +* once 64bpp pixel formats are supported. +*/ + if (max_pixclk > 54) + cdclk = 675000; + else if (max_pixclk > 45) + cdclk = 54; + else if (max_pixclk > 337500 || !IS_HSW_ULX(dev_priv)) + cdclk = 45; + else + cdclk = 337500; - for_each_intel_crtc(dev, intel_crtc) { - if (intel_crtc->new_enabled) - max_pixclk = max(max_pixclk, - intel_crtc->new_config->base.adjusted_mode.crtc_clock); + /* +* FIXME move the cdclk caclulation to +* compute_config() so we can fail gracegully. +*/ + if (cdclk > dev_priv->max_cdclk_freq) { + DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n", + cdclk, dev_priv->max_cdclk_freq); + cdclk = dev_priv->max_cdclk_freq; + } } - return max_pixclk; -} - -static void valleyview_modeset_global_pipes(struct drm_device *dev, - unsigned *prepare_pipes) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc *i
Re: [Intel-gfx] [PATCH] drm/i915: Make debugfs/i915_gem_request more friendly
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6112 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -4 272/272 268/272 ILK 302/302 302/302 SNB 303/303 303/303 IVB 338/338 338/338 BYT -1 287/287 286/287 HSW -1 361/361 360/361 BDW 308/308 308/308 -Detailed- Platform Testdrm-intel-nightly Series Applied *PNV igt@gem_fence_thrash@bo-write-verify-threaded-none PASS(4) FAIL(1)PASS(1) PNV igt@gem_tiled_pread_pwrite FAIL(3)PASS(6) FAIL(1)PASS(1) PNV igt@gem_userptr_blits@coherency-sync CRASH(4)PASS(5) CRASH(1)PASS(1) PNV igt@gen3_render_tiledx_blits FAIL(5)PASS(3) FAIL(2) *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(7) FAIL(1)PASS(1) *HSW igt@gem_storedw_loop_vebox PASS(2) DMESG_WARN(1)PASS(1) (dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...video_enhancement_ring_idle@Hangcheck timer elapsed... video enhancement ring idle Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/12] intel: Validate output of realloc()
On Thu, Apr 02, 2015 at 02:02:17PM +0530, Praveen Paneri wrote: > realloc will return NULL if failed to allocate the extra memory > requested. Return from function if it fails. NAK. Silently passing absolute addresses to the GPU to read and write is not a good idea. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Allow disabling the destination colorkey for overlay
On Thu, Apr 02, 2015 at 10:35:08AM +0100, Chris Wilson wrote: > Sometimes userspace wants a true overlay that is never clipped. In such > cases, we need to disable the destination colorkey. However, it is > currently unconditionally enabled in the overlay with no means of > disabling. So rectify that by always default to on, and extending the > UPDATE_ATTR ioctl to support explicit disabling of the colorkey. > > This is contrast to the spite code which requires explicit enabling of > either the destination or source colorkey. Handling source colorkey is > still todo for the overlay. (Of course it may be worth migrating overlay > to sprite before then.) > > Signed-off-by: Chris Wilson Looks all right to me. Reviewed-by: Ville Syrjälä > --- > The recent discussion over colorkey reminded me about this feature > discrepancy in the overlay. 2013! > --- > drivers/gpu/drm/i915/intel_overlay.c | 30 +++--- > include/uapi/drm/i915_drm.h | 1 + > 2 files changed, 20 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_overlay.c > b/drivers/gpu/drm/i915/intel_overlay.c > index 36400bbbf715..936cf160bb7d 100644 > --- a/drivers/gpu/drm/i915/intel_overlay.c > +++ b/drivers/gpu/drm/i915/intel_overlay.c > @@ -175,7 +175,8 @@ struct intel_overlay { > bool active; > bool pfit_active; > u32 pfit_vscale_ratio; /* shifted-point number, (1<<12) == 1.0 */ > - u32 color_key; > + u32 color_key:24; > + u32 color_key_enabled:1; > u32 brightness, contrast, saturation; > u32 old_xscale, old_yscale; > /* register access */ > @@ -630,31 +631,36 @@ static void update_colorkey(struct intel_overlay > *overlay, > struct overlay_registers __iomem *regs) > { > u32 key = overlay->color_key; > + u32 flags; > + > + flags = 0; > + if (overlay->color_key_enabled) > + flags |= DST_KEY_ENABLE; > > switch (overlay->crtc->base.primary->fb->bits_per_pixel) { > case 8: > - iowrite32(0, ®s->DCLRKV); > - iowrite32(CLK_RGB8I_MASK | DST_KEY_ENABLE, ®s->DCLRKM); > + key = 0; > + flags |= CLK_RGB8I_MASK; > break; > > case 16: > if (overlay->crtc->base.primary->fb->depth == 15) { > - iowrite32(RGB15_TO_COLORKEY(key), ®s->DCLRKV); > - iowrite32(CLK_RGB15_MASK | DST_KEY_ENABLE, > - ®s->DCLRKM); > + key = RGB15_TO_COLORKEY(key); > + flags |= CLK_RGB15_MASK; > } else { > - iowrite32(RGB16_TO_COLORKEY(key), ®s->DCLRKV); > - iowrite32(CLK_RGB16_MASK | DST_KEY_ENABLE, > - ®s->DCLRKM); > + key = RGB16_TO_COLORKEY(key); > + flags |= CLK_RGB16_MASK; > } > break; > > case 24: > case 32: > - iowrite32(key, ®s->DCLRKV); > - iowrite32(CLK_RGB24_MASK | DST_KEY_ENABLE, ®s->DCLRKM); > + flags |= CLK_RGB24_MASK; > break; > } > + > + iowrite32(key, ®s->DCLRKV); > + iowrite32(flags, ®s->DCLRKM); > } > > static u32 overlay_cmd_reg(struct put_image_params *params) > @@ -1331,6 +1337,7 @@ int intel_overlay_attrs(struct drm_device *dev, void > *data, > I915_WRITE(OGAMC5, attrs->gamma5); > } > } > + overlay->color_key_enabled = (attrs->flags & > I915_OVERLAY_DISABLE_DEST_COLORKEY) == 0; > > ret = 0; > out_unlock: > @@ -1397,6 +1404,7 @@ void intel_setup_overlay(struct drm_device *dev) > > /* init all values */ > overlay->color_key = 0x0101fe; > + overlay->color_key_enabled = true; > overlay->brightness = -19; > overlay->contrast = 75; > overlay->saturation = 146; > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 006f026f1ce0..c2e679be8903 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -998,6 +998,7 @@ struct drm_intel_overlay_put_image { > /* flags */ > #define I915_OVERLAY_UPDATE_ATTRS(1<<0) > #define I915_OVERLAY_UPDATE_GAMMA(1<<1) > +#define I915_OVERLAY_DISABLE_DEST_COLORKEY (1<<2) > struct drm_intel_overlay_attrs { > __u32 flags; > __u32 color_key; > -- > 2.1.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 19/19] drm/i915: Modeset global_pipes() update
On Thu, Apr 02, 2015 at 01:05:31PM +0300, Mika Kahola wrote: > Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()' > into one function 'intel_modeset_global_pipes()' > > v2: > - we don't modify 'disable_pipes', so passing this as a pointer > is removed (based on Ville's comment) > - introduced a new function 'intel_calc_cdclk()' that combines > routines from 'valleyview_calc_cdclk()' and 'haswell_calc_cdclk()' > > Signed-off-by: Mika Kahola > --- > drivers/gpu/drm/i915/intel_display.c | 191 > --- > 1 file changed, 88 insertions(+), 103 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 7b97907..18a1262 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5179,66 +5179,62 @@ static void cherryview_set_cdclk(struct drm_device > *dev, int cdclk) > intel_update_cdclk(dev); > } > > -static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, > +static int intel_calc_cdclk(struct drm_i915_private *dev_priv, >int max_pixclk) > { > - int freq_320 = (dev_priv->hpll_freq << 1) % 32 != 0 ? 33 : > 32; > - int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90; > + int cdclk = 20; > > - /* > - * Really only a few cases to deal with, as only 4 CDclks are supported: > - * 200MHz > - * 267MHz > - * 320/333MHz (depends on HPLL freq) > - * 400MHz (VLV only) > - * So we check to see whether we're above 90% (VLV) or 95% (CHV) > - * of the lower bin and adjust if needed. > - * > - * We seem to get an unstable or solid color picture at 200MHz. > - * Not sure what's wrong. For now use 200MHz only when all pipes > - * are off. > - */ > - if (!IS_CHERRYVIEW(dev_priv) && > - max_pixclk > freq_320*limit/100) > - return 40; > - else if (max_pixclk > 27*limit/100) > - return freq_320; > - else if (max_pixclk > 0) > - return 27; > - else > - return 20; > -} I'd leave the valleyview_calc_cdclk() and haswell_calc_cdclk() alone. Stuffing them into a single function makes the patch quite ugly. Also I suppose we might want to turn it into a vfunc in the future and having separate funcs for different platforms would make that easier. > - > -/* compute the max pixel clock for new configuration */ > -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) > -{ > - struct drm_device *dev = dev_priv->dev; > - struct intel_crtc *intel_crtc; > - int max_pixclk = 0; > + if (IS_VALLEYVIEW(dev_priv)) { > + int freq_320 = (dev_priv->hpll_freq << 1) % 32 != 0 ? > 33 : 32; > + int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90; > + /* > + * Really only a few cases to deal with, as only 4 CDclks are > supported: > + * 200MHz > + * 267MHz > + * 320/333MHz (depends on HPLL freq) > + * 400MHz (VLV only) > + * So we check to see whether we're above 90% (VLV) or 95% (CHV) > + * of the lower bin and adjust if needed. > + * > + * We seem to get an unstable or solid color picture at 200MHz. > + * Not sure what's wrong. For now use 200MHz only when all pipes > + * are off. > + */ > + if (!IS_CHERRYVIEW(dev_priv) && > + max_pixclk > freq_320*limit/100) > + cdclk = 40; > + else if (max_pixclk > 27*limit/100) > + cdclk = freq_320; > + else if (max_pixclk > 0) > + cdclk = 27; > + else > + cdclk = 20; > + } else if (IS_HASWELL(dev_priv)) { > + /* > + * FIXME should also account for plane ratio > + * once 64bpp pixel formats are supported. > + */ > + if (max_pixclk > 54) > + cdclk = 675000; > + else if (max_pixclk > 45) > + cdclk = 54; > + else if (max_pixclk > 337500 || !IS_HSW_ULX(dev_priv)) > + cdclk = 45; > + else > + cdclk = 337500; > > - for_each_intel_crtc(dev, intel_crtc) { > - if (intel_crtc->new_enabled) > - max_pixclk = max(max_pixclk, > - > intel_crtc->new_config->base.adjusted_mode.crtc_clock); > + /* > + * FIXME move the cdclk caclulation to > + * compute_config() so we can fail gracegully. > + */ > + if (cdclk > dev_priv->max_cdclk_freq) { > + DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d > kHz)\n", >
Re: [Intel-gfx] [PATCH 1/2] drm/i915/vlv: save/restore the power context base reg
On Thursday 02 April 2015 02:52 AM, Jesse Barnes wrote: Some BIOSes (e.g. the one on the Minnowboard) don't save/restore this reg. If it's unlocked, we can just restore the previous value, and if it's locked (in case the BIOS re-programmed it for us) the write will be ignored and we'll still have "did it move" sanity check in the PM code to warn us if something is still amiss. Looks fine to me Reviewed-by: Deepak S References: https://bugs.freedesktop.org/show_bug.cgi?id=89611 Signed-off-by: Jesse Barnes --- drivers/gpu/drm/i915/i915_drv.c | 2 ++ drivers/gpu/drm/i915/i915_drv.h | 1 + 2 files changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index c1a3cdb5..4d6d6f0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1104,6 +1104,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) /* Gunit-Display CZ domain, 0x182028-0x1821CF */ s->gu_ctl0 = I915_READ(VLV_GU_CTL0); s->gu_ctl1 = I915_READ(VLV_GU_CTL1); + s->pcbr = I915_READ(VLV_PCBR); s->clock_gate_dis2 = I915_READ(VLV_GUNIT_CLOCK_GATE2); /* @@ -1198,6 +1199,7 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv) /* Gunit-Display CZ domain, 0x182028-0x1821CF */ I915_WRITE(VLV_GU_CTL0, s->gu_ctl0); I915_WRITE(VLV_GU_CTL1, s->gu_ctl1); + I915_WRITE(VLV_PCBR,s->pcbr); I915_WRITE(VLV_GUNIT_CLOCK_GATE2, s->clock_gate_dis2); } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b13c552..f3ac683 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -994,6 +994,7 @@ struct vlv_s0ix_state { /* Display 2 CZ domain */ u32 gu_ctl0; u32 gu_ctl1; + u32 pcbr; u32 clock_gate_dis2; }; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/vlv: remove wait for previous GFX clk disable request
On Thursday 02 April 2015 02:52 AM, Jesse Barnes wrote: Looks like it was introduced in: commit 650ad970a39f8b6164fe8613edc150f585315289 Author: Imre Deak Date: Fri Apr 18 16:35:02 2014 +0300 drm/i915: vlv: factor out vlv_force_gfx_clock and check for pending force-of but I'm not sure why. It has caused problems for us in the past (see 85250ddff7a603dfe0ec0503a9e6395f79424f61 and 8d4eee9cd7a170342dc6fbc2ee19ae77031a8cd5) and doesn't seem to be required, so let's just drop it. References: https://bugs.freedesktop.org/show_bug.cgi?id=89611 Signed-off-by: Jesse Barnes --- drivers/gpu/drm/i915/i915_drv.c | 14 -- 1 file changed, 14 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 4d6d6f0..c3fdbb0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1208,21 +1208,7 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on) u32 val; int err; - val = I915_READ(VLV_GTLC_SURVIVABILITY_REG); - #define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT) - /* Wait for a previous force-off to settle */ - if (force_on && !IS_CHERRYVIEW(dev_priv->dev)) { - /* WARN_ON only for the Valleyview */ - WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on); - - err = wait_for(!COND, 20); - if (err) { - DRM_ERROR("timeout waiting for GFX clock force-off (%08x)\n", - I915_READ(VLV_GTLC_SURVIVABILITY_REG)); - return err; - } - } I agree, I do not think we need to wait for previous Gfx clk force-off. Also, I do not see any race condition happening between diff Gfx force clk in driver. Lets just drop it :) Reviewed-by: Deepak S val = I915_READ(VLV_GTLC_SURVIVABILITY_REG); val &= ~VLV_GFX_CLK_FORCE_ON_BIT; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC i-g-t v4] tests/gem_exec_pad_to_size: Test object padding at execbuf
From: Tvrtko Ursulin This tests the new EXEC_OBJECT_PAD_TO_SIZE exec_object2 flag. Similar to some other tests, it uses knowledge of the DRM allocation policy in order to get two objects mapped adjacent to each other. It is then possible to verify that the pad to size flag will move them apart. v2: Correct commit message. (Chris Wilson) v3: Changes after code review by Chris Wilson. * No need for gem_sync after execbuf. * Drop caches before running. * Allocate one additional bo per iteration. * Don't explicitly set unused execbuf fields to zero. * One improved comment. v4: Require simpler object ordering and fixed overlap test. (Chris Wilson) Signed-off-by: Tvrtko Ursulin Cc: Chris Wilson --- tests/Makefile.sources | 1 + tests/gem_exec_pad_to_size.c | 236 +++ 2 files changed, 237 insertions(+) create mode 100644 tests/gem_exec_pad_to_size.c diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 0a974a6..5f21728 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -34,6 +34,7 @@ TESTS_progs_M = \ gem_exec_bad_domains \ gem_exec_faulting_reloc \ gem_exec_nop \ + gem_exec_pad_to_size \ gem_exec_params \ gem_exec_parse \ gem_fenced_exec_thrash \ diff --git a/tests/gem_exec_pad_to_size.c b/tests/gem_exec_pad_to_size.c new file mode 100644 index 000..db56d15 --- /dev/null +++ b/tests/gem_exec_pad_to_size.c @@ -0,0 +1,236 @@ +/* + * Copyright © 2015 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Tvrtko Ursulin + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "drm.h" +#include "ioctl_wrappers.h" +#include "igt_core.h" +#include "drmtest.h" +#include "intel_reg.h" +#include "igt_debugfs.h" + +#ifndef PAGE_SIZE +#define PAGE_SIZE 4096 +#endif + +struct local_drm_i915_gem_exec_object2 { + /** +* User's handle for a buffer to be bound into the GTT for this +* operation. +*/ + __u32 handle; + + /** Number of relocations to be performed on this buffer */ + __u32 relocation_count; + /** +* Pointer to array of struct drm_i915_gem_relocation_entry containing +* the relocations to be performed in this buffer. +*/ + __u64 relocs_ptr; + + /** Required alignment in graphics aperture */ + __u64 alignment; + + /** +* Returned value of the updated offset of the object, for future +* presumed_offset writes. +*/ + __u64 offset; + +#define LOCAL_EXEC_OBJECT_NEEDS_FENCE (1<<0) +#define LOCAL_EXEC_OBJECT_NEEDS_GTT(1<<1) +#define LOCAL_EXEC_OBJECT_WRITE(1<<2) +#define LOCAL_EXEC_OBJECT_PAD_TO_SIZE (1<<3) +#define __LOCAL_EXEC_OBJECT_UNKNOWN_FLAGS -(LOCAL_EXEC_OBJECT_PAD_TO_SIZE<<1) + __u64 flags; + + __u64 pad_to_size; + __u64 rsvd2; +}; + +static int +exec(int fd, uint32_t handles[3], uint32_t pad_to_size[2], uint64_t *offsets) +{ + struct drm_i915_gem_execbuffer2 execbuf; + struct local_drm_i915_gem_exec_object2 gem_exec[3]; + int ret = 0; + + memset(gem_exec, 0, sizeof(gem_exec)); + + gem_exec[0].handle = handles[1]; + gem_exec[0].flags = pad_to_size[0] ? + LOCAL_EXEC_OBJECT_PAD_TO_SIZE : 0; + gem_exec[0].pad_to_size = pad_to_size[0]; + + gem_exec[1].handle = handles[2]; + gem_exec[1].flags = pad_to_size[1] ? + LOCAL_EXEC_OBJECT_PAD_TO_SIZE : 0; + gem_exec[1].pad_to_size = pad_to_size[1]; + + gem_exec[2].handle = handles[0]; + + memset(&execbuf, 0, sizeof(execbuf)); + + execbuf.buffers_ptr = (uintptr_t)gem_exec; + execbuf.buffer_count = 3; + execbuf.batch_len = 8; + +
Re: [Intel-gfx] [RFC i-g-t v3] tests/gem_exec_pad_to_size: Test object padding at execbuf
On 04/01/2015 05:39 PM, Chris Wilson wrote: On Wed, Apr 01, 2015 at 05:31:16PM +0100, Chris Wilson wrote: On Wed, Apr 01, 2015 at 05:07:25PM +0100, Tvrtko Ursulin wrote: On 04/01/2015 04:42 PM, Chris Wilson wrote: On Wed, Apr 01, 2015 at 04:14:52PM +0100, Tvrtko Ursulin wrote: + /* Re-exec with padding set. */ + igt_assert(exec(fd, eb_handles, pad_to_size, offsets) == 0); The crux of the test is that we generate two objects such that B_offset = A_offset + A_size and then tell the kernel that A is actually 2*size (A_pad_to_size) + if (offsets[1] > offsets[0]) + distance = offsets[1] - offsets[0]; + else + distance = offsets[0] - offsets[1]; The assertion I feel should only be that B_offset + B_size <= A_offset && B_offset >= A_offset + A_pad_to_size I don't get this. B starts after A + padding, but B ends before A? s/&&/||/ Sorry &&. Gah, must be time for a coffee break. The assertion is that the objects do not overlap based on the pad_to_size we expect the kernel to apply, rather than their natural size. If the kernel doesn't move the objects, they would it would fail. You know all this time I didn't realize you were saying me check was broken, just that it was confusing. :) Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] Correct drm display mode table about 1856x1392 @75Hz mode
From: "liu,lei" According DMT spec, vss of this mode should be 1393, vse should be 1396. VESA MONITOR TIMING STANDARD: Timing Name = 1856 x 1392 @ 75Hz Hor Total Time = 8.889; (usec) = 320 chars = 2560 Pixels Hor Sync Start = 6.889; (usec) = 248 chars = 1984 Pixels H Back Porch = 1.222; (usec) = 44 chars = 352 Pixels Ver Total Time = 13.333; (msec) = 1500 lines HT – (1.06xHA) Ver Sync Start = 12.382; (msec) = 1393 lines V Back Porch = 0.924; (msec) = 104 lines --- drivers/gpu/drm/drm_edid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 53bc7a6..16991ea 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -409,7 +409,7 @@ static const struct drm_display_mode drm_dmt_modes[] = { DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) }, /* 1856x1392@75Hz */ { DRM_MODE("1856x1392", DRM_MODE_TYPE_DRIVER, 288000, 1856, 1984, - 2208, 2560, 0, 1392, 1395, 1399, 1500, 0, + 2208, 2560, 0, 1392, 1393, 1396, 1500, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) }, /* 1856x1392@120Hz RB */ { DRM_MODE("1856x1392", DRM_MODE_TYPE_DRIVER, 356500, 1856, 1904, -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/8] drm/i915/skl: Add support to load SKL CSR firmware
On 04/01/2015 04:08 PM, Damien Lespiau wrote: On Wed, Apr 01, 2015 at 01:18:25PM +0530, Animesh Manna wrote: +struct intel_css_header { Just a small question, what does CSS mean in this context? that's the first time I see it. CSS stands for "Code signing service". In case on Guc/Huc, firmware is signed and more relevant to have css_header. Though dmc firmware in not signed still taken thinking of later future and consistent with Guc/Huc. Regards, Animesh ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Kernel panic every other reboot/poweroff since 3.19.3 ( commit 9a6f5130143 )
On Tue, 31 Mar 2015, Steven Honeyman wrote: > On 31 March 2015 at 17:50, Matt Roper wrote: >> On Tue, Mar 31, 2015 at 08:54:19AM +0200, Daniel Vetter wrote: >>> Adding mailing lists (and hooray for me mixing up addresses, so now >>> there's a disclaimer at the bottom). >>> -Daniel >> >> It looks like this is caused by 3.19.3 having >> >> commit 77f7ef95e2cf09150e5777454fd5df69af39edcd >> Author: Chris Wilson >> Date: Wed Feb 25 13:45:26 2015 + >> >> drm: Don't assign fbs for universal cursor support to files >> >> without also having >> >> commit 8218c3f4df3bb1c637c17552405039a6dd3c1ee1 >> Author: Daniel Vetter >> Date: Fri Feb 27 12:58:13 2015 +0100 >> >> drm: Fixup racy refcounting in plane_force_disable >> >> >> Can you try cherry-picking 8218c3f4df3bb1c637c17552405039a6dd3c1ee1 and >> see if it solves the problem? >> >> >> Matt > > It does! Thanks Matt. > > 5 successful reboots and a successful shutdown. The only change > between the two kernels was the above patch. Stable team, please ensure 8218c3f4df3bb1c637c17552405039a6dd3c1ee1, already tagged cc: stable, is picked up for v3.19.4. This is also reported as Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=95621 Thanks, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC i-g-t v4] tests/gem_exec_pad_to_size: Test object padding at execbuf
On Thu, Apr 02, 2015 at 11:45:59AM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > This tests the new EXEC_OBJECT_PAD_TO_SIZE exec_object2 flag. > > Similar to some other tests, it uses knowledge of the DRM > allocation policy in order to get two objects mapped adjacent > to each other. It is then possible to verify that the pad to > size flag will move them apart. > > v2: Correct commit message. (Chris Wilson) > v3: Changes after code review by Chris Wilson. > * No need for gem_sync after execbuf. > * Drop caches before running. > * Allocate one additional bo per iteration. > * Don't explicitly set unused execbuf fields to zero. > * One improved comment. > v4: Require simpler object ordering and fixed overlap test. (Chris Wilson) > > Signed-off-by: Tvrtko Ursulin > Cc: Chris Wilson Thanks, that reads really well. Reviewed-by: Chris Wilson Hah, just thought of one minor tweak I would do though: if ((offsets[1] - offsets[0]) == PAGE_SIZE) { neighbours = true; break; } } /* Otherwise test can't confidently run. */ if (neighbours) { /* Check the object don't move by themselves */ igt_assert(exec(fd, eb_handles, pad_to_size, offsets) == 0); igt_assert(offsets[1] - offset[0] == PAGE_SIZE); /* Then re-exec with padding set, and now they should move. */ pad_to_size[0] = 2*PAGE_SIZE; igt_assert(exec(fd, eb_handles, pad_to_size, offsets) == 0); /* Check that objects with padding do not overlap. */ igt_assert(offsets[0] >= offsets[1] + PAGE_SIZE || offsets[0] + 2 * PAGE_SIZE <= offsets[1]); } -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/49] drm/i915: Re-enable RPS wait-boosting for all engines
On Friday 27 March 2015 04:31 PM, Chris Wilson wrote: This reverts commit ec5cc0f9b019af95e4571a9fa162d94294c8d90b Author: Chris Wilson Date: Thu Jun 12 10:28:55 2014 +0100 drm/i915: Restrict GPU boost to the RCS engine The premise that media/blitter workloads are not affected by boosting is patently false with a trip through igt. The question that remains is what exactly is going wrong with the media workload that prompted this? Hopefully that would be fixed by the missing agressive downclocking, in addition to the extra restrictions imposed on how frequent a process is allowed to boost. we may have to look at media workload. Last time when we observed that for a 1080p HD clip GPU freq was staying at Rp0 most of the time. Hopefully aggressive downclocking should help Acked-by: Deepak S Signed-off-by: Chris Wilson Cc: Deepak S Cc: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index d54f6a277d82..05f94ee8ea37 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1222,7 +1222,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req, timeout_expire = timeout ? jiffies + nsecs_to_jiffies_timeout((u64)*timeout) : 0; - if (ring->id == RCS && INTEL_INFO(dev)->gen >= 6) + if (INTEL_INFO(dev)->gen >= 6) gen6_rps_boost(dev_priv, file_priv); if (!irq_test_in_progress && WARN_ON(!ring->irq_get(ring))) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/49] drm/i915: Agressive downclocking on Baytrail
On Friday 27 March 2015 04:31 PM, Chris Wilson wrote: Reuse the same reclocking strategy for Baytail as on its bigger brethren, Sandybridge and Ivybridge. In particular, this makes the device quicker to reclock (both up and down) though the tendency now is to downclock more aggressively to compensate for the RPS boosts. v2: Rebase v3: Exclude Cherrytrail as Deepak was concerned that the increased number of register writes would wake the common powerwell too often. Signed-off-by: Chris Wilson Cc: Deepak S Cc: Ville Syrjälä Cc: Rodrigo Vivi Cc: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.h | 3 +++ drivers/gpu/drm/i915/i915_irq.c | 4 ++-- drivers/gpu/drm/i915/i915_reg.h | 2 -- drivers/gpu/drm/i915/intel_pm.c | 8 +++- 4 files changed, 12 insertions(+), 5 deletions(-) Looks fine to me Reviewed-by: Deepak S diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 701079429832..c80e2e5e591a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1033,6 +1033,9 @@ struct intel_gen6_power_mgmt { u8 rp0_freq;/* Non-overclocked max frequency. */ u32 cz_freq; + u8 up_threshold; /* Current %busy required to uplock */ + u8 down_threshold; /* Current %busy required to downclock */ + int last_adj; enum { LOW_POWER, BETWEEN, HIGH_POWER } power; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 14ecb4d13a1a..128a6f40b450 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1049,7 +1049,7 @@ static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir) if (pm_iir & GEN6_PM_RP_DOWN_EI_EXPIRED) { if (!vlv_c0_above(dev_priv, &dev_priv->rps.down_ei, &now, - VLV_RP_DOWN_EI_THRESHOLD)) + dev_priv->rps.down_threshold)) events |= GEN6_PM_RP_DOWN_THRESHOLD; dev_priv->rps.down_ei = now; } @@ -1057,7 +1057,7 @@ static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir) if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) { if (vlv_c0_above(dev_priv, &dev_priv->rps.up_ei, &now, -VLV_RP_UP_EI_THRESHOLD)) +dev_priv->rps.up_threshold)) events |= GEN6_PM_RP_UP_THRESHOLD; dev_priv->rps.up_ei = now; } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b522eb6e59a4..faf8f829e61f 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -671,8 +671,6 @@ enum skl_disp_power_wells { #define FB_FMAX_VMIN_FREQ_LO_MASK 0xf800 #define VLV_CZ_CLOCK_TO_MILLI_SEC 10 -#define VLV_RP_UP_EI_THRESHOLD 90 -#define VLV_RP_DOWN_EI_THRESHOLD 70 /* vlv2 north clock has */ #define CCK_FUSE_REG 0x8 diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa4ccb346389..65b33a4f82fc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3930,6 +3930,8 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val) GEN6_RP_DOWN_IDLE_AVG); dev_priv->rps.power = new_power; + dev_priv->rps.up_threshold = threshold_up; + dev_priv->rps.down_threshold = threshold_down; dev_priv->rps.last_adj = 0; } @@ -4001,8 +4003,11 @@ static void valleyview_set_rps(struct drm_device *dev, u8 val) "Odd GPU freq value\n")) val &= ~1; - if (val != dev_priv->rps.cur_freq) + if (val != dev_priv->rps.cur_freq) { vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val); + if (!IS_CHERRYVIEW(dev_priv)) + gen6_set_rps_thresholds(dev_priv, val); + } I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val)); @@ -4051,6 +4056,7 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv) & GENFREQSTATUS) == 0, 100)) DRM_ERROR("timed out waiting for Punit\n"); + gen6_set_rps_thresholds(dev_priv, val); vlv_force_gfx_clock(dev_priv, false); I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val)); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
On vblank instant-off systems, we can get into a situation where the cost of enabling and disabling the vblank IRQ around a drmWaitVblank query dominates. However, we know that if the user wants the current vblank counter, they are also very likely to immediately queue a vblank wait and so we can keep the interrupt around and only turn it off if we have no further vblank requests in the interrupt interval. After vblank event delivery there is a shadow of one vblank where the interrupt is kept alive for the user to query and queue another vblank event. Similarly, if the user is using blocking drmWaitVblanks, the interrupt will be disabled on the IRQ following the wait completion. However, if the user is simply querying the current vblank counter and timestamp, the interrupt will be disabled after every IRQ and the user will enabled it again on the first query following the IRQ. Testcase: igt/kms_vblank Signed-off-by: Chris Wilson Cc: Ville Syrjälä Cc: Daniel Vetter Cc: Michel Dänzer Cc: Laurent Pinchart Cc: Dave Airlie , Cc: Mario Kleiner --- drivers/gpu/drm/drm_irq.c | 15 +-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index c8a34476570a..6f5dc18779e2 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1091,9 +1091,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc) if (atomic_dec_and_test(&vblank->refcount)) { if (drm_vblank_offdelay == 0) return; - else if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0) + else if (drm_vblank_offdelay < 0) vblank_disable_fn((unsigned long)vblank); - else + else if (!dev->vblank_disable_immediate) mod_timer(&vblank->disable_timer, jiffies + ((drm_vblank_offdelay * HZ)/1000)); } @@ -1697,6 +1697,17 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) spin_lock_irqsave(&dev->event_lock, irqflags); + if (dev->vblank_disable_immediate && !atomic_read(&vblank->refcount)) { + unsigned long vbl_lock_irqflags; + + spin_lock_irqsave(&dev->vbl_lock, vbl_lock_irqflags); + if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) { + DRM_DEBUG("disabling vblank on crtc %d\n", crtc); + vblank_disable_and_save(dev, crtc); + } + spin_unlock_irqrestore(&dev->vbl_lock, vbl_lock_irqflags); + } + /* Need timestamp lock to prevent concurrent execution with * vblank enable/disable, as this would cause inconsistent * or corrupted timestamps and vblank counts. -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/49] drm/i915: Re-enable RPS wait-boosting for all engines
On Thu, Apr 02, 2015 at 04:39:56PM +0530, Deepak S wrote: > > > On Friday 27 March 2015 04:31 PM, Chris Wilson wrote: > >This reverts commit ec5cc0f9b019af95e4571a9fa162d94294c8d90b > >Author: Chris Wilson > >Date: Thu Jun 12 10:28:55 2014 +0100 > > > > drm/i915: Restrict GPU boost to the RCS engine > > > >The premise that media/blitter workloads are not affected by boosting is > >patently false with a trip through igt. The question that remains is > >what exactly is going wrong with the media workload that prompted this? > >Hopefully that would be fixed by the missing agressive downclocking, in > >addition to the extra restrictions imposed on how frequent a process is > >allowed to boost. > > we may have to look at media workload. Last time when we observed that for > a 1080p HD clip GPU freq was staying at Rp0 most of the time. > Hopefully aggressive downclocking should help > > Acked-by: Deepak S I think here what will help most is limiting the RPS boost to once per client (per busy period). I've actually found a couple of other places where we will artificially boost clocks: mmioflips and sw-semaphores. I've patches to also restrict those to once per busy period. The plan is that we only give RPS boosts to missed pageflips (via the vblank tracker) and only the first time a client stalls on a bo. I think with those in place, we can have the best of both worlds - instant boost for compute/gpu bound applications, and low render frequencies for sustained throughput. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/6] drm/i915: Don't use staged config in intel_mst_pre_enable_dp()
For the conversion to atomic. The pre_enable() hooks are called as part of the crtc enable sequence, at which point the staged config was already made effective. Furthermore, the function actually changes hardware state, so it should anyway deal with current and not staged config. Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_dp_mst.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 5329c85..adcc5e6 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -150,14 +150,14 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder) enum port port = intel_dig_port->port; int ret; uint32_t temp; - struct intel_connector *found = NULL, *intel_connector; + struct intel_connector *found = NULL, *connector; int slots; struct drm_crtc *crtc = encoder->base.crtc; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - for_each_intel_connector(dev, intel_connector) { - if (intel_connector->new_encoder == encoder) { - found = intel_connector; + for_each_intel_connector(dev, connector) { + if (connector->base.state->best_encoder == &encoder->base) { + found = connector; break; } } -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/6] drm/i915: Don't use staged config in check_encoder_cloning()
Reduce dependency on the staged config by using the atomic state instead. Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_display.c | 35 +-- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b1fbe9d..bc8e221 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10625,16 +10625,24 @@ static bool encoders_cloneable(const struct intel_encoder *a, b->cloneable & (1 << a->type)); } -static bool check_single_encoder_cloning(struct intel_crtc *crtc, +static bool check_single_encoder_cloning(struct drm_atomic_state *state, +struct intel_crtc *crtc, struct intel_encoder *encoder) { - struct drm_device *dev = crtc->base.dev; struct intel_encoder *source_encoder; + struct drm_connector_state *connector_state; + int i; - for_each_intel_encoder(dev, source_encoder) { - if (source_encoder->new_crtc != crtc) + for (i = 0; i < state->num_connector; i++) { + if (!state->connectors[i]) continue; + connector_state = state->connector_states[i]; + if (connector_state->crtc != &crtc->base) + continue; + + source_encoder = + to_intel_encoder(connector_state->best_encoder); if (!encoders_cloneable(encoder, source_encoder)) return false; } @@ -10642,16 +10650,23 @@ static bool check_single_encoder_cloning(struct intel_crtc *crtc, return true; } -static bool check_encoder_cloning(struct intel_crtc *crtc) +static bool check_encoder_cloning(struct drm_atomic_state *state, + struct intel_crtc *crtc) { - struct drm_device *dev = crtc->base.dev; struct intel_encoder *encoder; + struct drm_connector_state *connector_state; + int i; - for_each_intel_encoder(dev, encoder) { - if (encoder->new_crtc != crtc) + for (i = 0; i < state->num_connector; i++) { + if (!state->connectors[i]) continue; - if (!check_single_encoder_cloning(crtc, encoder)) + connector_state = state->connector_states[i]; + if (connector_state->crtc != &crtc->base) + continue; + + encoder = to_intel_encoder(connector_state->best_encoder); + if (!check_single_encoder_cloning(state, crtc, encoder)) return false; } @@ -10731,7 +10746,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, int i; bool retry = true; - if (!check_encoder_cloning(to_intel_crtc(crtc))) { + if (!check_encoder_cloning(state, to_intel_crtc(crtc))) { DRM_DEBUG_KMS("rejecting invalid cloning configuration\n"); return ERR_PTR(-EINVAL); } -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/6] drm/i915: Don't use staged config in check_digital_port_conflicts()
Reduce dependency on the staged config by using the atomic state instead. Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_display.c | 22 ++ 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 207c713..b1fbe9d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10658,23 +10658,30 @@ static bool check_encoder_cloning(struct intel_crtc *crtc) return true; } -static bool check_digital_port_conflicts(struct drm_device *dev) +static bool check_digital_port_conflicts(struct drm_atomic_state *state) { - struct intel_connector *connector; + struct drm_device *dev = state->dev; + struct intel_encoder *encoder; + struct drm_connector_state *connector_state; unsigned int used_ports = 0; + int i; /* * Walk the connector list instead of the encoder * list to detect the problem on ddi platforms * where there's just one encoder per digital port. */ - for_each_intel_connector(dev, connector) { - struct intel_encoder *encoder = connector->new_encoder; + for (i = 0; i < state->num_connector; i++) { + if (!state->connectors[i]) + continue; - if (!encoder) + connector_state = state->connector_states[i]; + if (!connector_state->best_encoder) continue; - WARN_ON(!encoder->new_crtc); + encoder = to_intel_encoder(connector_state->best_encoder); + + WARN_ON(!connector_state->crtc); switch (encoder->type) { unsigned int port_mask; @@ -10716,7 +10723,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_atomic_state *state) { - struct drm_device *dev = crtc->dev; struct intel_encoder *encoder; struct intel_connector *connector; struct drm_connector_state *connector_state; @@ -10730,7 +10736,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, return ERR_PTR(-EINVAL); } - if (!check_digital_port_conflicts(dev)) { + if (!check_digital_port_conflicts(state)) { DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); return ERR_PTR(-EINVAL); } -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/6] drm/i915: Don't use intel_crtc->new_config in pll calculation code
Move towards atomic by using the atomic state instead. Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_display.c | 15 +++ 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a6cd8c7..0b7ddee 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11581,10 +11581,11 @@ intel_modeset_compute_config(struct drm_crtc *crtc, return intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));; } -static int __intel_set_mode_setup_plls(struct drm_device *dev, +static int __intel_set_mode_setup_plls(struct drm_atomic_state *state, unsigned modeset_pipes, unsigned disable_pipes) { + struct drm_device *dev = state->dev; struct drm_i915_private *dev_priv = to_i915(dev); unsigned clear_pipes = modeset_pipes | disable_pipes; struct intel_crtc *intel_crtc; @@ -11598,9 +11599,15 @@ static int __intel_set_mode_setup_plls(struct drm_device *dev, goto done; for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) { - struct intel_crtc_state *state = intel_crtc->new_config; + struct intel_crtc_state *crtc_state = + intel_atomic_get_crtc_state(state, intel_crtc); + + /* Modeset pipes should have a new state by now */ + if (WARN_ON(IS_ERR(crtc_state))) + continue; + ret = dev_priv->display.crtc_compute_clock(intel_crtc, - state); + crtc_state); if (ret) { intel_shared_dpll_abort_config(dev_priv); goto done; @@ -11658,7 +11665,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, prepare_pipes &= ~disable_pipes; } - ret = __intel_set_mode_setup_plls(dev, modeset_pipes, disable_pipes); + ret = __intel_set_mode_setup_plls(state, modeset_pipes, disable_pipes); if (ret) goto done; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/6] Remove more staged config usage
These patches remove usage of the staged config from the modeset path that I overlooked in my previous patch series. Ander Conselvan de Oliveira (6): drm/i915: Don't use staged config for VLV cdclk calculations drm/i915: Don't use intel_crtc->new_config in pll calculation code drm/i915: Remove intel_crtc->new_config drm/i915: Don't use staged config in check_digital_port_conflicts() drm/i915: Don't use staged config in check_encoder_cloning() drm/i915: Don't use staged config in intel_mst_pre_enable_dp() drivers/gpu/drm/i915/intel_display.c | 158 --- drivers/gpu/drm/i915/intel_dp_mst.c | 8 +- drivers/gpu/drm/i915/intel_drv.h | 1 - 3 files changed, 95 insertions(+), 72 deletions(-) -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/6] drm/i915: Don't use staged config for VLV cdclk calculations
Now that we use a drm atomic state for the legacy modeset, it is possible to get rid of the usage of intel_crtc->new_config in the function intel_mode_max_pixclk(). Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_display.c | 55 +++- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 84f5b41..a6cd8c7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5160,36 +5160,48 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, } /* compute the max pixel clock for new configuration */ -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) +static int intel_mode_max_pixclk(struct drm_atomic_state *state) { - struct drm_device *dev = dev_priv->dev; + struct drm_device *dev = state->dev; struct intel_crtc *intel_crtc; + struct intel_crtc_state *crtc_state; int max_pixclk = 0; for_each_intel_crtc(dev, intel_crtc) { - if (intel_crtc->new_enabled) - max_pixclk = max(max_pixclk, - intel_crtc->new_config->base.adjusted_mode.crtc_clock); + crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + + if (!crtc_state->base.enable) + continue; + + max_pixclk = max(max_pixclk, +crtc_state->base.adjusted_mode.crtc_clock); } return max_pixclk; } -static void valleyview_modeset_global_pipes(struct drm_device *dev, +static int valleyview_modeset_global_pipes(struct drm_atomic_state *state, unsigned *prepare_pipes) { - struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_private *dev_priv = to_i915(state->dev); struct intel_crtc *intel_crtc; - int max_pixclk = intel_mode_max_pixclk(dev_priv); + int max_pixclk = intel_mode_max_pixclk(state); + + if (max_pixclk < 0) + return max_pixclk; if (valleyview_calc_cdclk(dev_priv, max_pixclk) == dev_priv->vlv_cdclk_freq) - return; + return 0; /* disable/enable all currently active pipes while we change cdclk */ - for_each_intel_crtc(dev, intel_crtc) + for_each_intel_crtc(state->dev, intel_crtc) if (intel_crtc->base.state->enable) *prepare_pipes |= (1 << intel_crtc->pipe); + + return 0; } static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv) @@ -5232,8 +5244,18 @@ static void valleyview_modeset_global_resources(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; struct drm_i915_private *dev_priv = dev->dev_private; - int max_pixclk = intel_mode_max_pixclk(dev_priv); - int req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk); + int max_pixclk = intel_mode_max_pixclk(state); + int req_cdclk; + + /* The only reason this can fail is if we fail to add the crtc_state +* to the atomic state. But that can't happen since the call to +* intel_mode_max_pixclk() in valleyview_modeset_global_pipes() (which +* can't have failed otherwise the mode set would be aborted) added all +* the states already. */ + if (WARN_ON(max_pixclk < 0)) + return; + + req_cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk); if (req_cdclk != dev_priv->vlv_cdclk_freq) { /* @@ -11550,6 +11572,8 @@ intel_modeset_compute_config(struct drm_crtc *crtc, if (IS_ERR(pipe_config)) return pipe_config; + pipe_config->base.enable = true; + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]"); } @@ -11598,6 +11622,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct drm_display_mode *saved_mode; + struct drm_atomic_state *state = pipe_config->base.state; struct intel_crtc_state *crtc_state_copy = NULL; struct intel_crtc *intel_crtc; int ret = 0; @@ -11625,7 +11650,9 @@ static int __intel_set_mode(struct drm_crtc *crtc, * adjusted_mode bits in the crtc directly. */ if (IS_VALLEYVIEW(dev)) { - valleyview_modeset_global_pipes(dev, &prepare_pipes); + ret = valleyview_modeset_global_pipes(state, &prepare_pipes); + if (ret) + goto done; /* may have added more to prepare_pipes than we should */
[Intel-gfx] [PATCH 3/6] drm/i915: Remove intel_crtc->new_config
It's not needed anymore, now that all the users were converted to using an atomic state. Signed-off-by: Ander Conselvan de Oliveira --- drivers/gpu/drm/i915/intel_display.c | 31 --- drivers/gpu/drm/i915/intel_drv.h | 1 - 2 files changed, 32 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0b7ddee..207c713 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9171,7 +9171,6 @@ retry: intel_crtc = to_intel_crtc(crtc); intel_crtc->new_enabled = true; - intel_crtc->new_config = intel_crtc->config; old->dpms_mode = connector->dpms; old->load_detect_temp = true; old->release_fb = NULL; @@ -9227,10 +9226,6 @@ retry: fail: intel_crtc->new_enabled = crtc->state->enable; - if (intel_crtc->new_enabled) - intel_crtc->new_config = intel_crtc->config; - else - intel_crtc->new_config = NULL; fail_unlock: if (state) { drm_atomic_state_free(state); @@ -9276,7 +9271,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, to_intel_connector(connector)->new_encoder = NULL; intel_encoder->new_crtc = NULL; intel_crtc->new_enabled = false; - intel_crtc->new_config = NULL; connector_state->best_encoder = NULL; connector_state->crtc = NULL; @@ -10416,11 +10410,6 @@ static void intel_modeset_update_staged_output_state(struct drm_device *dev) for_each_intel_crtc(dev, crtc) { crtc->new_enabled = crtc->base.state->enable; - - if (crtc->new_enabled) - crtc->new_config = crtc->config; - else - crtc->new_config = NULL; } } @@ -10981,9 +10970,6 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes) /* Double check state. */ for_each_intel_crtc(dev, intel_crtc) { WARN_ON(intel_crtc->base.state->enable != intel_crtc_in_use(&intel_crtc->base)); - WARN_ON(intel_crtc->new_config && - intel_crtc->new_config != intel_crtc->config); - WARN_ON(intel_crtc->base.state->enable != !!intel_crtc->new_config); } list_for_each_entry(connector, &dev->mode_config.connector_list, head) { @@ -11646,9 +11632,6 @@ static int __intel_set_mode(struct drm_crtc *crtc, *saved_mode = crtc->mode; - if (modeset_pipes) - to_intel_crtc(crtc)->new_config = pipe_config; - /* * See if the config requires any additional preparation, e.g. * to adjust global state with pipes off. We need to do this @@ -11741,9 +11724,6 @@ done: sizeof *crtc_state_copy); intel_crtc->config = crtc_state_copy; intel_crtc->base.state = &crtc_state_copy->base; - - if (modeset_pipes) - intel_crtc->new_config = intel_crtc->config; } else { kfree(crtc_state_copy); } @@ -11922,11 +11902,6 @@ static void intel_set_config_restore_state(struct drm_device *dev, count = 0; for_each_intel_crtc(dev, crtc) { crtc->new_enabled = config->save_crtc_enabled[count++]; - - if (crtc->new_enabled) - crtc->new_config = crtc->config; - else - crtc->new_config = NULL; } count = 0; @@ -12153,11 +12128,6 @@ intel_modeset_stage_output_state(struct drm_device *dev, crtc->new_enabled ? "en" : "dis"); config->mode_changed = true; } - - if (crtc->new_enabled) - crtc->new_config = crtc->config; - else - crtc->new_config = NULL; } return 0; @@ -12184,7 +12154,6 @@ static void disable_crtc_nofb(struct intel_crtc *crtc) } crtc->new_enabled = false; - crtc->new_config = NULL; } static int intel_crtc_set_config(struct drm_mode_set *set) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4799b11..686014b 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -471,7 +471,6 @@ struct intel_crtc { struct intel_initial_plane_config plane_config; struct intel_crtc_state *config; - struct intel_crtc_state *new_config; bool new_enabled; /* reset counter value when the last flip was submitted */ -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Correct drm display mode table about 1856x1392 @75Hz mode
Please prefix the subject with "drm/edid: " and send the patch to dri-de...@lists.freedesktop.org. In general, scripts/get_maintainer.pl will tell you where to send the patches; intel-gfx is for stuff under drivers/gpu/drm/i915. Thanks, Jani. On Thu, 02 Apr 2015, "liu,lei" wrote: > From: "liu,lei" > > According DMT spec, vss of this mode should be 1393, vse should > be 1396. > > VESA MONITOR TIMING STANDARD: > > Timing Name = 1856 x 1392 @ 75Hz > Hor Total Time = 8.889; (usec) = 320 chars = 2560 Pixels > Hor Sync Start = 6.889; (usec) = 248 chars = 1984 Pixels > H Back Porch = 1.222; (usec) = 44 chars = 352 Pixels > > Ver Total Time = 13.333; (msec) = 1500 lines HT – (1.06xHA) > Ver Sync Start = 12.382; (msec) = 1393 lines > V Back Porch = 0.924; (msec) = 104 lines > --- > drivers/gpu/drm/drm_edid.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 53bc7a6..16991ea 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -409,7 +409,7 @@ static const struct drm_display_mode drm_dmt_modes[] = { > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) }, > /* 1856x1392@75Hz */ > { DRM_MODE("1856x1392", DRM_MODE_TYPE_DRIVER, 288000, 1856, 1984, > -2208, 2560, 0, 1392, 1395, 1399, 1500, 0, > +2208, 2560, 0, 1392, 1393, 1396, 1500, 0, > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) }, > /* 1856x1392@120Hz RB */ > { DRM_MODE("1856x1392", DRM_MODE_TYPE_DRIVER, 356500, 1856, 1904, > -- > 1.8.3.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] drm-intel-fixes
Hi Dave - A colorkey ioctl oops fix from Ville, cc: stable, and a backport of Chris' 0-length batch allocation fix from drm-next. BR, Jani. The following changes since commit e42391cd048809d903291d07f86ed3934ce138e9: Linux 4.0-rc6 (2015-03-29 15:26:31 -0700) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/drm-intel-fixes-2015-04-02 for you to fetch changes up to 840a1cf0cd533f30da792527ca5ff6a023d4a727: drm/i915: Reject the colorkey ioctls for primary and cursor planes (2015-04-02 11:25:50 +0300) Chris Wilson (1): drm/i915: Skip allocating shadow batch for 0-length batches Ville Syrjälä (1): drm/i915: Reject the colorkey ioctls for primary and cursor planes drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- drivers/gpu/drm/i915/intel_sprite.c| 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Allow legacy interface for legacy Y tiled display
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6113 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -2 272/272 270/272 ILK -1 302/302 301/302 SNB 303/303 303/303 IVB 338/338 338/338 BYT -1 287/287 286/287 HSW 361/361 361/361 BDW 308/308 308/308 -Detailed- Platform Testdrm-intel-nightly Series Applied PNV igt@gem_tiled_pread_pwrite FAIL(3)PASS(7) FAIL(1)PASS(1) PNV igt@gem_userptr_blits@coherency-sync CRASH(5)PASS(5) CRASH(2) *ILK igt@gem_fenced_exec_thrash@no-spare-fences-busy-interruptible PASS(2) DMESG_WARN(1)PASS(1) (dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...bsd_ring_idle@Hangcheck timer elapsed... bsd ring idle *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(8) FAIL(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC i-g-t v5] tests/gem_exec_pad_to_size: Test object padding at execbuf
From: Tvrtko Ursulin This tests the new EXEC_OBJECT_PAD_TO_SIZE exec_object2 flag. Similar to some other tests, it uses knowledge of the DRM allocation policy in order to get two objects mapped adjacent to each other. It is then possible to verify that the pad to size flag will move them apart. v2: Correct commit message. (Chris Wilson) v3: Changes after code review by Chris Wilson. * No need for gem_sync after execbuf. * Drop caches before running. * Allocate one additional bo per iteration. * Don't explicitly set unused execbuf fields to zero. * One improved comment. v4: Require simpler object ordering and fixed overlap test. (Chris Wilson) v5: Check unpadded offsets once more before padding. (Chris Wilson) Signed-off-by: Tvrtko Ursulin Reviewed-by: Chris Wilson Cc: Chris Wilson --- tests/Makefile.sources | 1 + tests/gem_exec_pad_to_size.c | 240 +++ 2 files changed, 241 insertions(+) create mode 100644 tests/gem_exec_pad_to_size.c diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 0a974a6..5f21728 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -34,6 +34,7 @@ TESTS_progs_M = \ gem_exec_bad_domains \ gem_exec_faulting_reloc \ gem_exec_nop \ + gem_exec_pad_to_size \ gem_exec_params \ gem_exec_parse \ gem_fenced_exec_thrash \ diff --git a/tests/gem_exec_pad_to_size.c b/tests/gem_exec_pad_to_size.c new file mode 100644 index 000..87afe40 --- /dev/null +++ b/tests/gem_exec_pad_to_size.c @@ -0,0 +1,240 @@ +/* + * Copyright © 2015 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Tvrtko Ursulin + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "drm.h" +#include "ioctl_wrappers.h" +#include "igt_core.h" +#include "drmtest.h" +#include "intel_reg.h" +#include "igt_debugfs.h" + +#ifndef PAGE_SIZE +#define PAGE_SIZE 4096 +#endif + +struct local_drm_i915_gem_exec_object2 { + /** +* User's handle for a buffer to be bound into the GTT for this +* operation. +*/ + __u32 handle; + + /** Number of relocations to be performed on this buffer */ + __u32 relocation_count; + /** +* Pointer to array of struct drm_i915_gem_relocation_entry containing +* the relocations to be performed in this buffer. +*/ + __u64 relocs_ptr; + + /** Required alignment in graphics aperture */ + __u64 alignment; + + /** +* Returned value of the updated offset of the object, for future +* presumed_offset writes. +*/ + __u64 offset; + +#define LOCAL_EXEC_OBJECT_NEEDS_FENCE (1<<0) +#define LOCAL_EXEC_OBJECT_NEEDS_GTT(1<<1) +#define LOCAL_EXEC_OBJECT_WRITE(1<<2) +#define LOCAL_EXEC_OBJECT_PAD_TO_SIZE (1<<3) +#define __LOCAL_EXEC_OBJECT_UNKNOWN_FLAGS -(LOCAL_EXEC_OBJECT_PAD_TO_SIZE<<1) + __u64 flags; + + __u64 pad_to_size; + __u64 rsvd2; +}; + +static int +exec(int fd, uint32_t handles[3], uint32_t pad_to_size[2], uint64_t *offsets) +{ + struct drm_i915_gem_execbuffer2 execbuf; + struct local_drm_i915_gem_exec_object2 gem_exec[3]; + int ret = 0; + + memset(gem_exec, 0, sizeof(gem_exec)); + + gem_exec[0].handle = handles[1]; + gem_exec[0].flags = pad_to_size[0] ? + LOCAL_EXEC_OBJECT_PAD_TO_SIZE : 0; + gem_exec[0].pad_to_size = pad_to_size[0]; + + gem_exec[1].handle = handles[2]; + gem_exec[1].flags = pad_to_size[1] ? + LOCAL_EXEC_OBJECT_PAD_TO_SIZE : 0; + gem_exec[1].pad_to_size = pad_to_size[1]; + + gem_exec[2].handle = handles[0]; + + memset(&execbuf, 0, sizeof(execbuf)); + + execbuf.buffers_ptr =
Re: [Intel-gfx] looks like a issue in do_switch() and mi_set_context() in i915_gem_context.c?
Hi Chris: I begin to understand that before the "prev" context object is unpinned, it's set to active by i915_vma_move_to_active, so the shrinker will wait for it. Thanks for the help. Every time I learned a lot from you. Thanks. :) -Original Message- From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] Sent: Thursday, April 02, 2015 3:18 PM To: Wang, Zhi A Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] looks like a issue in do_switch() and mi_set_context() in i915_gem_context.c? On Wed, Apr 01, 2015 at 08:01:56PM +0800, Zhi Wang wrote: > Hi Chris: > Thanks for the reply. :) I can understand that the backing storage > is pinned at this time, as the reference counter of context object > should not be zero. But for VMA, is there any chance that the vma will > be unbinded from GGTT at this time by shrinker? I saw that > i915_gem_object_ggtt_unpin() will decrease the VMA reference > counter... In order for the shrinker to evict an active object, it must first wait upon it. (So the shrinker will only do so as a last gasp measure.) Once the vma is unbound, we know that the GPU will have switched contexts away from the vma (because the last request that we waited upon for the vma included the instructions to do the switch away) and so the pages are swappable. This obviously relies on the hardware being correct... As would waiting upon the CCID! -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Clean-up idr table if context create fails.
On Monday 30 March 2015 09:13 PM, Daniel Vetter wrote: On Mon, Mar 30, 2015 at 08:03:58PM +0530, deepa...@linux.intel.com wrote: From: Deepak S Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context() Signed-off-by: Deepak S --- drivers/gpu/drm/i915/i915_gem_context.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index f3e84c4..69bebe5 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -287,6 +287,8 @@ err_unpin: if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); err_destroy: + if (ctx->file_priv) + idr_remove(&ctx->file_priv->context_idr, ctx->user_handle); The common approach is to add a new err_idr: label at the op of the unwind code and make the call to idr_remove unconditional. Thanks, Daniel Thanks Daniel for review. I do not think we can have a unconditional idr remove since for global ctx i915_gem_create_context called with file_priv=NULL? - Thanks, Deepak i915_gem_context_unreference(ctx); return ERR_PTR(ret); } -- 1.9.1 ___ 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
[Intel-gfx] [PATCH v2] drm/i915: Clean-up idr table if context create fails.
From: Deepak S Cleanup idr table if any error happens after __create_hw_context() in i915_gem_create_context() v2: add a new err_idr (Daniel) Signed-off-by: Deepak S --- drivers/gpu/drm/i915/i915_gem_context.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index f3e84c4..9b425a3 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -262,7 +262,7 @@ i915_gem_create_context(struct drm_device *dev, get_context_alignment(dev), 0); if (ret) { DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); - goto err_destroy; + goto err_idr; } } @@ -286,7 +286,9 @@ i915_gem_create_context(struct drm_device *dev, err_unpin: if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); -err_destroy: +err_idr: + if (ctx->file_priv) + idr_remove(&ctx->file_priv->context_idr, ctx->user_handle); i915_gem_context_unreference(ctx); return ERR_PTR(ret); } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Correct drm display mode table about 1856x1392 @75Hz mode
On Thu, Apr 02, 2015 at 06:54:34PM +0800, liu,lei wrote: > From: "liu,lei" > > According DMT spec, vss of this mode should be 1393, vse should > be 1396. > > VESA MONITOR TIMING STANDARD: > > Timing Name = 1856 x 1392 @ 75Hz > Hor Total Time = 8.889; (usec) = 320 chars = 2560 Pixels > Hor Sync Start = 6.889; (usec) = 248 chars = 1984 Pixels > H Back Porch = 1.222; (usec) = 44 chars = 352 Pixels > > Ver Total Time = 13.333; (msec) = 1500 lines HT – (1.06xHA) > Ver Sync Start = 12.382; (msec) = 1393 lines > V Back Porch = 0.924; (msec) = 104 lines > --- > drivers/gpu/drm/drm_edid.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 53bc7a6..16991ea 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -409,7 +409,7 @@ static const struct drm_display_mode drm_dmt_modes[] = { > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) }, > /* 1856x1392@75Hz */ > { DRM_MODE("1856x1392", DRM_MODE_TYPE_DRIVER, 288000, 1856, 1984, > -2208, 2560, 0, 1392, 1395, 1399, 1500, 0, > +2208, 2560, 0, 1392, 1393, 1396, 1500, 0, That is indeed what the spec says. However to review this I wrote a quick awk script to parse the spec, and that shows a few more differences. I'll toss out a patch with the results of my script... > DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC) }, > /* 1856x1392@120Hz RB */ > { DRM_MODE("1856x1392", DRM_MODE_TYPE_DRIVER, 356500, 1856, 1904, > -- > 1.8.3.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC] drm/i915: _wait_for might be called when irq is off
From: Deepak S Sometimes, i915 might call _wait_for when irq is disabled. If the cpu is the main cpu to process jiffies, jiffies wouldn't be increased as this cpu disables irq. Then, time_after(jiffies, timeout__) becomes meaningless. If gunit doesn't work now, kernel wouldn't exit as the timeout doesn't work. The patch fixes it by using sched_clock instead of jiffies. Signed-off-by: Deepak S --- drivers/gpu/drm/i915/intel_drv.h | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 6036e3b..2c6ebce 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -49,10 +49,12 @@ * we've never had a chance to check the condition before the timeout. */ #define _wait_for(COND, MS, W) ({ \ - unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1; \ + u64 timeout_ = sched_clock() + MS * ((u64) NSEC_PER_MSEC); \ + u64 clock; \ int ret__ = 0; \ while (!(COND)) { \ - if (time_after(jiffies, timeout__)) { \ + clock = sched_clock(); \ + if (clock >= timeout_) {\ if (!(COND))\ ret__ = -ETIMEDOUT; \ break; \ -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 20/20] drm/i915: Enable skylake sprite plane scaling using shared scalers
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6118 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -1 272/272 271/272 ILK 302/302 302/302 SNB 303/303 303/303 IVB 338/338 338/338 BYT -1 287/287 286/287 HSW 361/361 361/361 BDW 308/308 308/308 -Detailed- Platform Testdrm-intel-nightly Series Applied PNV igt@gen3_render_tiledx_blits FAIL(6)PASS(3) FAIL(2) *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(9) FAIL(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Allow userptr backchannel for passing aroung GTT mappings
Once userptr becomes part of client API, it is almost a certainly that eventually someone will try to create a new object from a mapping of another client object, e.g. new = vaImport(vaMap(old, &size), size); (using a hypothethical API, not meaning to pick on anyone!) Since this is actually fairly safe to implement and to allow (since it is within a single process space and the memory access passes the standard permissions test) let us not limit the Client possibilities. Signed-off-by: Chris Wilson Cc: Gwenole Beauchesne Cc: Michał Winiarski Cc: Tvrtko Ursulin --- drivers/gpu/drm/i915/i915_gem_userptr.c | 46 ++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c index d96276caab49..8031ebe424fd 100644 --- a/drivers/gpu/drm/i915/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c @@ -750,6 +750,35 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { .release = i915_gem_userptr_release, }; +static struct drm_i915_gem_object * +find_object_from_vma(struct drm_device *dev, +struct drm_i915_gem_userptr *args) +{ + struct drm_i915_gem_object *obj = NULL; + struct vm_area_struct *vma; + + down_read(¤t->mm->mmap_sem); + vma = find_vma(current->mm, args->user_ptr); + if (vma == NULL) + goto out; + + if (vma->vm_ops != dev->driver->gem_vm_ops) + goto out; + + if (vma->vm_start != args->user_ptr || + vma->vm_end != args->user_ptr + args->user_size) { + obj = ERR_PTR(-EINVAL); + goto out; + } + + obj = to_intel_bo(vma->vm_private_data); + drm_gem_object_reference(obj); + +out: + up_read(¤t->mm->mmap_sem); + return obj; +} + /** * Creates a new mm object that wraps some normal memory from the process * context - user memory. @@ -757,8 +786,11 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { * We impose several restrictions upon the memory being mapped * into the GPU. * 1. It must be page aligned (both start/end addresses, i.e ptr and size). - * 2. It must be normal system memory, not a pointer into another map of IO - *space (e.g. it must not be a GTT mmapping of another object). + * 2. It must either be: + *a) normal system memory, not a pointer into another map of IO + * space (e.g. it must not be part of a GTT mmapping of another object). + *b) a pointer to the complete GTT mmap of another object in your + * address space. * 3. We only allow a bo as large as we could in theory map into the GTT, *that is we limit the size to the total size of the GTT. * 4. The bo is marked as being snoopable. The backing pages are left @@ -812,6 +844,14 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file return -ENODEV; } + obj = find_object_from_vma(dev, args); + if (obj) { + if (IS_ERR(obj)) + return PTR_ERR(obj); + else + goto out; + } + obj = i915_gem_object_alloc(dev); if (obj == NULL) return -ENOMEM; @@ -833,7 +873,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file if (ret == 0) ret = i915_gem_userptr_init__mmu_notifier(obj, args->flags); if (ret == 0) - ret = drm_gem_handle_create(file, &obj->base, &handle); +out:ret = drm_gem_handle_create(file, &obj->base, &handle); /* drop reference from allocate - handle holds it now */ drm_gem_object_unreference_unlocked(&obj->base); -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/8] drm/i915/skl: Add support to load SKL CSR firmware
On ke, 2015-04-01 at 16:22 +0530, Animesh Manna wrote: > From: "A.Sunil Kamath" > > Display Context Save and Restore support is needed for > various SKL Display C states like DC5, DC6. > > This implementation is added based on first version of DMC CSR program > that we received from h/w team. > > Here we are using request_firmware based design. > Finally this firmware should end up in linux-firmware tree. > > For SKL platform its mandatory to ensure that we load this > csr program before enabling DC states like DC5/DC6. > > As CSR program gets reset on various conditions, we should ensure > to load it during boot and in future change to be added to load > this system resume sequence too. > > v1: Initial relese as RFC patch > > v2: Design change as per Daniel, Damien and Shobit's review comments > request firmware method followed. > > v3: Some optimization and functional changes. > Pulled register defines into drivers/gpu/drm/i915/i915_reg.h > Used kmemdup to allocate and duplicate firmware content. > Ensured to free allocated buffer. > > v4: Modified as per review comments from Satheesh and Daniel > Removed temporary buffer. > Optimized number of writes by replacing I915_WRITE with I915_WRITE64. > > v5: > Modified as per review comemnts from Damien. > - Changed name for functions and firmware. > - Introduced HAS_CSR. > - Reverted back previous change and used csr_buf with u8 size. > - Using cpu_to_be64 for endianness change. > > Modified as per review comments from Imre. > - Modified registers and macro names to be a bit closer to bspec terminology > and the existing register naming in the driver. > - Early return for non SKL platforms in intel_load_csr_program function. > - Added locking around CSR program load function as it may be called > concurrently during system/runtime resume. > - Releasing the fw before loading the program for consistency > - Handled error path during f/w load. > > v6: Modified as per review comments from Imre. > - Corrected out_freecsr sequence. > > v7: Modified as per review comments from Imre. > Fail loading fw if fw->size%8!=0. > > v8: Rebase to latest. > > v9: Rebase on top of -nightly (Damien) > > v10: Enabled support for dmc firmware ver 1.0. > According to ver 1.0 in a single binary package all the firmware's that are > required for different stepping's of the product will be stored. The package > contains the css header, followed by the package header and the actual dmc > firmwares. Package header contains the firmware/stepping mapping table and > the corresponding firmware offsets to the individual binaries, within the > package. Each individual program binary contains the header and the payload > sections whose size is specified in the header section. This changes are done > to extract the specific firmaware from the package. (Animesh) > > v11: Modified as per review comemnts from Imre. > - Added code comment from bpec for header structure elements. > - Added __packed to avoid structure padding. > - Added helper functions for stepping and substepping info. > - Added code comment for CSR_MAX_FW_SIZE. > - Disabled BXT firmware loading, will be enabled with dmc 1.0 support. > - Changed skl_stepping_info based on bspec, earlier used from config DB. > - Removed duplicate call of cpu_to_be* from intel_csr_load_program function. > - Used cpu_to_be32 instead of cpu_to_be64 as firmware binary in dword aligned. > - Added sanity check for header length. > - Added sanity check for mmio address got from firmware binary. > - kmalloc done separately for dmc header and dmc firmware. (Animesh) > > v12: Modified as per review comemnts from Imre. > - Corrected the typo error in skl stepping info structure. > - Added out-of-bound access for skl_stepping_info. > - Sanity check for mmio address modified. > - Sanity check added for stepping and substeppig. > - Modified the intel_dmc_info structure, cache only the required header info. > (Animesh) > > v13: clarify firmware load error message. > The reason for a firmware loading failure can be obscure if the driver > is built-in. Provide an explanation to the user about the likely reason for > the failure and how to resolve it. (Imre) > > v14: Suggested by Jani. > - fix s/I915/CONFIG_DRM_I915/ typo > - add fw_path to the firmware object instead of using a static ptr (Jani) > > Issue: VIZ-2569 > Signed-off-by: A.Sunil Kamath > Signed-off-by: Damien Lespiau > Signed-off-by: Animesh Manna > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/Makefile| 3 +- > drivers/gpu/drm/i915/i915_dma.c | 12 +- > drivers/gpu/drm/i915/i915_drv.c | 20 +++ > drivers/gpu/drm/i915/i915_drv.h | 132 > drivers/gpu/drm/i915/i915_reg.h | 18 +++ > drivers/gpu/drm/i915/intel_csr.c | 262 > +++ > drivers/gpu/drm/i915/intel_drv.h | 5 + > 7 files changed, 450 insertions(+), 2 deletions(-) > create mode 100644 drivers/gpu/drm/i915/intel_csr.c > > diff --git a/drivers/
[Intel-gfx] [PATCH] drm/i915: Silence a sparse warning
From: Ville Syrjälä ../drivers/gpu/drm/i915/intel_pm.c:3185:45: warning: Initializer entry defined twice ../drivers/gpu/drm/i915/intel_pm.c:3185:52: also defined here Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_pm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 8775509..b31b204 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3182,7 +3182,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv, { struct drm_device *dev = dev_priv->dev; struct skl_ddb_allocation *cur_ddb, *new_ddb; - bool reallocated[I915_MAX_PIPES] = {false, false, false}; + bool reallocated[I915_MAX_PIPES] = {}; struct intel_crtc *crtc; enum pipe pipe; -- 2.0.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] drm/i915: _wait_for might be called when irq is off
On Thu, Apr 02, 2015 at 07:21:40PM +0530, deepa...@linux.intel.com wrote: > From: Deepak S > > Sometimes, i915 might call _wait_for when irq is disabled. > If the cpu is the main cpu to process jiffies, jiffies > wouldn't be increased as this cpu disables irq. Then, > time_after(jiffies, timeout__) becomes meaningless. If > gunit doesn't work now, kernel wouldn't exit as the timeout > doesn't work. > > The patch fixes it by using sched_clock instead of jiffies. sched_clock() requires irq disabled, or at least so the header claims, at the very least it would require preemption disabled - definitely not for our general waits of many ms. Also local_clock() would seem to be the right choice in these tight loops. I think you want a specialised macro (if any) that is very aware of the constraints it is running under. I would hope we never have to busy spin with interrupts disabled. And so I want such code that does to blatantly obvious and scrutinised carefully. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/skl: Allow universal planes to position
On Thu, Apr 02, 2015 at 10:08:27AM +0530, Jindal, Sonika wrote: > > > On 4/1/2015 11:51 PM, Matt Roper wrote: > >On Mon, Mar 30, 2015 at 02:04:56PM +0530, Sonika Jindal wrote: > >>Signed-off-by: Sonika Jindal > > > >It looks like this is dependent on Ville's patch > > > > [PATCH v2 6/9] drm/i915: Pass the primary plane position to > > .update_primary_plane() > > > >to actually let us do something sensible with the destination rectangle > >at the hardware level. Looks like that patch has a r-b, but hasn't made > >it into di-nightly yet. > > > Right now, can_position is used to check for the scenarios where the > primary plane is not covering the complete crtc. This could be due > to positioning or a smaller fb on primary plane. > With Ville's patch, we would be able to allow positioning to happen. > But I need it here, to create a smaller fb for 90/270 rotation. > > I agree that, until Ville's patch is there, we won't be entertaining > any positioning requests on the primary plane and we will not be > throwing any error also. Right...and I think failing to throw an error would be seen as a bug, which is why I think Ville's patch needs to go in first. Since it's already been reviewed, I'm not aware of anything holding that up from happening. > But for the 90/270 testcase in kms_rotation_crc to go through, we > would need this to create a smaller fb so that we can rotate it. So is your worry here that drm_plane_helper_check_update() doesn't understand rotation and winds up mixing up width/height? If so, I think the proper course of action is to write a patch for the helper function that makes it rotation-aware. Matt > > >Assuming Ville's patch lands first, this is > >Reviewed-by: Matt Roper > > > > > >Matt > > > >>--- > >> drivers/gpu/drm/i915/intel_display.c |7 ++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/gpu/drm/i915/intel_display.c > >>b/drivers/gpu/drm/i915/intel_display.c > >>index ceb2e61..f0bbc22 100644 > >>--- a/drivers/gpu/drm/i915/intel_display.c > >>+++ b/drivers/gpu/drm/i915/intel_display.c > >>@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane, > >>struct drm_rect *dest = &state->dst; > >>struct drm_rect *src = &state->src; > >>const struct drm_rect *clip = &state->clip; > >>+ bool can_position = false; > >>int ret; > >> > >>crtc = crtc ? crtc : plane->crtc; > >>intel_crtc = to_intel_crtc(crtc); > >> > >>+ if (INTEL_INFO(dev)->gen >= 9) > >>+ can_position = true; > >>+ > >>ret = drm_plane_helper_check_update(plane, crtc, fb, > >>src, dest, clip, > >>DRM_PLANE_HELPER_NO_SCALING, > >>DRM_PLANE_HELPER_NO_SCALING, > >>- false, true, &state->visible); > >>+ can_position, true, > >>+ &state->visible); > >>if (ret) > >>return ret; > >> > >>-- > >>1.7.10.4 > >> > >>___ > >>Intel-gfx mailing list > >>Intel-gfx@lists.freedesktop.org > >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/8] drm/i915/skl: Implement enable/disable for Display C5 sttate.
On ke, 2015-04-01 at 16:22 +0530, Animesh Manna wrote: > From: "A.Sunil Kamath" > > This patch just implements the basic enable and disable > functions of DC5 state which is needed for both SKL and BXT. > > Its important to load respective CSR program before calling > enable, which anyways will happen as CSR program is executed > during boot. > > DC5 is a power saving state where hardware dynamically disables > power well 1 and the CDCLK PLL and saves the associated registers. > > DC5 can be entered when software allows it, power well 2 is > disabled, and hardware detects that all pipes are disabled > or pipe A is enabled with PSR active. > > Its better to configure display engine to have power well 2 disabled before > getting into DC5 enable function. Hence rpm framework will have to > ensure to check status of power well 2 before calling gen9_enable_dc5. > > Rather dc5 entry criteria should be decided based on power well 2 status. > If disabled, then call gen9_enable_dc5. > > v2: Replace HAS_ with IS_ check as per Daniel's review comments > > v3: Cleared the bits dc5/dc6 enable of DC_STATE_EN register > before setting them as per Satheesh's review comments. > > v4: call POSTING_READ for every write to a register to ensure that > its written immediately. > > v5: Modified as per review comments from Imre. > - Squashed register definitions into this patch. > - Finetuned comments and functions. > > v6: > Avoid redundant writes in gen9_set_dc_state_debugmask_memory_up function. > > v7: > 1] Rebase to latest. > 2] Move all runtime PM functions defined in intel_display.c to >intel_runtime_pm.c. > > v8: Rebased to drm-intel-nightly. (Animesh) > > Issue: VIZ-2819 > Signed-off-by: A.Sunil Kamath > Signed-off-by: Damien Lespiau > Signed-off-by: Animesh Manna Reviewed-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_reg.h | 11 > drivers/gpu/drm/i915/intel_runtime_pm.c | 47 > + > 2 files changed, 58 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 77faa2b..d064e95 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6794,6 +6794,17 @@ enum skl_disp_power_wells { > #define CSR_MMIO_END_RANGE 0x8 > #define CSR_MAX_MMIO_COUNT 8 > > +/* > +* SKL DC > +*/ > +#define DC_STATE_EN 0x45504 > +#define DC_STATE_EN_UPTO_DC5(1<<0) > +#define DC_STATE_EN_UPTO_DC6(2<<0) > +#define DC_STATE_EN_UPTO_DC5_DC6_MASK 0x3 > + > +#define DC_STATE_DEBUG 0x45520 > +#define DC_STATE_DEBUG_MASK_MEMORY_UP (1<<1) > + > /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this > register, > * since on HSW we can't write to it using I915_WRITE. */ > #define D_COMP_HSW (MCHBAR_MIRROR_BASE_SNB + 0x5F0C) > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index ce00e69..bc6cee9 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -319,6 +319,53 @@ static void hsw_set_power_well(struct drm_i915_private > *dev_priv, > SKL_DISPLAY_MISC_IO_POWER_DOMAINS)) | \ > BIT(POWER_DOMAIN_INIT)) > > +static void gen9_set_dc_state_debugmask_memory_up( > + struct drm_i915_private *dev_priv) > +{ > + uint32_t val; > + > + /* The below bit doesn't need to be cleared ever afterwards */ > + val = I915_READ(DC_STATE_DEBUG); > + if (!(val & DC_STATE_DEBUG_MASK_MEMORY_UP)) { > + val |= DC_STATE_DEBUG_MASK_MEMORY_UP; > + I915_WRITE(DC_STATE_DEBUG, val); > + POSTING_READ(DC_STATE_DEBUG); > + } > +} > + > +static void gen9_enable_dc5(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev = dev_priv->dev; > + uint32_t val; > + > + WARN_ON(!IS_GEN9(dev)); > + > + DRM_DEBUG_KMS("Enabling DC5\n"); > + > + gen9_set_dc_state_debugmask_memory_up(dev_priv); > + > + val = I915_READ(DC_STATE_EN); > + val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK; > + val |= DC_STATE_EN_UPTO_DC5; > + I915_WRITE(DC_STATE_EN, val); > + POSTING_READ(DC_STATE_EN); > +} > + > +static void gen9_disable_dc5(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev = dev_priv->dev; > + uint32_t val; > + > + WARN_ON(!IS_GEN9(dev)); > + > + DRM_DEBUG_KMS("Disabling DC5\n"); > + > + val = I915_READ(DC_STATE_EN); > + val &= ~DC_STATE_EN_UPTO_DC5; > + I915_WRITE(DC_STATE_EN, val); > + POSTING_READ(DC_STATE_EN); > +} > + > static void skl_set_power_well(struct drm_i915_private *dev_priv, > struct i915_power_well *power_well, bool enable) > { ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/i
Re: [Intel-gfx] [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation
On Thu, Apr 02, 2015 at 10:24:02AM +0530, Jindal, Sonika wrote: > > > On 4/1/2015 11:52 PM, Matt Roper wrote: > >On Mon, Mar 30, 2015 at 02:04:57PM +0530, Sonika Jindal wrote: > >>v2: Moving creation of property in a function, checking for 90/270 > >>rotation simultaneously (Chris) > >>Letting primary plane to be positioned > >>v3: Adding if/else for 90/270 and rest params programming, adding check for > >>pixel_format, some cleanup (review comments) > >>v4: Adding right pixel_formats, using src_* params instead of crtc_* for > >>offset > >>and size programming (Ville) > >>v5: Rebased on -nightly and Tvrtko's series for gtt remapping. > >>v6: Rebased on -nightly (Tvrtko's series merged) > >> > >>Signed-off-by: Sonika Jindal > >>--- > >> drivers/gpu/drm/i915/i915_reg.h |2 + > >> drivers/gpu/drm/i915/intel_display.c | 103 > >> +++--- > >> drivers/gpu/drm/i915/intel_drv.h |6 ++ > >> drivers/gpu/drm/i915/intel_sprite.c | 52 - > >> 4 files changed, 129 insertions(+), 34 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_reg.h > >>b/drivers/gpu/drm/i915/i915_reg.h > >>index b522eb6..564bbd5 100644 > >>--- a/drivers/gpu/drm/i915/i915_reg.h > >>+++ b/drivers/gpu/drm/i915/i915_reg.h > >>@@ -4854,7 +4854,9 @@ enum skl_disp_power_wells { > >> #define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4) > >> #define PLANE_CTL_ROTATE_MASK 0x3 > >> #define PLANE_CTL_ROTATE_0 0x0 > >>+#define PLANE_CTL_ROTATE_90 0x1 > >> #define PLANE_CTL_ROTATE_1800x2 > >>+#define PLANE_CTL_ROTATE_270 0x3 > >> #define _PLANE_STRIDE_1_A 0x70188 > >> #define _PLANE_STRIDE_2_A 0x70288 > >> #define _PLANE_STRIDE_3_A 0x70388 > >>diff --git a/drivers/gpu/drm/i915/intel_display.c > >>b/drivers/gpu/drm/i915/intel_display.c > >>index f0bbc22..86ee0f0 100644 > >>--- a/drivers/gpu/drm/i915/intel_display.c > >>+++ b/drivers/gpu/drm/i915/intel_display.c > >>@@ -2318,6 +2318,28 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, > >>struct drm_framebuffer *fb, > >>return -EINVAL; > >>} > >> > >>+ switch (fb->pixel_format) { > >>+ case DRM_FORMAT_XRGB: > >>+ case DRM_FORMAT_ARGB: > >>+ case DRM_FORMAT_XBGR: > >>+ case DRM_FORMAT_ABGR: > >>+ case DRM_FORMAT_XRGB2101010: > >>+ case DRM_FORMAT_ARGB2101010: > >>+ case DRM_FORMAT_XBGR2101010: > >>+ case DRM_FORMAT_ABGR2101010: > >>+ case DRM_FORMAT_YUYV: > >>+ case DRM_FORMAT_YVYU: > >>+ case DRM_FORMAT_UYVY: > >>+ case DRM_FORMAT_VYUY: > >>+ case DRM_FORMAT_NV12: > >>+ break; > >>+ > >>+ default: > >>+ DRM_DEBUG_KMS("Unsupported pixel format:%d for 90/270 > >>rotation!\n", > >>+ fb->pixel_format); > >>+ return -EINVAL; > >>+ } > > > >Shouldn't we be matching against the list of formats the plane supports > >(which may vary by platform, or by specific plane) rather than this > >generic list? We already specified what formats the plane can handle at > >plane init time, so it seems like what you'd really want is just a call > >to > > > > drm_plane_check_pixel_format(plane_state->plane, fb->pixel_format) > > > >then follow that up with explicit checks to exclude any formats that we > >can handle in 0/180, but not in 90/270. > > > I am not sure how it will help. drm_plane_check_pixel_format should > be used to check the pixel format of the fb which we should be doing > in some -check functions (I don't think we do that right now?) > against what is supported by the plane. > But to check for the formats which are allowed for 90/270, we would > need this kind of explicit check. Right, I guess there are two aspects here. First, we need to properly test for acceptable pixel formats for the plane in general; at the moment the DRM core setplane() tests this, but if we use the atomic ioctl it never gets checked (which is a bug). So as you say, we need a test in a _check() function to verify this. We probably also need to add an i-g-t test for it too. Once we know that the pixel format is valid in general, it makes sense to have a simpler test to reject some subset of those formats iff we notice we're doing 90/270 rotation. Maybe it's not really a big deal, but it seems like that's a little easier to understand and verify than having two completely separate lists, especially when future platforms may support different formats, or even different planes of the same platform have varying pixel format capabilities. Matt > >I'd also move this check to intel_plane_atomic_check(), since the > >'check' code path is where I'd usually go looking for these types of > >checks; the function you've got it in at the moment gets called from the > >'prepare' step which works as well, but seems a bit less obvious. > > > Yes, I agree, but this is on top of Tvrtko's patc
Re: [Intel-gfx] [RFC i-g-t v5] tests/gem_exec_pad_to_size: Test object padding at execbuf
On 2 April 2015 at 13:54, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > This tests the new EXEC_OBJECT_PAD_TO_SIZE exec_object2 flag. Just two things from an i-g-t perspective: the new binary needs adding to .gitignore and it would be good to include a short description of the test using the IGT_TEST_DESCRIPTION macro. > > Similar to some other tests, it uses knowledge of the DRM > allocation policy in order to get two objects mapped adjacent > to each other. It is then possible to verify that the pad to > size flag will move them apart. > > v2: Correct commit message. (Chris Wilson) > v3: Changes after code review by Chris Wilson. > * No need for gem_sync after execbuf. > * Drop caches before running. > * Allocate one additional bo per iteration. > * Don't explicitly set unused execbuf fields to zero. > * One improved comment. > v4: Require simpler object ordering and fixed overlap test. (Chris Wilson) > v5: Check unpadded offsets once more before padding. (Chris Wilson) > > Signed-off-by: Tvrtko Ursulin > Reviewed-by: Chris Wilson > Cc: Chris Wilson > --- > tests/Makefile.sources | 1 + > tests/gem_exec_pad_to_size.c | 240 > +++ > 2 files changed, 241 insertions(+) > create mode 100644 tests/gem_exec_pad_to_size.c > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > index 0a974a6..5f21728 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -34,6 +34,7 @@ TESTS_progs_M = \ > gem_exec_bad_domains \ > gem_exec_faulting_reloc \ > gem_exec_nop \ > + gem_exec_pad_to_size \ > gem_exec_params \ > gem_exec_parse \ > gem_fenced_exec_thrash \ > diff --git a/tests/gem_exec_pad_to_size.c b/tests/gem_exec_pad_to_size.c > new file mode 100644 > index 000..87afe40 > --- /dev/null > +++ b/tests/gem_exec_pad_to_size.c > @@ -0,0 +1,240 @@ > +/* > + * Copyright © 2015 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS > + * IN THE SOFTWARE. > + * > + * Authors: > + *Tvrtko Ursulin > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "drm.h" > +#include "ioctl_wrappers.h" > +#include "igt_core.h" > +#include "drmtest.h" > +#include "intel_reg.h" > +#include "igt_debugfs.h" > + > +#ifndef PAGE_SIZE > +#define PAGE_SIZE 4096 > +#endif > + > +struct local_drm_i915_gem_exec_object2 { > + /** > +* User's handle for a buffer to be bound into the GTT for this > +* operation. > +*/ > + __u32 handle; > + > + /** Number of relocations to be performed on this buffer */ > + __u32 relocation_count; > + /** > +* Pointer to array of struct drm_i915_gem_relocation_entry containing > +* the relocations to be performed in this buffer. > +*/ > + __u64 relocs_ptr; > + > + /** Required alignment in graphics aperture */ > + __u64 alignment; > + > + /** > +* Returned value of the updated offset of the object, for future > +* presumed_offset writes. > +*/ > + __u64 offset; > + > +#define LOCAL_EXEC_OBJECT_NEEDS_FENCE (1<<0) > +#define LOCAL_EXEC_OBJECT_NEEDS_GTT(1<<1) > +#define LOCAL_EXEC_OBJECT_WRITE(1<<2) > +#define LOCAL_EXEC_OBJECT_PAD_TO_SIZE (1<<3) > +#define __LOCAL_EXEC_OBJECT_UNKNOWN_FLAGS -(LOCAL_EXEC_OBJECT_PAD_TO_SIZE<<1) > + __u64 flags; > + > + __u64 pad_to_size; > + __u64 rsvd2; > +}; > + > +static int > +exec(int fd, uint32_t handles[3], uint32_t pad_to_size[2], uint64_t *offsets) > +{ > + struct drm_i915_gem_execbuffer2 execbuf; > + struct local_drm_i915_gem_exec_object2 gem_exec[3]; > + int ret = 0; > + > + memset(gem_exec, 0, sizeof(gem_exec)); > + > +
Re: [Intel-gfx] [PATCH] drm/i915: Allow userptr backchannel for passing aroung GTT mappings
Hi, Typo in subject, then below. On 04/02/2015 04:04 PM, Chris Wilson wrote: Once userptr becomes part of client API, it is almost a certainly that eventually someone will try to create a new object from a mapping of another client object, e.g. new = vaImport(vaMap(old, &size), size); (using a hypothethical API, not meaning to pick on anyone!) Since this is actually fairly safe to implement and to allow (since it is within a single process space and the memory access passes the standard permissions test) let us not limit the Client possibilities. Signed-off-by: Chris Wilson Cc: Gwenole Beauchesne Cc: Michał Winiarski Cc: Tvrtko Ursulin --- drivers/gpu/drm/i915/i915_gem_userptr.c | 46 ++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c index d96276caab49..8031ebe424fd 100644 --- a/drivers/gpu/drm/i915/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c @@ -750,6 +750,35 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { .release = i915_gem_userptr_release, }; +static struct drm_i915_gem_object * +find_object_from_vma(struct drm_device *dev, +struct drm_i915_gem_userptr *args) +{ + struct drm_i915_gem_object *obj = NULL; + struct vm_area_struct *vma; + + down_read(¤t->mm->mmap_sem); + vma = find_vma(current->mm, args->user_ptr); + if (vma == NULL) + goto out; + + if (vma->vm_ops != dev->driver->gem_vm_ops) + goto out; + + if (vma->vm_start != args->user_ptr || + vma->vm_end != args->user_ptr + args->user_size) { + obj = ERR_PTR(-EINVAL); + goto out; + } + + obj = to_intel_bo(vma->vm_private_data); + drm_gem_object_reference(obj); Hm, can't this race with last unreference in general, and with cleanup worker with userptr objects? + +out: + up_read(¤t->mm->mmap_sem); + return obj; +} + /** * Creates a new mm object that wraps some normal memory from the process * context - user memory. @@ -757,8 +786,11 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { * We impose several restrictions upon the memory being mapped * into the GPU. * 1. It must be page aligned (both start/end addresses, i.e ptr and size). - * 2. It must be normal system memory, not a pointer into another map of IO - *space (e.g. it must not be a GTT mmapping of another object). + * 2. It must either be: + *a) normal system memory, not a pointer into another map of IO + * space (e.g. it must not be part of a GTT mmapping of another object). + *b) a pointer to the complete GTT mmap of another object in your + * address space. * 3. We only allow a bo as large as we could in theory map into the GTT, *that is we limit the size to the total size of the GTT. * 4. The bo is marked as being snoopable. The backing pages are left @@ -812,6 +844,14 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file return -ENODEV; } + obj = find_object_from_vma(dev, args); + if (obj) { + if (IS_ERR(obj)) + return PTR_ERR(obj); + else + goto out; + } + obj = i915_gem_object_alloc(dev); if (obj == NULL) return -ENOMEM; @@ -833,7 +873,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file if (ret == 0) ret = i915_gem_userptr_init__mmu_notifier(obj, args->flags); if (ret == 0) - ret = drm_gem_handle_create(file, &obj->base, &handle); +out:ret = drm_gem_handle_create(file, &obj->base, &handle); /* drop reference from allocate - handle holds it now */ drm_gem_object_unreference_unlocked(&obj->base); Thing I don't like is how the user of this has no idea what kind of object it "imported". Maybe it doesn't matter, hm. Need to think about it more. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Allow userptr backchannel for passing aroung GTT mappings
On Thu, Apr 02, 2015 at 05:11:58PM +0100, Tvrtko Ursulin wrote: > >+static struct drm_i915_gem_object * > >+find_object_from_vma(struct drm_device *dev, > >+ struct drm_i915_gem_userptr *args) > >+{ > >+struct drm_i915_gem_object *obj = NULL; > >+struct vm_area_struct *vma; > >+ > >+down_read(¤t->mm->mmap_sem); > >+vma = find_vma(current->mm, args->user_ptr); > >+if (vma == NULL) > >+goto out; > >+ > >+if (vma->vm_ops != dev->driver->gem_vm_ops) > >+goto out; > >+ > >+if (vma->vm_start != args->user_ptr || > >+vma->vm_end != args->user_ptr + args->user_size) { > >+obj = ERR_PTR(-EINVAL); > >+goto out; > >+} > >+ > >+obj = to_intel_bo(vma->vm_private_data); > >+drm_gem_object_reference(obj); > > Hm, can't this race with last unreference in general, and with > cleanup worker with userptr objects? The vma holds a reference to the object and that reference is dropped whilst holding down_write(current->mm->mmap_sem), hence I think the down_read(current->mm->mmap_sem) is sufficient locking to acquire a reference for ourselves. > >+out: ret = drm_gem_handle_create(file, &obj->base, &handle); > > > > /* drop reference from allocate - handle holds it now */ > > drm_gem_object_unreference_unlocked(&obj->base); > > Thing I don't like is how the user of this has no idea what kind of > object it "imported". Maybe it doesn't matter, hm. Need to think > about it more. Indeed. But since the userptr is a strict subset of the general bo, if they follow the rules for userptr bo then they won't notice a difference. read/writes into the memory block are coherent (since the pointer is wc) so as far the caller is concerned I think it just ends up being slower cpu side, faster gpu side than a system memory snooped userptr bo. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 30/49] drm/i915/bxt: add display initialize/uninitialize sequence
On Tue, Mar 17, 2015 at 11:39:56AM +0200, Imre Deak wrote: > From: Vandana Kannan > > Add display clock/PHY initialization sequence as per BSpec. > > Until GOP/VBIOS provides an upper limit value for CDCLK, comparing clock > value with 624 MHz and returning 0 in case it exceeds. > > Note that the CD clock and PHY initialization/uninitialization are done > at their current place only for simplicity, in a future patch - when more > of the runtime PM features will be enabled - these will be moved to > power well#1 and modeset encoder enabling/disabling hooks respectively. > This also means that atm dynamic power gating power well #2 is > effectively disabled. OK, I've gone through the PHY stuff a bit now, and skipped the cdclk stuff this time. > > v1: Added function definitions in header files > v2: Imre's review comments addressed > - Moved CDCLK related definitions to i915_reg.h > - Removed defintions for CDCLK frequency > - Split uninit_cdclk() by adding a phy_uninit function > - Calculate freq and decimal based on input frequency > - Program SSA precharge based on input frequency > - Use wait_for 1ms instead 200us udelay for DE PLL locking > - Removed initial value for divider, freq, decimal, ratio. > - Replaced polling loops with wait_for > - Parameterized latency optim setting > - Fix the parts where DE PLL has to be disabled. > - Call CDCLK selection from mode set > > v3: (imre) > - add note about the plan to move the cdclk/phy init to a better place > - take rps.hw_lock around pcode access > - fix DDI PHY timeout value > - squash in Vandana's "PORT_CL2CM_DW6_A BUN fix", > "DDI PHY programming register defn", "Do ddi_phy_init always", > "Check CDCLK upper limit" patches > - move PHY register macros next to the corresponding CHV/VLV macros > - move DE PLL register macros here from another patch since they are > used here first > - add BXT_ prefix to CDCLK flags > - s/COMMON_RESET/COMMON_RESET_DIS/ and clarify related code comments > - fix incorrect read value for the RMW of BXT_PHY_CTL_FAMILY_DDI > - fix using GT_DISPLAY_EDP_POWER_ON vs. GT_DISPLAY_DDI_POWER_ON > when powering on DDI ports > - fix incorrect port when setting BXT_PORT_TX_DW14_LN for DDI ports > - add missing masking when programming CDCLK_FREQ_DECIMAL > - add missing powering on for DDI-C port, rename OCL2_LDOFUSE_PWR_EN > to OCL2_LDOFUSE_PWR_DIS to reduce confusion > - add note about mismatch with bspec in the PORT_REF_DW6 fields > - factor out PHY init code to a new function, so we can call it for > PHY_A and PHY_BC, instead of open-coding the same > > Signed-off-by: Vandana Kannan (v2) > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_reg.h | 126 +++ > drivers/gpu/drm/i915/intel_ddi.c | 291 > +++ > drivers/gpu/drm/i915/intel_display.c | 75 + > drivers/gpu/drm/i915/intel_drv.h | 4 + > 4 files changed, 496 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index b4474d3..a3579c0 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1120,6 +1120,110 @@ enum skl_disp_power_wells { > #define DPIO_FRC_LATENCY_SHFIT 8 > #define CHV_TX_DW14(ch, lane) _TXLANE(ch, lane, 0xb8) > #define DPIO_UPAR_SHIFT30 > + > +/* BXT PHY registers */ > +enum bxt_phy { > + BXT_PHY_A, > + BXT_PHY_BC > +}; We have enum dpio_phy already. Although here we have defined 0 to be the single channel PHY and 1 is the two channel PHY, whereas on CHV it's the other way around. I'm going to suggest we flip BXT over to use the CHV scheme to avoid any surpises later if we actualy try to unify the code. > + > +#define BXT_PHY(phy, a, b) ((a) + (phy) * ((b) - (a))) This seems to be just another _PIPE(), should at least have an underscore so that people don't confuse it with something they are supposed to use. > + > +#define BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR0x138090 We can drop the _0_2_0_GTTMMADR suffix, we've never included it for any other platforms either. > +#define _EDP_POWER_ON (1 << 1) > +#define _DDI_POWER_ON (1 << 0) > +#define GT_DISPLAY_POWER_ON(phy) BXT_PHY(phy, _EDP_POWER_ON, \ > + _DDI_POWER_ON) Using a _PIPE() type of macro for register bits seems a bit unusual. I'd just open code it as (1 << (phy)) or something. That also work better if we the PHYs around so that PHY0 is the dual channel PHY. > + > +#define _PHY_CTL_FAMILY_EDP 0x64C80 > +#define _PHY_CTL_FAMILY_DDI 0x64C90 > +#define COMMON_RESET_DIS (1 << 31) > +#define BXT_PHY_CTL_FAMILY(phy) BXT_PHY(phy, > _PHY_CTL_FAMILY_EDP, \ > + _PHY_CTL_FAMILY_DDI) > + > +/* BXT PHY common lane registers */ > +#define _PORT_CL1CM_DW0_A0x16200
[Intel-gfx] [RFC][PATCH] tracing/drm: Remove unused TRACE_SYSTEM_STRING define
The tracing infrastructure is adding a macro TRACE_SYSTEM_STRING, and hit the following build failure: In file included from include/trace/define_trace.h:90:0, from drivers/gpu/drm/.//radeon/radeon_trace.h:209, from drivers/gpu/drm/.//radeon/radeon_trace_points.c:9: >> include/trace/ftrace.h:28:0: warning: "TRACE_SYSTEM_STRING" redefined #define TRACE_SYSTEM_STRING __app(TRACE_SYSTEM_VAR,__trace_system_name) Seems that the DRM folks have added their own use to the TRACE_SYSTEM_STRING, with: #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM) Although, I can not find its use anywhere. I could simply use another name, but if this macro is not being used, it should be removed. Cc: Alex Deucher Cc: Christian König Cc: David Airlie Cc: Daniel Vetter Cc: Jani Nikula Reported-by: kbuild test robot Signed-off-by: Steven Rostedt --- Acks please. diff --git a/drivers/gpu/drm/drm_trace.h b/drivers/gpu/drm/drm_trace.h index 27cc95f36381..ce3c42813fbb 100644 --- a/drivers/gpu/drm/drm_trace.h +++ b/drivers/gpu/drm/drm_trace.h @@ -7,7 +7,6 @@ #undef TRACE_SYSTEM #define TRACE_SYSTEM drm -#define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM) #define TRACE_INCLUDE_FILE drm_trace TRACE_EVENT(drm_vblank_event, diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 6058a01b4443..d776621c8521 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -12,7 +12,6 @@ #undef TRACE_SYSTEM #define TRACE_SYSTEM i915 -#define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM) #define TRACE_INCLUDE_FILE i915_trace /* pipe updates */ diff --git a/drivers/gpu/drm/radeon/radeon_trace.h b/drivers/gpu/drm/radeon/radeon_trace.h index ce075cb08cb2..fdce4062901f 100644 --- a/drivers/gpu/drm/radeon/radeon_trace.h +++ b/drivers/gpu/drm/radeon/radeon_trace.h @@ -9,7 +9,6 @@ #undef TRACE_SYSTEM #define TRACE_SYSTEM radeon -#define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM) #define TRACE_INCLUDE_FILE radeon_trace TRACE_EVENT(radeon_bo_create, ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t v5] tests: install test programs to libexec
On 31 March 2015 at 13:53, Joonas Lahtinen wrote: > Install the test programs by default so that they can be packaged. > > Tested with the testdisplay test so that it still runs after the > modifications as it depends on a data file to be present. Need to > pass -r option to enable QR code display on success (PNG data file). > > Packaging is useful when building a complete software stack for a > DUT from scratch. This should bring us closer to achieving a > built-from-scratch testing workflow. > > Package maintainers can always decide to ignore the installed files. > > v2: > - Install more tests including scripts and their data > > v3: > - Add clarification to commit message about why we do this. > (Chris Wilson & Thomas Wood) > - Change libexec into pkglibexec to comply to standard > (Thomas Wood) > - Do not install $(common_files). (Thomas Wood) > - Make it really obvious the installed files are tests by using > tests directory name to avoid any confusion with packagers. > > v4: > - Fixed commit message. > > v5: > - Add file locator helper to retain backwards compatibility. > (Thomas Wood) > - Test with testdisplay -r option that draws the .png file. Thanks, patch merged. > > Cc: Chris Wilson > Cc: Thomas Wood > Signed-off-by: Joonas Lahtinen > --- > lib/igt_core.c | 16 > lib/igt_core.h | 13 + > tests/Makefile.am | 22 +++--- > tests/Makefile.sources | 16 ++-- > tests/testdisplay.c| 21 +++-- > 5 files changed, 81 insertions(+), 7 deletions(-) > > diff --git a/lib/igt_core.c b/lib/igt_core.c > index 783a219..8d60930 100644 > --- a/lib/igt_core.c > +++ b/lib/igt_core.c > @@ -54,6 +54,7 @@ > #include > #include > #include > +#include > > #include "drmtest.h" > #include "intel_chipset.h" > @@ -1735,3 +1736,18 @@ void igt_set_timeout(unsigned int seconds) > > alarm(seconds); > } > + > +FILE *__igt_fopen_data(const char* igt_srcdir, const char* igt_datadir, > + const char* filename) > +{ > + char path[PATH_MAX]; > + FILE *fp; > + > + snprintf(path, sizeof(path), "%s/%s", igt_datadir, filename); > + fp = fopen(path, "r"); > + if (!fp) { > + snprintf(path, sizeof(path), "%s/%s", igt_srcdir, filename); > + fp = fopen(path, "r"); > + } > + return fp; > +} > diff --git a/lib/igt_core.h b/lib/igt_core.h > index 33f8940..4e56be8 100644 > --- a/lib/igt_core.h > +++ b/lib/igt_core.h > @@ -33,6 +33,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -650,4 +651,16 @@ extern enum igt_log_level igt_log_level; > > void igt_set_timeout(unsigned int seconds); > > +FILE *__igt_fopen_data(const char* igt_srcdir, const char* igt_datadir, > + const char* filename); > +/** > + * igt_fopen_data: > + * @filename: filename to open. > + * > + * Open a datafile for test, first try from installation directory > + * then from build directory. > + */ > +#define igt_fopen_data(filename) \ > + __igt_fopen_data(IGT_SRCDIR, IGT_DATADIR, filename) > + > #endif /* IGT_CORE_H */ > diff --git a/tests/Makefile.am b/tests/Makefile.am > index f45c6c9..69c7c4e 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -27,8 +27,23 @@ multi-tests.txt: Makefile.sources > @echo ${multi_kernel_tests} >> $@ > @echo END TESTLIST >> $@ > > -EXTRA_PROGRAMS = $(TESTS_progs) $(TESTS_progs_M) $(HANG) > -EXTRA_DIST = $(TESTS_scripts) $(TESTS_scripts_M) $(scripts) $(IMAGES) > $(common_files) > +igt_tests_bin_PROGRAMS += \ > + $(TESTS_progs) \ > + $(TESTS_progs_M) \ > + $(NULL) > + > +igt_tests_bin_SCRIPTS += \ > + $(TESTS_scripts) \ > + $(TESTS_scripts_M) \ > + $(scripts) \ > + $(NULL) > + > +igt_tests_data_DATA += \ > + $(IMAGES) \ > + $(NULL) > + > +EXTRA_PROGRAMS = $(HANG) > +EXTRA_DIST = $(common_files) > > CLEANFILES = $(EXTRA_PROGRAMS) single-tests.txt multi-tests.txt > > @@ -36,7 +51,8 @@ AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) $(DEBUG_CFLAGS)\ > -I$(srcdir)/.. \ > -I$(srcdir)/../lib \ > -include "$(srcdir)/../lib/check-ndebug.h" \ > - -DIGT_DATADIR=\""$(abs_srcdir)"\" \ > + -DIGT_SRCDIR=\""$(abs_srcdir)"\" \ > + -DIGT_DATADIR=\""$(igt_tests_datadir)"\" \ > $(LIBUNWIND_CFLAGS) \ > $(NULL) > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > index 93e05e4..3e3aa57 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -1,10 +1,22 @@ > +igt_tests_bindir = $(pkglibexecdir)/tests > +igt_tests_datadir = $(pkgdatadir)/tests > + > noinst_PROGRAMS = \ > + $(HANG) \ > + $(TESTS_testsuite) \ > + $(NULL) > + > +igt_tests_bin_PROGRAMS = \ > gem_alive \ > gem_stress \ > $(TESTS_progs) \ > $(TESTS_progs_M) \ > - $(HANG) \ > - $(TESTS_testsuit
Re: [Intel-gfx] [PATCH 2/2] drm/i915/vlv: remove wait for previous GFX clk disable request
Jesse Barnes virtuousgeek.org> writes: > Looks like it was introduced in: > > commit 650ad970a39f8b6164fe8613edc150f585315289 > Author: Imre Deak intel.com> > Date: Fri Apr 18 16:35:02 2014 +0300 > > drm/i915: vlv: factor out vlv_force_gfx_clock and check for pending > force-of > > but I'm not sure why. It has caused problems for us in the past (see > 85250ddff7a603dfe0ec0503a9e6395f79424f61 and > 8d4eee9cd7a170342dc6fbc2ee19ae77031a8cd5) and doesn't seem to be > required, so let's just drop it. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=89611 > Signed-off-by: Jesse Barnes virtuousgeek.org> Thanks Jesse, With this and 1/2 applied I was able to suspend/resume twice in a row successfully on a MinnowBoard MAX dual-core (E3825) with the 0.78 firmware. Tested-by: Darren Hart ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Allow disabling the destination colorkey for overlay
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6119 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -2 272/272 270/272 ILK -2 302/302 300/302 SNB 303/303 303/303 IVB 338/338 338/338 BYT -1 287/287 286/287 HSW 361/361 361/361 BDW 308/308 308/308 -Detailed- Platform Testdrm-intel-nightly Series Applied PNV igt@gem_tiled_pread_pwrite FAIL(3)PASS(8) FAIL(1)PASS(1) PNV igt@gem_userptr_blits@coherency-sync CRASH(5)PASS(6) CRASH(1)PASS(1) *ILK igt@gem_workarounds@suspend-resume PASS(2) NO_RESULT(1)PASS(1) *ILK igt@kms_flip@flip-vs-dpms-interruptible PASS(2) DMESG_WARN(1)PASS(1) (dmesg patch applied)drm:drm_edid_block_valid[drm]]*ERROR*EDID_checksum_is_invalid,remainder_is@EDID checksum is .* remainder is *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(10) FAIL(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 20/20] drm/i915: Enable skylake sprite plane scaling using shared scalers
Hi Shuang, Looking at starting with '*': *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(9) FAIL(1)PASS(1) Above failure seems unrelated to my patch series. I suspect this pre-exist before my changes. Can you double check whether above failure is pre-existing before any action is taken from my side? Also I just ran it on skl (that's the system I have) without any failures: ~/gfx/sources/intel-gpu-tools/tests$ sudo ./gem_exec_bad_domains IGT-Version: 1.10-g4f076bc (x86_64) (Linux: 4.0.0-rc3+ x86_64) Subtest cpu-domain: SUCCESS (0.004s) Subtest gtt-domain: SUCCESS (0.000s) Subtest conflicting-write-domain: SUCCESS (0.000s) Subtest double-write-domain: SUCCESS (0.000s) Subtest invalid-gpu-domain: SUCCESS (0.000s) ~/gfx/sources/intel-gpu-tools/tests$ -Chandra > -Original Message- > From: He, Shuang > Sent: Thursday, April 02, 2015 7:45 AM > To: He, Shuang; Gao, Ethan; intel-gfx@lists.freedesktop.org; Konduru, Chandra > Subject: RE: [Intel-gfx] [PATCH 20/20] drm/i915: Enable skylake sprite plane > scaling using shared scalers > > Tested-By: PRC QA PRTS (Patch Regression Test System Contact: > shuang...@intel.com) Task id: 6118 > -Summary- > Platform Delta drm-intel-nightly Series Applied > PNV -1 272/272 271/272 > ILK 302/302 302/302 > SNB 303/303 303/303 > IVB 338/338 338/338 > BYT -1 287/287 286/287 > HSW 361/361 361/361 > BDW 308/308 308/308 > -Detailed- > Platform Testdrm-intel-nightly > Series Applied > PNV igt@gen3_render_tiledx_blits FAIL(6)PASS(3) FAIL(2) > *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(9) > FAIL(1)PASS(1) > Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: list self-refresh as enabled on newer platforms
I guess this is a lie for 8xx, but newer stuff takes care of this for us. References: https://bugs.freedesktop.org/show_bug.cgi?id=89792 Signed-off-by: Jesse Barnes --- drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 91c945b..a8f42a7 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1686,6 +1686,8 @@ static int i915_sr_status(struct seq_file *m, void *unused) sr_enabled = I915_READ(INSTPM) & INSTPM_SELF_EN; else if (IS_PINEVIEW(dev)) sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN; + else + sr_enabled = true; /* other platforms don't need enabling */ intel_runtime_pm_put(dev_priv); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: list self-refresh as enabled on newer platforms
On Thu, Apr 02, 2015 at 11:18:49AM -0700, Jesse Barnes wrote: > I guess this is a lie for 8xx, but newer stuff takes care of this for > us. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=89792 > Signed-off-by: Jesse Barnes > --- > drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 91c945b..a8f42a7 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1686,6 +1686,8 @@ static int i915_sr_status(struct seq_file *m, void > *unused) > sr_enabled = I915_READ(INSTPM) & INSTPM_SELF_EN; > else if (IS_PINEVIEW(dev)) > sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN; > + else > + sr_enabled = true; /* other platforms don't need enabling */ Not true actually. The line between maxfifo and SR is a blurry one. We treat them as the same thing. So I think this should just read out whatever registers we set up in intel_set_memory_cxsr(). On ILK+ it should actually check if LP1+ watermarks are enabled or not. And I can't recall enough details on SKL right now to have an idea what should be done there. That's all assuming we want this file to be at least somewhat useful. I think the other good option is to just remove the file entirely and depend on the new intel_watermark tool I wrote recently. > > intel_runtime_pm_put(dev_priv); > > -- > 1.9.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: list self-refresh as enabled on newer platforms
On 04/02/2015 11:42 AM, Ville Syrjälä wrote: > On Thu, Apr 02, 2015 at 11:18:49AM -0700, Jesse Barnes wrote: >> I guess this is a lie for 8xx, but newer stuff takes care of this for >> us. >> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=89792 >> Signed-off-by: Jesse Barnes >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >> b/drivers/gpu/drm/i915/i915_debugfs.c >> index 91c945b..a8f42a7 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -1686,6 +1686,8 @@ static int i915_sr_status(struct seq_file *m, void >> *unused) >> sr_enabled = I915_READ(INSTPM) & INSTPM_SELF_EN; >> else if (IS_PINEVIEW(dev)) >> sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN; >> +else >> +sr_enabled = true; /* other platforms don't need enabling */ > > Not true actually. > > The line between maxfifo and SR is a blurry one. We treat them as the > same thing. So I think this should just read out whatever registers > we set up in intel_set_memory_cxsr(). > > On ILK+ it should actually check if LP1+ watermarks are enabled or not. > And I can't recall enough details on SKL right now to have an idea what > should be done there. > > That's all assuming we want this file to be at least somewhat useful. > I think the other good option is to just remove the file entirely and > depend on the new intel_watermark tool I wrote recently. Yeah, that might be a better option. And beyond just this we want SR residency anyway, so finding the debug regs for that and making a tool is a better long term solution. Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6120 -Summary- Platform Delta drm-intel-nightly Series Applied PNV 272/272 272/272 ILK 302/302 302/302 SNB 303/303 303/303 IVB 338/338 338/338 BYT -1 287/287 286/287 HSW -1 361/361 360/361 BDW 308/308 308/308 -Detailed- Platform Testdrm-intel-nightly Series Applied *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(12) FAIL(1)PASS(1) *HSW igt@gem_storedw_loop_blt PASS(2) DMESG_WARN(1)PASS(1) (dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/8] drm/i915/skl: Add DC5 Trigger Sequence.
On Wed, 2015-04-01 at 16:22 +0530, Animesh Manna wrote: > From: Suketu Shah > > Add triggers as per expectations mentioned in gen9_enable_dc5 > and gen9_disable_dc5 patch. > > Also call POSTING_READ for every write to a register to ensure that > its written immediately. > > v1: Remove POSTING_READ calls as they've already been added in previous > patches. > > v2: Rebase to move all runtime pm specific changes to intel_runtime_pm.c file. > > Modified as per review comments from Imre: > 1] Change variable name 'dc5_allowed' to 'dc5_enabled' to correspond to > relevant >functions. > 2] Move the check dc5_enabled in skl_set_power_well() to disable DC5 into >gen9_disable_DC5 which is a more appropriate place. > 3] Convert checks for 'pm.dc5_enabled' and 'pm.suspended' in > skl_set_power_well() >to warnings. However, removing them for now as they'll be included in a > future patch >asserting DC-state entry/exit criteria. > 4] Enable DC5, only when CSR firmware is verified to be loaded. Create new > structure >to track 'enabled' and 'deferred' status of DC5. > 5] Ensure runtime PM reference is obtained, if CSR is not loaded, to avoid > entering >runtime-suspend and release it when it's loaded. > 6] Protect necessary CSR-related code with locks. > 7] Move CSR-loading call to runtime PM initialization, as power domains > needed to be >accessed during deferred DC5-enabling, are not initialized earlier. > > v3: Rebase to latest. > > Modified as per review comments from Imre: > 1] Use blocking wait for CSR-loading to finish to enable DC5 for simplicity, > instead of >deferring enabling DC5 until CSR is loaded. > 2] Obtain runtime PM reference during CSR-loading initialization itself as > deferred DC5- >enabling is removed and release it at the end of CSR-loading functionality. > 3] Revert calling CSR-loading functionality to the beginning of i915 > driver-load >functionality to avoid any delay in loading. > 4] Define another variable to track whether CSR-loading failed and use it to > avoid enabling >DC5 if it's true. > 5] Define CSR-load-status accessor functions for use later. > > v4: > 1] Disable DC5 before enabling PG2 instead of after it. > 2] DC5 was being mistaken enabled even when CSR-loading timed-out. Fix that. > 3] Enable DC5-related functionality using a macro. > 4] Remove dc5_enabled tracking variable and its use as it's not needed now. > > v5: > 1] Mark CSR failed to load where necessary in finish_csr_load function. > 2] Use mutex-protected accessor function to check if CSR loaded instead of > directly >accessing the variable. > 3] Prefix csr_load_status_get/set function names with intel_. > > v6: rebase to latest. > v7: Rebase on top of nightly (Damien) > v8: Squashed the patch from Imre - added csr helper pointers to simplify the > code. (Imre) > v9: After adding dmc ver 1.0 support rebased on top of nightly. (Animesh) > > Issue: VIZ-2819 > Signed-off-by: A.Sunil Kamath > Signed-off-by: Suketu Shah > Signed-off-by: Damien Lespiau > Signed-off-by: Imre Deak > Signed-off-by: Animesh Manna > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_csr.c| 50 > - > drivers/gpu/drm/i915/intel_drv.h| 2 ++ > drivers/gpu/drm/i915/intel_runtime_pm.c | 31 > 4 files changed, 84 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index dd572a0..3320fb4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -669,6 +669,8 @@ struct intel_uncore { > struct intel_csr { > int csr_size; > u8 *csr_buf; > + bool loaded; > + bool failed; Nitpick: just using an enum with loading, loaded, failed states would be clearer. > const char *fw_path; > }; > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > b/drivers/gpu/drm/i915/intel_csr.c > index f44f1cd..87d393a 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -60,6 +60,25 @@ char intel_get_substepping(struct drm_device *dev) > else > return -ENODATA; > } > + > +bool intel_csr_load_status_get(struct drm_i915_private *dev_priv) > +{ > + bool val = false; > + > + mutex_lock(&dev_priv->csr_lock); > + val = dev_priv->csr.loaded; > + mutex_unlock(&dev_priv->csr_lock); > + > + return val; > +} > + > +void intel_csr_load_status_set(struct drm_i915_private *dev_priv, bool val) > +{ > + mutex_lock(&dev_priv->csr_lock); > + dev_priv->csr.loaded = val; > + mutex_unlock(&dev_priv->csr_lock); > +} > + > void intel_csr_load_program(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -91,6 +110,8 @@ void intel_csr_load_program(struct drm_device *dev) > I915_WRITE(dev_priv->dmc_info.mmioaddr[i], > dev_priv->dmc_i
Re: [Intel-gfx] [PATCH 4/8] drm/i915/skl: Assert the requirements to enter or exit DC5.
On Wed, 2015-04-01 at 16:22 +0530, Animesh Manna wrote: > From: Suketu Shah > > Warn if the conditions to enter or exit DC5 are not satisfied such > as support for runtime PM, state of power well, CSR loading etc. > > v2: Removed camelcase in functions and variables. > > v3: Do some minimal check to assert if CSR program is not loaded. > > v4: > 1] Used an appropriate function lookup_power_well() to identify power well, > instead of using a magic number which can change in future. > 2] Split the conditions further in assert_can_enable_DC5() and added more > checks. > 3] Removed all WARNs from assert_can_disable_DC5 as they were unnecessary and > added two >new ones. > 4] Changed variable names as updated in earlier patches. > > v5: > 1] Change lookup_power_well function to take an int power well id. > 2] Define a new intel_display_power_well_is_enabled helper function to check > whether a >particular power well is enabled. > 3] Use CSR-related mutex in assert_csr_loaded function. > > v6: Remove use of dc5_enabled variable as it's no longer needed. > > v7: > 1] Rebase to latest. > 2] Move all DC5-related functions from intel_display.c to intel_runtime_pm.c. > > v8: After adding dmc ver 1.0 support rebased on top of nightly. (Animesh) > > Issue: VIZ-2819 > Signed-off-by: A.Sunil Kamath > Signed-off-by: Suketu Shah > Signed-off-by: Damien Lespiau > Signed-off-by: Animesh Manna > --- > drivers/gpu/drm/i915/intel_drv.h| 2 ++ > drivers/gpu/drm/i915/intel_runtime_pm.c | 61 > ++--- > 2 files changed, 58 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 39cb2dc..9aae624 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1232,6 +1232,8 @@ void intel_power_domains_fini(struct drm_i915_private > *); > void intel_power_domains_init_hw(struct drm_i915_private *dev_priv); > void intel_runtime_pm_enable(struct drm_i915_private *dev_priv); > > +bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, > + int power_well_id); I haven't seen this being used outside of intel_runtime_pm, so no need to export it. > bool intel_display_power_is_enabled(struct drm_i915_private *dev_priv, > enum intel_display_power_domain domain); > bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv, > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 8b917e2..f62d42b 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -335,12 +335,52 @@ static void gen9_set_dc_state_debugmask_memory_up( > } > } > > -static void gen9_enable_dc5(struct drm_i915_private *dev_priv) > +static void assert_csr_loaded(struct drm_i915_private *dev_priv) > +{ > + mutex_lock(&dev_priv->csr_lock); No point in taking the lock here. > + > + WARN(!dev_priv->csr.loaded, "CSR is not loaded.\n"); > + WARN(!I915_READ(CSR_PROGRAM_BASE), > + "CSR program storage start is NULL\n"); > + WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n"); > + WARN(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n"); > + > + mutex_unlock(&dev_priv->csr_lock); > +} > + > +static void assert_can_enable_dc5(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; > + bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv, > + SKL_DISP_PW_2); > + > + WARN(!IS_SKYLAKE(dev), "Platform doesn't support DC5.\n"); > + WARN(!HAS_RUNTIME_PM(dev), "Runtime PM not enabled.\n"); > + WARN(pg2_enabled, "PG2 not disabled to enable DC5.\n"); > + > + WARN((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5), > + "DC5 already programmed to be enabled.\n"); > + WARN(dev_priv->pm.suspended, > + "DC5 cannot be enabled, if platform is runtime-suspended.\n"); > + > + assert_csr_loaded(dev_priv); > +} > + > +static void assert_can_disable_dc5(struct drm_i915_private *dev_priv) > +{ > + bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv, > + SKL_DISP_PW_2); > + > + WARN(!pg2_enabled, "PG2 not enabled to disable DC5.\n"); > + WARN(dev_priv->pm.suspended, > + "Disabling of DC5 while platform is runtime-suspended should > never happen.\n"); > +} > + > +static void gen9_enable_dc5(struct drm_i915_private *dev_priv) > +{ > uint32_t val; > > - WARN_ON(!IS_GEN9(dev)); > + assert_can_enable_dc5(dev_priv); > > DRM_DEBUG_KMS("Enabling DC5\n"); > > @@ -355,10 +395,9 @@ static void gen9_enable_dc5(struct drm_i915_private > *dev_priv) > > static void gen9_disable_dc5(struct drm_i915_private *dev_priv) > { > - struct drm_device *dev =
Re: [Intel-gfx] [PATCH 5/8] drm/i915/skl: Implement enable/disable for Display C6 state.
On Wed, 2015-04-01 at 16:22 +0530, Animesh Manna wrote: > From: "A.Sunil Kamath" > > This patch just implements the basic enable and disable > functions of DC6 state which is needed for SKL platform. > > Its important to load SKL CSR program before calling enable. > > DC6 is a deeper power saving state where hardware dynamically > disables power well 0 and saves the associated registers. > DC6 can be entered when software allows it, the conditions > for DC5 are met, and the PCU allows DC6. > DC6 cannot be used if the backlight is being driven from the > display utility pin. > > Its better to configure display engine to have power well 2 > disabled before getting into DC6 enable function. Hence rpm > framework will ensure to check status of power well 2 and DC5 > before calling skl_enable_dc6. > > v2: Replace HAS_ with IS_ check as per Daniel's review comments > > v3: Cleared the bits dc5/dc6 enable of DC_STATE_EN register > before setting them as per Satheesh's review comments. > > v4: No need to call gen9_disable_dc5 inside enable sequence of > DC6, as its already take care above. > > v5: call POSTING_READ for every write to a register to ensure that > its written immediately. > Call intel_prepare_ddi during DC6 exit as it's required on low-power exit. > > v6: Protect DC6-enabling-disabling functionality with locks to synchronize > with CSR-loading code. > > v7: Remove grabbing CSR-related mutex in skl_enable/disable_dc6 functions as > deferred DC5-enabling functionality is now removed. > > v8: Remove 'Disabling DC5' from the debug comment during DC6 enabling as when > DC6 is allowed, DC5 is not programmed at all. > > v9: > 1] Rebase to latest. > 2] Move all DC6-related functions from intel_display.c to intel_runtime_pm.c. > > v10: After adding dmc ver 1.0 support rebased on top of nightly. (Animesh) > > Issue: VIZ-2819 > Signed-off-by: A.Sunil Kamath > Signed-off-by: Damien Lespiau > Signed-off-by: Animesh Manna Reviewed-by: Imre Deak > --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 35 > + > 1 file changed, 35 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index f62d42b..dae65e0 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -407,6 +407,41 @@ static void gen9_disable_dc5(struct drm_i915_private > *dev_priv) > POSTING_READ(DC_STATE_EN); > } > > +static void skl_enable_dc6(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev = dev_priv->dev; > + uint32_t val; > + > + WARN_ON(!IS_SKYLAKE(dev)); > + > + DRM_DEBUG_KMS("Enabling DC6\n"); > + > + gen9_set_dc_state_debugmask_memory_up(dev_priv); > + > + val = I915_READ(DC_STATE_EN); > + val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK; > + val |= DC_STATE_EN_UPTO_DC6; > + I915_WRITE(DC_STATE_EN, val); > + POSTING_READ(DC_STATE_EN); > +} > + > +static void skl_disable_dc6(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev = dev_priv->dev; > + uint32_t val; > + > + WARN_ON(!IS_SKYLAKE(dev)); > + > + DRM_DEBUG_KMS("Disabling DC6\n"); > + > + val = I915_READ(DC_STATE_EN); > + val &= ~DC_STATE_EN_UPTO_DC6; > + I915_WRITE(DC_STATE_EN, val); > + POSTING_READ(DC_STATE_EN); > + > + intel_prepare_ddi(dev); > +} > + > static void skl_set_power_well(struct drm_i915_private *dev_priv, > struct i915_power_well *power_well, bool enable) > { ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/8] drm/i915/skl: Add DC6 Trigger sequence.
On Wed, 2015-04-01 at 16:22 +0530, Animesh Manna wrote: > From: Suketu Shah > > Add triggers for DC6 as per details provided in skl_enable_dc6 > and skl_disable_dc6 implementations. > > Also Call POSTING_READ for every write to a register to ensure > it is written to immediately > > v1: Remove POSTING_READ and intel_prepare_ddi calls as they've been added in > previous patches. > > v2: > 1] Remove check for backlight disabled as it should be the case by that time. > 2] Mark DC5 as disabled when enabling DC6. > 3] Return from DC5-disabling function early if DC5 is already be disabled > which can happen >due to DC6-enabling earlier. > 3] Ensure CSR firmware is loaded after resume from DC6 as corresponding > memory contents won't >be retained after runtime-suspend. > 4] Ensure that CSR isn't identified as loaded before CSR-loading program is > called during >runtime-resume. > > v3: Rebase to latest > Modified as per review comments from Imre and after discussion with Art: > 1] DC6 should be preferably enabled when PG2 is disabled by SW as the check > for PG1 being >disabled is taken of by HW to enter DC6, and disabled when PG2 is enabled > respectively. >This helps save more power, especially in the case when display is > disabled but GT is >enabled. Accordingly, replacing DC5 trigger sequence with DC6 for SKL. > 2] DC6 could be enabled from intel_runtime_suspend() function, if DC5 is > already enabled. > 3] Move CSR-load-status setting code from intel_runtime_suspend function to a > new function. > > v4: > 1] Enable/disable DC6 only when toggling the power-well using a newly defined > macro ENABLE_DC6. > > v5: > 1] Load CSR on system resume too as firmware may be lost on system suspend > preventing >enabling DC5, DC6. > 2] DDI buffers shouldn't be programmed during driver-load/resume as it's > already done >during modeset initialization then and also that the encoder list is still > uninitialized by >then. Therefore, call intel_prepare_ddi function right after disabling DC6 > but outside >skl_disable_dc6 function and not during driver-load/resume. > > v6: > 1] Rebase to latest. > 2] Move SKL_ENABLE_DC6 macro definition from intel_display.c to > intel_runtime_pm.c. > > v7: > 1) Refactored the code for removing the warning got from checkpatch. > 2) After adding dmc ver 1.0 support rebased on top of nightly. (Animesh) > > Issue: VIZ-2819 > Signed-off-by: A.Sunil Kamath > Signed-off-by: Suketu Shah > Signed-off-by: Damien Lespiau > Signed-off-by: Animesh Manna > --- > drivers/gpu/drm/i915/i915_drv.c | 29 ++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_runtime_pm.c | 98 > + > 3 files changed, 81 insertions(+), 47 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 489caa6..352c7702 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -795,6 +795,8 @@ static int i915_drm_resume_early(struct drm_device *dev) > > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > hsw_disable_pc8(dev_priv); > + else if (IS_SKYLAKE(dev_priv)) > + ret = skl_resume_prepare(dev_priv); > > intel_uncore_sanitize(dev); > intel_power_domains_init_hw(dev_priv); > @@ -1009,6 +1011,19 @@ static int i915_pm_resume(struct device *dev) > return i915_drm_resume(drm_dev); > } > > +static int skl_suspend_complete(struct drm_i915_private *dev_priv) > +{ > + /* Enabling DC6 is not a hard requirement to enter runtime D3 */ > + > + /* > + * This is to ensure that CSR isn't identified as loaded before > + * CSR-loading program is called during runtime-resume. > + */ > + intel_csr_load_status_set(dev_priv, false); > + > + return 0; > +} > + > static int hsw_suspend_complete(struct drm_i915_private *dev_priv) > { > hsw_enable_pc8(dev_priv); > @@ -1016,6 +1031,16 @@ static int hsw_suspend_complete(struct > drm_i915_private *dev_priv) > return 0; > } > > +int skl_resume_prepare(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev = dev_priv->dev; > + > + if (!intel_csr_load_status_get(dev_priv)) No need to check the above, it's guaranteed the firmware is not loaded at this point. > + intel_csr_load_program(dev); > + > + return 0; > +} > + > /* > * Save all Gunit registers that may be lost after a D3 and a subsequent > * S0i[R123] transition. The list of registers needing a save/restore is > @@ -1484,6 +1509,8 @@ static int intel_runtime_resume(struct device *device) > > if (IS_GEN6(dev_priv)) > intel_init_pch_refclk(dev); > + else if (IS_SKYLAKE(dev)) > + ret = skl_resume_prepare(dev_priv); > else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > hsw_disable_pc8(dev_priv); > else if (IS_VALLEYV
Re: [Intel-gfx] [PATCH 8/8] drm/i915/skl: Enable runtime PM
On Wed, 2015-04-01 at 16:22 +0530, Animesh Manna wrote: > From: Suketu Shah > > Enable runtime PM for Skylake platform > > v2: After adding dmc ver 1.0 support rebased on top of nightly. (Animesh) > > Issue: VIZ-2819 > Signed-off-by: A.Sunil Kamath > Signed-off-by: Suketu Shah > Signed-off-by: Damien Lespiau > Signed-off-by: Animesh Manna Reviewed-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_drv.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 5dd8d61..e5fc1de 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2545,7 +2545,8 @@ struct drm_i915_cmd_table { >IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev) || \ >IS_SKYLAKE(dev)) > #define HAS_RUNTIME_PM(dev) (IS_GEN6(dev) || IS_HASWELL(dev) || \ > - IS_BROADWELL(dev) || IS_VALLEYVIEW(dev)) > + IS_BROADWELL(dev) || IS_VALLEYVIEW(dev) || \ > + IS_SKYLAKE(dev)) > #define HAS_RC6(dev) (INTEL_INFO(dev)->gen >= 6) > #define HAS_RC6p(dev)(INTEL_INFO(dev)->gen == 6 || > IS_IVYBRIDGE(dev)) > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] drm/i915/skl: Assert the requirements to enter or exit DC6.
On Wed, 2015-04-01 at 16:22 +0530, Animesh Manna wrote: > From: Suketu Shah > > Warn if the conditions to enter or exit DC6 are not satisfied such > as support for runtime PM, state of power well, CSR loading etc. > > v2: Removed camelcase in functions and variables. > > v3: Do some minimal check to assert if CSR program is not loaded. > > v4: > 1] Correct the check for backlight-disabling in assert_can_enable_dc6(). > 2] Check csr.loaded = false before disabling DC6 and simplify other checks. > > v5: > 1] Remove checks for DC5 state from assert_can_enable_dc6 function as DC5 is > no >longer enabled before enabling DC6. > 2] Correct the check for CSR-loading in assert_can_disable_dc6 function as > CSR must >be loaded for context restore to happen on DC6 disabling. > > v6: > 1] It's okay to explicitly disable DC6 during driver-load/resume even though > it might >already be disabled and so don't warn about it. > > v7: Rebase to latest. > > v8: Sqashed the patch from Imre - > [PATCH] drm/i915/skl: avoid false CSR fw not loaded WARN during driver > load/resume > > v9: After adding dmc ver 1.0 support rebased on top of nightly. (Animesh) > > Issue: VIZ-2819 > Signed-off-by: A.Sunil Kamath > Signed-off-by: Suketu Shah > Signed-off-by: Damien Lespiau > Signed-off-by: Animesh Manna > --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 38 > +++-- > 1 file changed, 32 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index cc94503..9196de3 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -1,5 +1,5 @@ > -/* > - * Copyright © 2012-2014 Intel Corporation > + /* > + * Cipyright © 2012-2014 Intel Corporation Unintentional change. > * > * Permission is hereby granted, free of charge, to any person obtaining a > * copy of this software and associated documentation files (the "Software"), > @@ -408,12 +408,39 @@ static void gen9_disable_dc5(struct drm_i915_private > *dev_priv) > POSTING_READ(DC_STATE_EN); > } > > -static void skl_enable_dc6(struct drm_i915_private *dev_priv) > +static void assert_can_enable_dc6(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; > + > + WARN(!IS_SKYLAKE(dev), "Platform doesn't support DC6.\n"); > + WARN(!HAS_RUNTIME_PM(dev), "Runtime PM not enabled.\n"); > + WARN(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE, > + "Backlight is not disabled.\n"); > + WARN((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6), > + "DC6 already programmed to be enabled.\n"); > + > + assert_csr_loaded(dev_priv); > +} > + > +static void assert_can_disable_dc6(struct drm_i915_private *dev_priv) > +{ > + /* > + * During initialization, the firmware may not be loaded yet. > + * We still want to make sure that the DC enabling flag is cleared. > + */ > + if (dev_priv->power_domains.initializing) > + return; I missed this earlier, but this early return should also be added to assert_can_disable_dc5(). With the above fixed: Reviewed-by: Imre Deak > + > + assert_csr_loaded(dev_priv); > + WARN(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6), > + "DC6 already programmed to be disabled.\n"); > +} > + > +static void skl_enable_dc6(struct drm_i915_private *dev_priv) > +{ > uint32_t val; > > - WARN_ON(!IS_SKYLAKE(dev)); > + assert_can_enable_dc6(dev_priv); > > DRM_DEBUG_KMS("Enabling DC6\n"); > > @@ -428,10 +455,9 @@ static void skl_enable_dc6(struct drm_i915_private > *dev_priv) > > static void skl_disable_dc6(struct drm_i915_private *dev_priv) > { > - struct drm_device *dev = dev_priv->dev; > uint32_t val; > > - WARN_ON(!IS_SKYLAKE(dev)); > + assert_can_disable_dc6(dev_priv); > > DRM_DEBUG_KMS("Disabling DC6\n"); > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/bxt: Determine BXT slice/subslice/EU info
On Wed, Apr 01, 2015 at 08:20:44AM +0200, Daniel Vetter wrote: > On Tue, Mar 31, 2015 at 09:59:22AM -0700, jeff.mc...@intel.com wrote: > > From: Jeff McGee > > > > BXT uses a subset of the SKL fuse registers, because it has at > > most 1 slice and at most 6 EU per subslice. > > > > Signed-off-by: Jeff McGee > > --- > > drivers/gpu/drm/i915/i915_dma.c | 47 > > + > > 1 file changed, 47 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c > > b/drivers/gpu/drm/i915/i915_dma.c > > index ec661fe..81afd31 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -733,6 +733,53 @@ static void intel_device_info_runtime_init(struct > > drm_device *dev) > > info->has_slice_pg = (info->slice_total > 1) ? 1 : 0; > > info->has_subslice_pg = 0; > > info->has_eu_pg = (info->eu_per_subslice > 2) ? 1 : 0; > > + } else if (IS_BROXTON(dev)) { > > By all reasonable standards this function is getting a bit too long. Can > you please create a patch to extract the various platform-specific sseu > detection logic. > > Also can't we just repurpose the skl version by limiting s_max > appropriately and applying > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 68e0c85a17cf..b164aeb09158 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -668,9 +668,8 @@ static void intel_device_info_runtime_init(struct > drm_device *dev) > ss_disable = (fuse2 & GEN9_F2_SS_DIS_MASK) >> >GEN9_F2_SS_DIS_SHIFT; > > - eu_disable[0] = I915_READ(GEN8_EU_DISABLE0); > - eu_disable[1] = I915_READ(GEN8_EU_DISABLE1); > - eu_disable[2] = I915_READ(GEN8_EU_DISABLE2); > + for (s = 0; s < s_max; s++) > + eu_disable[s] = I915_READ(GEN8_EU_DISABLE(s)); > > info->slice_total = hweight32(s_enable); > > with a suitable added #define ofc? > -Daniel Agree with all of the above. Regarding the 2nd patch in this series which performs related logic in i915_debugfs, would you like to see that broken up similarly? -Jeff ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Clean-up idr table if context create fails.
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6122 -Summary- Platform Delta drm-intel-nightly Series Applied PNV 272/272 272/272 ILK 302/302 302/302 SNB 303/303 303/303 IVB 338/338 338/338 BYT -1 287/287 286/287 HSW 361/361 361/361 BDW 308/308 308/308 -Detailed- Platform Testdrm-intel-nightly Series Applied *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(14) FAIL(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/20] drm/i915: skylake scaler structure definitions
On Wed, Apr 01, 2015 at 07:59:32PM -0700, Chandra Konduru wrote: > skylake scaler structure definitions. scalers live in crtc_state as > they are pipe resources. They can be used either as plane scaler or > panel fitter. > > scaler assigned to either plane (for plane scaling) or crtc (for panel > fitting) is saved in scaler_id in plane_state or crtc_state respectively. > > scaler_id is used instead of scaler pointer in plane or crtc state > to avoid updating scaler pointer everytime a new crtc_state is created. > > v2: > -made single copy of min/max values for scalers (Matt) > > v3: > -updated commentary for scaler_id (me) > > Signed-off-by: Chandra Konduru It seems like some of the things that were called out in the previous review cycle still haven't been addressed here. Repeating them below. > --- ... > +struct intel_scaler { > + int id; > + int in_use; > + uint32_t mode; > + uint32_t filter; As we noted in the last round of review, filter is constant in this patch series, so we don't need this field for now. We should only add this field if/when we decide to actually expose the other hardware filter types. > +}; > + > +struct intel_crtc_scaler_state { > +#define INTEL_MAX_SCALERS 2 > +#define SKL_NUM_SCALERS INTEL_MAX_SCALERS > + /* scalers available on this crtc */ > + int num_scalers; This is an unchanging hardware trait, not dynamic state, so we noted that this should move to the CRTC itself. The goal is to keep the state structure to things that truly are dynamic (and not trivial to recalculate from other state). > + struct intel_scaler scalers[INTEL_MAX_SCALERS]; > + > + /* > + * scaler_users: keeps track of users requesting scalers on this crtc. > + * > + * If a bit is set, a user is using a scaler. > + * Here user can be a plane or crtc as defined below: > + * bits 0-30 - plane (bit position is index from drm_plane_index) > + * bit 31- crtc > + * > + * Instead of creating a new index to cover planes and crtc, using > + * existing drm_plane_index for planes which is well less than 31 > + * planes and bit 31 for crtc. This should be fine to cover all > + * our platforms. > + * > + * intel_atomic_setup_scalers will setup available scalers to users > + * requesting scalers. It will gracefully fail if request exceeds > + * avilability. > + */ > +#define SKL_CRTC_INDEX 31 > + unsigned scaler_users; > + > + /* scaler used by crtc for panel fitting purpose */ > + int scaler_id; > + > + /* > + * Supported scaling ratio is represented as a range in [min max] > + * variables. This range covers both up and downscaling > + * where scaling ratio = (dst * 100)/src. > + * In above range any value: > + *< 100 represents downscaling coverage > + *> 100 represents upscaling coverage > + *= 100 represents no-scaling (i.e., 1:1) > + * e.g., a min value = 50 means -> supports upto 50% of original image > + * a max value = 200 means -> supports upto 200% of original image > + * > + * if incoming flip requires scaling in the supported [min max] range > + * then requested scaling will be performed. > + */ > + uint32_t min_hsr; > + uint32_t max_hsr; > + uint32_t min_vsr; > + uint32_t max_vsr; > + uint32_t min_hvsr; > + uint32_t max_hvsr; We noted these could be dropped in the last review cycle. > + > + uint32_t min_src_w; > + uint32_t max_src_w; > + uint32_t min_src_h; > + uint32_t max_src_h; > + uint32_t min_dst_w; > + uint32_t max_dst_w; > + uint32_t min_dst_h; > + uint32_t max_dst_h; And we noted that these should just be #define's. > +}; > + > struct intel_crtc_state { > struct drm_crtc_state base; > > @@ -391,6 +479,8 @@ struct intel_crtc_state { > > bool dp_encoder_is_mst; > int pbn; > + > + struct intel_crtc_scaler_state scaler_state; > }; > > struct intel_pipe_wm { > -- > 1.7.9.5 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/20] drm/i915: Adding drm helper function drm_plane_from_index().
On Wed, Apr 01, 2015 at 07:59:30PM -0700, Chandra Konduru wrote: > Adding drm helper function to return plane pointer from index where > index is a returned by drm_plane_index. > > v2: > -avoided nested loop by adding loop count (Daniel) > > Signed-off-by: Chandra Konduru This should just have a "drm:" headline prefix and should have a Cc: to dri-devel here since it's touching DRM core code. Otherwise, Reviewed-by: Matt Roper > --- > drivers/gpu/drm/drm_crtc.c | 22 ++ > include/drm/drm_crtc.h |1 + > 2 files changed, 23 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index d576a4d..0f0159b 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -1289,6 +1289,28 @@ unsigned int drm_plane_index(struct drm_plane *plane) > EXPORT_SYMBOL(drm_plane_index); > > /** > + * drm_plane_from_index - find the registered plane at an index > + * @idx: index of registered plane to find for > + * > + * Given a plane index, return the registered plane from DRM device's > + * list of planes with matching index. > + */ > +struct drm_plane * > +drm_plane_from_index(struct drm_device *dev, int idx) > +{ > + struct drm_plane *plane; > + unsigned int i = 0; > + > + list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > + if (i == idx) > + return plane; > + i++; > + } > + return NULL; > +} > +EXPORT_SYMBOL(drm_plane_from_index); > + > +/** > * drm_plane_force_disable - Forcibly disable a plane > * @plane: plane to disable > * > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 7b5c661..6b30036 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1264,6 +1264,7 @@ extern int drm_plane_init(struct drm_device *dev, > bool is_primary); > extern void drm_plane_cleanup(struct drm_plane *plane); > extern unsigned int drm_plane_index(struct drm_plane *plane); > +extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int > idx); > extern void drm_plane_force_disable(struct drm_plane *plane); > extern int drm_plane_check_pixel_format(const struct drm_plane *plane, > u32 format); > -- > 1.7.9.5 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/20] drm/i915: Helper function to update skylake scaling ratio.
On Wed, Apr 01, 2015 at 07:59:37PM -0700, Chandra Konduru wrote: > Helper function updates supported scaling ratios based on cdclk and > crtc clocks. > > v2: > -update single copy of scaling ratios (Matt) > > v3: > -min scaling ratio is limited by either display engine limit or clocks, > it is not related to previous ratio (Matt, me) > > Signed-off-by: Chandra Konduru As noted on an earlier patch (and on the previous review cycle), min_vsr and min_hvsr are never used, so we should just drop them completely for now. min_hsr is only used in a couple trivial places, so we should probably just replace that usage with a direct calculation of max(CONST, clockval). That would allow us to drop this patch completely. Matt > --- > drivers/gpu/drm/i915/intel_display.c | 25 + > 1 file changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 316c4c2..8b2eff4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4616,6 +4616,31 @@ static void haswell_mode_set_planes_workaround(struct > intel_crtc *crtc) > intel_wait_for_vblank(dev, other_active_crtc->pipe); > } > > +static void skl_update_scaling_ratio(struct drm_device *dev, > + struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + uint32_t crtc_clock, cdclk; > + struct intel_crtc_scaler_state *scaler_state; > + > + if (!crtc_state) > + return; > + > + crtc_clock = (uint32_t) crtc_state->base.adjusted_mode.crtc_clock; > + cdclk = (uint32_t) dev_priv->display.get_display_clock_speed(dev); > + > + if (!crtc_clock || !cdclk) > + return; > + > + scaler_state = &crtc_state->scaler_state; > + scaler_state->min_hsr = max((uint32_t)34, (crtc_clock * 100)/cdclk); > + scaler_state->min_vsr = max((uint32_t)34, (crtc_clock * 100)/cdclk); > + scaler_state->min_hvsr = max((uint32_t)12, (crtc_clock * 100)/cdclk); > + > + DRM_DEBUG_KMS("for crtc_state = %p crtc_clock = %d cdclk = %d\n", > crtc_state, > + crtc_clock, cdclk); > +} > + > static void haswell_crtc_enable(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > -- > 1.7.9.5 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints
On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote: > This patch converts intel_plane_state->src rect from 16.16 > values into regular ints. > > This approach aligns with sprite_plane_state->src rects > which are already in regular ints. > > Signed-off-by: Chandra Konduru You're not touching cursor state here, so I guess it stays in 16.16 form always? Does it need to be updated to behave the same way? Looking at our sprite code today, it treats intel_state->src as 16.16 for most of the check function, then re-writes it as integer pixels near the end, which I guess matches the type of change you're doing here. I still find this pretty confusing that our structure is sometimes interpreted in one way and other times interpreted a different way. Personally I think it would be less error-prone if we just treated src as 16.16 always, but if you to keep the current logic which changes the meaning at the end of the check() stage, can you add some comments to struct intel_plane_state clarifying that? With some extra comments to avoid future confusion, you can consider this Reviewed-by: Matt Roper Matt > --- > drivers/gpu/drm/i915/intel_display.c | 13 +++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index ee71bde..dddbe11 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12590,6 +12590,15 @@ intel_check_primary_plane(struct drm_plane *plane, > intel_crtc->atomic.update_wm = true; > } > > + /* > + * Hardware doesn't handle subpixel coordinates. > + * Adjust to (macro)pixel boundary. > + */ > + src->x1 >>= 16; > + src->x2 >>= 16; > + src->y1 >>= 16; > + src->y2 >>= 16; > + > return 0; > } > > @@ -12608,8 +12617,8 @@ intel_commit_primary_plane(struct drm_plane *plane, > intel_crtc = to_intel_crtc(crtc); > > plane->fb = fb; > - crtc->x = src->x1 >> 16; > - crtc->y = src->y1 >> 16; > + crtc->x = src->x1; > + crtc->y = src->y1; > > if (intel_crtc->active) { > if (state->visible) { > -- > 1.7.9.5 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/20] drm/i915: Add helper function to update scaler_users in crtc_state
On Wed, Apr 01, 2015 at 07:59:38PM -0700, Chandra Konduru wrote: > This helper function stages a scaler request for a plane/crtc into > crtc_state->scaler_users (which is a bit field). It also performs > required checks before staging any change into scaler_state. > > v2: > -updates to use single copy of scaler limits (Matt) > -added force detach parameter for pfit disable purpose (me) > > v3: > -updated function header to kerneldoc format (Matt) > -dropped need_scaling checks (Matt) > > v4: > -move clearing of scaler id from commit path to check path (Matt) > -updated colorkey checks based on recent updates (me) > -squashed scaler check while enabling colorkey to here (me) > -use values in plane_state->src as regular integers (me) > > Signed-off-by: Chandra Konduru As noted in the previous review cycle, this should at least be squashed with patch 17, which is when you start calling this. > --- > drivers/gpu/drm/i915/intel_display.c | 144 > ++ > drivers/gpu/drm/i915/intel_drv.h |3 + > drivers/gpu/drm/i915/intel_sprite.c |9 +++ > 3 files changed, 156 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 8b2eff4..603a2dc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12594,6 +12594,150 @@ intel_cleanup_plane_fb(struct drm_plane *plane, > } > } > > +/** > + * skl_update_scaler_users - Stages update to crtc's scaler state > + * @intel_crtc: crtc > + * @crtc_state: crtc_state > + * @plane: plane (NULL indicates crtc is requesting update) > + * @plane_state: plane's state > + * @force_detach: request unconditional detachment of scaler > + * > + * This function updates scaler state for requested plane or crtc. > + * To request scaler usage update for a plane, caller shall pass plane > pointer. > + * To request scaler usage update for crtc, caller shall pass plane pointer > + * as NULL. > + * > + * Return > + * 0 - scaler_usage updated successfully > + *error - requested scaling cannot be supported or other error condition > + */ > +int > +skl_update_scaler_users( > + struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state, > + struct intel_plane *intel_plane, struct intel_plane_state *plane_state, > + int force_detach) > +{ > + int need_scaling; > + int idx; > + int src_w, src_h, dst_w, dst_h; > + int *scaler_id; > + struct drm_framebuffer *fb; > + struct intel_crtc_scaler_state *scaler_state; > + > + if (!intel_crtc || !crtc_state || > + (intel_plane && intel_plane->base.type == > DRM_PLANE_TYPE_CURSOR)) It doesn't look possible to get here with a cursor plane to me. Maybe wrap that plane type test in a WARN_ON() so that we notice if we screw up? > + return 0; > + > + scaler_state = &crtc_state->scaler_state; > + > + if (!scaler_state->num_scalers) { > + DRM_DEBUG_KMS("crtc_state = %p, num_scalers = %d\n", crtc_state, > + scaler_state->num_scalers); > + return 0; > + } > + > + idx = intel_plane ? drm_plane_index(&intel_plane->base) : > SKL_CRTC_INDEX; > + fb = intel_plane ? plane_state->base.fb : NULL; > + > + if (intel_plane) { > + src_w = drm_rect_width(&plane_state->src); > + src_h = drm_rect_height(&plane_state->src); > + dst_w = drm_rect_width(&plane_state->dst); > + dst_h = drm_rect_height(&plane_state->dst); > + scaler_id = &plane_state->scaler_id; > + } else { > + struct drm_display_mode *adjusted_mode = > + &crtc_state->base.adjusted_mode; > + src_w = crtc_state->pipe_src_w; > + src_h = crtc_state->pipe_src_h; > + dst_w = adjusted_mode->hdisplay; > + dst_h = adjusted_mode->vdisplay; > + scaler_id = &scaler_state->scaler_id; > + } > + need_scaling = (src_w != dst_w || src_h != dst_h); > + > + /* > + * if plane is being disabled or scaler is no more required or force > detach > + * - free scaler binded to this plane/crtc > + * - in order to do this, update crtc->scaler_usage > + * > + * Here scaler state in crtc_state is set free so that > + * scaler can be assigned to other user. Actual register > + * update to free the scaler is done in plane/panel-fit programming. > + * For this purpose crtc/plane_state->scaler_id isn't reset here. > + */ > + if (force_detach || !need_scaling || (intel_plane && I guess this force_detach approach is a bit of a necessary evil because we haven't converted to atomic for full modesets yet (or have real crtc_state tracking yet). I'm a little bit worried that it will make the eventual atomic conversion a bit trickier, but I guess there isn't much we can do about that if we're planning to merge this series on today's dri
Re: [Intel-gfx] [PATCH 11/20] drm/i915: Helper function to detach a scaler from a plane or crtc
On Wed, Apr 01, 2015 at 07:59:40PM -0700, Chandra Konduru wrote: > This function is called from commit path of a plane or crtc. > It programs scaler registers to detach (aka. unbinds) scaler > from requested plane or crtc if it isn't in use. It also resets > scaler_id in crtc/plane state. The last sentence here is no longer true, so you should probably remove it to avoid confusion. Otherwise, Reviewed-by: Matt Roper > > v2: > -improved a log message (me) > > v3: > -improved commentary (Matt) > -added a case where scaler id needs to be reset (me) > > v4: > -changes made not to modify state in commit path (Matt) > > Signed-off-by: Chandra Konduru > --- > drivers/gpu/drm/i915/intel_display.c | 29 + > drivers/gpu/drm/i915/intel_drv.h |1 + > 2 files changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 603a2dc..8cf0d0e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2938,6 +2938,35 @@ unsigned long intel_plane_obj_offset(struct > intel_plane *intel_plane, > return i915_gem_obj_ggtt_offset_view(obj, view); > } > > +/* > + * This function detaches (aka. unbinds) unused scalers in hardware > + */ > +void skl_detach_scalers(struct intel_crtc *intel_crtc) > +{ > + struct drm_device *dev; > + struct drm_i915_private *dev_priv; > + struct intel_crtc_scaler_state *scaler_state; > + int i; > + > + if (!intel_crtc || !intel_crtc->config) > + return; > + > + dev = intel_crtc->base.dev; > + dev_priv = dev->dev_private; > + scaler_state = &intel_crtc->config->scaler_state; > + > + /* loop through and disable scalers that aren't in use */ > + for (i = 0; i < scaler_state->num_scalers; i++) { > + if (!scaler_state->scalers[i].in_use) { > + I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, i), 0); > + I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, i), 0); > + I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, i), 0); > + DRM_DEBUG_KMS("CRTC:%d Disabled scaler id %u.%u\n", > + intel_crtc->base.base.id, intel_crtc->pipe, i); > + } > + } > +} > + > static void skylake_update_primary_plane(struct drm_crtc *crtc, >struct drm_framebuffer *fb, >int x, int y) > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 1381d11..7bb4c44 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1146,6 +1146,7 @@ void intel_modeset_preclose(struct drm_device *dev, > struct drm_file *file); > int skl_update_scaler_users(struct intel_crtc *intel_crtc, > struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane, > struct intel_plane_state *plane_state, int force_detach); > +void skl_detach_scalers(struct intel_crtc *intel_crtc); > > unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane, >struct drm_i915_gem_object *obj); > -- > 1.7.9.5 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/20] drm/i915: Add atomic function to setup scalers scalers for a crtc.
On Wed, Apr 01, 2015 at 07:59:39PM -0700, Chandra Konduru wrote: > intel_atomic_setup_scalers sets up scalers based on staged scaling > requests coming from a crtc and its planes. This function should be > called from crtc level check path. > > If staged requests are supportable, function assigns scalers to > requested planes and crtc. This function also takes into account > the current planes using scalers but not being part of this > atomic state for optimal operation of scalers. Note that the scaler > assignement itself is staged into crtc_state and respective > plane_states for later commit after all checks have been done. > > overall high level flow: > - scaler requests are staged into crtc_state by planes/crtc > - check whether staged scaling requests can be supported > - add planes using scalers that aren't in current transaction > - assign scalers to requested users > - as part of plane commit, scalers will be committed >(i.e., either attached or detached) to respective planes in hw > - as part of crtc_commit, scaler will be either attached or detached >to crtc in hw > > v2: > -removed a log message (me) > -changed input parameter to crtc_state (me) > > v3: > -remove assigning plane_state returned by drm_atomic_get_plane_state (Matt) > -fail if there is an error from drm_atomic_get_plane_state (Matt) > > Signed-off-by: Chandra Konduru So looking ahead through the patch series, it looks like the places you call this are: * intel_crtc_compute_config() --- Will presumably move to check_crtc() once we're farther along with atomic conversion. * intel_atomic_check() --- Handles updates via atomic ioctl (will also handle legacy plane updates once we switch to full atomic helpers) * skylake_pfit_update() Since we're on transitional plane helpers today (which don't create a top-level atomic transaction and thus never call intel_atomic_check), does this ever get called for legacy SetPlane() operations on today's driver? Is it correct to assume that the switch back to full atomic helpers is a prereq for merging this, or am I overlooking something? Matt > --- > drivers/gpu/drm/i915/intel_atomic.c | 142 > +++ > drivers/gpu/drm/i915/intel_drv.h|3 + > 2 files changed, 145 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c > b/drivers/gpu/drm/i915/intel_atomic.c > index 3903b90..fab1f13 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -241,3 +241,145 @@ intel_crtc_destroy_state(struct drm_crtc *crtc, > { > drm_atomic_helper_crtc_destroy_state(crtc, state); > } > + > +/** > + * intel_atomic_setup_scalers() - setup scalers for crtc per staged requests > + * @dev: DRM device > + * @crtc: intel crtc > + * @crtc_state: incoming crtc_state to validate and setup scalers > + * > + * This function sets up scalers based on staged scaling requests for > + * a @crtc and its planes. It is called from crtc level check path. If > request > + * is a supportable request, it attaches scalers to requested planes and > crtc. > + * > + * This function takes into account the current scaler(s) in use by any > planes > + * not being part of this atomic state > + * > + * Returns: > + * 0 - scalers were setup succesfully > + * error code - otherwise > + */ > +int intel_atomic_setup_scalers(struct drm_device *dev, > + struct intel_crtc *intel_crtc, > + struct intel_crtc_state *crtc_state) > +{ > + struct drm_plane *plane = NULL; > + struct intel_plane *intel_plane; > + struct intel_plane_state *plane_state = NULL; > + struct intel_crtc_scaler_state *scaler_state; > + struct drm_atomic_state *drm_state; > + int num_scalers_need; > + int i, j; > + > + if (INTEL_INFO(dev)->gen < 9 || !intel_crtc || !crtc_state) > + return 0; > + > + scaler_state = &crtc_state->scaler_state; > + drm_state = crtc_state->base.state; > + > + num_scalers_need = hweight32(scaler_state->scaler_users); > + DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users = > 0x%x\n", > + crtc_state, num_scalers_need, scaler_state->num_scalers, > + scaler_state->scaler_users); > + > + /* if there is no change in scaler configuration, return */ > + if (scaler_state->scaler_users == > + intel_crtc->config->scaler_state.scaler_users) > + return 0; > + > + /* > + * High level flow: > + * - staged scaler requests are already in scaler_state->scaler_users > + * - check whether staged scaling requests can be supported > + * - add planes using scalers that aren't in current transaction > + * - assign scalers to requested users > + * - as part of plane commit, scalers will be committed > + * (i.e., either attached or detached) to respective planes in hw > + * - as part of crtc_commit, scaler will be either attached or detached > + *
Re: [Intel-gfx] [PATCH 13/20] drm/i915: use current scaler state during readout_hw_state.
On Wed, Apr 01, 2015 at 07:59:42PM -0700, Chandra Konduru wrote: > During readout_hw_state, rebuild crtc scaler_state from hw state: > - crtc scaler id > - scaler users This patch doesn't look like it actually does what you're advertising here. If your firmware or bootloader or whatever has programmed the display in a way that some scalers are in use, I think your state variables are still going to show all scalers as free after you're done here, right? > - scaling ratios I think this is the only thing that will actually match the hardware state, but we've already noted that those state fields can probably just go away since we can trivially calculate the proper values in the places we're currently using them. Matt > > Signed-off-by: Chandra Konduru > --- > drivers/gpu/drm/i915/intel_display.c |5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index aae4a22..834ea46 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8756,6 +8756,11 @@ static bool haswell_get_pipe_config(struct intel_crtc > *crtc, > > intel_get_pipe_timings(crtc, pipe_config); > > + if (INTEL_INFO(dev)->gen >= 9) { > + skl_init_scalers(dev, crtc->pipe, pipe_config); > + skl_update_scaling_ratio(dev, pipe_config); > + } > + > pfit_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe); > if (intel_display_power_is_enabled(dev_priv, pfit_domain)) { > if (IS_SKYLAKE(dev)) > -- > 1.7.9.5 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 19/20] drm/i915: Enable skylake primary plane scaling using shared scalers
On Wed, Apr 01, 2015 at 07:59:48PM -0700, Chandra Konduru wrote: > This patch enables skylake primary plane display scaling using shared > scalers atomic desgin. > > v2: > -use single copy of scaler limits (Matt) > > v3: > -move detach_scalers to crtc commit path (Matt) > -use values in plane_state->src as regular integers (me) > > Signed-off-by: Chandra Konduru > --- > drivers/gpu/drm/i915/intel_atomic.c |5 ++- > drivers/gpu/drm/i915/intel_display.c | 72 > +++--- > 2 files changed, 71 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c > b/drivers/gpu/drm/i915/intel_atomic.c > index 7b8efd4..f14c60e 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -167,7 +167,7 @@ int intel_atomic_commit(struct drm_device *dev, > plane->state->state = NULL; > } > > - /* swap crtc_state */ > + /* swap crtc_scaler_state */ > for (i = 0; i < dev->mode_config.num_crtc; i++) { > struct drm_crtc *crtc = state->crtcs[i]; > if (!crtc) { > @@ -176,6 +176,9 @@ int intel_atomic_commit(struct drm_device *dev, > > to_intel_crtc(crtc)->config->scaler_state = > > to_intel_crtc_state(state->crtc_states[i])->scaler_state; > + > + if (INTEL_INFO(dev)->gen >= 9) > + skl_detach_scalers(to_intel_crtc(crtc)); > } > > drm_atomic_helper_commit_planes(dev, state); > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 664b7fb..4e9d9f6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2978,6 +2978,14 @@ static void skylake_update_primary_plane(struct > drm_crtc *crtc, > int pipe = intel_crtc->pipe; > u32 plane_ctl, stride_div; > unsigned long surf_addr; > + struct intel_crtc_state *crtc_state = intel_crtc->config; > + struct intel_plane_state *plane_state; > + int src_x = 0, src_y = 0, src_w = 0, src_h = 0; > + int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0; > + int scaler_id = -1; > + > + plane_state = crtc->primary ? > + to_intel_plane_state(crtc->primary->state) : NULL; > > if (!intel_crtc->primary_enabled) { > I915_WRITE(PLANE_CTL(pipe, 0), 0); > @@ -3046,12 +3054,41 @@ static void skylake_update_primary_plane(struct > drm_crtc *crtc, > fb->pixel_format); > surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj); > > + if (plane_state) { > + scaler_id = plane_state->scaler_id; > + src_x = plane_state->src.x1; > + src_y = plane_state->src.y1; > + src_w = drm_rect_width(&plane_state->src); > + src_h = drm_rect_height(&plane_state->src); > + dst_x = plane_state->dst.x1; > + dst_y = plane_state->dst.y1; > + dst_w = drm_rect_width(&plane_state->dst); > + dst_h = drm_rect_height(&plane_state->dst); > + } > + > I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); > I915_WRITE(PLANE_POS(pipe, 0), 0); > + > + if (src_w && src_h && dst_w && dst_h && scaler_id >= 0) { > + uint32_t ps_ctrl = 0; > + > + WARN_ON(x != src_x || y != src_y); > + ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(0) | > + crtc_state->scaler_state.scalers[scaler_id].mode | > + crtc_state->scaler_state.scalers[scaler_id].filter; > + I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl); > + I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0); > + I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (dst_x << 16) | > dst_y); > + I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id), (dst_w << 16) | > dst_h); > + > + I915_WRITE(PLANE_SIZE(pipe, 0), ((src_h - 1) << 16) | (src_w - > 1)); > + } else { > + I915_WRITE(PLANE_SIZE(pipe, 0), > + (intel_crtc->config->pipe_src_h - 1) << 16 | > + (intel_crtc->config->pipe_src_w - 1)); > + } > + > I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x); > - I915_WRITE(PLANE_SIZE(pipe, 0), > -(intel_crtc->config->pipe_src_h - 1) << 16 | > -(intel_crtc->config->pipe_src_w - 1)); > I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div); > I915_WRITE(PLANE_SURF(pipe, 0), surf_addr); > > @@ -12821,19 +12858,33 @@ intel_check_primary_plane(struct drm_plane *plane, > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc = state->base.crtc; > struct intel_crtc *intel_crtc; > + struct intel_crtc_state *crtc_state; > struct drm_framebuffer *fb = state->base.fb; > struct drm_rect *dest = &state->dst; > struct drm_rect *src = &state->src; > cons
Re: [Intel-gfx] [PATCH 20/20] drm/i915: Enable skylake sprite plane scaling using shared scalers
On Wed, Apr 01, 2015 at 07:59:49PM -0700, Chandra Konduru wrote: > This patch enables skylake sprite plane display scaling using shared > scalers atomic desgin. > > v2: > -use single copy of scaler limits (Matt) > > v3: > -detaching scalers moved to crtc commit path (Matt) > > Signed-off-by: Chandra Konduru > --- > drivers/gpu/drm/i915/intel_sprite.c | 60 > +-- > 1 file changed, 50 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index c8feff7..562f90c 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -33,6 +33,7 @@ > #include > #include > #include > +#include > #include > #include "intel_drv.h" > #include > @@ -194,6 +195,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct > drm_crtc *crtc, > int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey; > unsigned long surf_addr; > + struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config; > + int scaler_id; > > plane_ctl = PLANE_CTL_ENABLE | > PLANE_CTL_PIPE_CSC_ENABLE; > @@ -264,6 +267,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct > drm_crtc *crtc, > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], > fb->pixel_format); > > + scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id; > + > /* Sizes are 0 based */ > src_w--; > src_h--; > @@ -283,10 +288,30 @@ skl_update_plane(struct drm_plane *drm_plane, struct > drm_crtc *crtc, > > surf_addr = intel_plane_obj_offset(intel_plane, obj); > > + /* program plane scaler */ > + if (scaler_id >= 0) { > + uint32_t ps_ctrl = 0; > + > + DRM_DEBUG_KMS("plane = %d PS_PLANE_SEL(plane) = 0x%x\n", plane, > + PS_PLANE_SEL(plane)); > + ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) | > + crtc_state->scaler_state.scalers[scaler_id].mode | > + crtc_state->scaler_state.scalers[scaler_id].filter; > + I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl); > + I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0); > + I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | > crtc_y); > + I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id), > + ((crtc_w + 1) << 16)|(crtc_h + 1)); > + > + I915_WRITE(PLANE_POS(pipe, plane), 0); > + I915_WRITE(PLANE_SIZE(pipe, plane), (src_h << 16) | src_w); > + } else { > + I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x); > + I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w); > + } > + > I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x); > I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div); > - I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x); > - I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w); > I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl); > I915_WRITE(PLANE_SURF(pipe, plane), surf_addr); > POSTING_READ(PLANE_SURF(pipe, plane)); > @@ -863,7 +888,9 @@ static int > intel_check_sprite_plane(struct drm_plane *plane, >struct intel_plane_state *state) > { > + struct drm_device *dev = plane->dev; > struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc); > + struct intel_crtc_state *crtc_state; > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_framebuffer *fb = state->base.fb; > int crtc_x, crtc_y; > @@ -872,11 +899,16 @@ intel_check_sprite_plane(struct drm_plane *plane, > struct drm_rect *src = &state->src; > struct drm_rect *dst = &state->dst; > const struct drm_rect *clip = &state->clip; > + struct intel_crtc_scaler_state *scaler_state; > int hscale, vscale; > int max_scale, min_scale; > int pixel_size; > + int ret; > > intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc); > + crtc_state = state->base.state ? > + intel_atomic_get_crtc_state(state->base.state, intel_crtc) : > NULL; > + scaler_state = crtc_state ? &crtc_state->scaler_state : NULL; > > if (!fb) { > state->visible = false; > @@ -903,6 +935,11 @@ intel_check_sprite_plane(struct drm_plane *plane, > max_scale = intel_plane->max_downscale << 16; > min_scale = intel_plane->can_scale ? 1 : (1 << 16); > > + if (INTEL_INFO(dev)->gen >= 9 && scaler_state && > scaler_state->num_scalers) { > + min_scale = 1; > + max_scale = (100 << 16) / scaler_state->min_hsr; > + } > + > drm_rect_rotate(src, fb->width << 16, fb->height << 16, > state->base.rotation)
Re: [Intel-gfx] [PATCH 00/20] skylake display scalers
On Wed, Apr 01, 2015 at 07:59:29PM -0700, Chandra Konduru wrote: > primary changes in this version are: > - moved changes to state from commit patch to check path (Matt) > - squashed few patches into others (Matt) > - rebased colorkey related patches ontop of recent updates (Daniel) > - rebased all patches onto today's drm-intel-nightly (me) > - initialized colorkey to NONE (me) > - changed primary plane src values to regular integers align with sprite (me) > - used updated get display clock function (me) > > Numbers of patches updated in this version: > 4, 6, 8, 9, 11, 18, 19, 20 > > Sending full patch series for completeness. Individual patch headers > have additional details on changes. This series should cleanly > merge to latest drm-intel-nighly. I've sent review feedback to several of the patches and some of those have my r-b assuming minor updates to the commit message or comments are made. You can also consider 2, 4, 5, 7, 12, and 14-18 to be Reviewed-by: Matt Roper A couple other notes/questions: * What is the high-level strategy for merging this? Do we need to wait for full atomic helpers to be switched back on? It looked to me like one of the patches here wouldn't work properly / get called for SetPlane() updates while using the transitional plane helpers. * I think it would be good to get Ander's thoughts on this as well since a lot of this directly intersects with the type of things he's working on. In a couple places it feels a little bit like we have to do things the "wrong" way here because it's the best we can do on the current codebase, so we'll have to backtrack and re-write things a bit once we have full atomic modesetting working through the full check/commit model. So Ander's high level ack would be good as well, since it may make his work more challenging. Matt > > Chandra Konduru (20): > drm/i915: Adding drm helper function drm_plane_from_index(). > drm/i915: Register definitions for skylake scalers > drm/i915: skylake scaler structure definitions > drm/i915: Initialize plane colorkey to NONE > drm/i915: Initialize skylake scalers > drm/i915: Convert primary plane 16.16 values to regular ints > drm/i915: Dump scaler_state too as part of dumping crtc_state > drm/i915: Helper function to update skylake scaling ratio. > drm/i915: Add helper function to update scaler_users in crtc_state > drm/i915: Add atomic function to setup scalers scalers for a crtc. > drm/i915: Helper function to detach a scaler from a plane or crtc > drm/i915: Preserve scaler state when clearing crtc_state > drm/i915: use current scaler state during readout_hw_state. > drm/i915: Update scaling ratio as part of crtc_compute_config > drm/i915: Ensure setting up scalers into staged crtc_state > drm/i915: copy staged scaler state from drm state to crtc->config. > drm/i915: stage panel fitting scaler request for fixed mode panel > drm/i915: Enable skylake panel fitting using skylake shared scalers > drm/i915: Enable skylake primary plane scaling using shared scalers > drm/i915: Enable skylake sprite plane scaling using shared scalers > > drivers/gpu/drm/drm_crtc.c | 22 ++ > drivers/gpu/drm/i915/i915_reg.h | 115 > drivers/gpu/drm/i915/intel_atomic.c | 168 > drivers/gpu/drm/i915/intel_display.c | 489 > +++--- > drivers/gpu/drm/i915/intel_dp.c |8 + > drivers/gpu/drm/i915/intel_drv.h | 97 +++ > drivers/gpu/drm/i915/intel_sprite.c | 71 - > include/drm/drm_crtc.h |1 + > 8 files changed, 923 insertions(+), 48 deletions(-) > > -- > 1.7.9.5 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Allow userptr backchannel for passing aroung GTT mappings
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6123 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -1 272/272 271/272 ILK -1 302/302 301/302 SNB -1 303/303 302/303 IVB -1 338/338 337/338 BYT -2 287/287 285/287 HSW -1 361/361 360/361 BDW -1 308/308 307/308 -Detailed- Platform Testdrm-intel-nightly Series Applied *PNV igt@gem_userptr_blits@invalid-gtt-mapping PASS(2) FAIL(2) *ILK igt@gem_userptr_blits@invalid-gtt-mapping PASS(2) FAIL(2) *SNB igt@gem_userptr_blits@invalid-gtt-mapping PASS(2) FAIL(2) *IVB igt@gem_userptr_blits@invalid-gtt-mapping PASS(2) FAIL(2) *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(15) FAIL(1)PASS(1) *BYT igt@gem_userptr_blits@invalid-gtt-mapping PASS(2) FAIL(2) *HSW igt@gem_userptr_blits@invalid-gtt-mapping PASS(2) FAIL(2) *BDW igt@gem_userptr_blits@invalid-gtt-mapping PASS(2) FAIL(2) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx