| Message ID | 20161030084926.15303-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, BTW, memory allocation in 'sun4i_layers_init()' looks spurious, especially the use of 'layer' in the for loop. Just my 2 cents. I also forgot to say that we could propagate the error code returned by sun4i_layers_init instead of returning -EINVAL unconditionally CJ Le 30/10/2016 à 09:49, Christophe JAILLET a écrit : > 'sun4i_layers_init()' returns an error pointer in case of error, not > NULL. So test it with IS_ERR. > > Signed-off-by: Christophe JAILLET<christophe.jaillet@wanadoo.fr> > --- > drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c > index 9776f0305834..628712e6edd6 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_drv.c > +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c > @@ -143,7 +143,7 @@ static int sun4i_drv_bind(struct device *dev) > > /* Create our layers */ > drv->layers = sun4i_layers_init(drm); > - if (!drv->layers) { > + if (IS_ERR(drv->layers)) { > dev_err(drm->dev, "Couldn't create the planes\n"); > ret = -EINVAL; > goto free_drm;
On Sun, Oct 30, 2016 at 09:49:26AM +0100, Christophe JAILLET wrote: > 'sun4i_layers_init()' returns an error pointer in case of error, not > NULL. So test it with IS_ERR. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Applied, thanks! Maxime
Hi, On Sun, Oct 30, 2016 at 12:53:02PM +0100, Christophe JAILLET wrote: > BTW, memory allocation in 'sun4i_layers_init()' looks spurious, especially > the use of 'layer' in the for loop. > Just my 2 cents. What do you mean by it's spurious? > I also forgot to say that we could propagate the error code returned by > sun4i_layers_init instead of returning -EINVAL unconditionally Indeed. Can you send a patch for that? Thanks, Maxime
Le 02/11/2016 à 19:14, Maxime Ripard a écrit : > Hi, > > On Sun, Oct 30, 2016 at 12:53:02PM +0100, Christophe JAILLET wrote: >> BTW, memory allocation in 'sun4i_layers_init()' looks spurious, especially >> the use of 'layer' in the for loop. >> Just my 2 cents. > What do you mean by it's spurious? Hi Maxime, what I mean is: > struct sun4i_layer **sun4i_layers_init(struct drm_device *drm) > { > struct sun4i_drv *drv = drm->dev_private; > struct sun4i_layer **layers; > int i; > > layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes), > sizeof(**layers), GFP_KERNEL); Here, we allocate some memory for ARRAY_SIZE(sun4i_backend_planes) (i.e. 2) 'struct sun4i_layer'. We do not allocate some space for some pointers but for some structure. Also, these structures are zeroed and seem to never be initialized by anything else. > if (!layers) > return ERR_PTR(-ENOMEM); > > /* > * The hardware is a bit unusual here. > * > * Even though it supports 4 layers, it does the composition > * in two separate steps. > * > * The first one is assigning a layer to one of its two > * pipes. If more that 1 layer is assigned to the same pipe, > * and if pixels overlaps, the pipe will take the pixel from > * the layer with the highest priority. > * > * The second step is the actual alpha blending, that takes > * the two pipes as input, and uses the eventual alpha > * component to do the transparency between the two. > * > * This two steps scenario makes us unable to guarantee a > * robust alpha blending between the 4 layers in all > * situations. So we just expose two layers, one per pipe. On > * SoCs that support it, sprites could fill the need for more > * layers. > */ The comment make me think that this driver (and this function) only handles 2 layers ("So we just expose two layers"), which is consistent with ARRAY_SIZE(sun4i_backend_planes) (i.e. 2) So I would expect that only 2 'struct sun4i_layer' to be allcoated > for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) { > const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i]; > struct sun4i_layer *layer = layers[i]; Here, we take the address of one of the 2 structure allocated above. This is overridden, just the line after. > > layer = sun4i_layer_init_one(drm, plane); 'sun4i_layer_init_one()' looks() like: struct sun4i_layer *layer; layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL); ... return layer; So we once more allocate some 'struct sun4i_layer' BUT, the corresponding address is stored into the 'layer' variable, and finally seems to get lost and no reference is kept of this. (i.e. 'layers' (with an s) is left unchanged) > if (IS_ERR(layer)) { > dev_err(drm->dev, "Couldn't initialize %s plane\n", > i ? "overlay" : "primary"); > return ERR_CAST(layer); > }; > > DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n", > i ? "overlay" : "primary", plane->pipe); > regmap_update_bits(drv->backend->regs, SUN4I_BACKEND_ATTCTL_REG0(i), > SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK, > SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(plane->pipe)); > > layer->id = i; > }; > > return layers; > } So, 4 'struct sun4i_layer' have been allocated. 2 in 'sun4i_layers_init()' and 2 in 'sun4i_layer_init_one()' (once at a time, but called twice) What looks spurious to me is either: - 'struct sun4i_layer *layer = layers[i];' which should just be 'struct sun4i_layer *layer;' or - 'layers' (with an s) should be an array of pointers and the addresses returned by 'sun4i_layer_init_one()' should be saved there. I don't know at all this driver, so I'm maybe completely wrong. What I would have expected would be something like: (un-tested, just to give an idea) ==============8<================================================ @@ -133,9 +133,9 @@ struct sun4i_layer **sun4i_layers_init(struct drm_device *drm) struct sun4i_layer **layers; int i; layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes), - sizeof(**layers), GFP_KERNEL); + sizeof(*layers), GFP_KERNEL); if (!layers) return ERR_PTR(-ENOMEM); /* @@ -160,16 +160,17 @@ struct sun4i_layer **sun4i_layers_init(struct drm_device *drm) * layers. */ for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) { const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i]; - struct sun4i_layer *layer = layers[i]; + struct sun4i_layer *layer; layer = sun4i_layer_init_one(drm, plane); if (IS_ERR(layer)) { dev_err(drm->dev, "Couldn't initialize %s plane\n", i ? "overlay" : "primary"); return ERR_CAST(layer); }; + layers[i] = layer; DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n", i ? "overlay" : "primary", plane->pipe); regmap_update_bits(drv->backend->regs, SUN4I_BACKEND_ATTCTL_REG0(i), Best regards, CJ
Salut, On Sat, Nov 05, 2016 at 07:15:45AM +0100, Christophe JAILLET wrote: > Le 02/11/2016 à 19:14, Maxime Ripard a écrit : > > Hi, > > > > On Sun, Oct 30, 2016 at 12:53:02PM +0100, Christophe JAILLET wrote: > > > BTW, memory allocation in 'sun4i_layers_init()' looks spurious, especially > > > the use of 'layer' in the for loop. > > > Just my 2 cents. > > What do you mean by it's spurious? > Hi Maxime, > > what I mean is: > > > struct sun4i_layer **sun4i_layers_init(struct drm_device *drm) > > { > > struct sun4i_drv *drv = drm->dev_private; > > struct sun4i_layer **layers; > > int i; > > > > layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes), > > sizeof(**layers), GFP_KERNEL); > Here, we allocate some memory for ARRAY_SIZE(sun4i_backend_planes) (i.e. 2) > 'struct sun4i_layer'. > We do not allocate some space for some pointers but for some structure. > > Also, these structures are zeroed and seem to never be initialized by > anything else. > > > if (!layers) > > return ERR_PTR(-ENOMEM); > > > > /* > > * The hardware is a bit unusual here. > > * > > * Even though it supports 4 layers, it does the composition > > * in two separate steps. > > * > > * The first one is assigning a layer to one of its two > > * pipes. If more that 1 layer is assigned to the same pipe, > > * and if pixels overlaps, the pipe will take the pixel from > > * the layer with the highest priority. > > * > > * The second step is the actual alpha blending, that takes > > * the two pipes as input, and uses the eventual alpha > > * component to do the transparency between the two. > > * > > * This two steps scenario makes us unable to guarantee a > > * robust alpha blending between the 4 layers in all > > * situations. So we just expose two layers, one per pipe. On > > * SoCs that support it, sprites could fill the need for more > > * layers. > > */ > The comment make me think that this driver (and this function) only handles > 2 layers ("So we just expose two layers"), which is consistent with > ARRAY_SIZE(sun4i_backend_planes) (i.e. 2) > So I would expect that only 2 'struct sun4i_layer' to be allcoated > > > for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) { > > const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i]; > > struct sun4i_layer *layer = layers[i]; > Here, we take the address of one of the 2 structure allocated above. > This is overridden, just the line after. > > > > > layer = sun4i_layer_init_one(drm, plane); > 'sun4i_layer_init_one()' looks() like: > > struct sun4i_layer *layer; > layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL); > ... > return layer; > > So we once more allocate some 'struct sun4i_layer' > > BUT, the corresponding address is stored into the 'layer' variable, and > finally seems to get lost and no reference is kept of this. (i.e. 'layers' > (with an s) is left unchanged) > > > if (IS_ERR(layer)) { > > dev_err(drm->dev, "Couldn't initialize %s plane\n", > > i ? "overlay" : "primary"); > > return ERR_CAST(layer); > > }; > > > > DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n", > > i ? "overlay" : "primary", plane->pipe); > > regmap_update_bits(drv->backend->regs, > SUN4I_BACKEND_ATTCTL_REG0(i), > > SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK, > > SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(plane->pipe)); > > > > layer->id = i; > > }; > > > > return layers; > > } > > > So, 4 'struct sun4i_layer' have been allocated. 2 in 'sun4i_layers_init()' > and 2 in 'sun4i_layer_init_one()' (once at a time, but called twice) > > What looks spurious to me is either: > - 'struct sun4i_layer *layer = layers[i];' which should just be 'struct > sun4i_layer *layer;' > or > - 'layers' (with an s) should be an array of pointers and the addresses > returned by 'sun4i_layer_init_one()' should be saved there. > > > I don't know at all this driver, so I'm maybe completely wrong. > What I would have expected would be something like: (un-tested, just to give > an idea) > > > ==============8<================================================ > > @@ -133,9 +133,9 @@ struct sun4i_layer **sun4i_layers_init(struct drm_device > *drm) > struct sun4i_layer **layers; > int i; > > layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes), > - sizeof(**layers), GFP_KERNEL); > + sizeof(*layers), GFP_KERNEL); > if (!layers) > return ERR_PTR(-ENOMEM); > > /* > @@ -160,16 +160,17 @@ struct sun4i_layer **sun4i_layers_init(struct > drm_device *drm) > * layers. > */ > for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) { > const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i]; > - struct sun4i_layer *layer = layers[i]; > + struct sun4i_layer *layer; > > layer = sun4i_layer_init_one(drm, plane); > if (IS_ERR(layer)) { > dev_err(drm->dev, "Couldn't initialize %s plane\n", > i ? "overlay" : "primary"); > return ERR_CAST(layer); > }; > + layers[i] = layer; > > DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n", > i ? "overlay" : "primary", plane->pipe); > regmap_update_bits(drv->backend->regs, > SUN4I_BACKEND_ATTCTL_REG0(i), You're totally right. Can you send this as a formal patch? Thanks! Maxime
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index 9776f0305834..628712e6edd6 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -143,7 +143,7 @@ static int sun4i_drv_bind(struct device *dev) /* Create our layers */ drv->layers = sun4i_layers_init(drm); - if (!drv->layers) { + if (IS_ERR(drv->layers)) { dev_err(drm->dev, "Couldn't create the planes\n"); ret = -EINVAL; goto free_drm;
'sun4i_layers_init()' returns an error pointer in case of error, not NULL. So test it with IS_ERR. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)