Message-ID: <CAACkh=9hY7eg_uuH7Psm=XJfSzwQTvzs8bvOXQ=wwkMPrC44SA@mail.gmail.com>
Date: Wed, 22 May 2024 12:07:36 +0200
From: Rino André Johnsen <rinoandrejohnsen@...il.com>
To: Mario Limonciello <mario.limonciello@....com>
Cc: Christian König <christian.koenig@....com>, 
	alexander.deucher@....com, Harry Wentland <harry.wentland@....com>, 
	Leo Li <sunpeng.li@....com>, Rodrigo Siqueira <Rodrigo.Siqueira@....com>, 
	"Pan, Xinhui" <Xinhui.Pan@....com>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	Aurabindo Pillai <aurabindo.pillai@....com>, Hersen Wu <hersenxs.wu@....com>, 
	Hamza Mahfooz <hamza.mahfooz@....com>, Wayne Lin <wayne.lin@....com>, 
	Srinivasan Shanmugam <srinivasan.shanmugam@....com>, Fangzhi Zuo <jerry.zuo@....com>, 
	Tom Chung <chiahsuan.chung@....com>, Nicholas Kazlauskas <nicholas.kazlauskas@....com>, 
	amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm/amd/display: Add pixel encoding info to debugfs

To be perfectly honest with you, I haven't given that much though. I
used the 'bpc' and 'colorspace' property in debugfs, since I could not
find that information anywhere else. And since I also needed to verify
the pixel encoding being used, I added it where those other values
were. That made for a simple and easy addition for this property.

If you want me to do this differently, let me know. And please point
me to the standardized DRM property where I should expose the values.

Rino

On Tue, May 21, 2024 at 10:55 PM Mario Limonciello
<mario.limonciello@....com> wrote:
>
> On 5/21/2024 15:06, Rino André Johnsen wrote:
> > What is already there in debugfs is 'bpc' and 'colorspace', but not
> > the pixel encoding/format.
> > I have searched high and low for that to be able to verify that my
> > monitor and computer are using my preferred combination of all those
> > three values.
> >
> > I do think it should be available as a standard DRM CRTC property, but
> > for the time being, I figured that a simple debugfs property would be
> > sufficient for time being.
> >
>
> It's just about as much work either way to populate it though, why do it
> twice instead of just doing it right the first time?
>
> > Rino
> >
> >
> > On Tue, May 21, 2024 at 9:04 PM Christian König
> > <christian.koenig@....com> wrote:
> >>
> >> Am 21.05.24 um 07:11 schrieb Rino Andre Johnsen:
> >>> [Why]
> >>> For debugging and testing purposes.
> >>>
> >>> [How]
> >>> Create amdgpu_current_pixelencoding debugfs entry.
> >>> Usage: cat /sys/kernel/debug/dri/1/crtc-0/amdgpu_current_pixelencoding
> >>
> >> Why isn't that available as standard DRM CRTC property in either sysfs
> >> or debugfs?
> >>
> >> I think the format specifiers should already be available somewhere there.
> >>
> >> Regards,
> >> Christian.
> >>
> >>>
> >>> Signed-off-by: Rino Andre Johnsen <rinoandrejohnsen@...il.com>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> 1. Do not initialize dm_crtc_state to NULL.
> >>> ---
> >>>    .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 47 +++++++++++++++++++
> >>>    1 file changed, 47 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> >>> index 27d5c6077630..4254d4a4b56b 100644
> >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> >>> @@ -1160,6 +1160,51 @@ static int amdgpu_current_colorspace_show(struct seq_file *m, void *data)
> >>>    }
> >>>    DEFINE_SHOW_ATTRIBUTE(amdgpu_current_colorspace);
> >>>
> >>> +/*
> >>> + * Returns the current pixelencoding for the crtc.
> >>> + * Example usage: cat /sys/kernel/debug/dri/0/crtc-0/amdgpu_current_pixelencoding
> >>> + */
> >>> +static int amdgpu_current_pixelencoding_show(struct seq_file *m, void *data)
> >>> +{
> >>> +     struct drm_crtc *crtc = m->private;
> >>> +     struct drm_device *dev = crtc->dev;
> >>> +     struct dm_crtc_state *dm_crtc_state;
> >>> +     int res = -ENODEV;
> >>> +
> >>> +     mutex_lock(&dev->mode_config.mutex);
> >>> +     drm_modeset_lock(&crtc->mutex, NULL);
> >>> +     if (crtc->state == NULL)
> >>> +             goto unlock;
> >>> +
> >>> +     dm_crtc_state = to_dm_crtc_state(crtc->state);
> >>> +     if (dm_crtc_state->stream == NULL)
> >>> +             goto unlock;
> >>> +
> >>> +     switch (dm_crtc_state->stream->timing.pixel_encoding) {
> >>> +     case PIXEL_ENCODING_RGB:
> >>> +             seq_puts(m, "RGB");
> >>> +             break;
> >>> +     case PIXEL_ENCODING_YCBCR422:
> >>> +             seq_puts(m, "YCBCR422");
> >>> +             break;
> >>> +     case PIXEL_ENCODING_YCBCR444:
> >>> +             seq_puts(m, "YCBCR444");
> >>> +             break;
> >>> +     case PIXEL_ENCODING_YCBCR420:
> >>> +             seq_puts(m, "YCBCR420");
> >>> +             break;
> >>> +     default:
> >>> +             goto unlock;
> >>> +     }
> >>> +     res = 0;
> >>> +
> >>> +unlock:
> >>> +     drm_modeset_unlock(&crtc->mutex);
> >>> +     mutex_unlock(&dev->mode_config.mutex);
> >>> +
> >>> +     return res;
> >>> +}
> >>> +DEFINE_SHOW_ATTRIBUTE(amdgpu_current_pixelencoding);
> >>>
> >>>    /*
> >>>     * Example usage:
> >>> @@ -3688,6 +3733,8 @@ void crtc_debugfs_init(struct drm_crtc *crtc)
> >>>                            crtc, &amdgpu_current_bpc_fops);
> >>>        debugfs_create_file("amdgpu_current_colorspace", 0644, crtc->debugfs_entry,
> >>>                            crtc, &amdgpu_current_colorspace_fops);
> >>> +     debugfs_create_file("amdgpu_current_pixelencoding", 0644, crtc->debugfs_entry,
> >>> +                         crtc, &amdgpu_current_pixelencoding_fops);
> >>>    }
> >>>
> >>>    /*
> >>
>

Powered by blists - more mailing lists