Update the DRM driver to use the new tasklet API, which does not rely
on the tasklet implementation details.
Signed-off-by: Steven Rostedt <[email protected]>
Index: linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
===================================================================
--- linux-2.6.21-rt9.orig/drivers/char/drm/drm_irq.c
+++ linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
@@ -461,7 +461,7 @@ void drm_locked_tasklet(drm_device_t *de
static DECLARE_TASKLET(drm_tasklet, drm_locked_tasklet_func, 0);
if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ) ||
- test_bit(TASKLET_STATE_SCHED, &drm_tasklet.state))
+ tasklet_is_scheduled(&drm_tasklet))
return;
spin_lock_irqsave(&dev->tasklet_lock, irqflags);
--
On Fri, 2007-06-22 at 00:00 -0400, Steven Rostedt wrote:
> plain text document attachment (tasklet-driver-hacks.patch)
> Update the DRM driver to use the new tasklet API, which does not rely
> on the tasklet implementation details.
>
> Signed-off-by: Steven Rostedt <[email protected]>
>
>
> Index: linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
> ===================================================================
> --- linux-2.6.21-rt9.orig/drivers/char/drm/drm_irq.c
> +++ linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
> @@ -461,7 +461,7 @@ void drm_locked_tasklet(drm_device_t *de
> static DECLARE_TASKLET(drm_tasklet, drm_locked_tasklet_func, 0);
>
> if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ) ||
> - test_bit(TASKLET_STATE_SCHED, &drm_tasklet.state))
> + tasklet_is_scheduled(&drm_tasklet))
> return;
>
> spin_lock_irqsave(&dev->tasklet_lock, irqflags);
>
No sense in having a patch just for this, may as well merge this with
patch 3 ..
Daniel
On Thu, 2007-06-21 at 23:36 -0700, Daniel Walker wrote:
> On Fri, 2007-06-22 at 00:00 -0400, Steven Rostedt wrote:
> > plain text document attachment (tasklet-driver-hacks.patch)
> > Update the DRM driver to use the new tasklet API, which does not rely
> > on the tasklet implementation details.
> >
> > Signed-off-by: Steven Rostedt <[email protected]>
> >
> >
> > Index: linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
> > ===================================================================
> > --- linux-2.6.21-rt9.orig/drivers/char/drm/drm_irq.c
> > +++ linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
> > @@ -461,7 +461,7 @@ void drm_locked_tasklet(drm_device_t *de
> > static DECLARE_TASKLET(drm_tasklet, drm_locked_tasklet_func, 0);
> >
> > if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ) ||
> > - test_bit(TASKLET_STATE_SCHED, &drm_tasklet.state))
> > + tasklet_is_scheduled(&drm_tasklet))
> > return;
> >
> > spin_lock_irqsave(&dev->tasklet_lock, irqflags);
> >
>
>
> No sense in having a patch just for this, may as well merge this with
> patch 3 ..
Wrong. patch 3 adds the API and this one makes use of it. Stevens split
makes perfectly sense.
tglx
On Fri, 2007-06-22 at 08:49 +0200, Thomas Gleixner wrote:
> On Thu, 2007-06-21 at 23:36 -0700, Daniel Walker wrote:
> > On Fri, 2007-06-22 at 00:00 -0400, Steven Rostedt wrote:
> > > plain text document attachment (tasklet-driver-hacks.patch)
> > > Update the DRM driver to use the new tasklet API, which does not rely
> > > on the tasklet implementation details.
> > >
> > > Signed-off-by: Steven Rostedt <[email protected]>
> > >
> > >
> > > Index: linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
> > > ===================================================================
> > > --- linux-2.6.21-rt9.orig/drivers/char/drm/drm_irq.c
> > > +++ linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
> > > @@ -461,7 +461,7 @@ void drm_locked_tasklet(drm_device_t *de
> > > static DECLARE_TASKLET(drm_tasklet, drm_locked_tasklet_func, 0);
> > >
> > > if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ) ||
> > > - test_bit(TASKLET_STATE_SCHED, &drm_tasklet.state))
> > > + tasklet_is_scheduled(&drm_tasklet))
> > > return;
> > >
> > > spin_lock_irqsave(&dev->tasklet_lock, irqflags);
> > >
> >
> >
> > No sense in having a patch just for this, may as well merge this with
> > patch 3 ..
>
> Wrong. patch 3 adds the API and this one makes use of it. Stevens split
> makes perfectly sense.
Clearly it doesn't make sense to me ;) .. The patches are too small to
split them up that way ..
Daniel
On Fri, 2007-06-22 at 00:08 -0700, Daniel Walker wrote:
> > > No sense in having a patch just for this, may as well merge this with
> > > patch 3 ..
> >
> > Wrong. patch 3 adds the API and this one makes use of it. Stevens split
> > makes perfectly sense.
>
> Clearly it doesn't make sense to me ;) .. The patches are too small to
> split them up that way ..
Daniel, you of all people should know. It's not the size of the patch
that matters, it's the way you use the patch ;-)
No these two patches should *not* be merged to one. If these are sitting
in -mm, and someone were to change the DRM to not to use the API and
someone else changed their driver to use the API, then what? Does
Andrew keep these maintenance patches on top of each other?
The split lets the DRM patch be dropped or replaced while keeping the
API patch around in case another driver uses the API.
The two patches have two different objectives, even though they are
related and currently on a 1 to 1 basis. The patches regardless, should
stay separate.
-- Steve
On Fri, 2007-06-22 at 08:15 -0400, Steven Rostedt wrote:
> On Fri, 2007-06-22 at 00:08 -0700, Daniel Walker wrote:
>
> > > > No sense in having a patch just for this, may as well merge this with
> > > > patch 3 ..
> > >
> > > Wrong. patch 3 adds the API and this one makes use of it. Stevens split
> > > makes perfectly sense.
> >
> > Clearly it doesn't make sense to me ;) .. The patches are too small to
> > split them up that way ..
>
> Daniel, you of all people should know. It's not the size of the patch
> that matters, it's the way you use the patch ;-)
>
Are you trying to say that you think I have a small patch Steven ;) ?
> No these two patches should *not* be merged to one. If these are sitting
> in -mm, and someone were to change the DRM to not to use the API and
> someone else changed their driver to use the API, then what? Does
> Andrew keep these maintenance patches on top of each other?
I read this 5 times at least .. I don't think I'm following what you
saying .. It sounds like you might be thinking to many steps ahead tho..
> The split lets the DRM patch be dropped or replaced while keeping the
> API patch around in case another driver uses the API.
Ok, but there are no other users currently, and I think it's unlikely
you'll have others in the future since TASKLET_STATE_SCHED seems more
like an internal part of tasklets .. This drm user seems like the one
aberrant.
> The two patches have two different objectives, even though they are
> related and currently on a 1 to 1 basis. The patches regardless, should
> stay separate.
I'm not convinced yet .. One more stab?
Daniel
On Friday 22 June 2007, Thomas Gleixner wrote:
> > > Index: linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
> > > ===================================================================
> > > --- linux-2.6.21-rt9.orig/drivers/char/drm/drm_irq.c
> > > +++ linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
> > > @@ -461,7 +461,7 @@ void drm_locked_tasklet(drm_device_t *de
> > > ????static DECLARE_TASKLET(drm_tasklet, drm_locked_tasklet_func, 0);
> > > ?
> > > ????if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ) ||
> > > -??? ? ?test_bit(TASKLET_STATE_SCHED, &drm_tasklet.state))
> > > +??? ? ?tasklet_is_scheduled(&drm_tasklet))
> > > ????????????return;
> > > ?
> > > ????spin_lock_irqsave(&dev->tasklet_lock, irqflags);
> > >
> >
> >
> > No sense in having a patch just for this, may as well merge this with
> > patch 3 ..
>
> Wrong. patch 3 adds the API and this one makes use of it. Stevens split
> makes perfectly sense.
Wouldn't the easy solution be to get rid of drm_locked_tasklet
entirely and convert i915_vblank_tasklet(), the only user, to use
a work queue right away?
The drm_locked_tasklet() function seems to have multiple bugs anyway,
so getting rid of it can only help, and it avoids exporting a new
tasklet_is_scheduled() interface.
Arnd <><
On Fri, 2007-06-22 at 18:10 +0200, Arnd Bergmann wrote:
> Wouldn't the easy solution be to get rid of drm_locked_tasklet
> entirely and convert i915_vblank_tasklet(), the only user, to use
> a work queue right away?
You recommend making the i915 use a workqueue to do this instead, and
remove the drm_lock_tasklet code from the drm_irq.c completely?
Unfortunately, I don't have any boxes with a i915, so I would not feel
comfortable with doing this myself.
>
> The drm_locked_tasklet() function seems to have multiple bugs anyway,
> so getting rid of it can only help, and it avoids exporting a new
> tasklet_is_scheduled() interface.
To be able to at least continue my work, I'll just keep these as
separate patches.
Thanks,
-- Steve
On Fri, Jun 22, 2007 at 06:10:55PM +0200, Arnd Bergmann wrote:
> On Friday 22 June 2007, Thomas Gleixner wrote:
> > > > Index: linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
> > > > ===================================================================
> > > > --- linux-2.6.21-rt9.orig/drivers/char/drm/drm_irq.c
> > > > +++ linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
> > > > @@ -461,7 +461,7 @@ void drm_locked_tasklet(drm_device_t *de
> > > > ????static DECLARE_TASKLET(drm_tasklet, drm_locked_tasklet_func, 0);
> > > > ?
> > > > ????if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ) ||
> > > > -??? ? ?test_bit(TASKLET_STATE_SCHED, &drm_tasklet.state))
> > > > +??? ? ?tasklet_is_scheduled(&drm_tasklet))
> > > > ????????????return;
> > > > ?
> > > > ????spin_lock_irqsave(&dev->tasklet_lock, irqflags);
> > > >
> > >
> > >
> > > No sense in having a patch just for this, may as well merge this with
> > > patch 3 ..
> >
> > Wrong. patch 3 adds the API and this one makes use of it. Stevens split
> > makes perfectly sense.
>
> Wouldn't the easy solution be to get rid of drm_locked_tasklet
> entirely and convert i915_vblank_tasklet(), the only user, to use
> a work queue right away?
>
> The drm_locked_tasklet() function seems to have multiple bugs anyway,
> so getting rid of it can only help, and it avoids exporting a new
> tasklet_is_scheduled() interface.
That's exactly what I though when looking over this code. There's
some really crappy in code in that area, and it should simply be
rewritten.
* Daniel Walker <[email protected]> wrote:
> > The two patches have two different objectives, even though they are
> > related and currently on a 1 to 1 basis. The patches regardless,
> > should stay separate.
>
> I'm not convinced yet .. One more stab?
uhm, i dont think Steve needs to 'convince' you. You have been (once
again ...) given extensive explanations about a really trivial topic,
but you keep pushing and demanding very agressively without apparently
reading the answers. If you dont want to or cannot understand it's
really your problem to solve, not Steve's ...
Ingo
On Sat, 2007-06-23 at 00:38 +0200, Ingo Molnar wrote:
> * Daniel Walker <[email protected]> wrote:
>
> > > The two patches have two different objectives, even though they are
> > > related and currently on a 1 to 1 basis. The patches regardless,
> > > should stay separate.
> >
> > I'm not convinced yet .. One more stab?
>
> uhm, i dont think Steve needs to 'convince' you. You have been (once
> again ...) given extensive explanations about a really trivial topic,
> but you keep pushing and demanding very agressively without apparently
> reading the answers. If you dont want to or cannot understand it's
> really your problem to solve, not Steve's ...
Your right Steven doesn't need to convince me. He requested comments and
I gave them, if he or you want to ignore them that's your prerogative.
How can you say I didn't read the answers .. I said "I read this 5
times" ! It doesn't get much more explicit than that .. In fact, my
response was mainly cause I thought his explanation was confusing.. How
can you fault _me_ for that, it's the medium we're using.
It seems like you assume all my emails are "demanding" and "aggressive",
where I'm just asking questions and making comments .. Who am I to be
making demands of anyone ?
<joke>
I DEMAND you do what I say! You better do it or else! I'll force Linus
to ignore you, so you better do it.
</joke>
Daniel
> >
> > The drm_locked_tasklet() function seems to have multiple bugs anyway,
> > so getting rid of it can only help, and it avoids exporting a new
> > tasklet_is_scheduled() interface.
>
> That's exactly what I though when looking over this code. There's
> some really crappy in code in that area, and it should simply be
> rewritten.
Can someone submit a patch or even a better review? btw removing the
core stuff and putting it i915 isn't acceptable, I'll have support for
this feature for other hw coming up so the generic code needs to be
available...
Dave.