[radeon] Hardcoded DRIVER_DATE?

[radeon] Hardcoded DRIVER_DATE?

[Sedat Dilek]

Resend to new ML-adress <dri-devel@lists.freedesktop.org>.

- Sedat -


---------- Forwarded message ----------
From: Sedat Dilek <sedat.dilek@googlemail.com>
Date: Mon, Apr 19, 2010 at 11:30 AM
Subject: [radeon] Hardcoded DRIVER_DATE?
To: DRI <dri-devel@lists.sourceforge.net>
Cc: Dave Airlie <airlied@redhat.com>


Hi,

today I pulled drm-linus GIT branch into Linus-tree (2.6.34-rc4-git6).

Again, I was seeing a version-bump of the radeon-kms wrapper/driver,
but not in the driver-date:

[dmesg]
...
[   71.347298] [drm] Initialized radeon 2.3.0 20080528 for
0000:01:00.0 on minor 0
...

While inspecting where the driver-date is defined, I found:

$ grep DRIVER_DATE -r drivers/gpu/drm/radeon/
drivers/gpu/drm/radeon/radeon_drv.h:#define DRIVER_DATE         "20080528"
drivers/gpu/drm/radeon/radeon_drv.c:    .date = DRIVER_DATE,
drivers/gpu/drm/radeon/radeon_drv.c:    .date = DRIVER_DATE,

Looking closer into both files ([1] and [2]) reveals:

[drivers/gpu/drm/radeon/radeon_drv.h]
...
#define DRIVER_NAME             "radeon"
#define DRIVER_DESC             "ATI Radeon"
#define DRIVER_DATE             "20080528"
...
 * 1.32- fixes for rv740 setup
 * 1.33- Add r6xx/r7xx const buffer support
 */
#define DRIVER_MAJOR            1
#define DRIVER_MINOR            33
#define DRIVER_PATCHLEVEL       0
...

[drivers/gpu/drm/radeon/radeon_drv.c]
...
/*
 * KMS wrapper.
 * - 2.0.0 - initial interface
 * - 2.1.0 - add square tiling interface
 * - 2.2.0 - add r6xx/r7xx const buffer support
 * - 2.3.0 - add MSPOS + 3D texture + r500 VAP regs
 */
#define KMS_DRIVER_MAJOR        2
#define KMS_DRIVER_MINOR        3
#define KMS_DRIVER_PATCHLEVEL   0
...
       .name = DRIVER_NAME,
       .desc = DRIVER_DESC,
       .date = DRIVER_DATE,
       .major = DRIVER_MAJOR,
       .minor = DRIVER_MINOR,
       .patchlevel = DRIVER_PATCHLEVEL,
...

My first thoughts on this was, if you manage two files with version
informations, it is human to forget to bump it in both.
Dave apologized to be "lazy" for the changes on IRC.

The second thought was, why the hell are there two version-strings?
1.33.0 (H-file) and 2.3.0 (C-file).
OK, the second one is for the KMS-wrapper, the first one is for radeon
kernel-module in general.

The version-infos from the H-file/C-file can be read in dmesg:
...
[    9.861334] [drm] Initialized radeon 1.33.0 20080528 for
0000:01:00.0 on minor 0
...
[   71.347298] [drm] Initialized radeon 2.3.0 20080528 for
0000:01:00.0 on minor 0
...

Do changes in radeon_drv.c (KMS-wrapper) require also a version-bump
in the header-file?
I think yes.

IIRC this is missing and version should be bumped:

[drivers/gpu/drm/radeon/radeon_drv.h]
...
 * 1.33- Add r6xx/r7xx const buffer support
+ * 1.34- add MSPOS + 3D texture + r500 VAP regs
 */
#define DRIVER_MAJOR            1
-#define DRIVER_MINOR           33
+#define DRIVER_MINOR           34
#define DRIVER_PATCHLEVEL       0
...

My third thought was, when there is a version-string for radeon
kernel-module why don't I see them via modinfo?

$ sudo modinfo radeon | grep -i ver
filename:
/lib/modules/2.6.34-rc4-iniza-686-kms/kernel/drivers/gpu/drm/radeon/radeon.ko
srcversion:     27576A7F4ADA88E07720FD4
vermagic:       2.6.34-rc4-iniza-686-kms SMP preempt mod_unload modversions 686

Unfortunately, NO.
Is there something speaking against to have it in the module-description?
How does other kernel-module handle such things? Usual - unusual? Guideline?

By reading this report one could think it is only "cosmetics" (and not
writing so much text), but why use a driver-date that is simply wrong
or unused?

Kind Regards,
- Sedat -

References:
=======
[1] http://git.kernel.org/?p=linux/kernel/git/airlied/drm-2.6.git;a=blob_plain;f=drivers/gpu/drm/radeon/radeon_drv.h;hb=cae94b0ad9d147152af77b971a7234faf20027a9
[2] http://git.kernel.org/?p=linux/kernel/git/airlied/drm-2.6.git;a=blob_plain;f=drivers/gpu/drm/radeon/radeon_drv.c;hb=cae94b0ad9d147152af77b971a7234faf20027a9
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [radeon] Hardcoded DRIVER_DATE?

[Dave Airlie]

On Mon, 2010-04-19 at 11:30 +0200, Sedat Dilek wrote:
> Hi,
> 
> today I pulled drm-linus GIT branch into Linus-tree (2.6.34-rc4-git6).
> 
> Again, I was seeing a version-bump of the radeon-kms wrapper/driver,
> but not in the driver-date:
> 
> [dmesg]
> ...
> [   71.347298] [drm] Initialized radeon 2.3.0 20080528 for
> 0000:01:00.0 on minor 0
> ...
> 
> While inspecting where the driver-date is defined, I found:
> 
> $ grep DRIVER_DATE -r drivers/gpu/drm/radeon/
> drivers/gpu/drm/radeon/radeon_drv.h:#define DRIVER_DATE		"20080528"
> drivers/gpu/drm/radeon/radeon_drv.c:	.date = DRIVER_DATE,
> drivers/gpu/drm/radeon/radeon_drv.c:	.date = DRIVER_DATE,
> 
> Looking closer into both files ([1] and [2]) reveals:
> 
> [drivers/gpu/drm/radeon/radeon_drv.h]
> ...
> #define DRIVER_NAME		"radeon"
> #define DRIVER_DESC		"ATI Radeon"
> #define DRIVER_DATE		"20080528"
> ...
>  * 1.32- fixes for rv740 setup
>  * 1.33- Add r6xx/r7xx const buffer support
>  */
> #define DRIVER_MAJOR		1
> #define DRIVER_MINOR		33
> #define DRIVER_PATCHLEVEL	0
> ...
> 
> [drivers/gpu/drm/radeon/radeon_drv.c]
> ...
> /*
>  * KMS wrapper.
>  * - 2.0.0 - initial interface
>  * - 2.1.0 - add square tiling interface
>  * - 2.2.0 - add r6xx/r7xx const buffer support
>  * - 2.3.0 - add MSPOS + 3D texture + r500 VAP regs
>  */
> #define KMS_DRIVER_MAJOR	2
> #define KMS_DRIVER_MINOR	3
> #define KMS_DRIVER_PATCHLEVEL	0
> ...
> 	.name = DRIVER_NAME,
> 	.desc = DRIVER_DESC,
> 	.date = DRIVER_DATE,
> 	.major = DRIVER_MAJOR,
> 	.minor = DRIVER_MINOR,
> 	.patchlevel = DRIVER_PATCHLEVEL,
> ...
> 
> My first thoughts on this was, if you manage two files with version
> informations, it is human to forget to bump it in both.
> Dave apologized to be "lazy" for the changes on IRC.
> 
> The second thought was, why the hell are there two version-strings?
> 1.33.0 (H-file) and 2.3.0 (C-file).
> OK, the second one is for the KMS-wrapper, the first one is for radeon
> kernel-module in general.
> 
> The version-infos from the H-file/C-file can be read in dmesg:
> ...
> [    9.861334] [drm] Initialized radeon 1.33.0 20080528 for
> 0000:01:00.0 on minor 0
> ...
> [   71.347298] [drm] Initialized radeon 2.3.0 20080528 for
> 0000:01:00.0 on minor 0
> ...
> 
> Do changes in radeon_drv.c (KMS-wrapper) require also a version-bump
> in the header-file?
> I think yes.


No they don't. KMS and UMS drivers are separate.

I referred to bumping the date as lazy, we rarely bothered doing it in
the past.

Dave.
> 
> IIRC this is missing and version should be bumped:
> 
> [drivers/gpu/drm/radeon/radeon_drv.h]
> ...
>  * 1.33- Add r6xx/r7xx const buffer support
> + * 1.34- add MSPOS + 3D texture + r500 VAP regs
>  */
> #define DRIVER_MAJOR		1
> -#define DRIVER_MINOR		33
> +#define DRIVER_MINOR		34
> #define DRIVER_PATCHLEVEL	0
> ...
> 
> My third thought was, when there is a version-string for radeon
> kernel-module why don't I see them via modinfo?
> 
> $ sudo modinfo radeon | grep -i ver
> filename:
> /lib/modules/2.6.34-rc4-iniza-686-kms/kernel/drivers/gpu/drm/radeon/radeon.ko
> srcversion:     27576A7F4ADA88E07720FD4
> vermagic:       2.6.34-rc4-iniza-686-kms SMP preempt mod_unload modversions 686
> 
> Unfortunately, NO.
> Is there something speaking against to have it in the module-description?
> How does other kernel-module handle such things? Usual - unusual? Guideline?
> 
> By reading this report one could think it is only "cosmetics" (and not
> writing so much text), but why use a driver-date that is simply wrong
> or unused?
> 
> Kind Regards,
> - Sedat -
> 
> References:
> =======
> [1] http://git.kernel.org/?p=linux/kernel/git/airlied/drm-2.6.git;a=blob_plain;f=drivers/gpu/drm/radeon/radeon_drv.h;hb=cae94b0ad9d147152af77b971a7234faf20027a9
> [2] http://git.kernel.org/?p=linux/kernel/git/airlied/drm-2.6.git;a=blob_plain;f=drivers/gpu/drm/radeon/radeon_drv.c;hb=cae94b0ad9d147152af77b971a7234faf20027a9



------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
--

Re: [radeon] Hardcoded DRIVER_DATE?

[Sedat Dilek]

[ Changed CC to new ML-address @ fd.o ]

On Mon, Apr 19, 2010 at 12:44 PM, Dave Airlie <airlied@redhat.com> wrote:
> On Mon, 2010-04-19 at 11:30 +0200, Sedat Dilek wrote:
[...]
>> Do changes in radeon_drv.c (KMS-wrapper) require also a version-bump
>> in the header-file?
>> I think yes.
>
>
> No they don't. KMS and UMS drivers are separate.
>
> I referred to bumping the date as lazy, we rarely bothered doing it in
> the past.
>

Only to clarify:

radeon_drv.h contains all version-informations (version, date,
changelog) for the radeon UserModeSetting driver and radeon_drv.c the
same for KernelModeSetting part?

If that is the case, it would be good to maintain a KMS_DRIVER_DATE
define and change accordingly in ".date = DRIVER_DATE" line(s) - not
sure if both lines or only one of them.

Positive side-effect could be people don't forget to bump the driver-date.

[radeon_drv.c]
...
#define KMS_DRIVER_MAJOR	2
#define KMS_DRIVER_MINOR	3
#define KMS_DRIVER_PATCHLEVEL	0
#define KMS_DRIVER_DATE YYYYMMDD
...
       .name = DRIVER_NAME,
       .desc = DRIVER_DESC,
       .date = KMS_DRIVER_DATE,
       .major = DRIVER_MAJOR,
       .minor = DRIVER_MINOR,
       .patchlevel = DRIVER_PATCHLEVEL,
...


Kind Regards,
- Sedat -


#define KMS_DRIVER_MAJOR	2
#define KMS_DRIVER_MINOR	3
#define KMS_DRIVER_PATCHLEVEL	0
#define KMS_DRIVER_DATE YYYYMMDD

- Sedat -

Re: [radeon] Hardcoded DRIVER_DATE?

[Sedat Dilek]

[ /me promises to execise more Cut-N-Paste ]

[radeon_drv.c]
...
#define KMS_DRIVER_MAJOR	2
#define KMS_DRIVER_MINOR	3
#define KMS_DRIVER_PATCHLEVEL	0
+#define KMS_DRIVER_DATE YYYYMMDD
...
	.name = DRIVER_NAME,
	.desc = DRIVER_DESC,
-	.date = DRIVER_DATE,
+	.date = KMS_DRIVER_DATE,
	.major = KMS_DRIVER_MAJOR,
	.minor = KMS_DRIVER_MINOR,
	.patchlevel = KMS_DRIVER_PATCHLEVEL,


- Sedat -

On Mon, Apr 19, 2010 at 2:15 PM, Sedat Dilek <sedat.dilek@googlemail.com> wrote:
> [ Changed CC to new ML-address @ fd.o ]
>
> On Mon, Apr 19, 2010 at 12:44 PM, Dave Airlie <airlied@redhat.com> wrote:
>> On Mon, 2010-04-19 at 11:30 +0200, Sedat Dilek wrote:
> [...]
>>> Do changes in radeon_drv.c (KMS-wrapper) require also a version-bump
>>> in the header-file?
>>> I think yes.
>>
>>
>> No they don't. KMS and UMS drivers are separate.
>>
>> I referred to bumping the date as lazy, we rarely bothered doing it in
>> the past.
>>
>
> Only to clarify:
>
> radeon_drv.h contains all version-informations (version, date,
> changelog) for the radeon UserModeSetting driver and radeon_drv.c the
> same for KernelModeSetting part?
>
> If that is the case, it would be good to maintain a KMS_DRIVER_DATE
> define and change accordingly in ".date = DRIVER_DATE" line(s) - not
> sure if both lines or only one of them.
>
> Positive side-effect could be people don't forget to bump the driver-date.
>
> [radeon_drv.c]
> ...
> #define KMS_DRIVER_MAJOR        2
> #define KMS_DRIVER_MINOR        3
> #define KMS_DRIVER_PATCHLEVEL   0
> #define KMS_DRIVER_DATE YYYYMMDD
> ...
>       .name = DRIVER_NAME,
>       .desc = DRIVER_DESC,
>       .date = KMS_DRIVER_DATE,
>       .major = DRIVER_MAJOR,
>       .minor = DRIVER_MINOR,
>       .patchlevel = DRIVER_PATCHLEVEL,
> ...
>
>
> Kind Regards,
> - Sedat -
>
>
> #define KMS_DRIVER_MAJOR        2
> #define KMS_DRIVER_MINOR        3
> #define KMS_DRIVER_PATCHLEVEL   0
> #define KMS_DRIVER_DATE YYYYMMDD
>
> - Sedat -
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel