drm/radeon/kms: a few nits

drm/radeon/kms: a few nits

[Ilija Hadzic]

The following three patches address various minor nits. They are all safe
and I have been running with them for several months on a wide variety
of AMD GPUs

The first patch:
[PATCH 1/3] drm/radeon/kms: use crtc-specific dpms functions in
does not change the functionality and affects the function that is used
only by r100 ASICs, but make the code more correct (strictly speaking).

The second patch:
[PATCH 2/3] drm/radeon/kms: fix the crtc number check
makes the crtc number check consistent with the way it is done
in the rest of the driver code without affecting the functionality.
It was actually sent a few months ago and reviewed, but apparently
forgotten (cf. http://lists.freedesktop.org/archives/dri-devel/2011-May/010935.html)

The third patch:
[PATCH 3/3] drm/radeon/kms: use num_crtc instead of hard-coded value
Prevents a possible trouble waiting to happen if AMD ever makes a GPU with
more than 6 CRTCs. I don't know what's cooking in their kitchen but
if such a GPU ever comes out, this will prevent at least one regression ;-)

[PATCH 1/3] drm/radeon/kms: use crtc-specific dpms functions in prepare and commit

[Ilija Hadzic]

it's better that radeon_crtc_commit and radeon_crtc_prepare call
crtc-specific dpms functions instead of hard-coding them to
radeon_crtc_dpms.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/radeon/radeon_legacy_crtc.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
index 41a5d48..0690a5b 100644
--- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
+++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
@@ -1036,8 +1036,11 @@ static void radeon_crtc_prepare(struct drm_crtc *crtc)
 	* The hardware wedges sometimes if you reconfigure one CRTC
 	* whilst another is running (see fdo bug #24611).
 	*/
-	list_for_each_entry(crtci, &dev->mode_config.crtc_list, head)
-		radeon_crtc_dpms(crtci, DRM_MODE_DPMS_OFF);
+	list_for_each_entry(crtci, &dev->mode_config.crtc_list, head) {
+		struct drm_crtc_helper_funcs *crtc_funcs = crtci->helper_private;
+		if  (crtc_funcs->dpms)
+			crtc_funcs->dpms(crtci, DRM_MODE_DPMS_OFF);
+	}
 }
 
 static void radeon_crtc_commit(struct drm_crtc *crtc)
@@ -1049,8 +1052,11 @@ static void radeon_crtc_commit(struct drm_crtc *crtc)
 	* Reenable the CRTCs that should be running.
 	*/
 	list_for_each_entry(crtci, &dev->mode_config.crtc_list, head) {
-		if (crtci->enabled)
-			radeon_crtc_dpms(crtci, DRM_MODE_DPMS_ON);
+		if (crtci->enabled) {
+			struct drm_crtc_helper_funcs *crtc_funcs = crtci->helper_private;
+			if  (crtc_funcs->dpms)
+				crtc_funcs->dpms(crtci, DRM_MODE_DPMS_ON);
+		}
 	}
 }
 
-- 
1.7.7

Re: [PATCH 1/3] drm/radeon/kms: use crtc-specific dpms functions in prepare and commit

[Michel Dänzer]

On Die, 2011-10-25 at 22:40 -0400, Ilija Hadzic wrote: 
> it's better that radeon_crtc_commit and radeon_crtc_prepare call
> crtc-specific dpms functions instead of hard-coding them to
> radeon_crtc_dpms.

Is it really better? If it's always radeon_crtc_dpms anyway, this
obfuscates that fact (and introduces a guaranteed branch prediction
miss, but that probably doesn't matter here).


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/radeon/kms: use crtc-specific dpms functions in prepare and commit

[Ilija Hadzic]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8; format=flowed, Size: 1100 bytes --]



On Wed, 26 Oct 2011, Michel [ISO-8859-1] Dänzer wrote:

> On Die, 2011-10-25 at 22:40 -0400, Ilija Hadzic wrote:
>> it's better that radeon_crtc_commit and radeon_crtc_prepare call
>> crtc-specific dpms functions instead of hard-coding them to
>> radeon_crtc_dpms.
>
> Is it really better? If it's always radeon_crtc_dpms anyway, this
> obfuscates that fact (and introduces a guaranteed branch prediction
> miss, but that probably doesn't matter here).
>

My reasoning for believing that it's better was that the functions are 
fairly generic: they loops through all CRTCs and disable/enable them all.

Although at this time these functions are only used as helpers for legacy 
crtcs (so indeed the pointer always resolves to radeon_crtc_dpms), if they 
are ever called through some other path (in the context of atombios CRTC), 
it doesn't hurt to make them ready for that.

Having said that, I don't really mind whatever the destiny of this patch 
ends up being.

(and yes, branch prediction slot penalty really don't matter here; it's a 
slow path anyway)

-- Ilija

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 2/3] drm/radeon/kms: fix the crtc number check

[Ilija Hadzic]

the crtc check in radeon_get_vblank_timestamp_kms should be against
the num_crtc field in radeon_device not against num_crtcs in drm_device

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/radeon/radeon_kms.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index a749c26..540a9ee 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -347,7 +347,7 @@ int radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
 	struct drm_crtc *drmcrtc;
 	struct radeon_device *rdev = dev->dev_private;
 
-	if (crtc < 0 || crtc >= dev->num_crtcs) {
+	if (crtc < 0 || crtc >= rdev->num_crtc) {
 		DRM_ERROR("Invalid crtc %d\n", crtc);
 		return -EINVAL;
 	}
-- 
1.7.7

Re: [PATCH 2/3] drm/radeon/kms: fix the crtc number check

[Alex Deucher]

On Tue, Oct 25, 2011 at 10:40 PM, Ilija Hadzic
<ihadzic@research.bell-labs.com> wrote:
> the crtc check in radeon_get_vblank_timestamp_kms should be against
> the num_crtc field in radeon_device not against num_crtcs in drm_device

This more correct, but would there ever be a case where they are different?

Alex

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
> ---
>  drivers/gpu/drm/radeon/radeon_kms.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index a749c26..540a9ee 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -347,7 +347,7 @@ int radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
>        struct drm_crtc *drmcrtc;
>        struct radeon_device *rdev = dev->dev_private;
>
> -       if (crtc < 0 || crtc >= dev->num_crtcs) {
> +       if (crtc < 0 || crtc >= rdev->num_crtc) {
>                DRM_ERROR("Invalid crtc %d\n", crtc);
>                return -EINVAL;
>        }
> --
> 1.7.7
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[PATCH 3/3] drm/radeon/kms: use num_crtc instead of hard-coded value 6

[Ilija Hadzic]

radeon_driver_irq_preinstall_kms and radeon_driver_irq_uninstall_kms
hard code the loop to 6 which happens to be the current maximum
number of crtcs; if one day an ASIC with more crtcs comes out, this
is a trouble waiting to happen. it's better to use num_crtc instead
(for ASICs that have fewer than 6 CRTCs, this is still OK because
higher numbers won't be looked at)

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/radeon/radeon_irq_kms.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c
index 9ec830c..a0f9d24 100644
--- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
@@ -69,7 +69,7 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev)
 	rdev->irq.gui_idle = false;
 	for (i = 0; i < rdev->num_crtc; i++)
 		rdev->irq.crtc_vblank_int[i] = false;
-	for (i = 0; i < 6; i++) {
+	for (i = 0; i < rdev->num_crtc; i++) {
 		rdev->irq.hpd[i] = false;
 		rdev->irq.pflip[i] = false;
 	}
@@ -101,7 +101,7 @@ void radeon_driver_irq_uninstall_kms(struct drm_device *dev)
 	rdev->irq.gui_idle = false;
 	for (i = 0; i < rdev->num_crtc; i++)
 		rdev->irq.crtc_vblank_int[i] = false;
-	for (i = 0; i < 6; i++) {
+	for (i = 0; i < rdev->num_crtc; i++) {
 		rdev->irq.hpd[i] = false;
 		rdev->irq.pflip[i] = false;
 	}
-- 
1.7.7

Re: [PATCH 3/3] drm/radeon/kms: use num_crtc instead of hard-coded value 6

[Alex Deucher]

On Tue, Oct 25, 2011 at 10:40 PM, Ilija Hadzic
<ihadzic@research.bell-labs.com> wrote:
> radeon_driver_irq_preinstall_kms and radeon_driver_irq_uninstall_kms
> hard code the loop to 6 which happens to be the current maximum
> number of crtcs; if one day an ASIC with more crtcs comes out, this
> is a trouble waiting to happen. it's better to use num_crtc instead
> (for ASICs that have fewer than 6 CRTCs, this is still OK because
> higher numbers won't be looked at)
>

This is actually not quite right.  The number of HPD (Hot Plug Detect)
pins is not equal to the number of crtcs.  Radeons have supported 6
HPD pins long before we supported 6 crtcs (e.g., cards with more
connectors than crtcs).  The logic should probably look like:

#define RADEON_MAX_HPD_PINS 6

for (i = 0; i < rdev->num_crtc; i++) {
        rdev->irq.crtc_vblank_int[i] = false;
        rdev->irq.pflip[i] = false;
}

for (i = 0; i < RADEON_MAX_HPD_PINS; i++)
        rdev->irq.hpd[i] = false;

Alex

> Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
> ---
>  drivers/gpu/drm/radeon/radeon_irq_kms.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> index 9ec830c..a0f9d24 100644
> --- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> @@ -69,7 +69,7 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev)
>        rdev->irq.gui_idle = false;
>        for (i = 0; i < rdev->num_crtc; i++)
>                rdev->irq.crtc_vblank_int[i] = false;
> -       for (i = 0; i < 6; i++) {
> +       for (i = 0; i < rdev->num_crtc; i++) {
>                rdev->irq.hpd[i] = false;
>                rdev->irq.pflip[i] = false;
>        }
> @@ -101,7 +101,7 @@ void radeon_driver_irq_uninstall_kms(struct drm_device *dev)
>        rdev->irq.gui_idle = false;
>        for (i = 0; i < rdev->num_crtc; i++)
>                rdev->irq.crtc_vblank_int[i] = false;
> -       for (i = 0; i < 6; i++) {
> +       for (i = 0; i < rdev->num_crtc; i++) {
>                rdev->irq.hpd[i] = false;
>                rdev->irq.pflip[i] = false;
>        }
> --
> 1.7.7
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH 3/3] drm/radeon/kms: use num_crtc instead of hard-coded value 6

[Ilija Hadzic]

On Wed, 26 Oct 2011, Alex Deucher wrote:

>
> This is actually not quite right.  The number of HPD (Hot Plug Detect)
> pins is not equal to the number of crtcs.  Radeons have supported 6
> HPD pins long before we supported 6 crtcs (e.g., cards with more
> connectors than crtcs).  The logic should probably look like:
> [SNIP]

I have just sent out a patch that I think rectifies this. Hopefully, it's 
good now.

-- Ilija