* [PATCH] drm/sun4i: Fix error handling
@ 2016-10-30 8:49 Christophe JAILLET
2016-10-30 11:53 ` Christophe JAILLET
2016-11-02 17:57 ` Maxime Ripard
0 siblings, 2 replies; 6+ messages in thread
From: Christophe JAILLET @ 2016-10-30 8:49 UTC (permalink / raw)
To: maxime.ripard, airlied, wens
Cc: dri-devel, linux-arm-kernel, linux-kernel, kernel-janitors,
Christophe JAILLET
'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;
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sun4i: Fix error handling
2016-10-30 8:49 [PATCH] drm/sun4i: Fix error handling Christophe JAILLET
@ 2016-10-30 11:53 ` Christophe JAILLET
2016-11-02 18:14 ` Maxime Ripard
2016-11-02 17:57 ` Maxime Ripard
1 sibling, 1 reply; 6+ messages in thread
From: Christophe JAILLET @ 2016-10-30 11:53 UTC (permalink / raw)
To: maxime.ripard, airlied, wens
Cc: dri-devel, linux-arm-kernel, linux-kernel, kernel-janitors
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sun4i: Fix error handling
2016-10-30 8:49 [PATCH] drm/sun4i: Fix error handling Christophe JAILLET
2016-10-30 11:53 ` Christophe JAILLET
@ 2016-11-02 17:57 ` Maxime Ripard
1 sibling, 0 replies; 6+ messages in thread
From: Maxime Ripard @ 2016-11-02 17:57 UTC (permalink / raw)
To: Christophe JAILLET
Cc: airlied, wens, dri-devel, linux-arm-kernel, linux-kernel,
kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 379 bytes --]
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
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sun4i: Fix error handling
2016-10-30 11:53 ` Christophe JAILLET
@ 2016-11-02 18:14 ` Maxime Ripard
2016-11-05 6:15 ` Christophe JAILLET
0 siblings, 1 reply; 6+ messages in thread
From: Maxime Ripard @ 2016-11-02 18:14 UTC (permalink / raw)
To: Christophe JAILLET
Cc: airlied, wens, dri-devel, linux-arm-kernel, linux-kernel,
kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 559 bytes --]
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
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sun4i: Fix error handling
2016-11-02 18:14 ` Maxime Ripard
@ 2016-11-05 6:15 ` Christophe JAILLET
2016-11-08 20:38 ` Maxime Ripard
0 siblings, 1 reply; 6+ messages in thread
From: Christophe JAILLET @ 2016-11-05 6:15 UTC (permalink / raw)
To: Maxime Ripard
Cc: airlied, wens, dri-devel, linux-arm-kernel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sun4i: Fix error handling
2016-11-05 6:15 ` Christophe JAILLET
@ 2016-11-08 20:38 ` Maxime Ripard
0 siblings, 0 replies; 6+ messages in thread
From: Maxime Ripard @ 2016-11-08 20:38 UTC (permalink / raw)
To: Christophe JAILLET
Cc: airlied, wens, dri-devel, linux-arm-kernel, linux-kernel,
kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 5922 bytes --]
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
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-08 20:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-30 8:49 [PATCH] drm/sun4i: Fix error handling Christophe JAILLET
2016-10-30 11:53 ` Christophe JAILLET
2016-11-02 18:14 ` Maxime Ripard
2016-11-05 6:15 ` Christophe JAILLET
2016-11-08 20:38 ` Maxime Ripard
2016-11-02 17:57 ` Maxime Ripard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).