sis_malloc / sis_free

sis_malloc / sis_free

[Jean Paul Sartre]

	Hello all.

	I was grepping through the SIS malloc/free code and I saw DRM
'shares' code with the fb code. What if I have SIS framebuffer disabled
and SIS DRM code enabled? In 2.4.18-rc2, the SIS DRM code does not compile
from the lack of sis_malloc and sis_free function.

	I would suggest 'duplicating' this code (yes, I *do* hate
duplicating codes) or making that memory allocation code *really* shared
between both modules (or we won't be able to successfully compile it,
since the DRM code is on drivers/char/drm and the FB code is on
drivers/video/sis/sis_main.c).

	If the suggestion of 'duplicating' code (argh) is reasonable
enough (for they are really different codes) I can work and submit a
patch. If not, I do not know how to 'share' both codes without tweaking
through the makefiles and dependencies (which should also compile extra
code that is not needed in the DRM code).

	Regards,
	Cesar Suga <sartre@linuxbr.com>

Re: sis_malloc / sis_free

[Jean Paul Sartre]

On Wed, 20 Feb 2002, Alan Cox wrote:

> Date: Wed, 20 Feb 2002 01:51:15 +0000 (GMT)
> From: Alan Cox <alan@lxorguk.ukuu.org.uk>
> To: Jean Paul Sartre <sartre@linuxbr.com>
> Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
> Subject: Re: sis_malloc / sis_free
>
> > 'shares' code with the fb code. What if I have SIS framebuffer disabled
> > and SIS DRM code enabled? In 2.4.18-rc2, the SIS DRM code does not compile
> > from the lack of sis_malloc and sis_free function.
>
> SIS DRM requires the SIS frame buffer.

	But this is a 'semantic' mode of requiring. The 'requirement' does
not apply in the source, as I saw (or it'd compile normally with the DRM
code, and FB code gives sis_malloc and sis_free (try grepping sis_malloc
in drivers/char/sis, please)).

Re: sis_malloc / sis_free

[Jean Paul Sartre]

On Wed, 20 Feb 2002, Alan Cox wrote:

> > 'shares' code with the fb code. What if I have SIS framebuffer disabled
> > and SIS DRM code enabled? In 2.4.18-rc2, the SIS DRM code does not compile
> > from the lack of sis_malloc and sis_free function.
>
> SIS DRM requires the SIS frame buffer.

	These are the compiling errors in make bzImage:

drivers/char/drm/drm.o: In function `sis_fb_alloc':
drivers/char/drm/drm.o(.text+0x451c6): undefined reference to `sis_malloc'
drivers/char/drm/drm.o(.text+0x4520d): undefined reference to `sis_free'
drivers/char/drm/drm.o: In function `sis_fb_free':
drivers/char/drm/drm.o(.text+0x45352): undefined reference to `sis_free'
drivers/char/drm/drm.o: In function `sis_final_context':
drivers/char/drm/drm.o(.text+0x45806): undefined reference to `sis_free'
make: *** [vmlinux] Error 1

	(so, DRM code does not take sis_malloc nor sis_free functions from
the FB code, that I expected to see)

	[]s,
	Cesar Suga <sartre@linuxbr.com>

Re: sis_malloc / sis_free

[Christoph Pittracher]

Hello!

On Wednesday 20 February 2002 02:26, Jean Paul Sartre wrote:
> 	I was grepping through the SIS malloc/free code and I saw DRM
> 'shares' code with the fb code. What if I have SIS framebuffer
> disabled and SIS DRM code enabled? In 2.4.18-rc2, the SIS DRM code
> does not compile from the lack of sis_malloc and sis_free function.

Yes, the sisfb/drm code has some design lacks.
Another lack is the necessary memory offset between framebuffer/drm and 
the X driver (see http://www.webit.at/~twinny/linuxsis630.shtml for 
details).

> 	I would suggest 'duplicating' this code (yes, I *do* hate
> duplicating codes) or making that memory allocation code *really*
> shared between both modules (or we won't be able to successfully
> compile it, since the DRM code is on drivers/char/drm and the FB code
> is on drivers/video/sis/sis_main.c).

I don't think that it would be a problem to duplicate the code. the 
sis_malloc / sis_free functions seems quite stable. I don't think that 
there will be big updates for this code.

> 	If the suggestion of 'duplicating' code (argh) is reasonable
> enough (for they are really different codes) I can work and submit a
> patch.

Thomas Winischhofer <tw@webit.com> is working on that SiS stuff for 
about 2 months. I think it would be best if you contact him and ask 
what he thinks about that. I know that he said it would be a good idea 
to seperate the sisfb and sis_drm code but he doesn't have enough time 
to do it...

regards,
Christoph

Re: sis_malloc / sis_free

[Alan Cox]

> 'shares' code with the fb code. What if I have SIS framebuffer disabled
> and SIS DRM code enabled? In 2.4.18-rc2, the SIS DRM code does not compile
> from the lack of sis_malloc and sis_free function.

SIS DRM requires the SIS frame buffer.

Re: sis_malloc / sis_free

[Jean Paul Sartre]

On Wed, 20 Feb 2002, Alan Cox wrote:

> > > SIS DRM requires the SIS frame buffer.
> >
> > 	But this is a 'semantic' mode of requiring. The 'requirement' does
> > not apply in the source, as I saw (or it'd compile normally with the DRM
> > code, and FB code gives sis_malloc and sis_free (try grepping sis_malloc
>
> Compiles fine for me. 2.4.18rc2-ac1 - and the SiS DRM works too tho on
> an old 6326 its not rocket speed.

	Okay, will try 2.4.18rc2-ac1. Ahn, If CONFIG_FB_SIS is set,
CONFIG_DRM_SIS still appears in the menuconfig option. Is it okay, as DRM
requires FB support?

	Regards,
	Cesar Suga <sartre@linuxbr.com>

Re: sis_malloc / sis_free

[Alan Cox]

> > SIS DRM requires the SIS frame buffer.
> 
> 	But this is a 'semantic' mode of requiring. The 'requirement' does
> not apply in the source, as I saw (or it'd compile normally with the DRM
> code, and FB code gives sis_malloc and sis_free (try grepping sis_malloc

Compiles fine for me. 2.4.18rc2-ac1 - and the SiS DRM works too tho on
an old 6326 its not rocket speed.

Alan

Re: sis_malloc / sis_free

[Jean Paul Sartre]

On Wed, 20 Feb 2002, Christoph Pittracher wrote:

> Hello!

> Yes, the sisfb/drm code has some design lacks.
> Another lack is the necessary memory offset between framebuffer/drm and
> the X driver (see http://www.webit.at/~twinny/linuxsis630.shtml for
> details).

	Oh, okay. Should that memory offset be a source of processor time
waste for allocating memory when switching through FB and X, for an
example? If so, sharing the code (instead of duplicating it) is the ideal
approach.

> I don't think that it would be a problem to duplicate the code. the
> sis_malloc / sis_free functions seems quite stable. I don't think that
> there will be big updates for this code.

	Hmm, but as you stated, I do not think code duplication should be
the best approach.

> Thomas Winischhofer <tw@webit.com> is working on that SiS stuff for
> about 2 months. I think it would be best if you contact him and ask
> what he thinks about that. I know that he said it would be a good idea
> to seperate the sisfb and sis_drm code but he doesn't have enough time
> to do it...

	If I have such time, I'll contact him. But for the moment, if the
code does not compile (still with 2.4.18-rc2-ac1) I'll duplicate the code
to get it working until a better solution raises.

	Regards,
	Cesar Suga <sartre@linuxbr.com>

Re: sis_malloc / sis_free

[Alan Cox]

> 	Okay, will try 2.4.18rc2-ac1. Ahn, If CONFIG_FB_SIS is set,
> CONFIG_DRM_SIS still appears in the menuconfig option. Is it okay, as DRM
> requires FB support?

You need both DRM_SIS and FB_SIS set. At the moment thats not properly
enforced in the CML1

Re: sis_malloc / sis_free

[James Simmons]

> 	I would suggest 'duplicating' this code (yes, I *do* hate
> duplicating codes) or making that memory allocation code *really* shared
> between both modules (or we won't be able to successfully compile it,
> since the DRM code is on drivers/char/drm and the FB code is on
> drivers/video/sis/sis_main.c).

I agree it is strange to have the DRM code in drivers/char. It should be
in drivers/video. I guess in time. As for DRI and FB sharing code. You
will see this happen more and more in the future. Eventually I like to
merge both interfaces into one universal interface, but before I do that
the whole fbdev layer needs to be cleaned up which I'm doing now. 

Re: sis_malloc / sis_free

[Alan Cox]

> will see this happen more and more in the future. Eventually I like to
> merge both interfaces into one universal interface, but before I do that
> the whole fbdev layer needs to be cleaned up which I'm doing now. 

For 2.5 now is the time to move DRM if you want to.  I agree it makes sense
to shove it in video/drm

Re: sis_malloc / sis_free

[Cesar Suga]

On Wed, 20 Feb 2002, Thomas Winischhofer wrote:

> >         (hm, you still lose CPU time when deallocating memory when
> > switching back to console (fb) and switching to X, it they are not really
> > shared, as with copy_from_user() and copy_to_user()...)

> AFAIK Alloc/Dealloc is not done when switching back and forth to/from
> VT. Why do you assume this? (Remember: We are talking about video RAM
> here)

# From sis_drm.h:
typedef struct {
  int context;
  unsigned int offset;
  unsigned int size;
  unsigned int free;
} drm_sis_mem_t;

	Ahm, sorry. eh. Well, as I said. I'm not too familiarized and I
thought the source disposition is not too obvious for me to understand.
(no, I am no C expert, so..)

> >         Yes, with those DRAM detection routines.
> And these are HUGE (check init.c - half that file deals with that)

	Hm, they actually convinced me that, for me, the better way to get
those things working with ease of expansion is how you stated (and I
asked, for the 'code split'), the module for video memory management.

> >         With the sisfb_poh_allocate() (which I by found itself the
> > complicated part), the display struct, the *huge* heap initializer which
> > depends on all the rest, eheh, the duplication would cost too much.

> Exactly :)

> >         Ahn, then you are speaking of moving SIS common heap functions,
> > command queue determination, queue area size and whatsoever common outside
> > to a common area? This is really a good approach. :)

> I am actually thinking of this - but

> 1) I haven't developed a new concept yet (which has to satisfy
> framebuffer, DRM and X), and

> 2) it's not on the top of my list. There are by far more important
> things to be done first (see below).

	I think a video memory management module would be *very*
important for new modules both for FB and DRM. So the modules would care
about the command queues, setting modes, etc. - nothing related to video
memory.

> >         In this current way, yes. But one can compile, let's say, the SIS
> > FB support as a module and the SIS DRM support as a builtin, which would
> > break the driver.

> I'd never compile the DRM "module" into the kernel - what for...? (But
> you're right, of course. That combination will cause trouble.)

	One should be, at least, adviced in the Configure.help whilst
configuring his/her kernel.

> >         Well, now that I've got into the code, neither I do. :)

> :)))

> >         Any idea on when you'll split allocation codes from the FB code?
> > (Or will you work on doing just one common module that'll give support for
> > both FB and DRM?)

> I figure leaving these routines inside the fb code is the best choice
> for now. I don't see any harm with that at the moment. (Apart from the
> compilation combination you mentioned, of course.)

> Or does anybody want a third module for just doing video memory
> management...? Yes? You, there in the second row? Please stand up.... :)

	Yes, eh. Hope someone can help (I do not know yet how to manage
that).

> I will, however, deal closer with that issue as soon as I got the X
> driver somewhat fixed - and this might take a few weeks (communication
> with SiS is kinda, let's say, time-taking and they aren't really
> generous with information on their devices). And aside from this - I got
> my attorney's exam on March 6 which will keep me from spending as much
> time as I'd like on SiS drivers until then... :(

	Hm. And I'll only be able to contribute in, say, documentation.

> What has the X driver to do with this, you ask? Well: The X driver now
> (after a hard struggle :) ) relates to sisfb by using the exact same
> code basis for the core functions. For assumingly understood
> maintainance reasons, I want to make the core functions re-usable for
> both of the drivers. This is partly finished but not complete yet. (If
> you want to test the latest drivers [both sisfb and X] and some more
> information on SiS VGA devices see
> http://www.webit.com/tw/linuxsis630.shtml)

> Another - rather important - issue is that SiS very soon releases a new
> chip (SiS 650) and a new video bridge (302B/301LV/302LV) and support for
> these should also be included in BOTH drivers. I am currently working on
> this.

	Hope you code that support with the possibility of splitting
functions between modules ;)

> For now, I believe patching the config dependencies is the best thing to
> do. I'd also patch the documentation so that folks don't compile DRM
> into the kernel (which is a waste of resources, IMHO)

	Yes, at least for now.

	Regards,
	Cesar Suga <sartre@linuxbr.com>

Re: sis_malloc / sis_free

[Thomas Winischhofer]

Jean Paul Sartre wrote:
> > This has nothing whatsoever with processor time to do. It's to keep
> > DRM/DRI from overwriting X's screen and off-screen buffers.
> 
>         (hm, you still lose CPU time when deallocating memory when
> switching back to console (fb) and switching to X, it they are not really
> shared, as with copy_from_user() and copy_to_user()...)

AFAIK Alloc/Dealloc is not done when switching back and forth to/from
VT. Why do you assume this? (Remember: We are talking about video RAM
here)

> > It's not done by simply duplicating the sis_malloc/sis_free functions as
> > these also require
> >
> > - detecting the type and size of the memory (of many different SiS
> > chipsets),
> 
>         Yes, with those DRAM detection routines.

And these are HUGE (check init.c - half that file deals with that)

>         With the sisfb_poh_allocate() (which I by found itself the
> complicated part), the display struct, the *huge* heap initializer which
> depends on all the rest, eheh, the duplication would cost too much.

Exactly :)

>         Ahn, then you are speaking of moving SIS common heap functions,
> command queue determination, queue area size and whatsoever common outside
> to a common area? This is really a good approach. :)

I am actually thinking of this - but 

1) I haven't developed a new concept yet (which has to satisfy
framebuffer, DRM and X), and

2) it's not on the top of my list. There are by far more important
things to be done first (see below). 

>         In this current way, yes. But one can compile, let's say, the SIS
> FB support as a module and the SIS DRM support as a builtin, which would
> break the driver.

I'd never compile the DRM "module" into the kernel - what for...? (But
you're right, of course. That combination will cause trouble.)

>         Well, now that I've got into the code, neither I do. :)

:)))

>         Any idea on when you'll split allocation codes from the FB code?
> (Or will you work on doing just one common module that'll give support for
> both FB and DRM?)

I figure leaving these routines inside the fb code is the best choice
for now. I don't see any harm with that at the moment. (Apart from the
compilation combination you mentioned, of course.)

Or does anybody want a third module for just doing video memory
management...? Yes? You, there in the second row? Please stand up.... :)

I will, however, deal closer with that issue as soon as I got the X
driver somewhat fixed - and this might take a few weeks (communication
with SiS is kinda, let's say, time-taking and they aren't really
generous with information on their devices). And aside from this - I got
my attorney's exam on March 6 which will keep me from spending as much
time as I'd like on SiS drivers until then... :(

What has the X driver to do with this, you ask? Well: The X driver now
(after a hard struggle :) ) relates to sisfb by using the exact same
code basis for the core functions. For assumingly understood
maintainance reasons, I want to make the core functions re-usable for
both of the drivers. This is partly finished but not complete yet. (If
you want to test the latest drivers [both sisfb and X] and some more
information on SiS VGA devices see
http://www.webit.com/tw/linuxsis630.shtml)

Another - rather important - issue is that SiS very soon releases a new
chip (SiS 650) and a new video bridge (302B/301LV/302LV) and support for
these should also be included in BOTH drivers. I am currently working on
this. 

For now, I believe patching the config dependencies is the best thing to
do. I'd also patch the documentation so that folks don't compile DRM
into the kernel (which is a waste of resources, IMHO)

Thomas


-- 
Thomas Winischhofer
Vienna/Austria
mailto:tw@webit.com                  *** http://www.webit.com/tw

Re: sis_malloc / sis_free

[Jean Paul Sartre]

On Wed, 20 Feb 2002, Thomas Winischhofer wrote:

> > > Yes, the sisfb/drm code has some design lacks.

> Well, well. I have by now come to the conclusion that this isn't really
> the case...

	Hm, now it is clear, thanks for the explanation. :)

> > > Another lack is the necessary memory offset between framebuffer/drm and
> > > the X driver (see http://www.webit.at/~twinny/linuxsis630.shtml for
> > > details).

> > Oh, okay. Should that memory offset be a source of processor time
> > waste for allocating memory when switching through FB and X, for an
> > example? If so, sharing the code (instead of duplicating it) is the ideal
> > approach.

> This has nothing whatsoever with processor time to do. It's to keep
> DRM/DRI from overwriting X's screen and off-screen buffers.

	(hm, you still lose CPU time when deallocating memory when
switching back to console (fb) and switching to X, it they are not really
shared, as with copy_from_user() and copy_to_user()...)

> > > I don't think that it would be a problem to duplicate the code. the
> > > sis_malloc / sis_free functions seems quite stable. I don't think that
> > > there will be big updates for this code.

> > Hmm, but as you stated, I do not think code duplication should be
> > the best approach.

> No, it would not. You've obviously not understood the problems.
>
> It's not done by simply duplicating the sis_malloc/sis_free functions as
> these also require
>
> - detecting the type and size of the memory (of many different SiS
> chipsets),

	Yes, with those DRAM detection routines.

> - initializing a heap (based on which you later allocate/free the
> memory) taking into account a possibe TurboQueue, a possible AGP command
> queue and the HWCursor memory area.

	With the sisfb_poh_allocate() (which I by found itself the
complicated part), the display struct, the *huge* heap initializer which
depends on all the rest, eheh, the duplication would cost too much.

> Believe me: I know the code by heart right now, and moving the memory
> management out of the framebuffer driver will require HUGE parts of the
> code copied (not speaking about maintainance issues).

> Finally: I intend to implement (real) 2D accelleration in the
> framebuffer driver. Thus, I will need a >common< memory management (ie.
> ONE SINGLE heap) for both the framebuffer driver as well as the X
> driver. Otherwise these two will again overwrite each others video
> memory, very possibly leading into problems with the accellerator
> engines (which use parts of the video RAM for buffering).

	Ahn, then you are speaking of moving SIS common heap functions,
command queue determination, queue area size and whatsoever common outside
to a common area? This is really a good approach. :)

> This can only be avoided by keeping the memory management inside the
> framebuffer driver. I don't think it's good to be forced to load the DRM
> module (if that one then contains memory management) for only being able
> to use the framebuffer driver...

	In this current way, yes. But one can compile, let's say, the SIS
FB support as a module and the SIS DRM support as a builtin, which would
break the driver. I threw a patch in the list with a brief description for
Documentation/Configure.help (which is lacking) and the correct dependency
checking for Config.in (that will make FB and DRM support be both modules
or builtin).

> It's not a time issue. As said, the concept isn't that bad for future
> development. I consider it wise enough to keep the drivers separated, as
> this also allows separate usage (framebuffer only, X only) without
> memory clashes.

> > If I have such time, I'll contact him. But for the moment, if the
> > code does not compile (still with 2.4.18-rc2-ac1) I'll duplicate the code
> > to get it working until a better solution raises.

> You don't have to do that. If you don't like a graphical console, simple
> start the framebuffer driver with "mode=none" (or as a kernel parameter
> "mode:none"). In this case, sisfb will only initialize the memory but
> leave the console alone.

> I don't recommend changing anything that big in the framebuffer driver
> at the moment. I am currently in close contact with SiS who help me
> fixing the (still) remaining problems with it - based on the existing
> code. In the very only case you'd like to reinclude a number of changes
> for a number of times - well, go ahead :)

	Well, now that I've got into the code, neither I do. :)

	Any idea on when you'll split allocation codes from the FB code?
(Or will you work on doing just one common module that'll give support for
both FB and DRM?)

	Regards,
	Cesar Suga <sartre@linuxbr.com>

Re: sis_malloc / sis_free

[Thomas Winischhofer]

On Wed, 20 Feb 2002, Christoph Pittracher wrote: 
> > Hello! 

Hi everyone,

> > Yes, the sisfb/drm code has some design lacks.

Well, well. I have by now come to the conclusion that this isn't really
the case...

> > Another lack is the necessary memory offset between framebuffer/drm and
> > the X driver (see http://www.webit.at/~twinny/linuxsis630.shtml for
> > details). 
>
> Oh, okay. Should that memory offset be a source of processor time
> waste for allocating memory when switching through FB and X, for an
> example? If so, sharing the code (instead of duplicating it) is the ideal
> approach. 

This has nothing whatsoever with processor time to do. It's to keep
DRM/DRI from overwriting X's screen and off-screen buffers.

> > I don't think that it would be a problem to duplicate the code. the
> > sis_malloc / sis_free functions seems quite stable. I don't think that
> > there will be big updates for this code. 
>
> Hmm, but as you stated, I do not think code duplication should be
> the best approach. 

No, it would not. You've obviously not understood the problems.

It's not done by simply duplicating the sis_malloc/sis_free functions as
these also require

- detecting the type and size of the memory (of many different SiS
chipsets),

- initializing a heap (based on which you later allocate/free the
memory) taking into account a possibe TurboQueue, a possible AGP command
queue and the HWCursor memory area.

Believe me: I know the code by heart right now, and moving the memory
management out of the framebuffer driver will require HUGE parts of the
code copied (not speaking about maintainance issues).

Finally: I intend to implement (real) 2D accelleration in the
framebuffer driver. Thus, I will need a >common< memory management (ie.
ONE SINGLE heap) for both the framebuffer driver as well as the X
driver. Otherwise these two will again overwrite each others video
memory, very possibly leading into problems with the accellerator
engines (which use parts of the video RAM for buffering). 

This can only be avoided by keeping the memory management inside the
framebuffer driver. I don't think it's good to be forced to load the DRM
module (if that one then contains memory management) for only being able
to use the framebuffer driver...

> > Thomas Winischhofer <tw@webit.com> is working on that SiS stuff for
> > about 2 months. I think it would be best if you contact him and ask
> > what he thinks about that. I know that he said it would be a good idea
> > to seperate the sisfb and sis_drm code but he doesn't have enough time
> > to do it... 

It's not a time issue. As said, the concept isn't that bad for future
development. I consider it wise enough to keep the drivers separated, as
this also allows separate usage (framebuffer only, X only) without
memory clashes.

> If I have such time, I'll contact him. But for the moment, if the
> code does not compile (still with 2.4.18-rc2-ac1) I'll duplicate the code
> to get it working until a better solution raises.

You don't have to do that. If you don't like a graphical console, simple
start the framebuffer driver with "mode=none" (or as a kernel parameter
"mode:none"). In this case, sisfb will only initialize the memory but
leave the console alone.

I don't recommend changing anything that big in the framebuffer driver
at the moment. I am currently in close contact with SiS who help me
fixing the (still) remaining problems with it - based on the existing
code. In the very only case you'd like to reinclude a number of changes
for a number of times - well, go ahead :)

Thomas

-- 
Thomas Winischhofer
Vienna/Austria
mailto:tw@webit.com                  *** http://www.webit.com/tw