| Message ID | 20210113170253.443820-1-kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: rcar-du: Use drmm_encoder_alloc() to manage encoder | expand |
Hi Kieran, Thank you for the patch. On Wed, Jan 13, 2021 at 05:02:53PM +0000, Kieran Bingham wrote: > The encoder allocation was converted to a DRM managed resource at the > same time as the addition of a new helper drmm_encoder_alloc() which > simplifies the same process. > > Convert the custom drm managed resource allocation of the encoder > with the helper to simplify the implementation, and prevent hitting a > WARN_ON() due to the handling the drm_encoder_init() call directly > without registering a .destroy() function op. > > Fixes: f5f16725edbc ("drm: rcar-du: Use DRM-managed allocation for encoders") We could equally point to the patch that has added drmm_encoder_alloc(), but I'm fine taking the blame :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 31 +++++------------------ > 1 file changed, 6 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > index ba8c6038cd63..ca3761772211 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > @@ -48,21 +48,12 @@ static unsigned int rcar_du_encoder_count_ports(struct device_node *node) > static const struct drm_encoder_funcs rcar_du_encoder_funcs = { > }; > > -static void rcar_du_encoder_release(struct drm_device *dev, void *res) > -{ > - struct rcar_du_encoder *renc = res; > - > - drm_encoder_cleanup(&renc->base); > - kfree(renc); > -} > - > int rcar_du_encoder_init(struct rcar_du_device *rcdu, > enum rcar_du_output output, > struct device_node *enc_node) > { > struct rcar_du_encoder *renc; > struct drm_bridge *bridge; > - int ret; > > /* > * Locate the DRM bridge from the DT node. For the DPAD outputs, if the > @@ -101,26 +92,16 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, > return -ENOLINK; > } > > - renc = kzalloc(sizeof(*renc), GFP_KERNEL); > - if (renc == NULL) > - return -ENOMEM; > - > - renc->output = output; > - > dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n", > enc_node, output); > > - ret = drm_encoder_init(&rcdu->ddev, &renc->base, &rcar_du_encoder_funcs, > - DRM_MODE_ENCODER_NONE, NULL); > - if (ret < 0) { > - kfree(renc); > - return ret; > - } > + renc = drmm_encoder_alloc(&rcdu->ddev, struct rcar_du_encoder, base, > + &rcar_du_encoder_funcs, DRM_MODE_ENCODER_NONE, > + NULL); > + if (!renc) > + return -ENOMEM; > > - ret = drmm_add_action_or_reset(&rcdu->ddev, rcar_du_encoder_release, > - renc); > - if (ret) > - return ret; > + renc->output = output; > > /* > * Attach the bridge to the encoder. The bridge will create the
On Wed, Jan 13, 2021 at 6:02 PM Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> wrote: > The encoder allocation was converted to a DRM managed resource at the > same time as the addition of a new helper drmm_encoder_alloc() which > simplifies the same process. > > Convert the custom drm managed resource allocation of the encoder > with the helper to simplify the implementation, and prevent hitting a > WARN_ON() due to the handling the drm_encoder_init() call directly > without registering a .destroy() function op. > > Fixes: f5f16725edbc ("drm: rcar-du: Use DRM-managed allocation for encoders") > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> As in "the WARNING from drm_encoder_init() is gone". Gr{oetje,eeting}s, Geert
Hi Laurent, On 13/01/2021 22:45, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Wed, Jan 13, 2021 at 05:02:53PM +0000, Kieran Bingham wrote: >> The encoder allocation was converted to a DRM managed resource at the >> same time as the addition of a new helper drmm_encoder_alloc() which >> simplifies the same process. >> >> Convert the custom drm managed resource allocation of the encoder >> with the helper to simplify the implementation, and prevent hitting a >> WARN_ON() due to the handling the drm_encoder_init() call directly >> without registering a .destroy() function op. >> >> Fixes: f5f16725edbc ("drm: rcar-du: Use DRM-managed allocation for encoders") > > We could equally point to the patch that has added drmm_encoder_alloc(), > but I'm fine taking the blame :-) Perhaps, we could point there indeed, I'm surprised that patch/series didn't seem to add any users of drmm_encoder_alloc() as far as I can see. I don't think this is a "blame" though. Just a reference to the most relevant change. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Thanks. Kieran >> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> --- >> drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 31 +++++------------------ >> 1 file changed, 6 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c >> index ba8c6038cd63..ca3761772211 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c >> @@ -48,21 +48,12 @@ static unsigned int rcar_du_encoder_count_ports(struct device_node *node) >> static const struct drm_encoder_funcs rcar_du_encoder_funcs = { >> }; >> >> -static void rcar_du_encoder_release(struct drm_device *dev, void *res) >> -{ >> - struct rcar_du_encoder *renc = res; >> - >> - drm_encoder_cleanup(&renc->base); >> - kfree(renc); >> -} >> - >> int rcar_du_encoder_init(struct rcar_du_device *rcdu, >> enum rcar_du_output output, >> struct device_node *enc_node) >> { >> struct rcar_du_encoder *renc; >> struct drm_bridge *bridge; >> - int ret; >> >> /* >> * Locate the DRM bridge from the DT node. For the DPAD outputs, if the >> @@ -101,26 +92,16 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, >> return -ENOLINK; >> } >> >> - renc = kzalloc(sizeof(*renc), GFP_KERNEL); >> - if (renc == NULL) >> - return -ENOMEM; >> - >> - renc->output = output; >> - >> dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n", >> enc_node, output); >> >> - ret = drm_encoder_init(&rcdu->ddev, &renc->base, &rcar_du_encoder_funcs, >> - DRM_MODE_ENCODER_NONE, NULL); >> - if (ret < 0) { >> - kfree(renc); >> - return ret; >> - } >> + renc = drmm_encoder_alloc(&rcdu->ddev, struct rcar_du_encoder, base, >> + &rcar_du_encoder_funcs, DRM_MODE_ENCODER_NONE, >> + NULL); >> + if (!renc) >> + return -ENOMEM; >> >> - ret = drmm_add_action_or_reset(&rcdu->ddev, rcar_du_encoder_release, >> - renc); >> - if (ret) >> - return ret; >> + renc->output = output; >> >> /* >> * Attach the bridge to the encoder. The bridge will create the >
On Thu, Jan 14, 2021 at 11:00 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Jan 13, 2021 at 6:02 PM Kieran Bingham > <kieran.bingham+renesas@ideasonboard.com> wrote: > > The encoder allocation was converted to a DRM managed resource at the > > same time as the addition of a new helper drmm_encoder_alloc() which > > simplifies the same process. > > > > Convert the custom drm managed resource allocation of the encoder > > with the helper to simplify the implementation, and prevent hitting a > > WARN_ON() due to the handling the drm_encoder_init() call directly > > without registering a .destroy() function op. > > > > Fixes: f5f16725edbc ("drm: rcar-du: Use DRM-managed allocation for encoders") > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > As in "the WARNING from drm_encoder_init() is gone". JFYI, the WARNING is now in v5.12-rc1, the fix isn't. Gr{oetje,eeting}s, Geert
Laurent, On 01/03/2021 10:30, Geert Uytterhoeven wrote: > On Thu, Jan 14, 2021 at 11:00 AM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Wed, Jan 13, 2021 at 6:02 PM Kieran Bingham >> <kieran.bingham+renesas@ideasonboard.com> wrote: >>> The encoder allocation was converted to a DRM managed resource at the >>> same time as the addition of a new helper drmm_encoder_alloc() which >>> simplifies the same process. >>> >>> Convert the custom drm managed resource allocation of the encoder >>> with the helper to simplify the implementation, and prevent hitting a >>> WARN_ON() due to the handling the drm_encoder_init() call directly >>> without registering a .destroy() function op. >>> >>> Fixes: f5f16725edbc ("drm: rcar-du: Use DRM-managed allocation for encoders") >>> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> >> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> >> As in "the WARNING from drm_encoder_init() is gone". > > JFYI, the WARNING is now in v5.12-rc1, the fix isn't. Does this patch go through you to get into the DRM tree? Or do I need to re-send it to someone else? > Gr{oetje,eeting}s, > > Geert >
Hi Kieran, On Mon, Mar 01, 2021 at 11:52:26AM +0000, Kieran Bingham wrote: > On 01/03/2021 10:30, Geert Uytterhoeven wrote: > > On Thu, Jan 14, 2021 at 11:00 AM Geert Uytterhoeven wrote: > >> On Wed, Jan 13, 2021 at 6:02 PM Kieran Bingham wrote: > >>> The encoder allocation was converted to a DRM managed resource at the > >>> same time as the addition of a new helper drmm_encoder_alloc() which > >>> simplifies the same process. > >>> > >>> Convert the custom drm managed resource allocation of the encoder > >>> with the helper to simplify the implementation, and prevent hitting a > >>> WARN_ON() due to the handling the drm_encoder_init() call directly > >>> without registering a .destroy() function op. > >>> > >>> Fixes: f5f16725edbc ("drm: rcar-du: Use DRM-managed allocation for encoders") > >>> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > >>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> > >> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > >> As in "the WARNING from drm_encoder_init() is gone". > > > > JFYI, the WARNING is now in v5.12-rc1, the fix isn't. > > Does this patch go through you to get into the DRM tree? Or do I need to > re-send it to someone else? I can send a pull request with this for the -fixes branch, but as there's a single patch, maybe David or Daniel could pick it up ?
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index ba8c6038cd63..ca3761772211 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -48,21 +48,12 @@ static unsigned int rcar_du_encoder_count_ports(struct device_node *node) static const struct drm_encoder_funcs rcar_du_encoder_funcs = { }; -static void rcar_du_encoder_release(struct drm_device *dev, void *res) -{ - struct rcar_du_encoder *renc = res; - - drm_encoder_cleanup(&renc->base); - kfree(renc); -} - int rcar_du_encoder_init(struct rcar_du_device *rcdu, enum rcar_du_output output, struct device_node *enc_node) { struct rcar_du_encoder *renc; struct drm_bridge *bridge; - int ret; /* * Locate the DRM bridge from the DT node. For the DPAD outputs, if the @@ -101,26 +92,16 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, return -ENOLINK; } - renc = kzalloc(sizeof(*renc), GFP_KERNEL); - if (renc == NULL) - return -ENOMEM; - - renc->output = output; - dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n", enc_node, output); - ret = drm_encoder_init(&rcdu->ddev, &renc->base, &rcar_du_encoder_funcs, - DRM_MODE_ENCODER_NONE, NULL); - if (ret < 0) { - kfree(renc); - return ret; - } + renc = drmm_encoder_alloc(&rcdu->ddev, struct rcar_du_encoder, base, + &rcar_du_encoder_funcs, DRM_MODE_ENCODER_NONE, + NULL); + if (!renc) + return -ENOMEM; - ret = drmm_add_action_or_reset(&rcdu->ddev, rcar_du_encoder_release, - renc); - if (ret) - return ret; + renc->output = output; /* * Attach the bridge to the encoder. The bridge will create the
The encoder allocation was converted to a DRM managed resource at the same time as the addition of a new helper drmm_encoder_alloc() which simplifies the same process. Convert the custom drm managed resource allocation of the encoder with the helper to simplify the implementation, and prevent hitting a WARN_ON() due to the handling the drm_encoder_init() call directly without registering a .destroy() function op. Fixes: f5f16725edbc ("drm: rcar-du: Use DRM-managed allocation for encoders") Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> --- drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 31 +++++------------------ 1 file changed, 6 insertions(+), 25 deletions(-)