Received-SPF: pass (google.com: domain of linux-kernel+bounces-186032-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <fa885eca-d7e6-415a-8a08-9103b002c6bb@amd.com> <20240521051140.30509-1-rinoandrejohnsen@gmail.com>
 <17782a6e-db84-4c20-874a-342b9655ffc5@amd.com> <CAACkh=-B-jH6g7KY7Nn_7Y_+gHPQ7G5Z5AZ0=a=_ifjcmsorcw@mail.gmail.com>
 <86410711-9b88-448c-9148-109f81b1ca55@amd.com>
In-Reply-To: <86410711-9b88-448c-9148-109f81b1ca55@amd.com>
From: =?UTF-8?Q?Rino_Andr=C3=A9_Johnsen?= <rinoandrejohnsen@gmail.com>
Date: Wed, 22 May 2024 12:07:36 +0200
Message-ID: <CAACkh=9hY7eg_uuH7Psm=XJfSzwQTvzs8bvOXQ=wwkMPrC44SA@mail.gmail.com>
Subject: Re: [PATCH v2] drm/amd/display: Add pixel encoding info to debugfs
To: Mario Limonciello <mario.limonciello@amd.com>
Cc: =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>, 
	alexander.deucher@amd.com, Harry Wentland <harry.wentland@amd.com>, 
	Leo Li <sunpeng.li@amd.com>, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>, 
	"Pan, Xinhui" <Xinhui.Pan@amd.com>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, 
	Aurabindo Pillai <aurabindo.pillai@amd.com>, Hersen Wu <hersenxs.wu@amd.com>, 
	Hamza Mahfooz <hamza.mahfooz@amd.com>, Wayne Lin <wayne.lin@amd.com>, 
	Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>, Fangzhi Zuo <jerry.zuo@amd.com>, 
	Tom Chung <chiahsuan.chung@amd.com>, Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>, 
	amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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=E2=80=AFPM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 5/21/2024 15:06, Rino Andr=C3=A9 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=E2=80=AFPM Christian K=C3=B6nig
> > <christian.koenig@amd.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_pixelencodin=
g
> >>
> >> 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 th=
ere.
> >>
> >> Regards,
> >> Christian.
> >>
> >>>
> >>> Signed-off-by: Rino Andre Johnsen <rinoandrejohnsen@gmail.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(stru=
ct 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, voi=
d *data)
> >>> +{
> >>> +     struct drm_crtc *crtc =3D m->private;
> >>> +     struct drm_device *dev =3D crtc->dev;
> >>> +     struct dm_crtc_state *dm_crtc_state;
> >>> +     int res =3D -ENODEV;
> >>> +
> >>> +     mutex_lock(&dev->mode_config.mutex);
> >>> +     drm_modeset_lock(&crtc->mutex, NULL);
> >>> +     if (crtc->state =3D=3D NULL)
> >>> +             goto unlock;
> >>> +
> >>> +     dm_crtc_state =3D to_dm_crtc_state(crtc->state);
> >>> +     if (dm_crtc_state->stream =3D=3D 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 =3D 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->d=
ebugfs_entry,
> >>>                            crtc, &amdgpu_current_colorspace_fops);
> >>> +     debugfs_create_file("amdgpu_current_pixelencoding", 0644, crtc-=
>debugfs_entry,
> >>> +                         crtc, &amdgpu_current_pixelencoding_fops);
> >>>    }
> >>>
> >>>    /*
> >>
>