[PATCH] drm/qxl: use qxl_num_crtc directly

[PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

Just use qxl_num_crtc directly everywhere instead of using
qdev->monitors_config->max_allowed.  Drops pointless indirection
and also is less confusing.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_display.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index ce0b9c40fc..6437369a31 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -80,10 +80,10 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 		DRM_DEBUG_KMS("no client monitors configured\n");
 		return status;
 	}
-	if (num_monitors > qdev->monitors_config->max_allowed) {
+	if (num_monitors > qxl_num_crtc) {
 		DRM_DEBUG_KMS("client monitors list will be truncated: %d < %d\n",
-			      qdev->monitors_config->max_allowed, num_monitors);
-		num_monitors = qdev->monitors_config->max_allowed;
+			      qxl_num_crtc, num_monitors);
+		num_monitors = qxl_num_crtc;
 	} else {
 		num_monitors = qdev->rom->client_monitors_config.count;
 	}
@@ -96,8 +96,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 		return status;
 	}
 	/* we copy max from the client but it isn't used */
-	qdev->client_monitors_config->max_allowed =
-				qdev->monitors_config->max_allowed;
+	qdev->client_monitors_config->max_allowed = qxl_num_crtc;
 	for (i = 0 ; i < qdev->client_monitors_config->count ; ++i) {
 		struct qxl_urect *c_rect =
 			&qdev->rom->client_monitors_config.heads[i];
@@ -204,7 +203,7 @@ static int qxl_add_monitors_config_modes(struct drm_connector *connector,
 
 	if (!qdev->monitors_config)
 		return 0;
-	if (h >= qdev->monitors_config->max_allowed)
+	if (h >= qxl_num_crtc)
 		return 0;
 	if (!qdev->client_monitors_config)
 		return 0;
@@ -307,8 +306,7 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
 		return;
 	}
 
-	if (!qdev->monitors_config ||
-	    qdev->monitors_config->max_allowed <= i)
+	if (!qdev->monitors_config || qxl_num_crtc <= i)
 		return;
 
 	head.id = i;
@@ -348,9 +346,10 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
 	if (oldcount != qdev->monitors_config->count)
 		DRM_DEBUG_KMS("active heads %d -> %d (%d total)\n",
 			      oldcount, qdev->monitors_config->count,
-			      qdev->monitors_config->max_allowed);
+			      qxl_num_crtc);
 
 	qdev->monitors_config->heads[i] = head;
+	qdev->monitors_config->max_allowed = qxl_num_crtc;
 	qxl_send_monitors_config(qdev);
 }
 
@@ -1097,9 +1096,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
 {
 	int ret;
 	struct drm_gem_object *gobj;
-	int max_allowed = qxl_num_crtc;
 	int monitors_config_size = sizeof(struct qxl_monitors_config) +
-		max_allowed * sizeof(struct qxl_head);
+		qxl_num_crtc * sizeof(struct qxl_head);
 
 	ret = qxl_gem_object_create(qdev, monitors_config_size, 0,
 				    QXL_GEM_DOMAIN_VRAM,
@@ -1121,7 +1119,6 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
 		qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
 
 	memset(qdev->monitors_config, 0, monitors_config_size);
-	qdev->monitors_config->max_allowed = max_allowed;
 	return 0;
 }
 
-- 
2.9.3

[PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

Just use qxl_num_crtc directly everywhere instead of using
qdev->monitors_config->max_allowed.  Drops pointless indirection
and also is less confusing.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_display.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index ce0b9c40fc..6437369a31 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -80,10 +80,10 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 		DRM_DEBUG_KMS("no client monitors configured\n");
 		return status;
 	}
-	if (num_monitors > qdev->monitors_config->max_allowed) {
+	if (num_monitors > qxl_num_crtc) {
 		DRM_DEBUG_KMS("client monitors list will be truncated: %d < %d\n",
-			      qdev->monitors_config->max_allowed, num_monitors);
-		num_monitors = qdev->monitors_config->max_allowed;
+			      qxl_num_crtc, num_monitors);
+		num_monitors = qxl_num_crtc;
 	} else {
 		num_monitors = qdev->rom->client_monitors_config.count;
 	}
@@ -96,8 +96,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 		return status;
 	}
 	/* we copy max from the client but it isn't used */
-	qdev->client_monitors_config->max_allowed =
-				qdev->monitors_config->max_allowed;
+	qdev->client_monitors_config->max_allowed = qxl_num_crtc;
 	for (i = 0 ; i < qdev->client_monitors_config->count ; ++i) {
 		struct qxl_urect *c_rect =
 			&qdev->rom->client_monitors_config.heads[i];
@@ -204,7 +203,7 @@ static int qxl_add_monitors_config_modes(struct drm_connector *connector,
 
 	if (!qdev->monitors_config)
 		return 0;
-	if (h >= qdev->monitors_config->max_allowed)
+	if (h >= qxl_num_crtc)
 		return 0;
 	if (!qdev->client_monitors_config)
 		return 0;
@@ -307,8 +306,7 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
 		return;
 	}
 
-	if (!qdev->monitors_config ||
-	    qdev->monitors_config->max_allowed <= i)
+	if (!qdev->monitors_config || qxl_num_crtc <= i)
 		return;
 
 	head.id = i;
@@ -348,9 +346,10 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
 	if (oldcount != qdev->monitors_config->count)
 		DRM_DEBUG_KMS("active heads %d -> %d (%d total)\n",
 			      oldcount, qdev->monitors_config->count,
-			      qdev->monitors_config->max_allowed);
+			      qxl_num_crtc);
 
 	qdev->monitors_config->heads[i] = head;
+	qdev->monitors_config->max_allowed = qxl_num_crtc;
 	qxl_send_monitors_config(qdev);
 }
 
@@ -1097,9 +1096,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
 {
 	int ret;
 	struct drm_gem_object *gobj;
-	int max_allowed = qxl_num_crtc;
 	int monitors_config_size = sizeof(struct qxl_monitors_config) +
-		max_allowed * sizeof(struct qxl_head);
+		qxl_num_crtc * sizeof(struct qxl_head);
 
 	ret = qxl_gem_object_create(qdev, monitors_config_size, 0,
 				    QXL_GEM_DOMAIN_VRAM,
@@ -1121,7 +1119,6 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
 		qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
 
 	memset(qdev->monitors_config, 0, monitors_config_size);
-	qdev->monitors_config->max_allowed = max_allowed;
 	return 0;
 }
 
-- 
2.9.3

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Frediano Ziglio]

> 
> Just use qxl_num_crtc directly everywhere instead of using
> qdev->monitors_config->max_allowed.  Drops pointless indirection
> and also is less confusing.
> 

To me is MORE confusing, why comparing number of something with
another number? Previously code was comparing number of monitors
with number of monitors, not number of CRTs with number of
monitors.

Why we are optimizing code that, as yours same saying, we are
not much actively improving?

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index ce0b9c40fc..6437369a31 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -80,10 +80,10 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>  		DRM_DEBUG_KMS("no client monitors configured\n");
>  		return status;
>  	}
> -	if (num_monitors > qdev->monitors_config->max_allowed) {
> +	if (num_monitors > qxl_num_crtc) {
>  		DRM_DEBUG_KMS("client monitors list will be truncated: %d < %d\n",
> -			      qdev->monitors_config->max_allowed, num_monitors);
> -		num_monitors = qdev->monitors_config->max_allowed;
> +			      qxl_num_crtc, num_monitors);
> +		num_monitors = qxl_num_crtc;
>  	} else {
>  		num_monitors = qdev->rom->client_monitors_config.count;
>  	}
> @@ -96,8 +96,7 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>  		return status;
>  	}
>  	/* we copy max from the client but it isn't used */
> -	qdev->client_monitors_config->max_allowed =
> -				qdev->monitors_config->max_allowed;
> +	qdev->client_monitors_config->max_allowed = qxl_num_crtc;
>  	for (i = 0 ; i < qdev->client_monitors_config->count ; ++i) {
>  		struct qxl_urect *c_rect =
>  			&qdev->rom->client_monitors_config.heads[i];
> @@ -204,7 +203,7 @@ static int qxl_add_monitors_config_modes(struct
> drm_connector *connector,
>  
>  	if (!qdev->monitors_config)
>  		return 0;
> -	if (h >= qdev->monitors_config->max_allowed)
> +	if (h >= qxl_num_crtc)
>  		return 0;
>  	if (!qdev->client_monitors_config)
>  		return 0;
> @@ -307,8 +306,7 @@ static void qxl_crtc_update_monitors_config(struct
> drm_crtc *crtc,
>  		return;
>  	}
>  
> -	if (!qdev->monitors_config ||
> -	    qdev->monitors_config->max_allowed <= i)
> +	if (!qdev->monitors_config || qxl_num_crtc <= i)
>  		return;
>  
>  	head.id = i;
> @@ -348,9 +346,10 @@ static void qxl_crtc_update_monitors_config(struct
> drm_crtc *crtc,
>  	if (oldcount != qdev->monitors_config->count)
>  		DRM_DEBUG_KMS("active heads %d -> %d (%d total)\n",
>  			      oldcount, qdev->monitors_config->count,
> -			      qdev->monitors_config->max_allowed);
> +			      qxl_num_crtc);
>  
>  	qdev->monitors_config->heads[i] = head;
> +	qdev->monitors_config->max_allowed = qxl_num_crtc;
>  	qxl_send_monitors_config(qdev);
>  }
>  
> @@ -1097,9 +1096,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>  {
>  	int ret;
>  	struct drm_gem_object *gobj;
> -	int max_allowed = qxl_num_crtc;
>  	int monitors_config_size = sizeof(struct qxl_monitors_config) +
> -		max_allowed * sizeof(struct qxl_head);
> +		qxl_num_crtc * sizeof(struct qxl_head);
>  
>  	ret = qxl_gem_object_create(qdev, monitors_config_size, 0,
>  				    QXL_GEM_DOMAIN_VRAM,
> @@ -1121,7 +1119,6 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>  		qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
>  
>  	memset(qdev->monitors_config, 0, monitors_config_size);
> -	qdev->monitors_config->max_allowed = max_allowed;
>  	return 0;
>  }
>  

Frediano

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Frediano Ziglio]

> 
> Just use qxl_num_crtc directly everywhere instead of using
> qdev->monitors_config->max_allowed.  Drops pointless indirection
> and also is less confusing.
> 

To me is MORE confusing, why comparing number of something with
another number? Previously code was comparing number of monitors
with number of monitors, not number of CRTs with number of
monitors.

Why we are optimizing code that, as yours same saying, we are
not much actively improving?

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index ce0b9c40fc..6437369a31 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -80,10 +80,10 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>  		DRM_DEBUG_KMS("no client monitors configured\n");
>  		return status;
>  	}
> -	if (num_monitors > qdev->monitors_config->max_allowed) {
> +	if (num_monitors > qxl_num_crtc) {
>  		DRM_DEBUG_KMS("client monitors list will be truncated: %d < %d\n",
> -			      qdev->monitors_config->max_allowed, num_monitors);
> -		num_monitors = qdev->monitors_config->max_allowed;
> +			      qxl_num_crtc, num_monitors);
> +		num_monitors = qxl_num_crtc;
>  	} else {
>  		num_monitors = qdev->rom->client_monitors_config.count;
>  	}
> @@ -96,8 +96,7 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>  		return status;
>  	}
>  	/* we copy max from the client but it isn't used */
> -	qdev->client_monitors_config->max_allowed =
> -				qdev->monitors_config->max_allowed;
> +	qdev->client_monitors_config->max_allowed = qxl_num_crtc;
>  	for (i = 0 ; i < qdev->client_monitors_config->count ; ++i) {
>  		struct qxl_urect *c_rect =
>  			&qdev->rom->client_monitors_config.heads[i];
> @@ -204,7 +203,7 @@ static int qxl_add_monitors_config_modes(struct
> drm_connector *connector,
>  
>  	if (!qdev->monitors_config)
>  		return 0;
> -	if (h >= qdev->monitors_config->max_allowed)
> +	if (h >= qxl_num_crtc)
>  		return 0;
>  	if (!qdev->client_monitors_config)
>  		return 0;
> @@ -307,8 +306,7 @@ static void qxl_crtc_update_monitors_config(struct
> drm_crtc *crtc,
>  		return;
>  	}
>  
> -	if (!qdev->monitors_config ||
> -	    qdev->monitors_config->max_allowed <= i)
> +	if (!qdev->monitors_config || qxl_num_crtc <= i)
>  		return;
>  
>  	head.id = i;
> @@ -348,9 +346,10 @@ static void qxl_crtc_update_monitors_config(struct
> drm_crtc *crtc,
>  	if (oldcount != qdev->monitors_config->count)
>  		DRM_DEBUG_KMS("active heads %d -> %d (%d total)\n",
>  			      oldcount, qdev->monitors_config->count,
> -			      qdev->monitors_config->max_allowed);
> +			      qxl_num_crtc);
>  
>  	qdev->monitors_config->heads[i] = head;
> +	qdev->monitors_config->max_allowed = qxl_num_crtc;
>  	qxl_send_monitors_config(qdev);
>  }
>  
> @@ -1097,9 +1096,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>  {
>  	int ret;
>  	struct drm_gem_object *gobj;
> -	int max_allowed = qxl_num_crtc;
>  	int monitors_config_size = sizeof(struct qxl_monitors_config) +
> -		max_allowed * sizeof(struct qxl_head);
> +		qxl_num_crtc * sizeof(struct qxl_head);
>  
>  	ret = qxl_gem_object_create(qdev, monitors_config_size, 0,
>  				    QXL_GEM_DOMAIN_VRAM,
> @@ -1121,7 +1119,6 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>  		qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
>  
>  	memset(qdev->monitors_config, 0, monitors_config_size);
> -	qdev->monitors_config->max_allowed = max_allowed;
>  	return 0;
>  }
>  

Frediano

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > 
> > Just use qxl_num_crtc directly everywhere instead of using
> > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > and also is less confusing.
> > 
> 
> To me is MORE confusing, why comparing number of something with
> another number? Previously code was comparing number of monitors
> with number of monitors, not number of CRTs with number of
> monitors.

Yes, spice/qxl and drm/kms use slightly different terminology.

drm crtc == qxl monitor.
drm framebuffer == qxl surface.

You need to know that anyway when looking at the qxl ksm code.  We
have function names like qxl_crtc_update_monitors_config().  I fail
to see why that is a problem ...

cheers,
  Gerd

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > 
> > Just use qxl_num_crtc directly everywhere instead of using
> > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > and also is less confusing.
> > 
> 
> To me is MORE confusing, why comparing number of something with
> another number? Previously code was comparing number of monitors
> with number of monitors, not number of CRTs with number of
> monitors.

Yes, spice/qxl and drm/kms use slightly different terminology.

drm crtc == qxl monitor.
drm framebuffer == qxl surface.

You need to know that anyway when looking at the qxl ksm code.  We
have function names like qxl_crtc_update_monitors_config().  I fail
to see why that is a problem ...

cheers,
  Gerd

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > 
> > Just use qxl_num_crtc directly everywhere instead of using
> > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > and also is less confusing.
> > 
> 
> To me is MORE confusing, why comparing number of something with
> another number? Previously code was comparing number of monitors
> with number of monitors, not number of CRTs with number of
> monitors.

Yes, spice/qxl and drm/kms use slightly different terminology.

drm crtc == qxl monitor.
drm framebuffer == qxl surface.

You need to know that anyway when looking at the qxl ksm code.  We
have function names like qxl_crtc_update_monitors_config().  I fail
to see why that is a problem ...

cheers,
  Gerd

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Frediano Ziglio]

> 
> On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > > 
> > > Just use qxl_num_crtc directly everywhere instead of using
> > > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > > and also is less confusing.
> > > 
> > 
> > To me is MORE confusing, why comparing number of something with
> > another number? Previously code was comparing number of monitors
> > with number of monitors, not number of CRTs with number of
> > monitors.
> 
> Yes, spice/qxl and drm/kms use slightly different terminology.
> 
> drm crtc == qxl monitor.
> drm framebuffer == qxl surface.
> 
> You need to know that anyway when looking at the qxl ksm code.  We
> have function names like qxl_crtc_update_monitors_config().  I fail
> to see why that is a problem ...
> 
> cheers,
>   Gerd
> 
> 

I don't see any problem too but you are explaining to me
why your rationale "and also is less confusing" does not
stand.

Frediano

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Frediano Ziglio]

> 
> On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > > 
> > > Just use qxl_num_crtc directly everywhere instead of using
> > > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > > and also is less confusing.
> > > 
> > 
> > To me is MORE confusing, why comparing number of something with
> > another number? Previously code was comparing number of monitors
> > with number of monitors, not number of CRTs with number of
> > monitors.
> 
> Yes, spice/qxl and drm/kms use slightly different terminology.
> 
> drm crtc == qxl monitor.
> drm framebuffer == qxl surface.
> 
> You need to know that anyway when looking at the qxl ksm code.  We
> have function names like qxl_crtc_update_monitors_config().  I fail
> to see why that is a problem ...
> 
> cheers,
>   Gerd
> 
> 

I don't see any problem too but you are explaining to me
why your rationale "and also is less confusing" does not
stand.

Frediano

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

On Thu, Dec 06, 2018 at 07:53:10AM -0500, Frediano Ziglio wrote:
> > 
> > On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > Just use qxl_num_crtc directly everywhere instead of using
> > > > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > > > and also is less confusing.
> > > > 
> > > 
> > > To me is MORE confusing, why comparing number of something with
> > > another number? Previously code was comparing number of monitors
> > > with number of monitors, not number of CRTs with number of
> > > monitors.
> > 
> > Yes, spice/qxl and drm/kms use slightly different terminology.
> > 
> > drm crtc == qxl monitor.
> > drm framebuffer == qxl surface.
> > 
> > You need to know that anyway when looking at the qxl ksm code.  We
> > have function names like qxl_crtc_update_monitors_config().  I fail
> > to see why that is a problem ...
> > 
> > cheers,
> >   Gerd
> 
> I don't see any problem too but you are explaining to me
> why your rationale "and also is less confusing" does not
> stand.

Well, it's less confusing because it takes away an indirection (not
because of the naming).

qdev->monitors_config->max_allowed is effectively set by a module
parameter.  So using the module parameter variable qxl_num_crtc
directly is better IMO.  The kernel doesn't need to dereference pointers
each time it needs the value, and when reading the code you don't have
to trace where and why qdev->monitors_config->max_allowed is set.

cheers,
  Gerd

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

On Thu, Dec 06, 2018 at 07:53:10AM -0500, Frediano Ziglio wrote:
> > 
> > On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > Just use qxl_num_crtc directly everywhere instead of using
> > > > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > > > and also is less confusing.
> > > > 
> > > 
> > > To me is MORE confusing, why comparing number of something with
> > > another number? Previously code was comparing number of monitors
> > > with number of monitors, not number of CRTs with number of
> > > monitors.
> > 
> > Yes, spice/qxl and drm/kms use slightly different terminology.
> > 
> > drm crtc == qxl monitor.
> > drm framebuffer == qxl surface.
> > 
> > You need to know that anyway when looking at the qxl ksm code.  We
> > have function names like qxl_crtc_update_monitors_config().  I fail
> > to see why that is a problem ...
> > 
> > cheers,
> >   Gerd
> 
> I don't see any problem too but you are explaining to me
> why your rationale "and also is less confusing" does not
> stand.

Well, it's less confusing because it takes away an indirection (not
because of the naming).

qdev->monitors_config->max_allowed is effectively set by a module
parameter.  So using the module parameter variable qxl_num_crtc
directly is better IMO.  The kernel doesn't need to dereference pointers
each time it needs the value, and when reading the code you don't have
to trace where and why qdev->monitors_config->max_allowed is set.

cheers,
  Gerd

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

On Thu, Dec 06, 2018 at 07:53:10AM -0500, Frediano Ziglio wrote:
> > 
> > On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > Just use qxl_num_crtc directly everywhere instead of using
> > > > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > > > and also is less confusing.
> > > > 
> > > 
> > > To me is MORE confusing, why comparing number of something with
> > > another number? Previously code was comparing number of monitors
> > > with number of monitors, not number of CRTs with number of
> > > monitors.
> > 
> > Yes, spice/qxl and drm/kms use slightly different terminology.
> > 
> > drm crtc == qxl monitor.
> > drm framebuffer == qxl surface.
> > 
> > You need to know that anyway when looking at the qxl ksm code.  We
> > have function names like qxl_crtc_update_monitors_config().  I fail
> > to see why that is a problem ...
> > 
> > cheers,
> >   Gerd
> 
> I don't see any problem too but you are explaining to me
> why your rationale "and also is less confusing" does not
> stand.

Well, it's less confusing because it takes away an indirection (not
because of the naming).

qdev->monitors_config->max_allowed is effectively set by a module
parameter.  So using the module parameter variable qxl_num_crtc
directly is better IMO.  The kernel doesn't need to dereference pointers
each time it needs the value, and when reading the code you don't have
to trace where and why qdev->monitors_config->max_allowed is set.

cheers,
  Gerd

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Frediano Ziglio]

> 
> On Thu, Dec 06, 2018 at 07:53:10AM -0500, Frediano Ziglio wrote:
> > > 
> > > On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > Just use qxl_num_crtc directly everywhere instead of using
> > > > > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > > > > and also is less confusing.
> > > > > 
> > > > 
> > > > To me is MORE confusing, why comparing number of something with
> > > > another number? Previously code was comparing number of monitors
> > > > with number of monitors, not number of CRTs with number of
> > > > monitors.
> > > 
> > > Yes, spice/qxl and drm/kms use slightly different terminology.
> > > 
> > > drm crtc == qxl monitor.
> > > drm framebuffer == qxl surface.
> > > 
> > > You need to know that anyway when looking at the qxl ksm code.  We
> > > have function names like qxl_crtc_update_monitors_config().  I fail
> > > to see why that is a problem ...
> > > 
> > > cheers,
> > >   Gerd
> > 
> > I don't see any problem too but you are explaining to me
> > why your rationale "and also is less confusing" does not
> > stand.
> 
> Well, it's less confusing because it takes away an indirection (not
> because of the naming).
> 

It does not confuse me.

> qdev->monitors_config->max_allowed is effectively set by a module
> parameter.  So using the module parameter variable qxl_num_crtc
> directly is better IMO.  The kernel doesn't need to dereference pointers
> each time it needs the value, and when reading the code you don't have
> to trace where and why qdev->monitors_config->max_allowed is set.
> 

That should go to the commit message!
With that the patch is fine for me.

Maybe there's no much point on reusing the same structure used
inside QXLRom/QXLRam but this is OT for this patch.

> cheers,
>   Gerd
> 
> 

Frediano

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Frediano Ziglio]

> 
> On Thu, Dec 06, 2018 at 07:53:10AM -0500, Frediano Ziglio wrote:
> > > 
> > > On Thu, Dec 06, 2018 at 05:59:25AM -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > Just use qxl_num_crtc directly everywhere instead of using
> > > > > qdev->monitors_config->max_allowed.  Drops pointless indirection
> > > > > and also is less confusing.
> > > > > 
> > > > 
> > > > To me is MORE confusing, why comparing number of something with
> > > > another number? Previously code was comparing number of monitors
> > > > with number of monitors, not number of CRTs with number of
> > > > monitors.
> > > 
> > > Yes, spice/qxl and drm/kms use slightly different terminology.
> > > 
> > > drm crtc == qxl monitor.
> > > drm framebuffer == qxl surface.
> > > 
> > > You need to know that anyway when looking at the qxl ksm code.  We
> > > have function names like qxl_crtc_update_monitors_config().  I fail
> > > to see why that is a problem ...
> > > 
> > > cheers,
> > >   Gerd
> > 
> > I don't see any problem too but you are explaining to me
> > why your rationale "and also is less confusing" does not
> > stand.
> 
> Well, it's less confusing because it takes away an indirection (not
> because of the naming).
> 

It does not confuse me.

> qdev->monitors_config->max_allowed is effectively set by a module
> parameter.  So using the module parameter variable qxl_num_crtc
> directly is better IMO.  The kernel doesn't need to dereference pointers
> each time it needs the value, and when reading the code you don't have
> to trace where and why qdev->monitors_config->max_allowed is set.
> 

That should go to the commit message!
With that the patch is fine for me.

Maybe there's no much point on reusing the same structure used
inside QXLRom/QXLRam but this is OT for this patch.

> cheers,
>   Gerd
> 
> 

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

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

> > qdev->monitors_config->max_allowed is effectively set by a module
> > parameter.  So using the module parameter variable qxl_num_crtc
> > directly is better IMO.  The kernel doesn't need to dereference pointers
> > each time it needs the value, and when reading the code you don't have
> > to trace where and why qdev->monitors_config->max_allowed is set.
> 
> That should go to the commit message!
> With that the patch is fine for me.

ok, I'll improve the message.

> Maybe there's no much point on reusing the same structure used
> inside QXLRom/QXLRam but this is OT for this patch.

It's part of the qxl device ABI, the same structure is used for both
host->guest (config hint from spice-client) and guest->host (actual
configuration).

cheers,
  Gerd

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

> > qdev->monitors_config->max_allowed is effectively set by a module
> > parameter.  So using the module parameter variable qxl_num_crtc
> > directly is better IMO.  The kernel doesn't need to dereference pointers
> > each time it needs the value, and when reading the code you don't have
> > to trace where and why qdev->monitors_config->max_allowed is set.
> 
> That should go to the commit message!
> With that the patch is fine for me.

ok, I'll improve the message.

> Maybe there's no much point on reusing the same structure used
> inside QXLRom/QXLRam but this is OT for this patch.

It's part of the qxl device ABI, the same structure is used for both
host->guest (config hint from spice-client) and guest->host (actual
configuration).

cheers,
  Gerd

Re: [Spice-devel] [PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

> > qdev->monitors_config->max_allowed is effectively set by a module
> > parameter.  So using the module parameter variable qxl_num_crtc
> > directly is better IMO.  The kernel doesn't need to dereference pointers
> > each time it needs the value, and when reading the code you don't have
> > to trace where and why qdev->monitors_config->max_allowed is set.
> 
> That should go to the commit message!
> With that the patch is fine for me.

ok, I'll improve the message.

> Maybe there's no much point on reusing the same structure used
> inside QXLRom/QXLRam but this is OT for this patch.

It's part of the qxl device ABI, the same structure is used for both
host->guest (config hint from spice-client) and guest->host (actual
configuration).

cheers,
  Gerd

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

[PATCH] drm/qxl: use qxl_num_crtc directly

[Gerd Hoffmann]

Just use qxl_num_crtc directly everywhere instead of using
qdev->monitors_config->max_allowed.  Drops pointless indirection
and also is less confusing.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_display.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index ce0b9c40fc..6437369a31 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -80,10 +80,10 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 		DRM_DEBUG_KMS("no client monitors configured\n");
 		return status;
 	}
-	if (num_monitors > qdev->monitors_config->max_allowed) {
+	if (num_monitors > qxl_num_crtc) {
 		DRM_DEBUG_KMS("client monitors list will be truncated: %d < %d\n",
-			      qdev->monitors_config->max_allowed, num_monitors);
-		num_monitors = qdev->monitors_config->max_allowed;
+			      qxl_num_crtc, num_monitors);
+		num_monitors = qxl_num_crtc;
 	} else {
 		num_monitors = qdev->rom->client_monitors_config.count;
 	}
@@ -96,8 +96,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 		return status;
 	}
 	/* we copy max from the client but it isn't used */
-	qdev->client_monitors_config->max_allowed =
-				qdev->monitors_config->max_allowed;
+	qdev->client_monitors_config->max_allowed = qxl_num_crtc;
 	for (i = 0 ; i < qdev->client_monitors_config->count ; ++i) {
 		struct qxl_urect *c_rect =
 			&qdev->rom->client_monitors_config.heads[i];
@@ -204,7 +203,7 @@ static int qxl_add_monitors_config_modes(struct drm_connector *connector,
 
 	if (!qdev->monitors_config)
 		return 0;
-	if (h >= qdev->monitors_config->max_allowed)
+	if (h >= qxl_num_crtc)
 		return 0;
 	if (!qdev->client_monitors_config)
 		return 0;
@@ -307,8 +306,7 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
 		return;
 	}
 
-	if (!qdev->monitors_config ||
-	    qdev->monitors_config->max_allowed <= i)
+	if (!qdev->monitors_config || qxl_num_crtc <= i)
 		return;
 
 	head.id = i;
@@ -348,9 +346,10 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
 	if (oldcount != qdev->monitors_config->count)
 		DRM_DEBUG_KMS("active heads %d -> %d (%d total)\n",
 			      oldcount, qdev->monitors_config->count,
-			      qdev->monitors_config->max_allowed);
+			      qxl_num_crtc);
 
 	qdev->monitors_config->heads[i] = head;
+	qdev->monitors_config->max_allowed = qxl_num_crtc;
 	qxl_send_monitors_config(qdev);
 }
 
@@ -1097,9 +1096,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
 {
 	int ret;
 	struct drm_gem_object *gobj;
-	int max_allowed = qxl_num_crtc;
 	int monitors_config_size = sizeof(struct qxl_monitors_config) +
-		max_allowed * sizeof(struct qxl_head);
+		qxl_num_crtc * sizeof(struct qxl_head);
 
 	ret = qxl_gem_object_create(qdev, monitors_config_size, 0,
 				    QXL_GEM_DOMAIN_VRAM,
@@ -1121,7 +1119,6 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
 		qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
 
 	memset(qdev->monitors_config, 0, monitors_config_size);
-	qdev->monitors_config->max_allowed = max_allowed;
 	return 0;
 }
 
-- 
2.9.3