From: Liviu Dudau <liviu.dudau@arm.com>
To: Abhishek Rajput <abhiraj21put@gmail.com>
Cc: maarten.lankhorst@linux.intel.com, mripard@kernel.org,
tzimmermann@suse.de, airlied@gmail.com, simona@ffwll.ch,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/komeda: Convert logging in d71_component.c to drm_* with drm_device parameter
Date: Tue, 11 Nov 2025 18:25:13 +0000 [thread overview]
Message-ID: <aRN_ierYW0i1VT0E@e110455-lin.cambridge.arm.com> (raw)
In-Reply-To: <20251111113717.139401-1-abhiraj21put@gmail.com>
On Tue, Nov 11, 2025 at 05:07:17PM +0530, Abhishek Rajput wrote:
> Replace DRM_ERROR() calls in
> drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> with the corresponding drm_err() helper.
>
> The drm_*() logging helpers take a struct drm_device * as the first
> argument, allowing the DRM core to prefix log messages with the
> corresponding device instance. This improves log traceability when
> multiple display controllers are present.
>
> The drm_device pointer is now safely obtained using
> komeda_kms_attach(d71->mdev), ensuring proper initialization and
> alignment with Komeda’s internal design.
That is absolutely not safe! komeda_kms_attach() is a heavy operation
that allocates a new drm device and it is meant to be used only at
probe time to "attach" a komeda KMS device. You cannot call it
from deep inside the implementation of a komeda_component.
When you've sent the previous patch I did have a look at d71_component.c
before replying but I've realised that it is not that easy to convert
to drm_err(). So we either remove all the error messages, which I'm
reluctant to do as they are useful for bring up, or we leave the file
alone.
For this patch:
NACKED by Liviu Dudau <liviu.dudau@arm.com>
Best regards,
Liviu
>
> This change aligns komeda with the DRM TODO item:
> "Convert logging to drm_* functions with drm_device parameter".
>
> Signed-off-by: Abhishek Rajput <abhiraj21put@gmail.com>
> ---
> .../arm/display/komeda/d71/d71_component.c | 34 ++++++++++++++-----
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> index 67e5d3b4190f..3524ca623d6e 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> @@ -409,6 +409,8 @@ static const struct komeda_component_funcs d71_layer_funcs = {
> static int d71_layer_init(struct d71_dev *d71,
> struct block_header *blk, u32 __iomem *reg)
> {
> + struct komeda_kms_dev *kms = komeda_kms_attach(d71->mdev);
> + struct drm_device *drm = &kms->base;
> struct komeda_component *c;
> struct komeda_layer *layer;
> u32 pipe_id, layer_id, layer_info;
> @@ -421,7 +423,7 @@ static int d71_layer_init(struct d71_dev *d71,
> get_valid_inputs(blk),
> 1, reg, "LPU%d_LAYER%d", pipe_id, layer_id);
> if (IS_ERR(c)) {
> - DRM_ERROR("Failed to add layer component\n");
> + drm_err(drm, "Failed to add layer component\n");
> return PTR_ERR(c);
> }
>
> @@ -527,6 +529,8 @@ static const struct komeda_component_funcs d71_wb_layer_funcs = {
> static int d71_wb_layer_init(struct d71_dev *d71,
> struct block_header *blk, u32 __iomem *reg)
> {
> + struct komeda_kms_dev *kms = komeda_kms_attach(d71->mdev);
> + struct drm_device *drm = &kms->base;
> struct komeda_component *c;
> struct komeda_layer *wb_layer;
> u32 pipe_id, layer_id;
> @@ -539,7 +543,7 @@ static int d71_wb_layer_init(struct d71_dev *d71,
> 1, get_valid_inputs(blk), 0, reg,
> "LPU%d_LAYER_WR", pipe_id);
> if (IS_ERR(c)) {
> - DRM_ERROR("Failed to add wb_layer component\n");
> + drm_err(drm, "Failed to add wb_layer component\n");
> return PTR_ERR(c);
> }
>
> @@ -837,6 +841,8 @@ static const struct komeda_component_funcs d71_scaler_funcs = {
> static int d71_scaler_init(struct d71_dev *d71,
> struct block_header *blk, u32 __iomem *reg)
> {
> + struct komeda_kms_dev *kms = komeda_kms_attach(d71->mdev);
> + struct drm_device *drm = &kms->base;
> struct komeda_component *c;
> struct komeda_scaler *scaler;
> u32 pipe_id, comp_id;
> @@ -851,7 +857,7 @@ static int d71_scaler_init(struct d71_dev *d71,
> pipe_id, BLOCK_INFO_BLK_ID(blk->block_info));
>
> if (IS_ERR(c)) {
> - DRM_ERROR("Failed to initialize scaler");
> + drm_err(drm, "Failed to initialize scaler");
> return PTR_ERR(c);
> }
>
> @@ -945,6 +951,8 @@ static const struct komeda_component_funcs d71_splitter_funcs = {
> static int d71_splitter_init(struct d71_dev *d71,
> struct block_header *blk, u32 __iomem *reg)
> {
> + struct komeda_kms_dev *kms = komeda_kms_attach(d71->mdev);
> + struct drm_device *drm = &kms->base;
> struct komeda_component *c;
> struct komeda_splitter *splitter;
> u32 pipe_id, comp_id;
> @@ -959,7 +967,7 @@ static int d71_splitter_init(struct d71_dev *d71,
> "CU%d_SPLITTER", pipe_id);
>
> if (IS_ERR(c)) {
> - DRM_ERROR("Failed to initialize splitter");
> + drm_err(drm, "Failed to initialize splitter");
> return -1;
> }
>
> @@ -1015,6 +1023,8 @@ static const struct komeda_component_funcs d71_merger_funcs = {
> static int d71_merger_init(struct d71_dev *d71,
> struct block_header *blk, u32 __iomem *reg)
> {
> + struct komeda_kms_dev *kms = komeda_kms_attach(d71->mdev);
> + struct drm_device *drm = &kms->base;
> struct komeda_component *c;
> struct komeda_merger *merger;
> u32 pipe_id, comp_id;
> @@ -1030,7 +1040,7 @@ static int d71_merger_init(struct d71_dev *d71,
> "CU%d_MERGER", pipe_id);
>
> if (IS_ERR(c)) {
> - DRM_ERROR("Failed to initialize merger.\n");
> + drm_err(drm, "Failed to initialize merger.\n");
> return PTR_ERR(c);
> }
>
> @@ -1126,6 +1136,8 @@ static const struct komeda_component_funcs d71_improc_funcs = {
> static int d71_improc_init(struct d71_dev *d71,
> struct block_header *blk, u32 __iomem *reg)
> {
> + struct komeda_kms_dev *kms = komeda_kms_attach(d71->mdev);
> + struct drm_device *drm = &kms->base;
> struct komeda_component *c;
> struct komeda_improc *improc;
> u32 pipe_id, comp_id, value;
> @@ -1139,7 +1151,7 @@ static int d71_improc_init(struct d71_dev *d71,
> get_valid_inputs(blk),
> IPS_NUM_OUTPUT_IDS, reg, "DOU%d_IPS", pipe_id);
> if (IS_ERR(c)) {
> - DRM_ERROR("Failed to add improc component\n");
> + drm_err(drm, "Failed to add improc component\n");
> return PTR_ERR(c);
> }
>
> @@ -1253,6 +1265,8 @@ static const struct komeda_component_funcs d71_timing_ctrlr_funcs = {
> static int d71_timing_ctrlr_init(struct d71_dev *d71,
> struct block_header *blk, u32 __iomem *reg)
> {
> + struct komeda_kms_dev *kms = komeda_kms_attach(d71->mdev);
> + struct drm_device *drm = &kms->base;
> struct komeda_component *c;
> struct komeda_timing_ctrlr *ctrlr;
> u32 pipe_id, comp_id;
> @@ -1266,7 +1280,7 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
> 1, BIT(KOMEDA_COMPONENT_IPS0 + pipe_id),
> BS_NUM_OUTPUT_IDS, reg, "DOU%d_BS", pipe_id);
> if (IS_ERR(c)) {
> - DRM_ERROR("Failed to add display_ctrl component\n");
> + drm_err(drm, "Failed to add display_ctrl component\n");
> return PTR_ERR(c);
> }
>
> @@ -1280,6 +1294,8 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
> int d71_probe_block(struct d71_dev *d71,
> struct block_header *blk, u32 __iomem *reg)
> {
> + struct komeda_kms_dev *kms = komeda_kms_attach(d71->mdev);
> + struct drm_device *drm = &kms->base;
> struct d71_pipeline *pipe;
> int blk_id = BLOCK_INFO_BLK_ID(blk->block_info);
>
> @@ -1346,8 +1362,8 @@ int d71_probe_block(struct d71_dev *d71,
> break;
>
> default:
> - DRM_ERROR("Unknown block (block_info: 0x%x) is found\n",
> - blk->block_info);
> + drm_err(drm, "Unknown block (block_info: 0x%x) is found\n",
> + blk->block_info);
> err = -EINVAL;
> break;
> }
> --
> 2.43.0
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
prev parent reply other threads:[~2025-11-11 18:25 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-11 11:37 Abhishek Rajput
2025-11-11 18:25 ` Liviu Dudau [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aRN_ierYW0i1VT0E@e110455-lin.cambridge.arm.com \
--to=liviu.dudau@arm.com \
--cc=abhiraj21put@gmail.com \
--cc=airlied@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=simona@ffwll.ch \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®