Re: R300 cleanup questions
On 5/8/07, Christoph Brill <[EMAIL PROTECTED]> wrote:
> I reviewed the cleanup done by Olliver McFadden and had the following
> questions:
>
> -int r300_get_num_verts(r300ContextPtr rmesa, int num_verts, int prim)
> +static int r300NumVerts(r300ContextPtr rmesa, int num_verts, int prim)
>
> Is it necessary/usefull that the function is static?
I think it's better to have static function, i am thinking of symbol export and
other things like that.
>
> -/* Immediate implementation has been removed from CVS. */
> -
> -/* vertex buffer implementation */
> -
> -static void inline fire_EB(r300ContextPtr rmesa, unsigned long addr
> +static void inline r300FireEB(r300ContextPtr rmesa, unsigned long addr
>
> Why move all the comments to the head of the file. IMO the method should
> have a doxygen comment that states it is the vertex buffer
> implementation of fire_EB, right?
>
>
> -if (num_verts > 65535) { /* not implemented yet */
> +if (num_verts > 65535) {
>
> Comments like this should be kept. Otherwise it looks like a hardware
> limitation while the limitation can be worked around or the limitation
> does not exist.
>
>
> Last but not least is
> r300_foo_bar
> preferred or
> r300FooBar
> Which is the one mesa uses?
We can use the one we like, i prefer r300_foo_bar over r300FooBar which
i dislike but the choice is up to the first person who do big cleanup :) and
we do not have to conform to mesa coding style for driver but use the one
we like the more.
best,
Jerome Glisse
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: drm-ttm-cleanup-branch
Dave Airlie wrote: >> I'll try it out as soon as there is time. > > I've just tested glxgears and a few mesa tests on it and it seems to > be working fine > > We should probably think about pulling this over into the DRM sooner > rather than later, there are also some changes to the DDX > i830_driver.c compat code to deal with... > > Yup. I've attached a patch (against the cleanup branch) with things I think may be needed. 1) 64-bit reordering. 64-bit scalars, structs and unions should probably be 64-bit aligned in parent structs. I had to insert padding in two cases, but this probably needs to be double-checked. 2) A "magic" member in the init ioctl. Checking this allows for verbose and friendly failure of code that uses the old interface. 3) Init major / minor versioning of the memory manager interface in case we need changes in the future. 4) expand_pads are 64-bit following Jesse Barnes recommendations. 5) The info_req carries a fence class for validation for a particular command submission mechanism. 6) The info_rep arg carries tile_strides and tile_info. The argument tile_strides is ((actual_tile_stride) << 16) | (desired_tile_stride)) The argument tile_info is driver-specific. (Could be tile width, x-major, y-major etc.) Finally, should we perhaps allow for 64-bit buffer object flags / mask at this point? I haven't done any user-space or kernel coding for this yet. Just want to know what you think. /Thomas - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: drm-ttm-cleanup-branch
...And the patch.
Thomas Hellström wrote:
Dave Airlie wrote:
I'll try it out as soon as there is time.
I've just tested glxgears and a few mesa tests on it and it seems to
be working fine
We should probably think about pulling this over into the DRM sooner
rather than later, there are also some changes to the DDX
i830_driver.c compat code to deal with...
Yup. I've attached a patch (against the cleanup branch) with things I
think may be needed.
1) 64-bit reordering. 64-bit scalars, structs and unions should
probably be 64-bit aligned in parent structs. I had to insert padding
in two cases, but this probably needs to be double-checked.
2) A "magic" member in the init ioctl. Checking this allows for
verbose and friendly failure of code that uses the old interface.
3) Init major / minor versioning of the memory manager interface in
case we need changes in the future.
4) expand_pads are 64-bit following Jesse Barnes recommendations.
5) The info_req carries a fence class for validation for a particular
command submission mechanism.
6) The info_rep arg carries tile_strides and tile_info.
The argument tile_strides is ((actual_tile_stride) << 16) |
(desired_tile_stride))
The argument tile_info is driver-specific. (Could be tile width,
x-major, y-major etc.)
Finally, should we perhaps allow for 64-bit buffer object flags / mask
at this point?
I haven't done any user-space or kernel coding for this yet.
Just want to know what you think.
/Thomas
>From 2b7d5bff5a6aeca08ccf1931828eeca74935bad5 Mon Sep 17 00:00:00 2001
From: Thomas Hellstrom
Date: Wed, 9 May 2007 09:56:06 +0200
Subject: [PATCH] New members and 64-bit reordering.
Signed-off-by: Thomas Hellstrom
---
shared-core/drm.h | 46 +-
1 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/shared-core/drm.h b/shared-core/drm.h
index ae308be..5cb2545 100644
--- a/shared-core/drm.h
+++ b/shared-core/drm.h
@@ -671,12 +671,13 @@ #define DRM_FENCE_MASK_DRIVER
#define DRM_FENCE_TYPE_EXE 0x0001
typedef struct drm_fence_arg {
- unsigned handle;
- int class;
- unsigned type;
- unsigned flags;
- unsigned signaled;
- unsigned expand_pad[4]; /*Future expansion */
+ unsigned int handle;
+ unsigned int class;
+ unsigned int type;
+ unsigned int flags;
+ unsigned int signaled;
+ unsigned int pad_64;
+ drm_u64_t expand_pad[3]; /*Future expansion */
} drm_fence_arg_t;
/* Buffer permissions, referring to how the GPU uses the buffers.
@@ -774,6 +775,10 @@ #define DRM_BO_HINT_DONT_FENCE 0x00
#define DRM_BO_HINT_WAIT_LAZY 0x0008
#define DRM_BO_HINT_ALLOW_UNFENCED_MAP 0x0010
+#define DRM_BO_INIT_MAGIC 0xfe769812
+#define DRM_BO_INIT_MAJOR 0
+#define DRM_BO_INIT_MINOR 1
+
typedef enum {
drm_bo_type_dc,
@@ -786,25 +791,26 @@ struct drm_bo_info_req {
unsigned int handle;
unsigned int mask;
unsigned int hint;
+ unsigned int fence_class;
};
struct drm_bo_create_req {
unsigned int mask;
unsigned int hint;
- unsigned page_alignment;
- drm_u64_t size;
+ unsigned int page_alignment;
drm_bo_type_t type;
+ drm_u64_t size;
drm_u64_t buffer_start;
};
struct drm_bo_op_req {
- struct drm_bo_info_req bo_req;
- unsigned int arg_handle;
enum {
drm_bo_validate,
drm_bo_fence,
drm_bo_ref_fence,
} op;
+ unsigned int arg_handle;
+ struct drm_bo_info_req bo_req;
};
/*
@@ -816,20 +822,22 @@ #define DRM_BO_REP_BUSY 0x0001
struct drm_bo_info_rep {
unsigned int handle;
unsigned int flags;
- drm_u64_t size;
- drm_u64_t offset;
- drm_u64_t arg_handle;
unsigned int mask;
- drm_u64_t buffer_start;
unsigned int fence_flags;
unsigned int rep_flags;
unsigned int page_alignment;
- unsigned int expand_pad[4]; /*Future expansion */
+ unsigned int tile_strides;
+ unsigned int tile_info;
+ drm_u64_t size;
+ drm_u64_t offset;
+ drm_u64_t arg_handle;
+ drm_u64_t buffer_start;
+ drm_u64_t expand_pad[4]; /*Future expansion */
};
struct drm_bo_arg_rep {
- int ret;
struct drm_bo_info_rep bo_info;
+ int ret;
};
struct drm_bo_create_arg {
@@ -859,6 +867,7 @@ struct drm_bo_map_wait_idle_arg {
struct drm_bo_op_arg {
int handled;
+ unsigned int pad_64;
drm_u64_t next;
union {
struct drm_bo_op_req req;
@@ -882,9 +891,12 @@ typedef struct drm_mm_type_arg {
} drm_mm_type_arg_t;
typedef struct drm_mm_init_arg {
+ unsigned int magic;
+ unsigned int major;
+ unsigned int minor;
+ unsigned int mem_type;
drm_u64_t p_offset;
drm_u64_t p_size;
- unsigned int mem_type;
} drm_mm
Re: drm-ttm-cleanup-branch
On 5/9/07, Thomas Hellström <[EMAIL PROTECTED]> wrote: > Dave Airlie wrote: > >> I'll try it out as soon as there is time. > > > > I've just tested glxgears and a few mesa tests on it and it seems to > > be working fine > > > > We should probably think about pulling this over into the DRM sooner > > rather than later, there are also some changes to the DDX > > i830_driver.c compat code to deal with... > > > > > Yup. I've attached a patch (against the cleanup branch) with things I > think may be needed. > > 1) 64-bit reordering. 64-bit scalars, structs and unions should probably > be 64-bit aligned in parent structs. I had to insert padding in two > cases, but this probably needs to be double-checked. > 2) A "magic" member in the init ioctl. Checking this allows for verbose > and friendly failure of code that uses the old interface. > 3) Init major / minor versioning of the memory manager interface in case > we need changes in the future. > 4) expand_pads are 64-bit following Jesse Barnes recommendations. > 5) The info_req carries a fence class for validation for a particular > command submission mechanism. > 6) The info_rep arg carries tile_strides and tile_info. > The argument tile_strides is ((actual_tile_stride) << 16) | > (desired_tile_stride)) Any reason you don't just separate actual_tile_stride and desired_tile_stride to 2xunsigned int? not sure why merging them really gives us anything... > The argument tile_info is driver-specific. (Could be tile width, > x-major, y-major etc.) > > Finally, should we perhaps allow for 64-bit buffer object flags / mask > at this point? > Possibly, the rest all seems like good ideas, I know we hit the 64-bit alignment on nouveau so good to get it fixed early > I haven't done any user-space or kernel coding for this yet. > Just want to know what you think. Well I'll be "offline" for a few weeks so I'll get the odd chance to read mail but no development... Dave. - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] make radeons fire fewer vblank interrupts
On Fri, 2007-05-04 at 14:47 -0700, Jesse Barnes wrote: > In playing around yesterday, we found that some drivers will > unnecessarily enable interrupts for vblank events. Since these tend to > happen frequently (60+ Hz), they'll cause your CPU to wake up a lot, > which will waste power if they're not really in use. > > This patch hacks the radeon driver to only enable vblank interrupts when > the user is waiting for one, rather than at IRQ setup time. I couldn't > find any code in the DDX that wanted vblank support, so I suppose the > real users are in the Mesa driver somewhere, so I haven't tested it > other than to see that my interrupt frequency really does decrease. > > Comments? I suspect doing it like this might break userspace expectations about the behaviour of the vblank counter. It would be better to do it similarly to how Eric Anholt did it for i915, i.e. by toggling the vblank interrupt in the 2D driver TransitionTo2/3D hooks. Or possibly even better (as this would e.g. still leave it enabled all the time when using a GLX compositing manager), the 3D driver could tell the DRM whether it needs the vblank interrupt or not. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: R300 cleanup questions
Jerome Glisse wrote:
> On 5/8/07, Christoph Brill <[EMAIL PROTECTED]> wrote:
>> I reviewed the cleanup done by Olliver McFadden and had the following
>> questions:
>>
>> -int r300_get_num_verts(r300ContextPtr rmesa, int num_verts, int prim)
>> +static int r300NumVerts(r300ContextPtr rmesa, int num_verts, int prim)
>>
>> Is it necessary/usefull that the function is static?
>
> I think it's better to have static function, i am thinking of symbol export
> and
> other things like that.
Yes, make functions static whenever possible.
>> -/* Immediate implementation has been removed from CVS. */
>> -
>> -/* vertex buffer implementation */
>> -
>> -static void inline fire_EB(r300ContextPtr rmesa, unsigned long addr
>> +static void inline r300FireEB(r300ContextPtr rmesa, unsigned long addr
>>
>> Why move all the comments to the head of the file. IMO the method should
>> have a doxygen comment that states it is the vertex buffer
>> implementation of fire_EB, right?
>>
>>
>> -if (num_verts > 65535) { /* not implemented yet */
>> +if (num_verts > 65535) {
>>
>> Comments like this should be kept. Otherwise it looks like a hardware
>> limitation while the limitation can be worked around or the limitation
>> does not exist.
>>
>>
>> Last but not least is
>> r300_foo_bar
>> preferred or
>> r300FooBar
>> Which is the one mesa uses?
>
> We can use the one we like, i prefer r300_foo_bar over r300FooBar which
> i dislike but the choice is up to the first person who do big cleanup :) and
> we do not have to conform to mesa coding style for driver but use the one
> we like the more.
Yes, core Mesa has a fairly consistant naming scheme but it's the
prerogative of the driver writers to choose their style. That said,
naming within each driver should be consistant.
-Brian
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: R300 cleanup questions
Hi,
I added the "not implemented yet" comment back, although there are other places
that use 65535 so it could be some kind of hardware limit...
The only reason that I went with "camel case" r300FooBar names is because that's
what 90% of the driver uses; it's easier to change a few r300_foo_bar to
r300FooBar than the other way around. The important thing is it's consistent.
Now I just hope I don't get shot for all the commits. ;)
On 5/9/07, Brian Paul <[EMAIL PROTECTED]> wrote:
> Jerome Glisse wrote:
> > On 5/8/07, Christoph Brill <[EMAIL PROTECTED]> wrote:
> >> I reviewed the cleanup done by Olliver McFadden and had the following
> >> questions:
> >>
> >> -int r300_get_num_verts(r300ContextPtr rmesa, int num_verts, int prim)
> >> +static int r300NumVerts(r300ContextPtr rmesa, int num_verts, int prim)
> >>
> >> Is it necessary/usefull that the function is static?
> >
> > I think it's better to have static function, i am thinking of symbol
> export and
> > other things like that.
>
> Yes, make functions static whenever possible.
>
>
> >> -/* Immediate implementation has been removed from CVS. */
> >> -
> >> -/* vertex buffer implementation */
> >> -
> >> -static void inline fire_EB(r300ContextPtr rmesa, unsigned long addr
> >> +static void inline r300FireEB(r300ContextPtr rmesa, unsigned long addr
> >>
> >> Why move all the comments to the head of the file. IMO the method should
> >> have a doxygen comment that states it is the vertex buffer
> >> implementation of fire_EB, right?
> >>
> >>
> >> -if (num_verts > 65535) { /* not implemented yet */
> >> +if (num_verts > 65535) {
> >>
> >> Comments like this should be kept. Otherwise it looks like a hardware
> >> limitation while the limitation can be worked around or the limitation
> >> does not exist.
> >>
> >>
> >> Last but not least is
> >> r300_foo_bar
> >> preferred or
> >> r300FooBar
> >> Which is the one mesa uses?
> >
> > We can use the one we like, i prefer r300_foo_bar over r300FooBar which
> > i dislike but the choice is up to the first person who do big cleanup :)
> and
> > we do not have to conform to mesa coding style for driver but use the one
> > we like the more.
>
> Yes, core Mesa has a fairly consistant naming scheme but it's the
> prerogative of the driver writers to choose their style. That said,
> naming within each driver should be consistant.
>
> -Brian
>
> -
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> --
> ___
> Dri-devel mailing list
> Dri-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/dri-devel
>
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: R300 cleanup questions
I also think we might need to add _dri_warning/_dri_error because the _mesa
versions output "Mesa warning: %s" which implies to the user this is a Mesa
problem, not a DRI driver problem.
I could add r300Warning and r300Error, but probably all DRI drivers need warning
and error functions... So maybe adding them to the common DRI code?
On 5/9/07, Oliver McFadden <[EMAIL PROTECTED]> wrote:
> Hi,
>
> I added the "not implemented yet" comment back, although there are other
> places
> that use 65535 so it could be some kind of hardware limit...
>
> The only reason that I went with "camel case" r300FooBar names is because
> that's
> what 90% of the driver uses; it's easier to change a few r300_foo_bar to
> r300FooBar than the other way around. The important thing is it's
> consistent.
>
> Now I just hope I don't get shot for all the commits. ;)
>
>
> On 5/9/07, Brian Paul <[EMAIL PROTECTED]> wrote:
> > Jerome Glisse wrote:
> > > On 5/8/07, Christoph Brill <[EMAIL PROTECTED]> wrote:
> > >> I reviewed the cleanup done by Olliver McFadden and had the following
> > >> questions:
> > >>
> > >> -int r300_get_num_verts(r300ContextPtr rmesa, int num_verts, int prim)
> > >> +static int r300NumVerts(r300ContextPtr rmesa, int num_verts, int prim)
> > >>
> > >> Is it necessary/usefull that the function is static?
> > >
> > > I think it's better to have static function, i am thinking of symbol
> > export and
> > > other things like that.
> >
> > Yes, make functions static whenever possible.
> >
> >
> > >> -/* Immediate implementation has been removed from CVS. */
> > >> -
> > >> -/* vertex buffer implementation */
> > >> -
> > >> -static void inline fire_EB(r300ContextPtr rmesa, unsigned long addr
> > >> +static void inline r300FireEB(r300ContextPtr rmesa, unsigned long addr
> > >>
> > >> Why move all the comments to the head of the file. IMO the method
> should
> > >> have a doxygen comment that states it is the vertex buffer
> > >> implementation of fire_EB, right?
> > >>
> > >>
> > >> -if (num_verts > 65535) { /* not implemented yet
> */
> > >> +if (num_verts > 65535) {
> > >>
> > >> Comments like this should be kept. Otherwise it looks like a hardware
> > >> limitation while the limitation can be worked around or the limitation
> > >> does not exist.
> > >>
> > >>
> > >> Last but not least is
> > >> r300_foo_bar
> > >> preferred or
> > >> r300FooBar
> > >> Which is the one mesa uses?
> > >
> > > We can use the one we like, i prefer r300_foo_bar over r300FooBar which
> > > i dislike but the choice is up to the first person who do big cleanup :)
> > and
> > > we do not have to conform to mesa coding style for driver but use the
> one
> > > we like the more.
> >
> > Yes, core Mesa has a fairly consistant naming scheme but it's the
> > prerogative of the driver writers to choose their style. That said,
> > naming within each driver should be consistant.
> >
> > -Brian
> >
> > -
> > This SF.net email is sponsored by DB2 Express
> > Download DB2 Express C - the FREE version of DB2 express and take
> > control of your XML. No limits. Just data. Click to get it now.
> > http://sourceforge.net/powerbar/db2/
> > --
> > ___
> > Dri-devel mailing list
> > Dri-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/dri-devel
> >
>
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: R300 cleanup questions
I'd like some input on the VBO stuff in r300. In r300_context.h we have the
following.
/* KW: Disable this code. Driver should hook into vbo module
* directly, see i965 driver for example.
*/
/* #define RADEON_VTXFMT_A */
#ifdef RADEON_VTXFMT_A
#define HW_VBOS
#endif
So the VTXFMT (radeon_vtxfmt_a.c) code is disabled anyway. This also disables
hardware VBOs. I guess this has been done since the new VBO branch was merged.
So, the question is, should this "dead" code be removed? I think all drivers are
(or should be) moving to the new VBO code anyway.
I've already made a patch for this, but I'm not committing until I get the okay
from a few people.
Thanks.
On 5/9/07, Oliver McFadden <[EMAIL PROTECTED]> wrote:
> I also think we might need to add _dri_warning/_dri_error because the _mesa
> versions output "Mesa warning: %s" which implies to the user this is a Mesa
> problem, not a DRI driver problem.
>
> I could add r300Warning and r300Error, but probably all DRI drivers need
> warning
> and error functions... So maybe adding them to the common DRI code?
>
>
> On 5/9/07, Oliver McFadden <[EMAIL PROTECTED]> wrote:
> > Hi,
> >
> > I added the "not implemented yet" comment back, although there are other
> > places
> > that use 65535 so it could be some kind of hardware limit...
> >
> > The only reason that I went with "camel case" r300FooBar names is because
> > that's
> > what 90% of the driver uses; it's easier to change a few r300_foo_bar to
> > r300FooBar than the other way around. The important thing is it's
> > consistent.
> >
> > Now I just hope I don't get shot for all the commits. ;)
> >
> >
> > On 5/9/07, Brian Paul <[EMAIL PROTECTED]> wrote:
> > > Jerome Glisse wrote:
> > > > On 5/8/07, Christoph Brill <[EMAIL PROTECTED]> wrote:
> > > >> I reviewed the cleanup done by Olliver McFadden and had the following
> > > >> questions:
> > > >>
> > > >> -int r300_get_num_verts(r300ContextPtr rmesa, int num_verts, int
> prim)
> > > >> +static int r300NumVerts(r300ContextPtr rmesa, int num_verts, int
> prim)
> > > >>
> > > >> Is it necessary/usefull that the function is static?
> > > >
> > > > I think it's better to have static function, i am thinking of symbol
> > > export and
> > > > other things like that.
> > >
> > > Yes, make functions static whenever possible.
> > >
> > >
> > > >> -/* Immediate implementation has been removed from CVS. */
> > > >> -
> > > >> -/* vertex buffer implementation */
> > > >> -
> > > >> -static void inline fire_EB(r300ContextPtr rmesa, unsigned long addr
> > > >> +static void inline r300FireEB(r300ContextPtr rmesa, unsigned long
> addr
> > > >>
> > > >> Why move all the comments to the head of the file. IMO the method
> > should
> > > >> have a doxygen comment that states it is the vertex buffer
> > > >> implementation of fire_EB, right?
> > > >>
> > > >>
> > > >> -if (num_verts > 65535) { /* not implemented yet
> > */
> > > >> +if (num_verts > 65535) {
> > > >>
> > > >> Comments like this should be kept. Otherwise it looks like a hardware
> > > >> limitation while the limitation can be worked around or the
> limitation
> > > >> does not exist.
> > > >>
> > > >>
> > > >> Last but not least is
> > > >> r300_foo_bar
> > > >> preferred or
> > > >> r300FooBar
> > > >> Which is the one mesa uses?
> > > >
> > > > We can use the one we like, i prefer r300_foo_bar over r300FooBar
> which
> > > > i dislike but the choice is up to the first person who do big cleanup
> :)
> > > and
> > > > we do not have to conform to mesa coding style for driver but use the
> > one
> > > > we like the more.
> > >
> > > Yes, core Mesa has a fairly consistant naming scheme but it's the
> > > prerogative of the driver writers to choose their style. That said,
> > > naming within each driver should be consistant.
> > >
> > > -Brian
> > >
> > >
> -
> > > This SF.net email is sponsored by DB2 Express
> > > Download DB2 Express C - the FREE version of DB2 express and take
> > > control of your XML. No limits. Just data. Click to get it now.
> > > http://sourceforge.net/powerbar/db2/
> > > --
> > > ___
> > > Dri-devel mailing list
> > > Dri-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/dri-devel
> > >
> >
>
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: R300 cleanup questions
Here is the patch.
On 5/9/07, Oliver McFadden <[EMAIL PROTECTED]> wrote:
I'd like some input on the VBO stuff in r300. In r300_context.h we have the
following.
/* KW: Disable this code. Driver should hook into vbo module
* directly, see i965 driver for example.
*/
/* #define RADEON_VTXFMT_A */
#ifdef RADEON_VTXFMT_A
#define HW_VBOS
#endif
So the VTXFMT (radeon_vtxfmt_a.c) code is disabled anyway. This also
disables
hardware VBOs. I guess this has been done since the new VBO branch was
merged.
So, the question is, should this "dead" code be removed? I think all drivers
are
(or should be) moving to the new VBO code anyway.
I've already made a patch for this, but I'm not committing until I get the
okay
from a few people.
Thanks.
On 5/9/07, Oliver McFadden <[EMAIL PROTECTED]> wrote:
> I also think we might need to add _dri_warning/_dri_error because the
_mesa
> versions output "Mesa warning: %s" which implies to the user this is a
Mesa
> problem, not a DRI driver problem.
>
> I could add r300Warning and r300Error, but probably all DRI drivers need
> warning
> and error functions... So maybe adding them to the common DRI code?
>
>
> On 5/9/07, Oliver McFadden <[EMAIL PROTECTED]> wrote:
> > Hi,
> >
> > I added the "not implemented yet" comment back, although there are other
> > places
> > that use 65535 so it could be some kind of hardware limit...
> >
> > The only reason that I went with "camel case" r300FooBar names is
because
> > that's
> > what 90% of the driver uses; it's easier to change a few r300_foo_bar to
> > r300FooBar than the other way around. The important thing is it's
> > consistent.
> >
> > Now I just hope I don't get shot for all the commits. ;)
> >
> >
> > On 5/9/07, Brian Paul <[EMAIL PROTECTED]> wrote:
> > > Jerome Glisse wrote:
> > > > On 5/8/07, Christoph Brill <[EMAIL PROTECTED]> wrote:
> > > >> I reviewed the cleanup done by Olliver McFadden and had the
following
> > > >> questions:
> > > >>
> > > >> -int r300_get_num_verts(r300ContextPtr rmesa, int num_verts, int
> prim)
> > > >> +static int r300NumVerts(r300ContextPtr rmesa, int num_verts, int
> prim)
> > > >>
> > > >> Is it necessary/usefull that the function is static?
> > > >
> > > > I think it's better to have static function, i am thinking of symbol
> > > export and
> > > > other things like that.
> > >
> > > Yes, make functions static whenever possible.
> > >
> > >
> > > >> -/* Immediate implementation has been removed from CVS. */
> > > >> -
> > > >> -/* vertex buffer implementation */
> > > >> -
> > > >> -static void inline fire_EB(r300ContextPtr rmesa, unsigned long
addr
> > > >> +static void inline r300FireEB(r300ContextPtr rmesa, unsigned long
> addr
> > > >>
> > > >> Why move all the comments to the head of the file. IMO the method
> > should
> > > >> have a doxygen comment that states it is the vertex buffer
> > > >> implementation of fire_EB, right?
> > > >>
> > > >>
> > > >> -if (num_verts > 65535) { /* not implemented
yet
> > */
> > > >> +if (num_verts > 65535) {
> > > >>
> > > >> Comments like this should be kept. Otherwise it looks like a
hardware
> > > >> limitation while the limitation can be worked around or the
> limitation
> > > >> does not exist.
> > > >>
> > > >>
> > > >> Last but not least is
> > > >> r300_foo_bar
> > > >> preferred or
> > > >> r300FooBar
> > > >> Which is the one mesa uses?
> > > >
> > > > We can use the one we like, i prefer r300_foo_bar over r300FooBar
> which
> > > > i dislike but the choice is up to the first person who do big
cleanup
> :)
> > > and
> > > > we do not have to conform to mesa coding style for driver but use
the
> > one
> > > > we like the more.
> > >
> > > Yes, core Mesa has a fairly consistant naming scheme but it's the
> > > prerogative of the driver writers to choose their style. That said,
> > > naming within each driver should be consistant.
> > >
> > > -Brian
> > >
> > >
> -
> > > This SF.net email is sponsored by DB2 Express
> > > Download DB2 Express C - the FREE version of DB2 express and take
> > > control of your XML. No limits. Just data. Click to get it now.
> > > http://sourceforge.net/powerbar/db2/
> > > --
> > > ___
> > > Dri-devel mailing list
> > > Dri-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/dri-devel
> > >
> >
>
From 89e03351b354231117240f34bf7cc48c76a3e99b Mon Sep 17 00:00:00 2001
From: Oliver McFadden <[EMAIL PROTECTED]>
Date: Wed, 9 May 2007 15:50:11 +
Subject: [PATCH] r300: Removed the deprecated VTXFMT code.
---
src/mesa/drivers/dri/r300/Makefile |1 -
src/mesa/drivers/dri/r300/r300_context.c| 10 +-
src/mesa/drivers/dri/r300/r300_context.h| 19 -
src/mesa/drivers/dri/r300/r300_ioctl.c |8 -
src/mesa/drivers/dri/r300/r300_maos.c | 26 -
src/mesa/drivers/dri/r300/r300_render.c | 44 ++
src/mesa
Re: R300 cleanup questions
Oliver McFadden wrote: > I'd like some input on the VBO stuff in r300. In r300_context.h we have the > following. > > /* KW: Disable this code. Driver should hook into vbo module > * directly, see i965 driver for example. > */ > /* #define RADEON_VTXFMT_A */ > #ifdef RADEON_VTXFMT_A > #define HW_VBOS > #endif > > So the VTXFMT (radeon_vtxfmt_a.c) code is disabled anyway. This also disables > hardware VBOs. I guess this has been done since the new VBO branch was merged. > > So, the question is, should this "dead" code be removed? I think all drivers > are > (or should be) moving to the new VBO code anyway. > > I've already made a patch for this, but I'm not committing until I get the > okay > from a few people. Yes, the old code should go. I guess there might be some starting points in there for beginning the vbo work, that's about the only reason to keep it. Keith - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] make radeons fire fewer vblank interrupts
On Wed, 2007-05-09 at 12:33 +0200, Michel Dänzer wrote: > On Fri, 2007-05-04 at 14:47 -0700, Jesse Barnes wrote: > > In playing around yesterday, we found that some drivers will > > unnecessarily enable interrupts for vblank events. Since these tend to > > happen frequently (60+ Hz), they'll cause your CPU to wake up a lot, > > which will waste power if they're not really in use. > > > > This patch hacks the radeon driver to only enable vblank interrupts when > > the user is waiting for one, rather than at IRQ setup time. I couldn't > > find any code in the DDX that wanted vblank support, so I suppose the > > real users are in the Mesa driver somewhere, so I haven't tested it > > other than to see that my interrupt frequency really does decrease. > > > > Comments? > > I suspect doing it like this might break userspace expectations about > the behaviour of the vblank counter. It would be better to do it > similarly to how Eric Anholt did it for i915, i.e. by toggling the > vblank interrupt in the 2D driver TransitionTo2/3D hooks. Or possibly > even better (as this would e.g. still leave it enabled all the time when > using a GLX compositing manager), the 3D driver could tell the DRM > whether it needs the vblank interrupt or not. Yeah, I wasn't sure if the 3D driver would be able to know easily when it might need to wait on an absolute vblank. Arjan suggested that we not turn off the vblank when 3d is running until nobody's waited one one for (for example) a second, and then if someone waits on one after that, the kernel can re-enable the interrupt and extrapolate the vblank counter using the system clock. Hiding vblank enable/disable knowledge in the kernel sounds nicer than what I did on i915. -- Eric Anholt [EMAIL PROTECTED] [EMAIL PROTECTED] [EMAIL PROTECTED] signature.asc Description: This is a digitally signed message part - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: R300 cleanup questions
Oliver McFadden wrote: > I'd like some input on the VBO stuff in r300. In r300_context.h we have the > following. > > /* KW: Disable this code. Driver should hook into vbo module > * directly, see i965 driver for example. > */ > /* #define RADEON_VTXFMT_A */ > #ifdef RADEON_VTXFMT_A > #define HW_VBOS > #endif > > So the VTXFMT (radeon_vtxfmt_a.c) code is disabled anyway. This also > disables > hardware VBOs. I guess this has been done since the new VBO branch was > merged. > > So, the question is, should this "dead" code be removed? I think all > drivers are > (or should be) moving to the new VBO code anyway. > > I've already made a patch for this, but I'm not committing until I get > the okay > from a few people. I'm no expert on the R300 code so I'll defer. Keith might have some comments but he's very busy with another project ATM. On thing: I'd like to keep the trunk/master code stable for the upcoming 7.0 release. If you think there's some risk, let me first make the 7.0/stable branch in git. -Brian - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: R300 cleanup questions
Oliver McFadden wrote: > I also think we might need to add _dri_warning/_dri_error because the _mesa > versions output "Mesa warning: %s" which implies to the user this is a Mesa > problem, not a DRI driver problem. > > I could add r300Warning and r300Error, but probably all DRI drivers need > warning > and error functions... So maybe adding them to the common DRI code? You could just stick with _mesa_warning() and prefix the messages with "R300". -Brian - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: R300 cleanup questions
Well both Keith and Jerome are okay with me removing the VTXFMT code, so I'll go ahead and do that. I don't think there is any serious risk as I'm only removing code that is already disabled. :) Brian, let me know if you want to make a branch so I know when I can push. On 5/9/07, Brian Paul <[EMAIL PROTECTED]> wrote: > Oliver McFadden wrote: > > I also think we might need to add _dri_warning/_dri_error because the > _mesa > > versions output "Mesa warning: %s" which implies to the user this is a > Mesa > > problem, not a DRI driver problem. > > > > I could add r300Warning and r300Error, but probably all DRI drivers need > > warning > > and error functions... So maybe adding them to the common DRI code? > > You could just stick with _mesa_warning() and prefix the messages with > "R300". > > -Brian > - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: R300 cleanup questions
If it's just dead code removal, go ahead. -Brian Oliver McFadden wrote: > Well both Keith and Jerome are okay with me removing the VTXFMT code, so > I'll go > ahead and do that. > > I don't think there is any serious risk as I'm only removing code that is > already disabled. :) Brian, let me know if you want to make a branch so > I know > when I can push. > > > On 5/9/07, Brian Paul <[EMAIL PROTECTED]> wrote: >> Oliver McFadden wrote: >> > I also think we might need to add _dri_warning/_dri_error because the >> _mesa >> > versions output "Mesa warning: %s" which implies to the user this is a >> Mesa >> > problem, not a DRI driver problem. >> > >> > I could add r300Warning and r300Error, but probably all DRI drivers >> need >> > warning >> > and error functions... So maybe adding them to the common DRI code? >> >> You could just stick with _mesa_warning() and prefix the messages with >> "R300". >> >> -Brian >> > - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: R300 cleanup questions
Done. On 5/9/07, Brian Paul <[EMAIL PROTECTED]> wrote: > If it's just dead code removal, go ahead. > > -Brian > > Oliver McFadden wrote: > > Well both Keith and Jerome are okay with me removing the VTXFMT code, so > > I'll go > > ahead and do that. > > > > I don't think there is any serious risk as I'm only removing code that is > > already disabled. :) Brian, let me know if you want to make a branch so > > I know > > when I can push. > > > > > > On 5/9/07, Brian Paul <[EMAIL PROTECTED]> wrote: > >> Oliver McFadden wrote: > >> > I also think we might need to add _dri_warning/_dri_error because the > >> _mesa > >> > versions output "Mesa warning: %s" which implies to the user this is a > >> Mesa > >> > problem, not a DRI driver problem. > >> > > >> > I could add r300Warning and r300Error, but probably all DRI drivers > >> need > >> > warning > >> > and error functions... So maybe adding them to the common DRI code? > >> > >> You could just stick with _mesa_warning() and prefix the messages with > >> "R300". > >> > >> -Brian > >> > > > > - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
[Bug 10892] Mesa build failure: drm_i915_flip conflict between intel_buffers.c and DRM
http://bugs.freedesktop.org/show_bug.cgi?id=10892 [EMAIL PROTECTED] changed: What|Removed |Added Status|NEW |RESOLVED Resolution||NOTABUG --- Comment #1 from [EMAIL PROTECTED] 2007-05-09 10:12 PST --- The i915tex Makefile checks the libdrm version, so this can only happen when there's a mismatch between the system i915_drm.h and libdrm.pc. -- Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee. - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
[R300] minor cleanups
Hi list, I've been browsing through the code to get more in touch with it. On my way I added few doxygen comments and removed the use of a unnecessary function. The removal was agreed by Jerome Glisse. Please check and push these. Thanks in advance, Christoph Brill 0001-Add-few-doxygen-comments-to-r300_context.h.patch Description: application/mbox 0002-Few-doxgen-comments.patch Description: application/mbox - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/-- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
[Bug 10892] Mesa build failure: drm_i915_flip conflict between intel_buffers.c and DRM
http://bugs.freedesktop.org/show_bug.cgi?id=10892 --- Comment #2 from [EMAIL PROTECTED] 2007-05-09 12:07 PST --- Thanks, Michel. Your reply resolved my problem. I'd just like to confirm the behavior of the DRM and Mesa makefiles now. For DRM, $ ./autogen.sh --prefix=/usr --exec-prefix=/ results in libdrm.pc being placed in /lib/pkgconfig/ and NOT usr/lib/pkgconfig/ DRM seems to be using the --exec-prefix rather than the --prefix setting. Is this the intended behavior? By default, Mesa expects libdrm.pc to be installed to: /usr/lib/pkgconfig/ which would be okay, if DRM used the --prefix setting for the path to libdrm.pc instead of the --exec-prefix. I had to set the Mesa path to / rather than /usr: export PKG_CONFIG_PATH=/lib/pkgconfig:$PKG_CONFIG_PATH make linux-dri Regardless of whether or not the makefiles are behaving correctly or I just misunderstood the guide, I'm a noob so maybe some other noobs will benefit from my report. I attempted to follow the guide, below, but the /lib/pkgconfig/ path was unexpected, based on what I read there. http://intellinuxgraphics.org/install.html -Cal -- Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee. - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH -mm] working 3D/DRI intel-agp.ko resume for i815 chip; Intel chipset testers wanted! (was: Re: intel-agp PM experiences ...)
Hi,
On Sat, May 05, 2007 at 07:56:05PM +0200, Andreas Mohr wrote:
> Due to the "scathing" (well, thorough ;) code review I'll send a patch
> with those cleanups against 2.6.21-mm1 very soon.
working 3D/DRI intel-agp.ko resume for i815 chip cleanup patch:
- revert register define name change to ease backporting etc.
- shorten resume issues warning message to two lines while still preserving
all required information (use many shorter words)
I'm not ultimately sure about the register define names, since there's
a general inconsistency in naming here which perhaps would be a good idea
to resolve (my original renaming happened to decrease inconsistency but didn't
quite get rid of it, so it was kind of useless to do it...).
The warning message, while definitely ugly (and that's its job ;), IMHO is
very important to make knowledgeable people see where the problem is.
Those people, if they experience issues, WILL look at kernel logs...
Splitting the remember_state function in two parts isn't very useful given
that it's supposed to be a temporary, "make it work at all" measure only
until there's generic save/restore support (I want to keep this "debug"
code at a minimum). Once there's further chipset data and/or information
how to implement this cleanly, I'm definitely interested in continuing that.
Run-tested on 2.6.21-mm2, resumes fine.
Signed-off-by: Andreas Mohr <[EMAIL PROTECTED]>
--- linux-2.6.21-mm2/drivers/char/agp/intel-agp.c 2007-05-19
06:25:00.0 +0200
+++ linux-2.6.21-mm2.agp/drivers/char/agp/intel-agp.c 2007-05-19
06:23:51.0 +0200
@@ -32,15 +32,15 @@
/* Intel i815 registers, see Intel spec #29068801 */
-#define I815_GMCHCFG 0x50
-#define I815_APCONT0x51
-#define I815_UNKNOWN_800x80
-#define I815_ATTBASE_MASK ~0x1FFF
-#define I815_SM_RCOMP 0x98 /* Sys Mem R Compensation Ctrl */
-#define I815_SM0x9c /* System Memory Control Reg */
-#define I815_AGPCMD0xa8 /* AGP Command Register */
-#define I815_ERRSTS0xc8 /* undocumented in i815 spec; since this
one is modified on resume and many other related chipsets have it, I assume it
*is* ERRSTS */
-#define I815_UNKNOWN_E80xe8
+#define INTEL_I815_GMCHCFG 0x50
+#define INTEL_I815_APCONT 0x51
+#define INTEL_I815_UNKNOWN_80 0x80
+#define INTEL_I815_ATTBASE_MASK~0x1FFF
+#define INTEL_I815_SM_RCOMP0x98 /* Sys Mem R Compensation Ctrl */
+#define INTEL_I815_SM 0x9c /* System Memory Control Reg */
+#define INTEL_I815_AGPCMD 0xa8 /* AGP Command Register */
+#define INTEL_I815_ERRSTS 0xc8 /* undocumented in i815 spec; since this
one is modified on resume and many other related chipsets have it, I assume it
*is* ERRSTS */
+#define INTEL_I815_UNKNOWN_E8 0xe8
/* Intel i820 registers */
#define INTEL_I820_RDCR0x51
@@ -1110,7 +1110,7 @@
/* attbase - aperture base */
/* the Intel 815 chipset spec. says that bits 29-31 in the
* ATTBASE register are reserved -> try not to write them */
- if (agp_bridge->gatt_bus_addr & I815_ATTBASE_MASK) {
+ if (agp_bridge->gatt_bus_addr & INTEL_I815_ATTBASE_MASK) {
printk (KERN_EMERG PFX "gatt bus addr too high\n");
return -EINVAL;
}
@@ -1126,7 +1126,7 @@
agp_bridge->gart_bus_addr = (temp & PCI_BASE_ADDRESS_MEM_MASK);
pci_read_config_dword(agp_bridge->dev, INTEL_ATTBASE, &addr);
- addr &= I815_ATTBASE_MASK;
+ addr &= INTEL_I815_ATTBASE_MASK;
addr |= agp_bridge->gatt_bus_addr;
pci_write_config_dword(agp_bridge->dev, INTEL_ATTBASE, addr);
@@ -1134,8 +1134,8 @@
pci_write_config_dword(agp_bridge->dev, INTEL_AGPCTRL, 0x);
/* apcont */
- pci_read_config_byte(agp_bridge->dev, I815_APCONT, &temp2);
- pci_write_config_byte(agp_bridge->dev, I815_APCONT, temp2 | (1 << 1));
+ pci_read_config_byte(agp_bridge->dev, INTEL_I815_APCONT, &temp2);
+ pci_write_config_byte(agp_bridge->dev, INTEL_I815_APCONT, temp2 | (1 <<
1));
/* clear any possible error conditions */
/* Oddness : this chipset seems to have no ERRSTS register ! */
@@ -2018,7 +2018,6 @@
}
#ifdef CONFIG_PM
-
static int agp_i815_remember_state(struct pci_dev *pdev, int restore)
{
typedef struct {
@@ -2030,18 +2029,18 @@
* to be able to successfully restore X11 when AGP 3D is enabled
* (register values given are after resume vs. before suspend):
*
-* I815_GMCHCFG (0x50; we need to set bit 1 (Aperture Access Global
-* Enable) of I815_APCONT (0x51),
-* thus use I815_GMCHCFG (0x50) as 32bit base register);
+* INTEL_I815_GMCHCFG (0x50; we need to set bit 1
+* (Aperture Access Global Enable) of INTEL_I815_APCONT (0x51),
+* thus use INTEL_I815_GMCHCFG (0x50) as 32bit base register);
