Re: [PATCH 04/12] drm/nouveau/bar/nvc0: support chips without BAR3

[Thierry Reding]

On Mon, Mar 24, 2014 at 05:42:26PM +0900, Alexandre Courbot wrote:
[...]
> diff --git a/drivers/gpu/drm/nouveau/core/subdev/bar/nvc0.c b/drivers/gpu/drm/nouveau/core/subdev/bar/nvc0.c
[...]
>  static int
> -nvc0_bar_ctor(struct nouveau_object *parent, struct nouveau_object *engine,
> -	      struct nouveau_oclass *oclass, void *data, u32 size,
> -	      struct nouveau_object **pobject)
> +nvc0_bar_init_vm(struct nvc0_bar_priv *priv, int nr, int bar)
>  {
[...]
> -	/* BAR3 */
>  	ret = nouveau_gpuobj_new(nv_object(priv), NULL, 0x1000, 0, 0,
> -				&priv->bar[0].mem);
> -	mem = priv->bar[0].mem;
> +				&priv->bar[nr].mem);
> +	mem = priv->bar[nr].mem;
>  	if (ret)
>  		return ret;
>  
>  	ret = nouveau_gpuobj_new(nv_object(priv), NULL, 0x8000, 0, 0,
> -				&priv->bar[0].pgd);
> +				&priv->bar[nr].pgd);
>  	if (ret)
>  		return ret;
[...]
> +static int
> +nvc0_bar_ctor(struct nouveau_object *parent, struct nouveau_object *engine,
> +	      struct nouveau_oclass *oclass, void *data, u32 size,
> +	      struct nouveau_object **pobject)
> +{
[...]
> +	/* BAR3 */
> +	if (has_bar3) {
> +		ret = nvc0_bar_init_vm(priv, 0, 3);
[...]
> +	/* BAR1 */
> +	ret = nvc0_bar_init_vm(priv, 1, 1);
>  	if (ret)
>  		return ret;

The calls to nvc0_bar_init_vm() are somewhat confusing in my opinion. It
is hard to see from the invocation what these numbers mean and therefore
distinguish which parameter is which.

Perhaps a slightly more readable way would be to pass in a pointer to a
structure as second parameter instead of the index into an array. So
it'd look somewhat like this:

	if (has_bar3) {
		ret = nvc0_bar_init_vm(priv, &priv->bar[0], 3);
		...
	}
	...
	ret = nvc0_bar_init_vm(priv, &priv->bar[1], 1);
	...

Unfortunately that would require a new type to be created for the bar[]
structures, so it'd be slightly more intrusive.

Thierry

Attachment: pgpGepnzLCHyv.pgp
Description: PGP signature