On 5/23/2023 4:53 PM, Dmitry Baryshkov wrote:
On Wed, 24 May 2023 at 02:37, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 5/21/2023 12:22 PM, Dmitry Baryshkov wrote:
Use drm_debugfs_add_file() for encoder's status file. This changes the
name of the status file from encoder%d/status to just encoder%d.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
This patch depends on
https://patchwork.freedesktop.org/patch/538294/?series=118079&rev=1 right?
No, there is no dependency. I have sent that patch as we discussed it
earlier. But this one is a reimplementation of the previous idea.
In this patch you are also removing the early_unregister callback.
.early_unregister = dpu_encoder_early_unregister
Which we discussed was needed to balance the corner case we discussed.
The DRM core patch fixes the corner case by calling debugfs_cleanup()
even when drm_modeset_register_all() fails.
So isnt there a dependency?
What is wrong with having a per encoder directory and reading from
there? It gives room for expanding this to dump more encoder specific
information.
At the moment it looks light because we have only status but better to
have a directory per encoder right?
I started writing that I can not imagine additional per-encoder data,
but then I found the generic enough piece: bridge chain enumeration.
I'll give it additional thought and maybe I'll refactor this patch
further.
Ack,
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 40 ++++++---------------
1 file changed, 11 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index af34932729db..0ac68f44ec74 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -14,6 +14,7 @@
#include <drm/drm_atomic.h>
#include <drm/drm_crtc.h>
+#include <drm/drm_debugfs.h>
#include <drm/drm_file.h>
#include <drm/drm_probe_helper.h>
@@ -142,7 +143,6 @@ enum dpu_enc_rc_states {
* @crtc_kickoff_cb: Callback into CRTC that will flush & start
* all CTL paths
* @crtc_kickoff_cb_data: Opaque user data given to crtc_kickoff_cb
- * @debugfs_root: Debug file system root file node
* @enc_lock: Lock around physical encoder
* create/destroy/enable/disable
* @frame_busy_mask: Bitmask tracking which phys_enc we are still
@@ -186,7 +186,6 @@ struct dpu_encoder_virt {
struct drm_crtc *crtc;
struct drm_connector *connector;
- struct dentry *debugfs_root;
struct mutex enc_lock;
DECLARE_BITMAP(frame_busy_mask, MAX_PHYS_ENCODERS_PER_VIRTUAL);
void (*crtc_frame_event_cb)(void *, u32 event);
@@ -2091,7 +2090,8 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
#ifdef CONFIG_DEBUG_FS
static int _dpu_encoder_status_show(struct seq_file *s, void *data)
{
- struct dpu_encoder_virt *dpu_enc = s->private;
+ struct drm_debugfs_entry *entry = s->private;
+ struct dpu_encoder_virt *dpu_enc = entry->file.data;
int i;
mutex_lock(&dpu_enc->enc_lock);
@@ -2110,48 +2110,31 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
return 0;
}
-DEFINE_SHOW_ATTRIBUTE(_dpu_encoder_status);
-
-static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
+static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
{
struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
-
- char name[12];
+ char *name;
if (!drm_enc->dev) {
DPU_ERROR("invalid encoder or kms\n");
- return -EINVAL;
+ return;
}
- snprintf(name, sizeof(name), "encoder%u", drm_enc->base.id);
+ name = devm_kasprintf(drm_enc->dev->dev, GFP_KERNEL, "encoder%u", drm_enc->base.id);
- /* create overall sub-directory for the encoder */
- dpu_enc->debugfs_root = debugfs_create_dir(name,
- drm_enc->dev->primary->debugfs_root);
-
- /* don't error check these */
- debugfs_create_file("status", 0600,
- dpu_enc->debugfs_root, dpu_enc, &_dpu_encoder_status_fops);
-
- return 0;
+ drm_debugfs_add_file(drm_enc->dev, name, _dpu_encoder_status_show, dpu_enc);
}
#else
-static int _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
+static void _dpu_encoder_init_debugfs(struct drm_encoder *drm_enc)
{
- return 0;
}
#endif
static int dpu_encoder_late_register(struct drm_encoder *encoder)
{
- return _dpu_encoder_init_debugfs(encoder);
-}
-
-static void dpu_encoder_early_unregister(struct drm_encoder *encoder)
-{
- struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
+ _dpu_encoder_init_debugfs(encoder);
- debugfs_remove_recursive(dpu_enc->debugfs_root);
+ return 0;
}
static int dpu_encoder_virt_add_phys_encs(
@@ -2380,7 +2363,6 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
static const struct drm_encoder_funcs dpu_encoder_funcs = {
.destroy = dpu_encoder_destroy,
.late_register = dpu_encoder_late_register,
- .early_unregister = dpu_encoder_early_unregister,
};
int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
