[PATCH 14/15] gma500: nuke the PSB debug stuff

[Daniel Vetter]

On Mon, Jun 13, 2011 at 04:44:03PM +0100, Alan Cox wrote:
> > Given that I've mucked around in drm_mm quite a bit I'd be very interested
> > in your opinion about what's weird in it (and presumably what could be
> > improved). Can you elaborate?
> 
> Mostly the API, which is also somewhat poorly documented. I've not dug
> into the internals beyond trying to figure out how to use it. Looking at
> the users the API is non-obvious, the interface involves a mix of calls
> and digging deep into struct internals.

Well, I've rewritten the api but that transition is not yet complete, i.e.
the new interface is there, but no driver is converted yet. The old api
has a two-step allocation (search_free + get_block) and frees allocated
ranges with put_block. The new api has just insert_node and remove_node,
struct drm_mm_node must be allocated by the caller (to get rid of the racy
preallocation scheme and allow embedding of drm_mm_node).

There's also the lru scanning helpers (only used by intel). Under specific
conditions (should be all documented in comments) this can be used as an
eviction roaster.

Generally only node->start should be used by drivers, otherwise
drm_mm_node should be opaque. At least that's the idea.

btw, old conversion patches for i915 to the new interface are at:

http://cgit.freedesktop.org/~danvet/drm/log/?h=embed-gtt-space

When creating the new interfaces I've tried to document them. Ideas to
improve that highly welcome.

> I'd have expected an API that had allocate/free type methods and exposed
> the needed information directly or very easily not one that has drivers
> doing. drm_sman seems to be attempt in that direction but its also rather
> odd with interfaces like

Should be fixed, see above ;-)

[snip]

> Possibly drm_sman and friends should just wrap whatever simple native
> allocator exists on the OS ?

Well, drm_mm has some graphics specific magic (range restricted scans and
the eviction helpers). But simpler stuff like sman is imo just "drm likes
to reinvent the wheel". Unfortunately I don't have the hw, so I haven't
dared to touch it (the snippet you've posted is a prime example why).

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 14/15] gma500: nuke the PSB debug stuff

[Alan Cox]

> Given that I've mucked around in drm_mm quite a bit I'd be very interested
> in your opinion about what's weird in it (and presumably what could be
> improved). Can you elaborate?

Mostly the API, which is also somewhat poorly documented. I've not dug
into the internals beyond trying to figure out how to use it. Looking at
the users the API is non-obvious, the interface involves a mix of calls
and digging deep into struct internals.

I'd have expected an API that had allocate/free type methods and exposed
the needed information directly or very easily not one that has drivers
doing. drm_sman seems to be attempt in that direction but its also rather
odd with interfaces like


item = drm_sman_alloc(_priv->sman, mem->type, tmpSize, 0,
  (unsigned long)file_priv);
mutex_unlock(>struct_mutex);
if (item) {
mem->offset = ((mem->type == VIA_MEM_VIDEO) ?
  dev_priv->vram_offset :
dev_priv->agp_offset) + (item->mm->
 offset(item->mm, item->mm_info) <<
VIA_MM_ALIGN_SHIFT); mem->index = item->user_hash.key;


where the item->... gymnastics are spectacular to say the least given the
basic function of the allocator appears to be to provide said offset in
the first place.

And ultimately this is why I went with using allocate_region and friends
plus using GEM to do handles, which was a good choice for other reasons
as GEM is rather nicely designed barring the lack of a couple of helpers
and the few lines needed to split the shmem/handle abstraction properly.

Possibly drm_sman and friends should just wrap whatever simple native
allocator exists on the OS ?

Alan

Re: [PATCH 14/15] gma500: nuke the PSB debug stuff

[Alan Cox]

 Given that I've mucked around in drm_mm quite a bit I'd be very interested
 in your opinion about what's weird in it (and presumably what could be
 improved). Can you elaborate?

Mostly the API, which is also somewhat poorly documented. I've not dug
into the internals beyond trying to figure out how to use it. Looking at
the users the API is non-obvious, the interface involves a mix of calls
and digging deep into struct internals.

I'd have expected an API that had allocate/free type methods and exposed
the needed information directly or very easily not one that has drivers
doing. drm_sman seems to be attempt in that direction but its also rather
odd with interfaces like


item = drm_sman_alloc(dev_priv-sman, mem-type, tmpSize, 0,
  (unsigned long)file_priv);
mutex_unlock(dev-struct_mutex);
if (item) {
mem-offset = ((mem-type == VIA_MEM_VIDEO) ?
  dev_priv-vram_offset :
dev_priv-agp_offset) + (item-mm-
 offset(item-mm, item-mm_info) 
VIA_MM_ALIGN_SHIFT); mem-index = item-user_hash.key;


where the item-... gymnastics are spectacular to say the least given the
basic function of the allocator appears to be to provide said offset in
the first place.

And ultimately this is why I went with using allocate_region and friends
plus using GEM to do handles, which was a good choice for other reasons
as GEM is rather nicely designed barring the lack of a couple of helpers
and the few lines needed to split the shmem/handle abstraction properly.

Possibly drm_sman and friends should just wrap whatever simple native
allocator exists on the OS ?

Alan


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/15] gma500: nuke the PSB debug stuff

[Daniel Vetter]

On Mon, Jun 13, 2011 at 04:44:03PM +0100, Alan Cox wrote:
  Given that I've mucked around in drm_mm quite a bit I'd be very interested
  in your opinion about what's weird in it (and presumably what could be
  improved). Can you elaborate?
 
 Mostly the API, which is also somewhat poorly documented. I've not dug
 into the internals beyond trying to figure out how to use it. Looking at
 the users the API is non-obvious, the interface involves a mix of calls
 and digging deep into struct internals.

Well, I've rewritten the api but that transition is not yet complete, i.e.
the new interface is there, but no driver is converted yet. The old api
has a two-step allocation (search_free + get_block) and frees allocated
ranges with put_block. The new api has just insert_node and remove_node,
struct drm_mm_node must be allocated by the caller (to get rid of the racy
preallocation scheme and allow embedding of drm_mm_node).

There's also the lru scanning helpers (only used by intel). Under specific
conditions (should be all documented in comments) this can be used as an
eviction roaster.

Generally only node-start should be used by drivers, otherwise
drm_mm_node should be opaque. At least that's the idea.

btw, old conversion patches for i915 to the new interface are at:

http://cgit.freedesktop.org/~danvet/drm/log/?h=embed-gtt-space

When creating the new interfaces I've tried to document them. Ideas to
improve that highly welcome.

 I'd have expected an API that had allocate/free type methods and exposed
 the needed information directly or very easily not one that has drivers
 doing. drm_sman seems to be attempt in that direction but its also rather
 odd with interfaces like

Should be fixed, see above ;-)

[snip]

 Possibly drm_sman and friends should just wrap whatever simple native
 allocator exists on the OS ?

Well, drm_mm has some graphics specific magic (range restricted scans and
the eviction helpers). But simpler stuff like sman is imo just drm likes
to reinvent the wheel. Unfortunately I don't have the hw, so I haven't
dared to touch it (the snippet you've posted is a prime example why).

Yours, Daniel
-- 
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 14/15] gma500: nuke the PSB debug stuff

[Daniel Vetter]

Hi Alan.

On Thu, Jun 9, 2011 at 14:04, Alan Cox  wrote:
> The gma500 driver uses a lot of direct Linux services rather than
> disappearing into the weird world of drm_mm and the like so any
> portability is going to be a bit of an illusion at best.

Given that I've mucked around in drm_mm quite a bit I'd be very interested
in your opinion about what's weird in it (and presumably what could be
improved). Can you elaborate?

Thanks a lot, Daniel
-- 
Daniel Vetter
daniel.vetter at ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] gma500: nuke the PSB debug stuff

[Daniel Vetter]

Hi Alan.

On Thu, Jun 9, 2011 at 14:04, Alan Cox a...@lxorguk.ukuu.org.uk wrote:
 The gma500 driver uses a lot of direct Linux services rather than
 disappearing into the weird world of drm_mm and the like so any
 portability is going to be a bit of an illusion at best.

Given that I've mucked around in drm_mm quite a bit I'd be very interested
in your opinion about what's weird in it (and presumably what could be
improved). Can you elaborate?

Thanks a lot, Daniel
-- 
Daniel Vetter
daniel.vet...@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 14/15] gma500: nuke the PSB debug stuff

[Dave Airlie]

On Thu, 2011-06-09 at 09:11 +0100, Alan Cox wrote:
> On Thu, 9 Jun 2011 03:10:03 +0200
> Patrik Jakobsson  wrote:
> 
> > Hi Alan
> > 
> > Just a thought. Shouldn't we use the DRM macros for printing debug info?
> 
> Linux has perfectly good printing functions and using them means we can
> use dev_dbg() which supports things like nice runtime switching.

You mean like the drm debug functions runtime switching? that predated
the kernel ones and nobody ever ported :-)

Though if psb wants to be different to other drm drivers it can lead the
way, though it'll be a total nightmare for all the people who follow
documentation on how to debug drm drivers using drm.debug=1,2,4,8. for
various code paths.

Dave.

[PATCH 14/15] gma500: nuke the PSB debug stuff

[Patrik Jakobsson]

On Thu, Jun 9, 2011 at 12:36 PM, Dave Airlie wrote:
> On Thu, 2011-06-09 at 09:11 +0100, Alan Cox wrote:
>> On Thu, 9 Jun 2011 03:10:03 +0200
>> Patrik Jakobsson wrote:
>>
>> > Hi Alan
>> >
>> > Just a thought. Shouldn't we use the DRM macros for printing debug info?
>>
>> Linux has perfectly good printing functions and using them means we can
>> use dev_dbg() which supports things like nice runtime switching.
>
> You mean like the drm debug functions runtime switching? that predated
> the kernel ones and nobody ever ported :-)
>
> Though if psb wants to be different to other drm drivers it can lead the
> way, though it'll be a total nightmare for all the people who follow
> documentation on how to debug drm drivers using drm.debug=1,2,4,8. for
> various code paths.

Yes, my concern was about drm.debug and use of all the DRM portability stuff
(like using DRM_IRQ_HANDLED instead of IRQ_HANDLED, etc...)

The portability might not be important at this point but I just wanted to raise
the question so I know what is right / wrong.

Alan, I've been working on the output code but think I've reached a dead end.
I'm up to 2 lines of changes and it's just a big mess. I'm gonna go for
slowly fixing up the current code instead. I also got my hands on a laptop
with a gma500 so I can test that my changes doesn't break LVDS. Stay tuned.

Thanks

[PATCH 14/15] gma500: nuke the PSB debug stuff

[Alan Cox]

> Yes, my concern was about drm.debug and use of all the DRM portability stuff
> (like using DRM_IRQ_HANDLED instead of IRQ_HANDLED, etc...)
> 
> The portability might not be important at this point but I just wanted to 
> raise
> the question so I know what is right / wrong.

The gma500 driver uses a lot of direct Linux services rather than
disappearing into the weird world of drm_mm and the like so any
portability is going to be a bit of an illusion at best.

If someone ports it to another GPL licensed OS then I may have to
reconsider a bit, we shall see.

> Alan, I've been working on the output code but think I've reached a dead end.
> I'm up to 2 lines of changes and it's just a big mess. I'm gonna go for
> slowly fixing up the current code instead. I also got my hands on a laptop
> with a gma500 so I can test that my changes doesn't break LVDS. Stay tuned.

Will do. Got another chunk of patches to fire at Greg shortly and I've
now beaten pretty much all of it into passing CodingStyle so hopefully
any noise will settle down at that point.

Still not ready to leave staging, passing CodingStyle and being clean are
not quite the same thing in this case !

[PATCH 14/15] gma500: nuke the PSB debug stuff

[Alan Cox]

> Though if psb wants to be different to other drm drivers it can lead the
> way, though it'll be a total nightmare for all the people who follow
> documentation on how to debug drm drivers using drm.debug=1,2,4,8. for
> various code paths.

Actually it seems to work out nicely because you can debug DRM core
goings on and driver goings on separately. Also the driver ones can be
turned on/off on their own at runtime which is a godsend.

As you can imagine I've been testing the debugging a fair bit !

Alan

[PATCH 14/15] gma500: nuke the PSB debug stuff

[Alan Cox]

On Thu, 9 Jun 2011 03:10:03 +0200
Patrik Jakobsson  wrote:

> Hi Alan
> 
> Just a thought. Shouldn't we use the DRM macros for printing debug info?

Linux has perfectly good printing functions and using them means we can
use dev_dbg() which supports things like nice runtime switching.

[PATCH 14/15] gma500: nuke the PSB debug stuff

[Patrik Jakobsson]

Hi Alan

Just a thought. Shouldn't we use the DRM macros for printing debug info?

-Patrik

On Wed, Jun 8, 2011 at 12:15 PM, Alan Cox  wrote:
> From: Alan Cox 
>
> Lose all the PSB debug gunge. We can replace it with dev_dbg() like normal
> drivers if and when we need debug on stuff.
>
> Signed-off-by: Alan Cox 
> ---
>
> ?drivers/staging/gma500/mrst_crtc.c ? ? ? ? ?| ? 23 ++
> ?drivers/staging/gma500/mrst_lvds.c ? ? ? ? ?| ? 12 +--
> ?drivers/staging/gma500/psb_bl.c ? ? ? ? ? ? | ? ?8 --
> ?drivers/staging/gma500/psb_drv.c ? ? ? ? ? ?| ? 33 +++--
> ?drivers/staging/gma500/psb_drv.h ? ? ? ? ? ?| ? 96 +++---
> ?drivers/staging/gma500/psb_fb.c ? ? ? ? ? ? | ? 34 +
> ?drivers/staging/gma500/psb_gem.c ? ? ? ? ? ?| ? ?6 +-
> ?drivers/staging/gma500/psb_gtt.c ? ? ? ? ? ?| ? ?6 +-
> ?drivers/staging/gma500/psb_intel_bios.c ? ? | ? 17 +
> ?drivers/staging/gma500/psb_intel_display.c ?| ? 99 
> +++
> ?drivers/staging/gma500/psb_intel_lvds.c ? ? | ? 53 ++
> ?drivers/staging/gma500/psb_intel_opregion.c | ? ?7 --
> ?drivers/staging/gma500/psb_intel_sdvo.c ? ? | ? 34 -
> ?drivers/staging/gma500/psb_irq.c ? ? ? ? ? ?| ? 33 ++---
> ?14 files changed, 87 insertions(+), 374 deletions(-)
>
> diff --git a/drivers/staging/gma500/mrst_crtc.c 
> b/drivers/staging/gma500/mrst_crtc.c
> index fd97c80..fb9f2a2 100644
> --- a/drivers/staging/gma500/mrst_crtc.c
> +++ b/drivers/staging/gma500/mrst_crtc.c
> @@ -103,7 +103,7 @@ static const struct mrst_limit_t *mrst_limit(struct 
> drm_crtc *crtc)
> ? ? ? ? ? ? ? ?}
> ? ? ? ?} else {
> ? ? ? ? ? ? ? ?limit = NULL;
> - ? ? ? ? ? ? ? PSB_DEBUG_ENTRY("mrst_limit Wrong display type.\n");
> + ? ? ? ? ? ? ? dev_err(dev->dev, "mrst_limit Wrong display type.\n");
> ? ? ? ?}
>
> ? ? ? ?return limit;
> @@ -117,7 +117,7 @@ static void mrst_clock(int refclk, struct mrst_clock_t 
> *clock)
>
> ?void mrstPrintPll(char *prefix, struct mrst_clock_t *clock)
> ?{
> - ? ? ? PSB_DEBUG_ENTRY("%s: dotclock = %d, ?m = %d, p1 = %d.\n",
> + ? ? ? pr_debug("%s: dotclock = %d, ?m = %d, p1 = %d.\n",
> ? ? ? ? ? ? prefix, clock->dot, clock->m, clock->p1);
> ?}
>
> @@ -149,8 +149,7 @@ mrstFindBestPLL(struct drm_crtc *crtc, int target, int 
> refclk,
> ? ? ? ? ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ?}
> ? ? ? ?}
> - ? ? ? DRM_DEBUG("mrstFindBestPLL err = %d.\n", err);
> -
> + ? ? ? dev_dbg(crtc->dev->dev, "mrstFindBestPLL err = %d.\n", err);
> ? ? ? ?return err != target;
> ?}
>
> @@ -172,8 +171,6 @@ static void mrst_crtc_dpms(struct drm_crtc *crtc, int 
> mode)
> ? ? ? ?u32 temp;
> ? ? ? ?bool enabled;
>
> - ? ? ? PSB_DEBUG_ENTRY("mode = %d, pipe = %d\n", mode, pipe);
> -
> ? ? ? ?if (!gma_power_begin(dev, true))
> ? ? ? ? ? ? ? ?return;
>
> @@ -320,8 +317,6 @@ static int mrst_crtc_mode_set(struct drm_crtc *crtc,
> ? ? ? ?uint64_t scalingType = DRM_MODE_SCALE_FULLSCREEN;
> ? ? ? ?struct drm_encoder *encoder;
>
> - ? ? ? PSB_DEBUG_ENTRY("pipe = 0x%x\n", pipe);
> -
> ? ? ? ?if (!gma_power_begin(dev, true))
> ? ? ? ? ? ? ? ?return 0;
>
> @@ -446,10 +441,9 @@ static int mrst_crtc_mode_set(struct drm_crtc *crtc,
> ? ? ? ?ok = mrstFindBestPLL(crtc, adjusted_mode->clock, refclk, );
>
> ? ? ? ?if (!ok) {
> - ? ? ? ? ? ? ? PSB_DEBUG_ENTRY(
> - ? ? ? ? ? ? ? ? ? ? ? "mrstFindBestPLL fail in mrst_crtc_mode_set.\n");
> + ? ? ? ? ? ? ? dev_dbg(dev->dev, "mrstFindBestPLL fail in 
> mrst_crtc_mode_set.\n");
> ? ? ? ?} else {
> - ? ? ? ? ? ? ? PSB_DEBUG_ENTRY("mrst_crtc_mode_set pixel clock = %d,"
> + ? ? ? ? ? ? ? dev_dbg(dev->dev, "mrst_crtc_mode_set pixel clock = %d,"
> ? ? ? ? ? ? ? ? ? ? ? ? "m = %x, p1 = %x.\n", clock.dot, clock.m,
> ? ? ? ? ? ? ? ? ? ? ? ? clock.p1);
> ? ? ? ?}
> @@ -540,11 +534,9 @@ int mrst_pipe_set_base(struct drm_crtc *crtc,
> ? ? ? ?u32 dspcntr;
> ? ? ? ?int ret = 0;
>
> - ? ? ? PSB_DEBUG_ENTRY("\n");
> -
> ? ? ? ?/* no fb bound */
> ? ? ? ?if (!crtc->fb) {
> - ? ? ? ? ? ? ? DRM_DEBUG("No FB bound\n");
> + ? ? ? ? ? ? ? dev_dbg(dev->dev, "No FB bound\n");
> ? ? ? ? ? ? ? ?return 0;
> ? ? ? ?}
>
> @@ -574,13 +566,12 @@ int mrst_pipe_set_base(struct drm_crtc *crtc,
> ? ? ? ? ? ? ? ?dspcntr |= DISPPLANE_32BPP_NO_ALPHA;
> ? ? ? ? ? ? ? ?break;
> ? ? ? ?default:
> - ? ? ? ? ? ? ? DRM_ERROR("Unknown color depth\n");
> + ? ? ? ? ? ? ? dev_err(dev->dev, "Unknown color depth\n");
> ? ? ? ? ? ? ? ?ret = -EINVAL;
> ? ? ? ? ? ? ? ?goto pipe_set_base_exit;
> ? ? ? ?}
> ? ? ? ?REG_WRITE(dspcntr_reg, dspcntr);
>
> - ? ? ? DRM_DEBUG("Writing base %08lX %08lX %d %d\n", start, offset, x, y);
> ? ? ? ?if (0 /* FIXMEAC - check what PSB needs */) {
> ? ? ? ? ? ? ? ?REG_WRITE(dspbase, offset);
> ? ? ? ? ? ? ? ?REG_READ(dspbase);
> diff --git a/drivers/staging/gma500/mrst_lvds.c 
> b/drivers/staging/gma500/mrst_lvds.c
> index 22ea00e..aac80cc 100644
> --- a/drivers/staging/gma500/mrst_lvds.c
> +++ b/drivers/staging/gma500/mrst_lvds.c
> @@ -47,7 +47,6 @@ static void mrst_lvds_set_power(struct drm_device *dev,
> ?{
> ? ? ? ?u32 pp_status;
> ? ? ? 

Re: [PATCH 14/15] gma500: nuke the PSB debug stuff

[Alan Cox]

On Thu, 9 Jun 2011 03:10:03 +0200
Patrik Jakobsson patrik.r.jakobs...@gmail.com wrote:

 Hi Alan
 
 Just a thought. Shouldn't we use the DRM macros for printing debug info?

Linux has perfectly good printing functions and using them means we can
use dev_dbg() which supports things like nice runtime switching.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/15] gma500: nuke the PSB debug stuff

[Dave Airlie]

On Thu, 2011-06-09 at 09:11 +0100, Alan Cox wrote:
 On Thu, 9 Jun 2011 03:10:03 +0200
 Patrik Jakobsson patrik.r.jakobs...@gmail.com wrote:
 
  Hi Alan
  
  Just a thought. Shouldn't we use the DRM macros for printing debug info?
 
 Linux has perfectly good printing functions and using them means we can
 use dev_dbg() which supports things like nice runtime switching.

You mean like the drm debug functions runtime switching? that predated
the kernel ones and nobody ever ported :-)

Though if psb wants to be different to other drm drivers it can lead the
way, though it'll be a total nightmare for all the people who follow
documentation on how to debug drm drivers using drm.debug=1,2,4,8. for
various code paths.

Dave.


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/15] gma500: nuke the PSB debug stuff

[Patrik Jakobsson]

On Thu, Jun 9, 2011 at 12:36 PM, Dave Airlie wrote:
 On Thu, 2011-06-09 at 09:11 +0100, Alan Cox wrote:
 On Thu, 9 Jun 2011 03:10:03 +0200
 Patrik Jakobsson wrote:

  Hi Alan
 
  Just a thought. Shouldn't we use the DRM macros for printing debug info?

 Linux has perfectly good printing functions and using them means we can
 use dev_dbg() which supports things like nice runtime switching.

 You mean like the drm debug functions runtime switching? that predated
 the kernel ones and nobody ever ported :-)

 Though if psb wants to be different to other drm drivers it can lead the
 way, though it'll be a total nightmare for all the people who follow
 documentation on how to debug drm drivers using drm.debug=1,2,4,8. for
 various code paths.

Yes, my concern was about drm.debug and use of all the DRM portability stuff
(like using DRM_IRQ_HANDLED instead of IRQ_HANDLED, etc...)

The portability might not be important at this point but I just wanted to raise
the question so I know what is right / wrong.

Alan, I've been working on the output code but think I've reached a dead end.
I'm up to 2 lines of changes and it's just a big mess. I'm gonna go for
slowly fixing up the current code instead. I also got my hands on a laptop
with a gma500 so I can test that my changes doesn't break LVDS. Stay tuned.

Thanks
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/15] gma500: nuke the PSB debug stuff

[Alan Cox]

 Though if psb wants to be different to other drm drivers it can lead the
 way, though it'll be a total nightmare for all the people who follow
 documentation on how to debug drm drivers using drm.debug=1,2,4,8. for
 various code paths.

Actually it seems to work out nicely because you can debug DRM core
goings on and driver goings on separately. Also the driver ones can be
turned on/off on their own at runtime which is a godsend.

As you can imagine I've been testing the debugging a fair bit !

Alan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/15] gma500: nuke the PSB debug stuff

[Alan Cox]

 Yes, my concern was about drm.debug and use of all the DRM portability stuff
 (like using DRM_IRQ_HANDLED instead of IRQ_HANDLED, etc...)
 
 The portability might not be important at this point but I just wanted to 
 raise
 the question so I know what is right / wrong.

The gma500 driver uses a lot of direct Linux services rather than
disappearing into the weird world of drm_mm and the like so any
portability is going to be a bit of an illusion at best.

If someone ports it to another GPL licensed OS then I may have to
reconsider a bit, we shall see.
 
 Alan, I've been working on the output code but think I've reached a dead end.
 I'm up to 2 lines of changes and it's just a big mess. I'm gonna go for
 slowly fixing up the current code instead. I also got my hands on a laptop
 with a gma500 so I can test that my changes doesn't break LVDS. Stay tuned.

Will do. Got another chunk of patches to fire at Greg shortly and I've
now beaten pretty much all of it into passing CodingStyle so hopefully
any noise will settle down at that point.

Still not ready to leave staging, passing CodingStyle and being clean are
not quite the same thing in this case !
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 14/15] gma500: nuke the PSB debug stuff

[Alan Cox]

From: Alan Cox 

Lose all the PSB debug gunge. We can replace it with dev_dbg() like normal
drivers if and when we need debug on stuff.

Signed-off-by: Alan Cox 
---

 drivers/staging/gma500/mrst_crtc.c  |   23 ++
 drivers/staging/gma500/mrst_lvds.c  |   12 +--
 drivers/staging/gma500/psb_bl.c |8 --
 drivers/staging/gma500/psb_drv.c|   33 +++--
 drivers/staging/gma500/psb_drv.h|   96 +++---
 drivers/staging/gma500/psb_fb.c |   34 +
 drivers/staging/gma500/psb_gem.c|6 +-
 drivers/staging/gma500/psb_gtt.c|6 +-
 drivers/staging/gma500/psb_intel_bios.c |   17 +
 drivers/staging/gma500/psb_intel_display.c  |   99 +++
 drivers/staging/gma500/psb_intel_lvds.c |   53 ++
 drivers/staging/gma500/psb_intel_opregion.c |7 --
 drivers/staging/gma500/psb_intel_sdvo.c |   34 -
 drivers/staging/gma500/psb_irq.c|   33 ++---
 14 files changed, 87 insertions(+), 374 deletions(-)

diff --git a/drivers/staging/gma500/mrst_crtc.c 
b/drivers/staging/gma500/mrst_crtc.c
index fd97c80..fb9f2a2 100644
--- a/drivers/staging/gma500/mrst_crtc.c
+++ b/drivers/staging/gma500/mrst_crtc.c
@@ -103,7 +103,7 @@ static const struct mrst_limit_t *mrst_limit(struct 
drm_crtc *crtc)
}
} else {
limit = NULL;
-   PSB_DEBUG_ENTRY("mrst_limit Wrong display type.\n");
+   dev_err(dev->dev, "mrst_limit Wrong display type.\n");
}

return limit;
@@ -117,7 +117,7 @@ static void mrst_clock(int refclk, struct mrst_clock_t 
*clock)

 void mrstPrintPll(char *prefix, struct mrst_clock_t *clock)
 {
-   PSB_DEBUG_ENTRY("%s: dotclock = %d,  m = %d, p1 = %d.\n",
+   pr_debug("%s: dotclock = %d,  m = %d, p1 = %d.\n",
 prefix, clock->dot, clock->m, clock->p1);
 }

@@ -149,8 +149,7 @@ mrstFindBestPLL(struct drm_crtc *crtc, int target, int 
refclk,
}
}
}
-   DRM_DEBUG("mrstFindBestPLL err = %d.\n", err);
-
+   dev_dbg(crtc->dev->dev, "mrstFindBestPLL err = %d.\n", err);
return err != target;
 }

@@ -172,8 +171,6 @@ static void mrst_crtc_dpms(struct drm_crtc *crtc, int mode)
u32 temp;
bool enabled;

-   PSB_DEBUG_ENTRY("mode = %d, pipe = %d\n", mode, pipe);
-
if (!gma_power_begin(dev, true))
return;

@@ -320,8 +317,6 @@ static int mrst_crtc_mode_set(struct drm_crtc *crtc,
uint64_t scalingType = DRM_MODE_SCALE_FULLSCREEN;
struct drm_encoder *encoder;

-   PSB_DEBUG_ENTRY("pipe = 0x%x\n", pipe);
-
if (!gma_power_begin(dev, true))
return 0;

@@ -446,10 +441,9 @@ static int mrst_crtc_mode_set(struct drm_crtc *crtc,
ok = mrstFindBestPLL(crtc, adjusted_mode->clock, refclk, );

if (!ok) {
-   PSB_DEBUG_ENTRY(
-   "mrstFindBestPLL fail in mrst_crtc_mode_set.\n");
+   dev_dbg(dev->dev, "mrstFindBestPLL fail in 
mrst_crtc_mode_set.\n");
} else {
-   PSB_DEBUG_ENTRY("mrst_crtc_mode_set pixel clock = %d,"
+   dev_dbg(dev->dev, "mrst_crtc_mode_set pixel clock = %d,"
 "m = %x, p1 = %x.\n", clock.dot, clock.m,
 clock.p1);
}
@@ -540,11 +534,9 @@ int mrst_pipe_set_base(struct drm_crtc *crtc,
u32 dspcntr;
int ret = 0;

-   PSB_DEBUG_ENTRY("\n");
-
/* no fb bound */
if (!crtc->fb) {
-   DRM_DEBUG("No FB bound\n");
+   dev_dbg(dev->dev, "No FB bound\n");
return 0;
}

@@ -574,13 +566,12 @@ int mrst_pipe_set_base(struct drm_crtc *crtc,
dspcntr |= DISPPLANE_32BPP_NO_ALPHA;
break;
default:
-   DRM_ERROR("Unknown color depth\n");
+   dev_err(dev->dev, "Unknown color depth\n");
ret = -EINVAL;
goto pipe_set_base_exit;
}
REG_WRITE(dspcntr_reg, dspcntr);

-   DRM_DEBUG("Writing base %08lX %08lX %d %d\n", start, offset, x, y);
if (0 /* FIXMEAC - check what PSB needs */) {
REG_WRITE(dspbase, offset);
REG_READ(dspbase);
diff --git a/drivers/staging/gma500/mrst_lvds.c 
b/drivers/staging/gma500/mrst_lvds.c
index 22ea00e..aac80cc 100644
--- a/drivers/staging/gma500/mrst_lvds.c
+++ b/drivers/staging/gma500/mrst_lvds.c
@@ -47,7 +47,6 @@ static void mrst_lvds_set_power(struct drm_device *dev,
 {
u32 pp_status;
struct drm_psb_private *dev_priv = dev->dev_private;
-   PSB_DEBUG_ENTRY("\n");

if (!gma_power_begin(dev, true))
return;
@@ -77,8 +76,6 @@ static void mrst_lvds_dpms(struct drm_encoder *encoder, int 
mode)
struct drm_device *dev = encoder->dev;
struct psb_intel_output *output = 

[PATCH 14/15] gma500: nuke the PSB debug stuff

[Alan Cox]

From: Alan Cox a...@linux.intel.com

Lose all the PSB debug gunge. We can replace it with dev_dbg() like normal
drivers if and when we need debug on stuff.

Signed-off-by: Alan Cox a...@linux.intel.com
---

 drivers/staging/gma500/mrst_crtc.c  |   23 ++
 drivers/staging/gma500/mrst_lvds.c  |   12 +--
 drivers/staging/gma500/psb_bl.c |8 --
 drivers/staging/gma500/psb_drv.c|   33 +++--
 drivers/staging/gma500/psb_drv.h|   96 +++---
 drivers/staging/gma500/psb_fb.c |   34 +
 drivers/staging/gma500/psb_gem.c|6 +-
 drivers/staging/gma500/psb_gtt.c|6 +-
 drivers/staging/gma500/psb_intel_bios.c |   17 +
 drivers/staging/gma500/psb_intel_display.c  |   99 +++
 drivers/staging/gma500/psb_intel_lvds.c |   53 ++
 drivers/staging/gma500/psb_intel_opregion.c |7 --
 drivers/staging/gma500/psb_intel_sdvo.c |   34 -
 drivers/staging/gma500/psb_irq.c|   33 ++---
 14 files changed, 87 insertions(+), 374 deletions(-)

diff --git a/drivers/staging/gma500/mrst_crtc.c 
b/drivers/staging/gma500/mrst_crtc.c
index fd97c80..fb9f2a2 100644
--- a/drivers/staging/gma500/mrst_crtc.c
+++ b/drivers/staging/gma500/mrst_crtc.c
@@ -103,7 +103,7 @@ static const struct mrst_limit_t *mrst_limit(struct 
drm_crtc *crtc)
}
} else {
limit = NULL;
-   PSB_DEBUG_ENTRY(mrst_limit Wrong display type.\n);
+   dev_err(dev-dev, mrst_limit Wrong display type.\n);
}
 
return limit;
@@ -117,7 +117,7 @@ static void mrst_clock(int refclk, struct mrst_clock_t 
*clock)
 
 void mrstPrintPll(char *prefix, struct mrst_clock_t *clock)
 {
-   PSB_DEBUG_ENTRY(%s: dotclock = %d,  m = %d, p1 = %d.\n,
+   pr_debug(%s: dotclock = %d,  m = %d, p1 = %d.\n,
 prefix, clock-dot, clock-m, clock-p1);
 }
 
@@ -149,8 +149,7 @@ mrstFindBestPLL(struct drm_crtc *crtc, int target, int 
refclk,
}
}
}
-   DRM_DEBUG(mrstFindBestPLL err = %d.\n, err);
-
+   dev_dbg(crtc-dev-dev, mrstFindBestPLL err = %d.\n, err);
return err != target;
 }
 
@@ -172,8 +171,6 @@ static void mrst_crtc_dpms(struct drm_crtc *crtc, int mode)
u32 temp;
bool enabled;
 
-   PSB_DEBUG_ENTRY(mode = %d, pipe = %d\n, mode, pipe);
-
if (!gma_power_begin(dev, true))
return;
 
@@ -320,8 +317,6 @@ static int mrst_crtc_mode_set(struct drm_crtc *crtc,
uint64_t scalingType = DRM_MODE_SCALE_FULLSCREEN;
struct drm_encoder *encoder;
 
-   PSB_DEBUG_ENTRY(pipe = 0x%x\n, pipe);
-
if (!gma_power_begin(dev, true))
return 0;
 
@@ -446,10 +441,9 @@ static int mrst_crtc_mode_set(struct drm_crtc *crtc,
ok = mrstFindBestPLL(crtc, adjusted_mode-clock, refclk, clock);
 
if (!ok) {
-   PSB_DEBUG_ENTRY(
-   mrstFindBestPLL fail in mrst_crtc_mode_set.\n);
+   dev_dbg(dev-dev, mrstFindBestPLL fail in 
mrst_crtc_mode_set.\n);
} else {
-   PSB_DEBUG_ENTRY(mrst_crtc_mode_set pixel clock = %d,
+   dev_dbg(dev-dev, mrst_crtc_mode_set pixel clock = %d,
 m = %x, p1 = %x.\n, clock.dot, clock.m,
 clock.p1);
}
@@ -540,11 +534,9 @@ int mrst_pipe_set_base(struct drm_crtc *crtc,
u32 dspcntr;
int ret = 0;
 
-   PSB_DEBUG_ENTRY(\n);
-
/* no fb bound */
if (!crtc-fb) {
-   DRM_DEBUG(No FB bound\n);
+   dev_dbg(dev-dev, No FB bound\n);
return 0;
}
 
@@ -574,13 +566,12 @@ int mrst_pipe_set_base(struct drm_crtc *crtc,
dspcntr |= DISPPLANE_32BPP_NO_ALPHA;
break;
default:
-   DRM_ERROR(Unknown color depth\n);
+   dev_err(dev-dev, Unknown color depth\n);
ret = -EINVAL;
goto pipe_set_base_exit;
}
REG_WRITE(dspcntr_reg, dspcntr);
 
-   DRM_DEBUG(Writing base %08lX %08lX %d %d\n, start, offset, x, y);
if (0 /* FIXMEAC - check what PSB needs */) {
REG_WRITE(dspbase, offset);
REG_READ(dspbase);
diff --git a/drivers/staging/gma500/mrst_lvds.c 
b/drivers/staging/gma500/mrst_lvds.c
index 22ea00e..aac80cc 100644
--- a/drivers/staging/gma500/mrst_lvds.c
+++ b/drivers/staging/gma500/mrst_lvds.c
@@ -47,7 +47,6 @@ static void mrst_lvds_set_power(struct drm_device *dev,
 {
u32 pp_status;
struct drm_psb_private *dev_priv = dev-dev_private;
-   PSB_DEBUG_ENTRY(\n);
 
if (!gma_power_begin(dev, true))
return;
@@ -77,8 +76,6 @@ static void mrst_lvds_dpms(struct drm_encoder *encoder, int 
mode)
struct drm_device *dev = encoder-dev;
struct psb_intel_output *output =