Re: [Intel-gfx] [PATCH] drm/i915: GEN7_MSG_CONTROL is ivb-only
On Sun, Jan 26, 2014 at 08:43:53PM +, Runyan, Arthur J wrote: > > > >-Original Message- > >From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] > >Sent: Wednesday, January 22, 2014 2:40 PM > >To: Intel Graphics Development > >Cc: Daniel Vetter; Chris Wilson; Runyan, Arthur J; Dave Airlie > >Subject: [PATCH] drm/i915: GEN7_MSG_CONTROL is ivb-only > > > >At least I couldn't find it in the Haswell Bspec any more and we've > >tried to test-boot a Haswell machine with num_pipes forced to 0 (i.e. > >hit the PCH_NOP path) and the unclaimed register logic complained. > > > >So restrict this dance to just ivb platforms. > > > >v2: Art pointed out that the bits simply moved on hsw+ > > > >v3: Buy code terseneness with a notch of sublety as suggested by > >Chris. > > > >v4: Frob the right bit, spotted by Art. > > > >Cc: Chris Wilson > >Cc: Arthur Ranyan > >Cc: Dave Airlie > >Signed-off-by: Daniel Vetter > > Reviewed-by: Art Runyan Queued for -next, thanks for the review. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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 5/5] drm/i915: Fix error capture on BYT/BDW
On Sun, Jan 26, 2014 at 07:55:59PM +, Chris Wilson wrote: > On Sun, Jan 26, 2014 at 11:05:40AM -0800, Ben Widawsky wrote: > > On Sun, Jan 26, 2014 at 11:47:40AM +, Chris Wilson wrote: > > > On Fri, Jan 24, 2014 at 06:17:45PM -0800, Ben Widawsky wrote: > > > > The previous check during error capture of whether or not the current VM > > > > should be scanned used, gen < 7. That was more or less trying to > > > > determine if there was a full PPGTT. At the time, this was sort of what > > > > I meant to do because I was more interested in working backwards from > > > > hardware state. However, this is incorrect because it will not include > > > > platforms that are greater than gen7, and not having PPGTT. Example > > > > would be BYT which is gen7 but doesn't have PPGTT, BDW, or any platform > > > > greater than gen7 with the PPGTT module parameter invoked. > > > > > > > > I am /assuming/ BYT was broken, I have not actually checked. > > > > > > > > While here, clean up the file a bit to avoid duplicate reads (now that > > > > the PPGTT info is in the error state). > > > > > > > > I think Mika/Chris may have been looking at this too. > > > > > > Sure, we are looking (for identifying the guilty request/batch) by using > > > the older, simpler mechanism of finding the first incomplete request. I > > > think that search is now definite since we preallocate the request and no > > > longer do request collascing if ENOMEM (i.e. there is a 1:1 relationship > > > between seqno/batch/request). > > > > > > That should also apply here and be much simpler. > > > > How does that solve hangs which aren't caused by requests? > > Was that an intentional rhetorical question? > > The code you touch here only deals with requests - finding the current > batchbuffer if any. > -Chris > It wasn't rhetorical. I temporarily ignored that all batches are tied to a request. So what's the plan now? Just looking at the callers, we seem to have a couple of callers that can't easily identify the bad request. -- Ben Widawsky, 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: GEN7_MSG_CONTROL is ivb-only
>-Original Message-
>From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch]
>Sent: Wednesday, January 22, 2014 2:40 PM
>To: Intel Graphics Development
>Cc: Daniel Vetter; Chris Wilson; Runyan, Arthur J; Dave Airlie
>Subject: [PATCH] drm/i915: GEN7_MSG_CONTROL is ivb-only
>
>At least I couldn't find it in the Haswell Bspec any more and we've
>tried to test-boot a Haswell machine with num_pipes forced to 0 (i.e.
>hit the PCH_NOP path) and the unclaimed register logic complained.
>
>So restrict this dance to just ivb platforms.
>
>v2: Art pointed out that the bits simply moved on hsw+
>
>v3: Buy code terseneness with a notch of sublety as suggested by
>Chris.
>
>v4: Frob the right bit, spotted by Art.
>
>Cc: Chris Wilson
>Cc: Arthur Ranyan
>Cc: Dave Airlie
>Signed-off-by: Daniel Vetter
Reviewed-by: Art Runyan
>---
> drivers/gpu/drm/i915/i915_gem.c | 12 +---
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>index 32636a470367..c57ecff4ea00 100644
>--- a/drivers/gpu/drm/i915/i915_gem.c
>+++ b/drivers/gpu/drm/i915/i915_gem.c
>@@ -4496,9 +4496,15 @@ i915_gem_init_hw(struct drm_device *dev)
> LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED);
>
> if (HAS_PCH_NOP(dev)) {
>- u32 temp = I915_READ(GEN7_MSG_CTL);
>- temp &= ~(WAIT_FOR_PCH_FLR_ACK | WAIT_FOR_PCH_RESET_ACK);
>- I915_WRITE(GEN7_MSG_CTL, temp);
>+ if (IS_IVYBRIDGE(dev)) {
>+ u32 temp = I915_READ(GEN7_MSG_CTL);
>+ temp &= ~(WAIT_FOR_PCH_FLR_ACK |
>WAIT_FOR_PCH_RESET_ACK);
>+ I915_WRITE(GEN7_MSG_CTL, temp);
>+ } else if (INTEL_INFO(dev)->gen >= 7) {
>+ u32 temp = I915_READ(HSW_NDE_RSTWRN_OPT);
>+ temp &= ~RESET_PCH_HANDSHAKE_ENABLE;
>+ I915_WRITE(HSW_NDE_RSTWRN_OPT, temp);
>+ }
> }
>
> i915_gem_init_swizzling(dev);
>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>index 76126e0ae609..775fc26d53b8 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
>@@ -4117,6 +4117,8 @@
> #define GEN7_MSG_CTL 0x45010
> #define WAIT_FOR_PCH_RESET_ACK (1<<1)
> #define WAIT_FOR_PCH_FLR_ACK (1<<0)
>+#define HSW_NDE_RSTWRN_OPT0x46408
>+#define RESET_PCH_HANDSHAKE_ENABLE (1<<4)
>
> /* GEN7 chicken */
> #define GEN7_COMMON_SLICE_CHICKEN10x7010
>--
>1.8.5.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Round up object allocations
On Sun, Jan 26, 2014 at 11:00:52AM -0800, Ben Widawsky wrote: > On Sun, Jan 26, 2014 at 10:07:45AM +, Chris Wilson wrote: > > On Sun, Jan 26, 2014 at 11:01:50AM +0100, Daniel Vetter wrote: > > > On Sun, Jan 26, 2014 at 09:57:08AM +, Chris Wilson wrote: > > > > On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > > > > > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky > > > > > wrote: > > > > > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > > > > > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > > > > >> > DRM gets very mad when you request an object which occupies a > > > > > >> > partial > > > > > >> > page. As a DRM driver, i915 never really wants to anger DRM, and > > > > > >> > would > > > > > >> > always just want the rounding done for us. > > > > > >> > > > > > > >> > Signed-off-by: Ben Widawsky > > > > > >> > --- > > > > > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > > > > >> > 1 file changed, 2 insertions(+) > > > > > >> > > > > > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > b/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > index 024e454..8cd1134 100644 > > > > > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object > > > > > >> > *i915_gem_alloc_object(struct drm_device *dev, > > > > > >> > struct address_space *mapping; > > > > > >> > gfp_t mask; > > > > > >> > > > > > > >> > + size = round_up(size, PAGE_SIZE); > > > > > >> > + > > > > > >> > > > > > >> Nope, if there's some code that doesn't do page-aligend bo > > > > > >> allocations it > > > > > >> needs to be fixed there. If you want throw a WARN_ON and early > > > > > >> return in > > > > > >> here. > > > > > >> -Daniel > > > > > > > > > > > > Why? > > > > > > > > > > Because allocating a non-page aligned gem bo is a bug. All current > > > > > in-kernel allocations are already aligned. I've thought that we also > > > > > reject unaligned request from userspace but apparently we help out > > > > > since forever (i.e. gem was merged). Might be worth a shot to turn > > > > > that into an -EINVAL if libdrm does the right rounding ... > > > > > > > > We already have an -EINVAL guard on our create ioctl(s). > > > > > > Only for size == 0, not for non-aligned size, at least afaics (no coffee > > > yet ...). > > > > The lack of caffeine was mine. I say if (blah(PAGE_SIZE)) return -EINVAL > > and was happy. > > > > But as it happens it still implies that the BUG_ON in drm/drm_gem.c is > > relevant as is and we shouldn't be papering over it. > > -Chris > > > > I was referring to internal, driver callers. If all internal callers > round up, we may as well do it for them. If the earlier assertion is > that we block bad sizes already at the IOCTL hanlder... then what's the > problem again? You are suggesting we write: size = PAGE_ALIGN(size); BUG_ON(size & ~PAGE_MASK); If you want to change something remove the BUG_ON, do the rounding and update all callers to not worry about page alignment. Tell me how many call sites require alteration? It seems to be an almost universal assumption that the size is page-aligned (despite my own attempts to get subpage allocations into the ABI). -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/5] drm/i915: Round up object allocations
On Sun, Jan 26, 2014 at 11:00:52AM -0800, Ben Widawsky wrote: > On Sun, Jan 26, 2014 at 10:07:45AM +, Chris Wilson wrote: > > On Sun, Jan 26, 2014 at 11:01:50AM +0100, Daniel Vetter wrote: > > > On Sun, Jan 26, 2014 at 09:57:08AM +, Chris Wilson wrote: > > > > On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > > > > > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky > > > > > wrote: > > > > > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > > > > > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > > > > >> > DRM gets very mad when you request an object which occupies a > > > > > >> > partial > > > > > >> > page. As a DRM driver, i915 never really wants to anger DRM, and > > > > > >> > would > > > > > >> > always just want the rounding done for us. > > > > > >> > > > > > > >> > Signed-off-by: Ben Widawsky > > > > > >> > --- > > > > > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > > > > >> > 1 file changed, 2 insertions(+) > > > > > >> > > > > > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > b/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > index 024e454..8cd1134 100644 > > > > > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object > > > > > >> > *i915_gem_alloc_object(struct drm_device *dev, > > > > > >> > struct address_space *mapping; > > > > > >> > gfp_t mask; > > > > > >> > > > > > > >> > + size = round_up(size, PAGE_SIZE); > > > > > >> > + > > > > > >> > > > > > >> Nope, if there's some code that doesn't do page-aligend bo > > > > > >> allocations it > > > > > >> needs to be fixed there. If you want throw a WARN_ON and early > > > > > >> return in > > > > > >> here. > > > > > >> -Daniel > > > > > > > > > > > > Why? > > > > > > > > > > Because allocating a non-page aligned gem bo is a bug. All current > > > > > in-kernel allocations are already aligned. I've thought that we also > > > > > reject unaligned request from userspace but apparently we help out > > > > > since forever (i.e. gem was merged). Might be worth a shot to turn > > > > > that into an -EINVAL if libdrm does the right rounding ... > > > > > > > > We already have an -EINVAL guard on our create ioctl(s). > > > > > > Only for size == 0, not for non-aligned size, at least afaics (no coffee > > > yet ...). > > > > The lack of caffeine was mine. I say if (blah(PAGE_SIZE)) return -EINVAL > > and was happy. > > > > But as it happens it still implies that the BUG_ON in drm/drm_gem.c is > > relevant as is and we shouldn't be papering over it. > > -Chris > > > > I was referring to internal, driver callers. If all internal callers > round up, we may as well do it for them. If the earlier assertion is > that we block bad sizes already at the IOCTL hanlder... then what's the > problem again? create2_ioctl should imo reject non-page-aligned requests. I guess we could dig through libdrm history and check whether that's also possible for the current create ioctl. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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 5/5] drm/i915: Fix error capture on BYT/BDW
On Sun, Jan 26, 2014 at 11:05:40AM -0800, Ben Widawsky wrote: > On Sun, Jan 26, 2014 at 11:47:40AM +, Chris Wilson wrote: > > On Fri, Jan 24, 2014 at 06:17:45PM -0800, Ben Widawsky wrote: > > > The previous check during error capture of whether or not the current VM > > > should be scanned used, gen < 7. That was more or less trying to > > > determine if there was a full PPGTT. At the time, this was sort of what > > > I meant to do because I was more interested in working backwards from > > > hardware state. However, this is incorrect because it will not include > > > platforms that are greater than gen7, and not having PPGTT. Example > > > would be BYT which is gen7 but doesn't have PPGTT, BDW, or any platform > > > greater than gen7 with the PPGTT module parameter invoked. > > > > > > I am /assuming/ BYT was broken, I have not actually checked. > > > > > > While here, clean up the file a bit to avoid duplicate reads (now that > > > the PPGTT info is in the error state). > > > > > > I think Mika/Chris may have been looking at this too. > > > > Sure, we are looking (for identifying the guilty request/batch) by using > > the older, simpler mechanism of finding the first incomplete request. I > > think that search is now definite since we preallocate the request and no > > longer do request collascing if ENOMEM (i.e. there is a 1:1 relationship > > between seqno/batch/request). > > > > That should also apply here and be much simpler. > > How does that solve hangs which aren't caused by requests? Was that an intentional rhetorical question? The code you touch here only deals with requests - finding the current batchbuffer if any. -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 4/5] drm/i915: Capture PPGTT info on error capture
On Sun, Jan 26, 2014 at 11:06:40AM -0800, Ben Widawsky wrote:
> On Sun, Jan 26, 2014 at 11:42:22AM +, Chris Wilson wrote:
> > On Fri, Jan 24, 2014 at 06:17:44PM -0800, Ben Widawsky wrote:
> > > Cc: Chris Wilson
> > > Signed-off-by: Ben Widawsky
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 7 ++
> > > drivers/gpu/drm/i915/i915_gpu_error.c | 41
> > > +++
> > > 2 files changed, 48 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 6f68515..5105fd4 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -359,6 +359,13 @@ struct drm_i915_error_state {
> > > s32 ring:4;
> > > u32 cache_level:3;
> > > } **active_bo, **pinned_bo;
> > > + struct drm_i915_vm_info {
> > > + u32 gfx_mode;
> > > + union {
> > > + u64 pdp[4];
> > > + u32 pp_dir_base;
> > > + };
> > > + } vm_info[I915_NUM_RINGS];
> >
> > Note for future janitorial work: let's coalesce all the per-ring
> > information into the ring error struct.
> >
> > > u32 *active_bo_count, *pinned_bo_count;
> >
> > For instance, I thought active_bo was already being tracked per-ring.
> > (Pinned bo is global since that exists more or less to make sure that
> > our registers are pointing into pinned objects.)
> >
> > Do we also want to capture?
> > GAC_ECO_BITS /* gen6,7 */
> > GAM_ECOCHK /* gen6,7 */
> > GAB_CTL /* gen6 */
> > GFX_MODE /* gen6 */
> >
> > The rest looks good.
> > -Chris
> >
>
> I agree. I was pretty unhappy too with how we've done things, but as
> this information is immediately useful, I'd really like to not postpone.
> Does "The rest looks good." mean reviewed-by?
Yes. If you spend the 2 minutes to add reviewed-by to the changelog, you
can also add the extra registers. ;-)
-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 4/5] drm/i915: Capture PPGTT info on error capture
On Sun, Jan 26, 2014 at 11:42:22AM +, Chris Wilson wrote:
> On Fri, Jan 24, 2014 at 06:17:44PM -0800, Ben Widawsky wrote:
> > Cc: Chris Wilson
> > Signed-off-by: Ben Widawsky
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 7 ++
> > drivers/gpu/drm/i915/i915_gpu_error.c | 41
> > +++
> > 2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 6f68515..5105fd4 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -359,6 +359,13 @@ struct drm_i915_error_state {
> > s32 ring:4;
> > u32 cache_level:3;
> > } **active_bo, **pinned_bo;
> > + struct drm_i915_vm_info {
> > + u32 gfx_mode;
> > + union {
> > + u64 pdp[4];
> > + u32 pp_dir_base;
> > + };
> > + } vm_info[I915_NUM_RINGS];
>
> Note for future janitorial work: let's coalesce all the per-ring
> information into the ring error struct.
>
> > u32 *active_bo_count, *pinned_bo_count;
>
> For instance, I thought active_bo was already being tracked per-ring.
> (Pinned bo is global since that exists more or less to make sure that
> our registers are pointing into pinned objects.)
>
> Do we also want to capture?
> GAC_ECO_BITS /* gen6,7 */
> GAM_ECOCHK /* gen6,7 */
> GAB_CTL /* gen6 */
> GFX_MODE /* gen6 */
>
> The rest looks good.
> -Chris
>
I agree. I was pretty unhappy too with how we've done things, but as
this information is immediately useful, I'd really like to not postpone.
Does "The rest looks good." mean reviewed-by?
--
Ben Widawsky, 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 5/5] drm/i915: Fix error capture on BYT/BDW
On Sun, Jan 26, 2014 at 11:47:40AM +, Chris Wilson wrote: > On Fri, Jan 24, 2014 at 06:17:45PM -0800, Ben Widawsky wrote: > > The previous check during error capture of whether or not the current VM > > should be scanned used, gen < 7. That was more or less trying to > > determine if there was a full PPGTT. At the time, this was sort of what > > I meant to do because I was more interested in working backwards from > > hardware state. However, this is incorrect because it will not include > > platforms that are greater than gen7, and not having PPGTT. Example > > would be BYT which is gen7 but doesn't have PPGTT, BDW, or any platform > > greater than gen7 with the PPGTT module parameter invoked. > > > > I am /assuming/ BYT was broken, I have not actually checked. > > > > While here, clean up the file a bit to avoid duplicate reads (now that > > the PPGTT info is in the error state). > > > > I think Mika/Chris may have been looking at this too. > > Sure, we are looking (for identifying the guilty request/batch) by using > the older, simpler mechanism of finding the first incomplete request. I > think that search is now definite since we preallocate the request and no > longer do request collascing if ENOMEM (i.e. there is a 1:1 relationship > between seqno/batch/request). > > That should also apply here and be much simpler. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre How does that solve hangs which aren't caused by requests? -- Ben Widawsky, 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 1/5] drm/i915: Round up object allocations
On Sun, Jan 26, 2014 at 10:07:45AM +, Chris Wilson wrote: > On Sun, Jan 26, 2014 at 11:01:50AM +0100, Daniel Vetter wrote: > > On Sun, Jan 26, 2014 at 09:57:08AM +, Chris Wilson wrote: > > > On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > > > > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky wrote: > > > > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > > > > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > > > >> > DRM gets very mad when you request an object which occupies a > > > > >> > partial > > > > >> > page. As a DRM driver, i915 never really wants to anger DRM, and > > > > >> > would > > > > >> > always just want the rounding done for us. > > > > >> > > > > > >> > Signed-off-by: Ben Widawsky > > > > >> > --- > > > > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > > > >> > 1 file changed, 2 insertions(+) > > > > >> > > > > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > > >> > b/drivers/gpu/drm/i915/i915_gem.c > > > > >> > index 024e454..8cd1134 100644 > > > > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > > > > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object > > > > >> > *i915_gem_alloc_object(struct drm_device *dev, > > > > >> > struct address_space *mapping; > > > > >> > gfp_t mask; > > > > >> > > > > > >> > + size = round_up(size, PAGE_SIZE); > > > > >> > + > > > > >> > > > > >> Nope, if there's some code that doesn't do page-aligend bo > > > > >> allocations it > > > > >> needs to be fixed there. If you want throw a WARN_ON and early > > > > >> return in > > > > >> here. > > > > >> -Daniel > > > > > > > > > > Why? > > > > > > > > Because allocating a non-page aligned gem bo is a bug. All current > > > > in-kernel allocations are already aligned. I've thought that we also > > > > reject unaligned request from userspace but apparently we help out > > > > since forever (i.e. gem was merged). Might be worth a shot to turn > > > > that into an -EINVAL if libdrm does the right rounding ... > > > > > > We already have an -EINVAL guard on our create ioctl(s). > > > > Only for size == 0, not for non-aligned size, at least afaics (no coffee > > yet ...). > > The lack of caffeine was mine. I say if (blah(PAGE_SIZE)) return -EINVAL > and was happy. > > But as it happens it still implies that the BUG_ON in drm/drm_gem.c is > relevant as is and we shouldn't be papering over it. > -Chris > I was referring to internal, driver callers. If all internal callers round up, we may as well do it for them. If the earlier assertion is that we block bad sizes already at the IOCTL hanlder... then what's the problem again? -- Ben Widawsky, 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 3/3] drm/i915: Get rid of acthd based batch search on reset stats
On Fri, Jan 17, 2014 at 04:20:31PM +0200, Mika Kuoppala wrote:
> As we seek the guilty one using request ordering and hangcheck
> score, these were needed only for debug output.
>
As it's only for debug output, I'd rather just leave this here until
we're completely convinced the new mechanism works.
However, the patch looks functionally correct to me, so I'll leave the
decision up to Daniel.
Reviewed-by: Ben Widawsky
> Signed-off-by: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/i915_gem.c | 87
> ++-
> 1 file changed, 3 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 27a97c3..d796c7f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2241,70 +2241,6 @@ i915_gem_request_remove_from_client(struct
> drm_i915_gem_request *request)
> spin_unlock(&file_priv->mm.lock);
> }
>
> -static bool i915_head_inside_object(u32 acthd, struct drm_i915_gem_object
> *obj,
> - struct i915_address_space *vm)
> -{
> - if (acthd >= i915_gem_obj_offset(obj, vm) &&
> - acthd < i915_gem_obj_offset(obj, vm) + obj->base.size)
> - return true;
> -
> - return false;
> -}
> -
> -static bool i915_head_inside_request(const u32 acthd_unmasked,
> - const u32 request_start,
> - const u32 request_end)
> -{
> - const u32 acthd = acthd_unmasked & HEAD_ADDR;
> -
> - if (request_start < request_end) {
> - if (acthd >= request_start && acthd < request_end)
> - return true;
> - } else if (request_start > request_end) {
> - if (acthd >= request_start || acthd < request_end)
> - return true;
> - }
> -
> - return false;
> -}
> -
> -static struct i915_address_space *
> -request_to_vm(struct drm_i915_gem_request *request)
> -{
> - struct drm_i915_private *dev_priv = request->ring->dev->dev_private;
> - struct i915_address_space *vm;
> -
> - if (request->ctx)
> - vm = request->ctx->vm;
> - else
> - vm = &dev_priv->gtt.base;
> -
> - return vm;
> -}
> -
> -static bool i915_request_guilty(struct drm_i915_gem_request *request,
> - const u32 acthd, bool *inside)
> -{
> - /* There is a possibility that unmasked head address
> - * pointing inside the ring, matches the batch_obj address range.
> - * However this is extremely unlikely.
> - */
> - if (request->batch_obj) {
> - if (i915_head_inside_object(acthd, request->batch_obj,
> - request_to_vm(request))) {
> - *inside = true;
> - return true;
> - }
> - }
> -
> - if (i915_head_inside_request(acthd, request->head, request->tail)) {
> - *inside = false;
> - return true;
> - }
> -
> - return false;
> -}
> -
> static bool i915_context_is_banned(const struct i915_ctx_hang_stats *hs)
> {
> const unsigned long elapsed = get_seconds() - hs->guilty_ts;
> @@ -2322,25 +2258,9 @@ static bool i915_context_is_banned(const struct
> i915_ctx_hang_stats *hs)
>
> static void i915_set_reset_status(struct intel_ring_buffer *ring,
> struct drm_i915_gem_request *request,
> - u32 acthd, const bool guilty)
> + const bool guilty)
> {
> struct i915_ctx_hang_stats *hs = NULL;
> - bool inside;
> - unsigned long offset = 0;
> -
> - if (request->batch_obj)
> - offset = i915_gem_obj_offset(request->batch_obj,
> - request_to_vm(request));
> -
> - if (guilty &&
> - i915_request_guilty(request, acthd, &inside)) {
> - DRM_DEBUG("%s hung %s bo (0x%lx ctx %d) at 0x%x\n",
> - ring->name,
> - inside ? "inside" : "flushing",
> - offset,
> - request->ctx ? request->ctx->id : 0,
> - acthd);
> - }
>
> /* If contexts are disabled or this is the default context, use
>* file_priv->reset_state
> @@ -2376,7 +2296,6 @@ static void i915_gem_reset_ring_status(struct
> drm_i915_private *dev_priv,
> struct intel_ring_buffer *ring)
> {
> u32 completed_seqno = ring->get_seqno(ring, false);
> - u32 acthd = intel_ring_get_active_head(ring);
> struct drm_i915_gem_request *request;
> bool guilty = false;
>
> @@ -2386,9 +2305,9 @@ static void i915_gem_reset_ring_status(struct
> drm_i915_private *dev_priv,
>
> if (!guilty && ring->hangcheck.score >= HANGCHECK_SCORE_GUILTY)
> {
> guilty = true;
> - i915_set_reset_statu
Re: [Intel-gfx] [PATCH 2/3] drm/i915: Seek only one guilty batch per hanged ring
On Fri, Jan 17, 2014 at 04:20:30PM +0200, Mika Kuoppala wrote:
> Instead of going through all the requests to find a batch that
> hanged the machine, use hangcheck score and the fact that
hung, hanged???
> first noncompleted request on hanged ring is, with great
> probability, the guilty one. This also ensure that we get one
> guilty batch per hang instead of possibly more (for each ring)
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73652
> Signed-off-by: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/i915_gem.c | 19 ++-
> drivers/gpu/drm/i915/i915_irq.c |3 +--
> drivers/gpu/drm/i915/intel_ringbuffer.h |2 ++
> 3 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d270351..27a97c3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2322,20 +2322,17 @@ static bool i915_context_is_banned(const struct
> i915_ctx_hang_stats *hs)
>
> static void i915_set_reset_status(struct intel_ring_buffer *ring,
> struct drm_i915_gem_request *request,
> - u32 acthd)
> + u32 acthd, const bool guilty)
> {
> struct i915_ctx_hang_stats *hs = NULL;
> - bool inside, guilty;
> + bool inside;
> unsigned long offset = 0;
>
> - /* Innocent until proven guilty */
> - guilty = false;
> -
> if (request->batch_obj)
> offset = i915_gem_obj_offset(request->batch_obj,
>request_to_vm(request));
>
> - if (ring->hangcheck.action != HANGCHECK_WAIT &&
> + if (guilty &&
> i915_request_guilty(request, acthd, &inside)) {
> DRM_DEBUG("%s hung %s bo (0x%lx ctx %d) at 0x%x\n",
> ring->name,
> @@ -2343,8 +2340,6 @@ static void i915_set_reset_status(struct
> intel_ring_buffer *ring,
> offset,
> request->ctx ? request->ctx->id : 0,
> acthd);
> -
> - guilty = true;
> }
>
> /* If contexts are disabled or this is the default context, use
> @@ -2383,12 +2378,18 @@ static void i915_gem_reset_ring_status(struct
> drm_i915_private *dev_priv,
> u32 completed_seqno = ring->get_seqno(ring, false);
> u32 acthd = intel_ring_get_active_head(ring);
> struct drm_i915_gem_request *request;
> + bool guilty = false;
>
> list_for_each_entry(request, &ring->request_list, list) {
> if (i915_seqno_passed(completed_seqno, request->seqno))
> continue;
>
> - i915_set_reset_status(ring, request, acthd);
> + if (!guilty && ring->hangcheck.score >= HANGCHECK_SCORE_GUILTY)
> {
checkpatch complains about the above.
> + guilty = true;
> + i915_set_reset_status(ring, request, acthd, true);
> + } else {
> + i915_set_reset_status(ring, request, acthd, false);
> + }
I don't think the logic is correct. This will find the first request
(sequentially) that was hung, and not the first ring that hung.
Shouldn't we scan everything and take the first incomplete request with
the highest hangcheck.score?
Maybe I am crazy, but suppose we emit seqno 1 to ring X, and seqno 2 to
ring Y (the latter occurring after the first hang check)
event RING X RING Y
seqno 1 emit
seqno 2 emitBUSY
HUNG HUNG
HUNG HUNG
The case here is somewhat academic since they both hung, but one might
argue that the first hang on ring Y is what caused the hang on ring X.
BTW, this change has convinced me that we really need to define
BUSY/KICK/HUNG as relative and not absolute values...
> }
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6d11e25..e24f9ef 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2473,7 +2473,6 @@ static void i915_hangcheck_elapsed(unsigned long data)
> #define BUSY 1
> #define KICK 5
> #define HUNG 20
> -#define FIRE 30
>
> if (!i915_enable_hangcheck)
> return;
> @@ -2557,7 +2556,7 @@ static void i915_hangcheck_elapsed(unsigned long data)
> }
>
> for_each_ring(ring, dev_priv, i) {
> - if (ring->hangcheck.score > FIRE) {
> + if (ring->hangcheck.score >= HANGCHECK_SCORE_GUILTY) {
> DRM_INFO("%s on %s\n",
>stuck[i] ? "stuck" : "no progress",
>ring->name);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 71a73f4..6018793 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -41,6
Re: [Intel-gfx] [PATCH v2 1/3] drm/i915: Tune down debug output when context is banned
On Wed, Jan 22, 2014 at 05:41:29PM +0200, Mika Kuoppala wrote:
> If we have stopped rings then we know that test is running
> so no need for spam. In addition, only spam when default
> context gets banned.
>
> v2: - make sure default context ban gets shown (Chris)
> - use helper for checking for default context, everywhere (Chris)
>
Overall, passing requests around this much to these functions seems
awkward to me. A lot of my comments are targeting that. If you're
totally of a different mind, I can reconsider.
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=73652
> Signed-off-by: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/i915_drv.h |5 +
> drivers/gpu/drm/i915/i915_gem.c | 18 +++---
> drivers/gpu/drm/i915/i915_gem_context.c |9 ++---
> 3 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f888fea..3a43388 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2267,6 +2267,11 @@ static inline void i915_gem_context_unreference(struct
> i915_hw_context *ctx)
> kref_put(&ctx->ref, i915_gem_context_free);
> }
>
> +static inline bool i915_gem_context_is_default(struct i915_hw_context *ctx)
> +{
> + return (ctx->id == DEFAULT_CONTEXT_ID);
> +}
> +
Drop the parenthesis - I know that it's my bad.
> int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file);
> int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5fcdb14..36d18d8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2305,15 +2305,20 @@ static bool i915_request_guilty(struct
> drm_i915_gem_request *request,
> return false;
> }
>
> -static bool i915_context_is_banned(const struct i915_ctx_hang_stats *hs)
> +static bool i915_context_is_banned(const struct drm_i915_gem_request
> *request,
> +const struct i915_ctx_hang_stats *hs)
> {
> const unsigned long elapsed = get_seconds() - hs->guilty_ts;
> + struct drm_i915_private *dev_priv = request->ring->dev->dev_private;
>
> if (hs->banned)
> return true;
>
> if (elapsed <= DRM_I915_CTX_BAN_PERIOD) {
> - DRM_ERROR("context hanging too fast, declaring banned!\n");
> + if (dev_priv->gpu_error.stop_rings == 0 &&
> + request->ctx && i915_gem_context_is_default(request->ctx))
> + DRM_ERROR("context hanging too fast, declaring
> banned!\n");
> +
Correct me if I am wrong, but, request->ctx should always be non-NULL
when using full PPGTT, and hardware contexts (not sure if this patch is
only targeted for that). In case of the latter, HAS_HW_CONTEXTS should
be == request->ctx. Passing a request to a function which shouldn't care
about requests feels bad to me. As such, I'd vote for:
i915_context_is_banned(struct i915_hw_context *ctx) - you can similarly
thread the ctx up through the call stack, instead of the hs.
I'd also suggest making i915_gem_context_is_default() check regardless
of stop_rings. The last request is optional.
> return true;
> }
>
> @@ -2347,17 +2352,16 @@ static void i915_set_reset_status(struct
> intel_ring_buffer *ring,
> guilty = true;
> }
>
> - /* If contexts are disabled or this is the default context, use
> - * file_priv->reset_state
> - */
> - if (request->ctx && request->ctx->id != DEFAULT_CONTEXT_ID)
> + /* For default context, we track reset statistics per file_priv
> + * to allow more fine grained control */
> + if (request->ctx && !i915_gem_context_is_default(request->ctx))
As I said above, you could replace request->ctx here if you wanted.
> hs = &request->ctx->hang_stats;
> else if (request->file_priv)
> hs = &request->file_priv->private_default_ctx->hang_stats;
>
> if (hs) {
> if (guilty) {
> - hs->banned = i915_context_is_banned(hs);
> + hs->banned = i915_context_is_banned(request, hs);
> hs->batch_active++;
> hs->guilty_ts = get_seconds();
> } else {
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 112f865..4c55730 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -228,11 +228,6 @@ err_out:
> return ERR_PTR(ret);
> }
>
> -static inline bool is_default_context(struct i915_hw_context *ctx)
> -{
> - return (ctx->id == DEFAULT_CONTEXT_ID);
> -}
> -
> /**
> * The default context needs to exist per ring that uses contexts. It stores
> the
> * context stat
Re: [Intel-gfx] [PATCH v3 3/5] drm/i915: Make sprite updates atomic
On Tue, Jan 21, 2014 at 04:12:38PM +0200, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä
>
> Add a mechanism by which we can evade the leading edge of vblank. This
> guarantees that no two sprite register writes will straddle on either
> side of the vblank start, and that means all the writes will be latched
> together in one atomic operation.
>
> We do the vblank evade by checking the scanline counter, and if it's too
> close to the start of vblank (too close has been hardcoded to 100usec
> for now), we will wait for the vblank start to pass. In order to
> eliminate random delayes from the rest of the system, we operate with
> interrupts disabled, except when waiting for the vblank obviously.
>
> Note that we now go digging through pipe_to_crtc_mapping[] in the
> vblank interrupt handler, which is a bit dangerous since we set up
> interrupts before the crtcs. However in this case since it's the vblank
> interrupt, we don't actually unmask it until some piece of code
> requests it.
>
> v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
> Hook up the vblank irq stuff on BDW as well
> v3: Pass intel_crtc instead of drm_crtc (Daniel)
> Warn if crtc.mutex isn't locked (Daniel)
> Add an explicit compiler barrier and document the barriers (Daniel)
> Note the irq vs. modeset setup madness in the commit message (Daniel)
>
> Reviewed-by: Jesse Barnes
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/i915_irq.c | 29 --
> drivers/gpu/drm/i915/intel_display.c | 2 +
> drivers/gpu/drm/i915/intel_drv.h | 3 +
> drivers/gpu/drm/i915/intel_sprite.c | 103
> +++
> 4 files changed, 133 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index ffb56a9..8ef9edb 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1439,6 +1439,15 @@ static void gen6_rps_irq_handler(struct
> drm_i915_private *dev_priv, u32 pm_iir)
> }
> }
>
> +static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> +{
> + struct intel_crtc *intel_crtc =
> + to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> +
> + intel_crtc->vbl_received = true;
> + wake_up(&intel_crtc->vbl_wait);
> +}
> +
> static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> {
> struct drm_device *dev = (struct drm_device *) arg;
> @@ -1479,8 +1488,10 @@ static irqreturn_t valleyview_irq_handler(int irq,
> void *arg)
> spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>
> for_each_pipe(pipe) {
> - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
> drm_handle_vblank(dev, pipe);
> + intel_pipe_handle_vblank(dev, pipe);
> + }
>
> if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> intel_prepare_page_flip(dev, pipe);
> @@ -1679,8 +1690,10 @@ static void ilk_display_irq_handler(struct drm_device
> *dev, u32 de_iir)
> DRM_ERROR("Poison interrupt\n");
>
> for_each_pipe(pipe) {
> - if (de_iir & DE_PIPE_VBLANK(pipe))
> + if (de_iir & DE_PIPE_VBLANK(pipe)) {
> drm_handle_vblank(dev, pipe);
> + intel_pipe_handle_vblank(dev, pipe);
> + }
>
> if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> @@ -1729,8 +1742,10 @@ static void ivb_display_irq_handler(struct drm_device
> *dev, u32 de_iir)
> intel_opregion_asle_intr(dev);
>
> for_each_pipe(i) {
> - if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
> + if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
> drm_handle_vblank(dev, i);
> + intel_pipe_handle_vblank(dev, i);
> + }
>
> /* plane/pipes map 1:1 on ilk+ */
> if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
> @@ -1872,8 +1887,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> continue;
>
> pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> - if (pipe_iir & GEN8_PIPE_VBLANK)
> + if (pipe_iir & GEN8_PIPE_VBLANK) {
> drm_handle_vblank(dev, pipe);
> + intel_pipe_handle_vblank(dev, pipe);
> + }
>
> if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
> intel_prepare_page_flip(dev, pipe);
> @@ -3161,6 +3178,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> if (!drm_handle_vblank(dev, pipe))
> return false;
>
> + intel_pipe_handle_vblank(dev, pipe);
> +
> if ((iir & flip_pending) == 0)
> return false;
>
> @@ -3344,6 +3363,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
> if (!drm_handle_vblank(dev, pipe))
> return false;
>
> + intel_pipe_handle_vblank(dev, pipe);
> +
> if ((iir & flip_pending) == 0)
> return false;
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index d1cc7ea..b39e445 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev,
> int pipe)
> intel_crtc->plane =
Re: [Intel-gfx] [PATCH v2] drm/i915: fix dp/sdvo i2c cleanup
On Sun, Jan 26, 2014 at 01:11:15PM +0200, Imre Deak wrote: > On Sun, 2014-01-26 at 10:21 +0100, Daniel Vetter wrote: > > On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak wrote: > > > On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote: > > >> On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote: > > >> > Atm we try to remove the connector's i2c sysfs entry too late in the > > >> > encoder's destroy callback. By that time the kobject used as the parent > > >> > for all connector sysfs entries is already removed when we do an early > > >> > removal of all connector sysfs entries in intel_modeset_cleanup(). Fix > > >> > this by adding an early_destroy encoder callback, where we remove the > > >> > encoder's i2c adapter. > > >> > > > >> > v2: > > >> > - add missing static to function, use existing sdvo cast helper, > > >> > s/intel_sdvo/sdvo/ (Chris) > > >> > > > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523 > > >> > > > >> > Signed-off-by: Imre Deak > > >> > > >> This looks fishy ... sysfs should all be reference-counted, so I'm > > >> confused what's going on here. Also I think this smells a bit like it's > > >> going overall in the wrong direction - essentially with sysfs we can't > > >> really force-remove stuff but have to wait until the refcount drops to 0. > > >> Or at least that's how I think it works, I'd need to blow through a pile > > >> of time to figure this all out. > > > > > > Hm, I haven't thought about refcounting :) Now, I agree that should > > > normally allow for removing a parent and child device in both order. > > > > > > What happens and why we can't remove first the parent then the child: > > > > > > In > > > > > > intel_dp_init_connector()-> > > > intel_dp_i2c_init() > > > > > > we set the i2c adapter.dev.parent = intel_connector->base.kdev; > > > > > > device_register will then add the i2c sysfs entry to the connector sysfs > > > dir. Refcounting here looks ok, both the parent connector kobject and > > > its sysfs dir entry gets a reference from the child. > > > > > > During module cleanup, we call > > > > > > intel_modeset_cleanup()-> > > > drm_sysfs_connector_remove() > > > device_unregister(connector->kdev) > > > > > > which is the i2c adapter's parent kdev. Then: > > > > > > device_del()-> > > > kobject_del()-> > > > sysfs_remove_dir() > > > > > > will remove all entries recursively in the connector's sysfs dir, along > > > with all the i2c sysfs entries. Afterwards the intel encoder->destroy() > > > callback calls i2c_del_adapter()->device_unregister()->device_del() and > > > that will try to remove its own sysfs attributes, namely the power sysfs > > > group, but won't find it since it was removed above and give a WARN. > > > > > > Note that the parent's recursive removal happens regardless of its > > > kobject's or sysfs entry's refcount. The kobject itself will be put > > > after device_del() in put_device() and only destroyed after the i2c > > > adapter releases the refcount it holds on it. I admit it feels strange > > > that the sysfs entries are removed before the last reference on the > > > kobject is dropped, not sure if it's by design or an overlook.. > > > > I have no idea either how exactly this is supposed to work, and I > > quick scan through Documentation/ didn't point me into a useful > > direction either. > > > > Adding Greg (and more mailing lists) for insight. > > Attached the corresponding dmesg. > > Also one more thought. Imo whether or not it's a valid thing to delete > first a parent device and only then its child device, in this case we > don't have a reason to do so. We created first the connector device > (parent) and then the i2c adapter device (child) and the cleanup should > happen in reverse order. This is so regardless of what order the > corresponding kobjects get destroyed based on their refcounts. The kernel used to not complain if you removed a parent before the child, but now it does. As you already have all of the needed information, just switch the removal and then all should be fine. thanks, greg k-h ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/12] drm/i915: Remove INTEL_INFO()
On Mon, Jan 06, 2014 at 07:17:22PM +, Damien Lespiau wrote: > Finally, we reach the goal of the last few patches: the removal of > INTEL_INFO() in favor of a direct dev_priv usage. > > Signed-off-by: Damien Lespiau Please pardon my incompetence, but I think I need to pull a heelturn on this one :( Apparently product groups noticed again that we have a unified driver across all platforms ever shipped and they don't like the space overhead that much. Since the usual approaches of splattering #ifdefs all over the code or (gasp!) editing all the sources files are complete no-goes for upstream we need something else. My idea would be to: - convert all HAS_FOO macros to used INTEL_INFO underneath. - allow hard-coding of INTEL_INFO structs. - enable LTE on i915 and let gcc's DCE path take care of all the dead code removal. Minimal overhead for upstream, still tidy code and a good chance to get product groups what they want. But it also means we'll need to keep INTEL_INFO around, hence why I'll nak patches 1-5. Patch 6 is merged with Paulo's r-b (it still applied). Ofc to_i915_dev is still really nice, so can I sheepishly ask you to resurrect that part of your patches and roll to_i915_dev a bit out over the tree? I think I'll owe you a few rounds of your $favourite_beverage the next time we meet for recompense, maybe at fosdem ... Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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 v2 2/2] drm/i915: move module parameters into a struct, in a new file
On Tue, Jan 21, 2014 at 11:24:25AM +0200, Jani Nikula wrote: > With 20+ module parameters, I think referring to them via a struct > improves clarity over just having a bunch of globals. While at it, move > the parameter initialization and definitions into a new file > i915_params.c to reduce clutter in i915_drv.c. > > Apart from the ill-named i915_enable_rc6, i915_enable_fbc and > i915_enable_ppgtt parameters, for which we lose the "i915_" prefix > internally, the module parameters now look the same both on the kernel > command line and in code. For example, "i915.modeset". > > The downsides of the change are losing static on a couple of variables > and not having the initialization and module_param_named() right next to > each other. On the other hand, all module parameters are now defined in > one place at i915_params.c. Plus you can do this to find all module > parameter references: > > $ git grep "i915\." -- drivers/gpu/drm/i915 > > v2: > - move the definitions into a new file > - s/i915_params/i915/ > - make i915_try_reset i915.reset, for consistency > > Signed-off-by: Jani Nikula Queued for -next, thanks for the patch. While we're at it: Can you please smash another patch on top ot drop the i915_ prefix from the module parameters? i915.i915_ really is a bit verbose ;-) and I think we can declare this to be non-abi, i.e. a good excuse to update all the various bug reports ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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 00/10] drm/i915: Some less complex FBC fixes
On Sun, Jan 26, 2014 at 03:33:32PM +0100, Daniel Vetter wrote: > On Sat, Jan 25, 2014 at 08:59:49PM +0100, Daniel Vetter wrote: > > On Thu, Jan 23, 2014 at 07:47:59PM +, Chris Wilson wrote: > > > On Thu, Jan 23, 2014 at 04:49:07PM +0200, ville.syrj...@linux.intel.com > > > wrote: > > > > From: Ville Syrjälä > > > > > > > > Since fixing the FBC locking is a bigger task that will take a while, > > > > I decided to pull all the simple fixes from my branch and post them > > > > right away. > > > > > > > > Some of these I've posted before, some others have seen a bit of action > > > > by being in a public branch. > > > > > > > > The FBC_FENCE_OFF change is just a guess at this point. The odd offset > > > > just caught my eye while reading throguh i915_reg.h. > > > > > > I didn't spot anything offensive in the series and each patch only does > > > what it says on the tin. So I am going to stick my neck out and say > > > > > > Reviewed-by: Chris Wilson > > > > > > for the series. Being picky, I guess Fix FBC_FENCE_OFF should only be an > > > acked-by since we have no way to review it... > > > > Entire series merged, and I'll fire up my g4x here to see what happens ;-) > > Hm, seems to fail on my g4x when I enable fbc. This is on latest -nightly: Also, shouldn't we enable fbc now on all gen5+ machines? Or is there still something outstanding to fix issues? Atm we only have it enabled on haswell :( -Daniel > > > > IGT-Version: 1.5-gb5109e62cea1 (x86_64) (Linux: 3.13.0-rc8+ x86_64) > Subtest bad-pipe: SUCCESS > Subtest bad-source: SUCCESS > Subtest bad-nb-words-1: SUCCESS > Subtest bad-nb-words-3: SUCCESS > read-crc-pipe-A: Testing connector 5 > Subtest read-crc-pipe-A: SUCCESS > read-crc-pipe-A-frame-sequence: Testing connector 5 > Subtest read-crc-pipe-A-frame-sequence: SUCCESS > read-crc-pipe-B: Testing connector 5 > Subtest read-crc-pipe-B: SUCCESS > read-crc-pipe-B-frame-sequence: Testing connector 5 > Subtest read-crc-pipe-B-frame-sequence: SUCCESS > Test requirement not met in function test_read_crc, file > kms_pipe_crc_basic.c:210: > Last errno: 0, Success > Test requirement: (!(valid_connectors)) > No connector found for pipe 2 > Subtest read-crc-pipe-C: SKIP > Test requirement not met in function test_read_crc, file > kms_pipe_crc_basic.c:210: > Last errno: 0, Success > Test requirement: (!(valid_connectors)) > No connector found for pipe 2 > Subtest read-crc-pipe-C-frame-sequence: SKIP > root@gina:/home/daniel/xorg/intel-gpu-tools# tests/kms_fbc_crc > IGT-Version: 1.5-gb5109e62cea1 (x86_64) (Linux: 3.13.0-rc8+ x86_64) > Beginning page_flip on crtc 3, connector 5 > > page_flip on crtc 3, connector 5: PASSED > > Beginning page_flip on crtc 4, connector 5 > > page_flip on crtc 4, connector 5: PASSED > > Subtest page_flip: SUCCESS > Beginning mmap_cpu on crtc 3, connector 5 > > mmap_cpu on crtc 3, connector 5: PASSED > > Beginning mmap_cpu on crtc 4, connector 5 > > mmap_cpu on crtc 4, connector 5: PASSED > > Subtest mmap_cpu: SUCCESS > Beginning mmap_gtt on crtc 3, connector 5 > > mmap_gtt on crtc 3, connector 5: PASSED > > Beginning mmap_gtt on crtc 4, connector 5 > > mmap_gtt on crtc 4, connector 5: PASSED > > Subtest mmap_gtt: SUCCESS > Beginning blt on crtc 3, connector 5 > > blt on crtc 3, connector 5: PASSED > > Beginning blt on crtc 4, connector 5 > > blt on crtc 4, connector 5: PASSED > > Subtest blt: SUCCESS > Beginning render on crtc 3, connector 5 > Test requirement not met in function fill_render, file kms_fbc_crc.c:212: > Last errno: 0, Success > Test requirement: (!rendercopy) > Subtest render: SKIP > Test requirement not met in function prepare_crtc, file kms_fbc_crc.c:398: > Last errno: 19, No such device > Test requirement: (!(data->ctx[0])) > Subtest context: SKIP > Beginning page_flip_and_mmap_cpu on crtc 3, connector 5 > Test assertion failure function test_crc, file kms_fbc_crc.c:315: > Last errno: 0, Success > Failed assertion: !igt_crc_equal(&crcs[0], &data->ref_crc[1]) > Subtest page_flip_and_mmap_cpu: FAIL > Beginning page_flip_and_mmap_gtt on crtc 3, connector 5 > Test assertion failure function test_crc, file kms_fbc_crc.c:315: > Last errno: 0, Success > Failed assertion: !igt_crc_equal(&crcs[0], &data->ref_crc[1]) > Subtest page_flip_and_mmap_gtt: FAIL > Beginning page_flip_and_blt on crtc 3, connector 5 > > page_flip_and_blt on crtc 3, connector 5: PASSED > > Beginning page_flip_and_blt on crtc 4, connector 5 > > page_flip_and_blt on crtc 4, connector 5: PASSED > > Subtest page_flip_and_blt: SUCCESS > Beginning page_flip_and_render on crtc 3, connector 5 > Test requirement not met in function fill_render, file kms_fbc_crc.c:212: > Last errno: 0, Success > Test requirement: (!rendercopy) > Subtest page_flip_and_render: SKIP > Test requirement not met in function prepare_crtc, file kms_fbc_crc.c:398: > Last errno: 19, No such device > Test requirement: (!(data->ctx[0])) > Subtest page_flip_and_context: SKIP > > So something with flip + fron
Re: [Intel-gfx] [PATCH 00/10] drm/i915: Some less complex FBC fixes
On Sat, Jan 25, 2014 at 08:59:49PM +0100, Daniel Vetter wrote: > On Thu, Jan 23, 2014 at 07:47:59PM +, Chris Wilson wrote: > > On Thu, Jan 23, 2014 at 04:49:07PM +0200, ville.syrj...@linux.intel.com > > wrote: > > > From: Ville Syrjälä > > > > > > Since fixing the FBC locking is a bigger task that will take a while, > > > I decided to pull all the simple fixes from my branch and post them > > > right away. > > > > > > Some of these I've posted before, some others have seen a bit of action > > > by being in a public branch. > > > > > > The FBC_FENCE_OFF change is just a guess at this point. The odd offset > > > just caught my eye while reading throguh i915_reg.h. > > > > I didn't spot anything offensive in the series and each patch only does > > what it says on the tin. So I am going to stick my neck out and say > > > > Reviewed-by: Chris Wilson > > > > for the series. Being picky, I guess Fix FBC_FENCE_OFF should only be an > > acked-by since we have no way to review it... > > Entire series merged, and I'll fire up my g4x here to see what happens ;-) Hm, seems to fail on my g4x when I enable fbc. This is on latest -nightly: > IGT-Version: 1.5-gb5109e62cea1 (x86_64) (Linux: 3.13.0-rc8+ x86_64) Subtest bad-pipe: SUCCESS Subtest bad-source: SUCCESS Subtest bad-nb-words-1: SUCCESS Subtest bad-nb-words-3: SUCCESS read-crc-pipe-A: Testing connector 5 Subtest read-crc-pipe-A: SUCCESS read-crc-pipe-A-frame-sequence: Testing connector 5 Subtest read-crc-pipe-A-frame-sequence: SUCCESS read-crc-pipe-B: Testing connector 5 Subtest read-crc-pipe-B: SUCCESS read-crc-pipe-B-frame-sequence: Testing connector 5 Subtest read-crc-pipe-B-frame-sequence: SUCCESS Test requirement not met in function test_read_crc, file kms_pipe_crc_basic.c:210: Last errno: 0, Success Test requirement: (!(valid_connectors)) No connector found for pipe 2 Subtest read-crc-pipe-C: SKIP Test requirement not met in function test_read_crc, file kms_pipe_crc_basic.c:210: Last errno: 0, Success Test requirement: (!(valid_connectors)) No connector found for pipe 2 Subtest read-crc-pipe-C-frame-sequence: SKIP root@gina:/home/daniel/xorg/intel-gpu-tools# tests/kms_fbc_crc IGT-Version: 1.5-gb5109e62cea1 (x86_64) (Linux: 3.13.0-rc8+ x86_64) Beginning page_flip on crtc 3, connector 5 page_flip on crtc 3, connector 5: PASSED Beginning page_flip on crtc 4, connector 5 page_flip on crtc 4, connector 5: PASSED Subtest page_flip: SUCCESS Beginning mmap_cpu on crtc 3, connector 5 mmap_cpu on crtc 3, connector 5: PASSED Beginning mmap_cpu on crtc 4, connector 5 mmap_cpu on crtc 4, connector 5: PASSED Subtest mmap_cpu: SUCCESS Beginning mmap_gtt on crtc 3, connector 5 mmap_gtt on crtc 3, connector 5: PASSED Beginning mmap_gtt on crtc 4, connector 5 mmap_gtt on crtc 4, connector 5: PASSED Subtest mmap_gtt: SUCCESS Beginning blt on crtc 3, connector 5 blt on crtc 3, connector 5: PASSED Beginning blt on crtc 4, connector 5 blt on crtc 4, connector 5: PASSED Subtest blt: SUCCESS Beginning render on crtc 3, connector 5 Test requirement not met in function fill_render, file kms_fbc_crc.c:212: Last errno: 0, Success Test requirement: (!rendercopy) Subtest render: SKIP Test requirement not met in function prepare_crtc, file kms_fbc_crc.c:398: Last errno: 19, No such device Test requirement: (!(data->ctx[0])) Subtest context: SKIP Beginning page_flip_and_mmap_cpu on crtc 3, connector 5 Test assertion failure function test_crc, file kms_fbc_crc.c:315: Last errno: 0, Success Failed assertion: !igt_crc_equal(&crcs[0], &data->ref_crc[1]) Subtest page_flip_and_mmap_cpu: FAIL Beginning page_flip_and_mmap_gtt on crtc 3, connector 5 Test assertion failure function test_crc, file kms_fbc_crc.c:315: Last errno: 0, Success Failed assertion: !igt_crc_equal(&crcs[0], &data->ref_crc[1]) Subtest page_flip_and_mmap_gtt: FAIL Beginning page_flip_and_blt on crtc 3, connector 5 page_flip_and_blt on crtc 3, connector 5: PASSED Beginning page_flip_and_blt on crtc 4, connector 5 page_flip_and_blt on crtc 4, connector 5: PASSED Subtest page_flip_and_blt: SUCCESS Beginning page_flip_and_render on crtc 3, connector 5 Test requirement not met in function fill_render, file kms_fbc_crc.c:212: Last errno: 0, Success Test requirement: (!rendercopy) Subtest page_flip_and_render: SKIP Test requirement not met in function prepare_crtc, file kms_fbc_crc.c:398: Last errno: 19, No such device Test requirement: (!(data->ctx[0])) Subtest page_flip_and_context: SKIP So something with flip + frontbuffer access seems still busted. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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] lib: Export interval_tree
On Sun, Jan 26, 2014 at 12:24:33PM +, Chris Wilson wrote: > lib/interval_tree.c provides a simple interface for an interval-tree > (an augmented red-black tree) but is only built when testing the generic > macros for building interval-trees. For drivers with modest needs, > export the simple interval-tree library as is. > > v2: Lots of help from Michel Lespinasse to only compile the code > as required: > - make INTERVAL_TREE a config option > - make INTERVAL_TREE_TEST select the library functions > and sanitize the filenames & Makefile > - prepare interval_tree for being built as a module if required > > Signed-off-by: Chris Wilson > Cc: Michel Lespinasse > Cc: Rik van Riel > Cc: Peter Zijlstra > Cc: Andrea Arcangeli > Cc: David Woodhouse > Cc: Andrew Morton > --- [snip] > diff --git a/lib/interval_tree.c b/lib/interval_tree.c > index e6eb406f2d65..e4109f624e51 100644 > --- a/lib/interval_tree.c > +++ b/lib/interval_tree.c > @@ -1,6 +1,7 @@ > #include > #include > #include > +#include > > #define START(node) ((node)->start) > #define LAST(node) ((node)->last) > @@ -8,3 +9,8 @@ > INTERVAL_TREE_DEFINE(struct interval_tree_node, rb, >unsigned long, __subtree_last, >START, LAST,, interval_tree) > + > +EXPORT_SYMBOL(interval_tree_insert); > +EXPORT_SYMBOL(interval_tree_remove); > +EXPORT_SYMBOL(interval_tree_iter_first); > +EXPORT_SYMBOL(interval_tree_iter_next); Hm, I've thought kernel coding style nowadays is to put the EXPORT_SYMBOL right below the definition of the function? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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/3] lib: Always build and export interval_tree
On Sun, Jan 26, 2014 at 03:58:47AM -0800, Michel Lespinasse wrote: > On Sun, Jan 26, 2014 at 3:27 AM, Chris Wilson > wrote: > > +config INTERVAL_TREE > > + boolean > > +help > > + Simple, embeddable, interval-tree. Can find the start of an > > + overlapping range in log(n) time and then iterate over all > > + overlapping nodes. The algorithm is implemented as an > > + augmented rbtree. > > + > > + See: > > + > > + Documentation/rbtree.txt > > + > > + for more information. > > > > Though you probably have some better spiel. :) > > No, that's fine. I'm not good at writing these things either :) > > >> in lib/Kconfig.debug: > >> make INTERVAL_TREE_TEST depend on m && DEBUG_KERNEL && INTERVAL_TREE > > > > Done. Are you sure you do not just want to select INTERVAL_TREE here to > > maintain the status quo? > > You're right, select would actually be better. > > >> in lib/Makefile: > >> obj-$(CONFIG_INTERVAL_TREE) += interval_tree.o > > One more thing I forget, lib/interval_tree_test_main.c should probably > be renamed to interval_tree_test.c and in lib/Makefile the > interval_tree_test-objs line forcing linking of interval_tree.o into > the test module should be removed too. > > > Thanks! > > -Chris > > I assume you're going to resend with these changes ? I'll be happy to > aprove the patch then :) Indeed. Resent with a wider audience (forgot to include lkml on the first pass). -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] lib: Export interval_tree
lib/interval_tree.c provides a simple interface for an interval-tree
(an augmented red-black tree) but is only built when testing the generic
macros for building interval-trees. For drivers with modest needs,
export the simple interval-tree library as is.
v2: Lots of help from Michel Lespinasse to only compile the code
as required:
- make INTERVAL_TREE a config option
- make INTERVAL_TREE_TEST select the library functions
and sanitize the filenames & Makefile
- prepare interval_tree for being built as a module if required
Signed-off-by: Chris Wilson
Cc: Michel Lespinasse
Cc: Rik van Riel
Cc: Peter Zijlstra
Cc: Andrea Arcangeli
Cc: David Woodhouse
Cc: Andrew Morton
---
Note for maintainers, this is being proposed for use by i915.ko, so it
may make the most sense to merge it via the drm/i915 tree in the next
cycle.
---
lib/Kconfig | 14 ++
lib/Kconfig.debug | 1 +
lib/Makefile | 3 +-
lib/interval_tree.c | 6 +++
lib/interval_tree_test.c | 106 ++
lib/interval_tree_test_main.c | 106 --
6 files changed, 128 insertions(+), 108 deletions(-)
create mode 100644 lib/interval_tree_test.c
delete mode 100644 lib/interval_tree_test_main.c
diff --git a/lib/Kconfig b/lib/Kconfig
index 991c98bc4a3f..04270aae4b60 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -322,6 +322,20 @@ config TEXTSEARCH_FSM
config BTREE
boolean
+config INTERVAL_TREE
+ boolean
+ help
+ Simple, embeddable, interval-tree. Can find the start of an
+ overlapping range in log(n) time and then iterate over all
+ overlapping nodes. The algorithm is implemented as an
+ augmented rbtree.
+
+ See:
+
+ Documentation/rbtree.txt
+
+ for more information.
+
config ASSOCIATIVE_ARRAY
bool
help
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index db25707aa41b..a29e9b84f102 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1478,6 +1478,7 @@ config RBTREE_TEST
config INTERVAL_TREE_TEST
tristate "Interval tree test"
depends on m && DEBUG_KERNEL
+ select INTERVAL_TREE
help
A benchmark measuring the performance of the interval tree library
diff --git a/lib/Makefile b/lib/Makefile
index a459c31e8c6b..fc04948548ad 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -47,6 +47,7 @@ CFLAGS_hweight.o = $(subst
$(quote),,$(CONFIG_ARCH_HWEIGHT_CFLAGS))
obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
obj-$(CONFIG_BTREE) += btree.o
+obj-$(CONFIG_INTERVAL_TREE) += interval_tree.o
obj-$(CONFIG_ASSOCIATIVE_ARRAY) += assoc_array.o
obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
obj-$(CONFIG_DEBUG_LIST) += list_debug.o
@@ -152,8 +153,6 @@ lib-$(CONFIG_LIBFDT) += $(libfdt_files)
obj-$(CONFIG_RBTREE_TEST) += rbtree_test.o
obj-$(CONFIG_INTERVAL_TREE_TEST) += interval_tree_test.o
-interval_tree_test-objs := interval_tree_test_main.o interval_tree.o
-
obj-$(CONFIG_PERCPU_TEST) += percpu_test.o
obj-$(CONFIG_ASN1) += asn1_decoder.o
diff --git a/lib/interval_tree.c b/lib/interval_tree.c
index e6eb406f2d65..e4109f624e51 100644
--- a/lib/interval_tree.c
+++ b/lib/interval_tree.c
@@ -1,6 +1,7 @@
#include
#include
#include
+#include
#define START(node) ((node)->start)
#define LAST(node) ((node)->last)
@@ -8,3 +9,8 @@
INTERVAL_TREE_DEFINE(struct interval_tree_node, rb,
unsigned long, __subtree_last,
START, LAST,, interval_tree)
+
+EXPORT_SYMBOL(interval_tree_insert);
+EXPORT_SYMBOL(interval_tree_remove);
+EXPORT_SYMBOL(interval_tree_iter_first);
+EXPORT_SYMBOL(interval_tree_iter_next);
diff --git a/lib/interval_tree_test.c b/lib/interval_tree_test.c
new file mode 100644
index ..245900b98c8e
--- /dev/null
+++ b/lib/interval_tree_test.c
@@ -0,0 +1,106 @@
+#include
+#include
+#include
+#include
+
+#define NODES100
+#define PERF_LOOPS 10
+#define SEARCHES 100
+#define SEARCH_LOOPS 1
+
+static struct rb_root root = RB_ROOT;
+static struct interval_tree_node nodes[NODES];
+static u32 queries[SEARCHES];
+
+static struct rnd_state rnd;
+
+static inline unsigned long
+search(unsigned long query, struct rb_root *root)
+{
+ struct interval_tree_node *node;
+ unsigned long results = 0;
+
+ for (node = interval_tree_iter_first(root, query, query); node;
+node = interval_tree_iter_next(node, query, query))
+ results++;
+ return results;
+}
+
+static void init(void)
+{
+ int i;
+ for (i = 0; i < NODES; i++) {
+ u32 a = prandom_u32_state(&rnd);
+ u32 b = prandom_u32_state(&rnd);
+ if (a <= b) {
+ nodes[i].start = a;
+ nodes[i].last = b;
+ } else {
+ nodes[i].start = b;
+
Re: [Intel-gfx] [PATCH 5/5] drm/i915: Fix error capture on BYT/BDW
On Fri, Jan 24, 2014 at 06:17:45PM -0800, Ben Widawsky wrote: > The previous check during error capture of whether or not the current VM > should be scanned used, gen < 7. That was more or less trying to > determine if there was a full PPGTT. At the time, this was sort of what > I meant to do because I was more interested in working backwards from > hardware state. However, this is incorrect because it will not include > platforms that are greater than gen7, and not having PPGTT. Example > would be BYT which is gen7 but doesn't have PPGTT, BDW, or any platform > greater than gen7 with the PPGTT module parameter invoked. > > I am /assuming/ BYT was broken, I have not actually checked. > > While here, clean up the file a bit to avoid duplicate reads (now that > the PPGTT info is in the error state). > > I think Mika/Chris may have been looking at this too. Sure, we are looking (for identifying the guilty request/batch) by using the older, simpler mechanism of finding the first incomplete request. I think that search is now definite since we preallocate the request and no longer do request collascing if ENOMEM (i.e. there is a 1:1 relationship between seqno/batch/request). That should also apply here and be much simpler. -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 4/5] drm/i915: Capture PPGTT info on error capture
On Fri, Jan 24, 2014 at 06:17:44PM -0800, Ben Widawsky wrote:
> Cc: Chris Wilson
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_drv.h | 7 ++
> drivers/gpu/drm/i915/i915_gpu_error.c | 41
> +++
> 2 files changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6f68515..5105fd4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -359,6 +359,13 @@ struct drm_i915_error_state {
> s32 ring:4;
> u32 cache_level:3;
> } **active_bo, **pinned_bo;
> + struct drm_i915_vm_info {
> + u32 gfx_mode;
> + union {
> + u64 pdp[4];
> + u32 pp_dir_base;
> + };
> + } vm_info[I915_NUM_RINGS];
Note for future janitorial work: let's coalesce all the per-ring
information into the ring error struct.
> u32 *active_bo_count, *pinned_bo_count;
For instance, I thought active_bo was already being tracked per-ring.
(Pinned bo is global since that exists more or less to make sure that
our registers are pointing into pinned objects.)
Do we also want to capture?
GAC_ECO_BITS /* gen6,7 */
GAM_ECOCHK /* gen6,7 */
GAB_CTL /* gen6 */
GFX_MODE /* gen6 */
The rest looks good.
-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/3] lib: Always build and export interval_tree
On Fri, Jan 24, 2014 at 08:42:18PM -0800, Michel Lespinasse wrote: > Hi Chris, > > On Tue, Jan 21, 2014 at 7:07 AM, Chris Wilson > wrote: > > lib/interval_tree.c provides a simple interface for an interval-tree > > (an augmented red-black tree) but is only built when testing the generic > > macros for building interval-trees. For drivers with modest needs, > > export the simple interval-tree library as is. > > Thanks for suggesting this. I did plan for this use case, and thought > it would show up earlier. > > My only concern is that I think we should keep the code under a config > option (unless a use case shows up in core kernel). So I would > suggest: > > in lib/Kconfig: > config INTERVAL_TREE >bool +config INTERVAL_TREE + boolean +help + Simple, embeddable, interval-tree. Can find the start of an + overlapping range in log(n) time and then iterate over all + overlapping nodes. The algorithm is implemented as an + augmented rbtree. + + See: + + Documentation/rbtree.txt + + for more information. Though you probably have some better spiel. :) > in lib/Kconfig.debug: > make INTERVAL_TREE_TEST depend on m && DEBUG_KERNEL && INTERVAL_TREE Done. Are you sure you do not just want to select INTERVAL_TREE here to maintain the status quo? > in lib/Makefile: > obj-$(CONFIG_INTERVAL_TREE) += interval_tree.o > > You would probably also need to add #include in > lib/interval_tree.c to plan for that code being configured as a > module. > > Hope this helps, Thanks! -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 v2] drm/i915: fix dp/sdvo i2c cleanup
On Sun, 2014-01-26 at 10:21 +0100, Daniel Vetter wrote: > On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak wrote: > > On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote: > >> On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote: > >> > Atm we try to remove the connector's i2c sysfs entry too late in the > >> > encoder's destroy callback. By that time the kobject used as the parent > >> > for all connector sysfs entries is already removed when we do an early > >> > removal of all connector sysfs entries in intel_modeset_cleanup(). Fix > >> > this by adding an early_destroy encoder callback, where we remove the > >> > encoder's i2c adapter. > >> > > >> > v2: > >> > - add missing static to function, use existing sdvo cast helper, > >> > s/intel_sdvo/sdvo/ (Chris) > >> > > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523 > >> > > >> > Signed-off-by: Imre Deak > >> > >> This looks fishy ... sysfs should all be reference-counted, so I'm > >> confused what's going on here. Also I think this smells a bit like it's > >> going overall in the wrong direction - essentially with sysfs we can't > >> really force-remove stuff but have to wait until the refcount drops to 0. > >> Or at least that's how I think it works, I'd need to blow through a pile > >> of time to figure this all out. > > > > Hm, I haven't thought about refcounting :) Now, I agree that should > > normally allow for removing a parent and child device in both order. > > > > What happens and why we can't remove first the parent then the child: > > > > In > > > > intel_dp_init_connector()-> > > intel_dp_i2c_init() > > > > we set the i2c adapter.dev.parent = intel_connector->base.kdev; > > > > device_register will then add the i2c sysfs entry to the connector sysfs > > dir. Refcounting here looks ok, both the parent connector kobject and > > its sysfs dir entry gets a reference from the child. > > > > During module cleanup, we call > > > > intel_modeset_cleanup()-> > > drm_sysfs_connector_remove() > > device_unregister(connector->kdev) > > > > which is the i2c adapter's parent kdev. Then: > > > > device_del()-> > > kobject_del()-> > > sysfs_remove_dir() > > > > will remove all entries recursively in the connector's sysfs dir, along > > with all the i2c sysfs entries. Afterwards the intel encoder->destroy() > > callback calls i2c_del_adapter()->device_unregister()->device_del() and > > that will try to remove its own sysfs attributes, namely the power sysfs > > group, but won't find it since it was removed above and give a WARN. > > > > Note that the parent's recursive removal happens regardless of its > > kobject's or sysfs entry's refcount. The kobject itself will be put > > after device_del() in put_device() and only destroyed after the i2c > > adapter releases the refcount it holds on it. I admit it feels strange > > that the sysfs entries are removed before the last reference on the > > kobject is dropped, not sure if it's by design or an overlook.. > > I have no idea either how exactly this is supposed to work, and I > quick scan through Documentation/ didn't point me into a useful > direction either. > > Adding Greg (and more mailing lists) for insight. Attached the corresponding dmesg. Also one more thought. Imo whether or not it's a valid thing to delete first a parent device and only then its child device, in this case we don't have a reason to do so. We created first the connector device (parent) and then the i2c adapter device (child) and the cleanup should happen in reverse order. This is so regardless of what order the corresponding kobjects get destroyed based on their refcounts. --Imre [ 7516.461543] WARNING: CPU: 1 PID: 4976 at fs/sysfs/group.c:215 sysfs_remove_group+0x5e/0xc0() [ 7516.461555] sysfs group 81d11960 not found for kobject 'i2c-6' name 'power' [ 7516.461566] Modules linked in: tun ccm fuse snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec snd_hwdep snd_pcm_oss snd_mixer_oss snd_pcm snd_page_alloc snd_seq_dummy snd_seq_oss arc4 snd_seq_midi iwldvm mac80211 snd_rawmidi i915(-) serio_raw snd_seq_midi_event snd_seq iwlwifi snd_seq_device btusb bnep rfcomm snd_timer bluetooth cfbfillrect thinkpad_acpi cfg80211 cfbimgblt tpm_tis tpm i2c_algo_bit cfbcopyarea snd lpc_ich drm_kms_helper rfkill drm mfd_core intel_smartconnect wmi soundcore vfat fat usbhid [last unloaded: snd_hda_intel] [ 7516.461739] CPU: 1 PID: 4976 Comm: rmmod Not tainted 3.13.0-rc8+ #38 [ 7516.461763] Hardware name: LENOVO 3460CC6/3460CC6, BIOS G6ET93WW (2.53 ) 02/04/2013 [ 7516.461788] 0009 8800d36f5b90 816fd5bc 8800d36f5bd8 [ 7516.461830] 8800d36f5bc8 81057bcc 81d11960 [ 7516.461860] 8800d494a1b8 8800d494a1a8 8800d494a290 8800d36f5c28 [ 7516.461890] Call Trace: [ 7516.461908] [] dump_stack+0x4e/0x7a [ 7516.461931] [] warn_slowpath_common+0x8c/0xc0 [ 7516.461953] [] warn_slowpath_fmt+0x4c/0x50 [ 7516.461974] [] ? m
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Place the Global GTT VM first in the list of VM
On Sun, Jan 26, 2014 at 10:26 AM, Daniel Vetter wrote: > On Sun, Jan 26, 2014 at 6:09 AM, Ben Widawsky > wrote: >> Daniel, the issue is exactly with aliasing (any platform >= gen7 with >> aliasing to be precise). I did ask Ken to try some of Mika's patches >> before pursuing this, but since then I've asked Mika to send me exactly >> what I need to review. I am not sure they are exactly what I asked Ken >> to test. > > Oh, iirc I've looked at gen6 and there it seemed to worked ok. Could > this just be a ALIASING vs. FULL vs. HW_PPGTT mixup? If so I'd prefer > just the minimal patch to remedy that and then fix the full ppgtt > dumper in a 2nd step. And since we seem to have a bit a confusion going on with no-ppgtt, aliasing ppgtt and full ppgtt a testcase for this would be useful. Should be fairly simply by submitting a bit of special noise after the MI_BB_END command for the hanging batches in the hangman testcase. Then we can just grep for that in the resulting error state. Bonus points for doing that on each ring and parsing the error state to make sure the cookie dword is indeed in the batch (and not part of some other date we dump). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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/5] drm/i915: Round up object allocations
On Sun, Jan 26, 2014 at 11:01:50AM +0100, Daniel Vetter wrote: > On Sun, Jan 26, 2014 at 09:57:08AM +, Chris Wilson wrote: > > On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > > > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky wrote: > > > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > > > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > > >> > DRM gets very mad when you request an object which occupies a partial > > > >> > page. As a DRM driver, i915 never really wants to anger DRM, and > > > >> > would > > > >> > always just want the rounding done for us. > > > >> > > > > >> > Signed-off-by: Ben Widawsky > > > >> > --- > > > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > > >> > 1 file changed, 2 insertions(+) > > > >> > > > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > >> > b/drivers/gpu/drm/i915/i915_gem.c > > > >> > index 024e454..8cd1134 100644 > > > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > > > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object > > > >> > *i915_gem_alloc_object(struct drm_device *dev, > > > >> > struct address_space *mapping; > > > >> > gfp_t mask; > > > >> > > > > >> > + size = round_up(size, PAGE_SIZE); > > > >> > + > > > >> > > > >> Nope, if there's some code that doesn't do page-aligend bo allocations > > > >> it > > > >> needs to be fixed there. If you want throw a WARN_ON and early return > > > >> in > > > >> here. > > > >> -Daniel > > > > > > > > Why? > > > > > > Because allocating a non-page aligned gem bo is a bug. All current > > > in-kernel allocations are already aligned. I've thought that we also > > > reject unaligned request from userspace but apparently we help out > > > since forever (i.e. gem was merged). Might be worth a shot to turn > > > that into an -EINVAL if libdrm does the right rounding ... > > > > We already have an -EINVAL guard on our create ioctl(s). > > Only for size == 0, not for non-aligned size, at least afaics (no coffee > yet ...). The lack of caffeine was mine. I say if (blah(PAGE_SIZE)) return -EINVAL and was happy. But as it happens it still implies that the BUG_ON in drm/drm_gem.c is relevant as is and we shouldn't be papering over it. -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/5] drm/i915: Round up object allocations
On Sun, Jan 26, 2014 at 09:57:08AM +, Chris Wilson wrote: > On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky wrote: > > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > >> > DRM gets very mad when you request an object which occupies a partial > > >> > page. As a DRM driver, i915 never really wants to anger DRM, and would > > >> > always just want the rounding done for us. > > >> > > > >> > Signed-off-by: Ben Widawsky > > >> > --- > > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > >> > 1 file changed, 2 insertions(+) > > >> > > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > >> > b/drivers/gpu/drm/i915/i915_gem.c > > >> > index 024e454..8cd1134 100644 > > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object > > >> > *i915_gem_alloc_object(struct drm_device *dev, > > >> > struct address_space *mapping; > > >> > gfp_t mask; > > >> > > > >> > + size = round_up(size, PAGE_SIZE); > > >> > + > > >> > > >> Nope, if there's some code that doesn't do page-aligend bo allocations it > > >> needs to be fixed there. If you want throw a WARN_ON and early return in > > >> here. > > >> -Daniel > > > > > > Why? > > > > Because allocating a non-page aligned gem bo is a bug. All current > > in-kernel allocations are already aligned. I've thought that we also > > reject unaligned request from userspace but apparently we help out > > since forever (i.e. gem was merged). Might be worth a shot to turn > > that into an -EINVAL if libdrm does the right rounding ... > > We already have an -EINVAL guard on our create ioctl(s). Only for size == 0, not for non-aligned size, at least afaics (no coffee yet ...). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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/5] drm/i915: Round up object allocations
On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky wrote: > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > >> > DRM gets very mad when you request an object which occupies a partial > >> > page. As a DRM driver, i915 never really wants to anger DRM, and would > >> > always just want the rounding done for us. > >> > > >> > Signed-off-by: Ben Widawsky > >> > --- > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > >> > 1 file changed, 2 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c > >> > b/drivers/gpu/drm/i915/i915_gem.c > >> > index 024e454..8cd1134 100644 > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object > >> > *i915_gem_alloc_object(struct drm_device *dev, > >> > struct address_space *mapping; > >> > gfp_t mask; > >> > > >> > + size = round_up(size, PAGE_SIZE); > >> > + > >> > >> Nope, if there's some code that doesn't do page-aligend bo allocations it > >> needs to be fixed there. If you want throw a WARN_ON and early return in > >> here. > >> -Daniel > > > > Why? > > Because allocating a non-page aligned gem bo is a bug. All current > in-kernel allocations are already aligned. I've thought that we also > reject unaligned request from userspace but apparently we help out > since forever (i.e. gem was merged). Might be worth a shot to turn > that into an -EINVAL if libdrm does the right rounding ... We already have an -EINVAL guard on our create ioctl(s). -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/5] drm/i915: Place the Global GTT VM first in the list of VM
On Sun, Jan 26, 2014 at 6:09 AM, Ben Widawsky wrote: > Daniel, the issue is exactly with aliasing (any platform >= gen7 with > aliasing to be precise). I did ask Ken to try some of Mika's patches > before pursuing this, but since then I've asked Mika to send me exactly > what I need to review. I am not sure they are exactly what I asked Ken > to test. Oh, iirc I've looked at gen6 and there it seemed to worked ok. Could this just be a ALIASING vs. FULL vs. HW_PPGTT mixup? If so I'd prefer just the minimal patch to remedy that and then fix the full ppgtt dumper in a 2nd step. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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 3/5] drm/i915: Create a USES_PPGTT macro
On Sun, Jan 26, 2014 at 6:45 AM, Ben Widawsky
wrote:
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > b/drivers/gpu/drm/i915/i915_drv.h
>> > index e851a82..6f68515 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -1844,6 +1844,7 @@ struct drm_i915_file_private {
>> > #define HAS_PPGTT(dev) (INTEL_INFO(dev)->gen >= 7 &&
>> > !IS_VALLEYVIEW(dev) && !IS_BROADWELL(dev))
>> > #define USES_ALIASING_PPGTT(dev) intel_enable_ppgtt(dev, false)
>> > #define USES_FULL_PPGTT(dev) intel_enable_ppgtt(dev, true)
>> > +#define USES_PPGTT(dev)(USES_ALIASING_PPGTT(dev) ||
>> > USES_FULL_PPGTT(dev))
>>
>> Just do an s/ALIASING/HW/ and we get to the same point with overall
>> clearer code.
>> -Daniel
>
> I had plans to make USES_ALIASING actually mean it's using aliasing, and
> NOT full PPGTT. However, at this point, they are indeed logically
> equivalent. Let me go over the code again and see if we actually want an
> USES_ALIASING_PPGTT(), and if not, I'll do it your way. First, let's
> figure out if the series will get merged at all.
Well I've had a giant wtf moment when looking through your ppgtt
patches before the holidays due to this, until I've noticed that
USES_ALIASING_PPGTT also holds for full ppgtt. So I'm voting very much
for a cleanup here, the current code is confusing as hell in some
areas, at least for incompetent me ;-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 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 v2] drm/i915: fix dp/sdvo i2c cleanup
On Sun, Jan 26, 2014 at 1:51 AM, Imre Deak wrote: > On Sat, 2014-01-25 at 21:37 +0100, Daniel Vetter wrote: >> On Fri, Jan 24, 2014 at 02:47:33PM +0200, Imre Deak wrote: >> > Atm we try to remove the connector's i2c sysfs entry too late in the >> > encoder's destroy callback. By that time the kobject used as the parent >> > for all connector sysfs entries is already removed when we do an early >> > removal of all connector sysfs entries in intel_modeset_cleanup(). Fix >> > this by adding an early_destroy encoder callback, where we remove the >> > encoder's i2c adapter. >> > >> > v2: >> > - add missing static to function, use existing sdvo cast helper, >> > s/intel_sdvo/sdvo/ (Chris) >> > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70523 >> > >> > Signed-off-by: Imre Deak >> >> This looks fishy ... sysfs should all be reference-counted, so I'm >> confused what's going on here. Also I think this smells a bit like it's >> going overall in the wrong direction - essentially with sysfs we can't >> really force-remove stuff but have to wait until the refcount drops to 0. >> Or at least that's how I think it works, I'd need to blow through a pile >> of time to figure this all out. > > Hm, I haven't thought about refcounting :) Now, I agree that should > normally allow for removing a parent and child device in both order. > > What happens and why we can't remove first the parent then the child: > > In > > intel_dp_init_connector()-> > intel_dp_i2c_init() > > we set the i2c adapter.dev.parent = intel_connector->base.kdev; > > device_register will then add the i2c sysfs entry to the connector sysfs > dir. Refcounting here looks ok, both the parent connector kobject and > its sysfs dir entry gets a reference from the child. > > During module cleanup, we call > > intel_modeset_cleanup()-> > drm_sysfs_connector_remove() > device_unregister(connector->kdev) > > which is the i2c adapter's parent kdev. Then: > > device_del()-> > kobject_del()-> > sysfs_remove_dir() > > will remove all entries recursively in the connector's sysfs dir, along > with all the i2c sysfs entries. Afterwards the intel encoder->destroy() > callback calls i2c_del_adapter()->device_unregister()->device_del() and > that will try to remove its own sysfs attributes, namely the power sysfs > group, but won't find it since it was removed above and give a WARN. > > Note that the parent's recursive removal happens regardless of its > kobject's or sysfs entry's refcount. The kobject itself will be put > after device_del() in put_device() and only destroyed after the i2c > adapter releases the refcount it holds on it. I admit it feels strange > that the sysfs entries are removed before the last reference on the > kobject is dropped, not sure if it's by design or an overlook.. I have no idea either how exactly this is supposed to work, and I quick scan through Documentation/ didn't point me into a useful direction either. Adding Greg (and more mailing lists) for insight. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - 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/5] drm/i915: Round up object allocations
On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky wrote: > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: >> > DRM gets very mad when you request an object which occupies a partial >> > page. As a DRM driver, i915 never really wants to anger DRM, and would >> > always just want the rounding done for us. >> > >> > Signed-off-by: Ben Widawsky >> > --- >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c >> > b/drivers/gpu/drm/i915/i915_gem.c >> > index 024e454..8cd1134 100644 >> > --- a/drivers/gpu/drm/i915/i915_gem.c >> > +++ b/drivers/gpu/drm/i915/i915_gem.c >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object >> > *i915_gem_alloc_object(struct drm_device *dev, >> > struct address_space *mapping; >> > gfp_t mask; >> > >> > + size = round_up(size, PAGE_SIZE); >> > + >> >> Nope, if there's some code that doesn't do page-aligend bo allocations it >> needs to be fixed there. If you want throw a WARN_ON and early return in >> here. >> -Daniel > > Why? Because allocating a non-page aligned gem bo is a bug. All current in-kernel allocations are already aligned. I've thought that we also reject unaligned request from userspace but apparently we help out since forever (i.e. gem was merged). Might be worth a shot to turn that into an -EINVAL if libdrm does the right rounding ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
