Re: [PATCH v2] drm/amd/display: Add pixel encoding info to debugfs
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. Rino On Tue, May 21, 2024 at 9:04 PM Christian König 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 > > --- > > > > 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); > > } > > > > /* >
Re: [PATCH v2] drm/amd/display: Add pixel encoding info to debugfs
Looked through the patch series from Andri Yngvason and that does exactly what I wanted in the first place. I think that the patch series should be encouraged to be merged in as fast as possible. For the patch I have submitted, it stands on its own, since the patch series from Andri Yngvason does not include anything in the debugfs. This means whenever or not the patch series gets merged, at least those with an AMD gpu can figure out which pixel encoding that is used. Rino On Wed, May 22, 2024 at 3:40 PM Simon Ser wrote: > > On Wednesday, May 22nd, 2024 at 15:36, Mario Limonciello > wrote: > > > > 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. > > FWIW, there is a patch from Andri to add a similar (?) property: > https://lore.kernel.org/dri-devel/20240115160554.720247-1-an...@yngvason.is/ > > The patch also allows user-space to set the "pixel encoding".
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 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 > > 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 > >>> --- > >>> > >>> 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); > >>>} > >>> > >>>/* > >> >