Re: [PATCH] drm/amdgpu/dc: Pixel encoding DRM property and module parameter
On 28/09/2020 21:12, Kazlauskas, Nicholas wrote: DRM modes don't specify the encoding. The property as part of this patch lets userspace override it but the userspace GUI support isn't there on Wayland IIRC. I'm fine with adding the properties but I don't think the module parameter is the right solution here. I think it's better if we try to get this into atomic userspace as well or revive efforts that have been already started before. The problem with the module parameters is that it'd be applying a default to every DRM connector. No way to specify different defaults per DRM connector, nor do we know the full connector set at driver initialization. The list is dynamic and can change when you plug/unplug MST displays. What about using the existing "video=" kernel command-line option? I looks like this acts at the DRM level and is a driver-neutral mechanism for specifying defaults on a per-connector basis. It also has a straightforward means for parsing additional arbitrary options that could be used -- from what I can make out this would require adding pixel encoding to struct drm_cmdline_mode. Regards, -James ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/dc: Pixel encoding DRM property and module parameter
On Mon, 2020-09-28 at 16:32 -0400, Alex Deucher wrote: > Yes, but module parameters global for all devices claimed by that > driver. E.g., if you have a system with 2 or 3 AMD GPUs in it, that > will be applied to all of them. I think if we want to be able to set > KMS properties on the kernel command line, we should add the > functionality to the drm core so it works for all drivers and > properties. We already have this for basic modesetting. > > Alex Thanks for clarifying that -- I see what was meant now. Personally I'm not wedded to the module parameter, but I think if this is to be of use to those who are experiencing problems it does need *some* way of specifying the encoding pretty early on (for things like boot splashes and GDM -- which I believe is on Wayland). -James ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/dc: Pixel encoding DRM property and module parameter
On Mon, 2020-09-28 at 16:12 -0400, Kazlauskas, Nicholas wrote: > > The problem with the module parameters is that it'd be applying a > default to every DRM connector. No way to specify different defaults > per > DRM connector, nor do we know the full connector set at driver > initialization. The list is dynamic and can change when you > plug/unplug > MST displays. I just want to point out that the module parameter does support connector-specific values, e.g. amdgpu.pixel_encoding=HDMI-A-1:rgb should only affect the connector named HDMI-A-1 (unless I've coded it wrong -- I don't have enough video ports to test it thoroughly). If there's no such-named connector the parameter should be ignored. In contrast with no named connector given, e.g. amdgpu.pixel_encoding=rgb should match any connector. Multiple connectors can be given comma- separated. I admit the solution here is crude -- it just stores the string and rescans it when required. -James ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/dc: Pixel encoding DRM property and module parameter
On Mon, 2020-09-28 at 10:26 -0400, Harry Wentland wrote: > > On 2020-09-25 5:18 p.m., Alex Deucher wrote: > > On Tue, Sep 22, 2020 at 4:51 PM James Ettle > > wrote: > > > On 22/09/2020 21:33, Alex Deucher wrote: > > > > > +/** > > > > > + * DOC: pixel_encoding (string) > > > > > + * Specify the initial pixel encoding used by a connector. > > > > > + */ > > > > > +static char amdgpu_pixel_encoding[MAX_INPUT]; > > > > > +MODULE_PARM_DESC(pixel_encoding, "Override pixel encoding"); > > > > > +module_param_string(pixel_encoding, amdgpu_pixel_encoding, > > > > > sizeof(amdgpu_pixel_encoding), 0444); > > > > > > > > You can drop this part. We don't need a module parameter if we > > > > have a > > > > kms property. > > > > > > > > Alex > > > > > > OK, but is there then an alternative means of setting the pixel > > > encoding to be used immediately on boot or when amdgpu loads? > > > Also are there user tools other than xrandr to change a KMS > > > property, for Wayland and console users? > > > > You can force some things on the kernel command line, but I don't > > recall whether that includes kms properties or not. As for ways to > > change properties, the KMS API provides a way. those are exposed > > via > > randr when using X. When using wayland compositors, it depends on > > the > > compositor. > > > > I'm not aware of a way to specify KMS properties through the kernel > command line. I don't think it's possible. > > For atomic userspace the userspace wants to be the authority on the > KMS > config. I'm not sure there's a way to set these properties with > Wayland > unless a Wayland compositor plumbs them through. > > Can you summarize on a higher level what problem you're trying to > solve? > I wonder if it'd be better solved with properties on a DRM level that > all drivers can follow if desired. > > Harry > > > Alex > > The problem this is trying to solve is that the pixel format defaults to YCbCr444 on HDMI if the monitor claims to support it, in preference to RGB. This behaviour is hard-coded (see the comment fill_stream_properties_from_drm_display_mode) and there is no way for the user to change the pixel format to RGB, other than hacking the EDID to disable the YCbCr flags. Using YCbCr (rather than RGB) has been reported to cause suboptimal results for some users: colour quality issues or perceptible conversion latency at the monitor end -- see: https://gitlab.freedesktop.org/drm/amd/-/issues/476 for the full details. This patch tries to solve this issue by adding a DRM property so Xorg users can change the pixel encoding on-the-fly, and a module parameter to set the default encoding at amdgpu's init for all users. [I suppose an alternative here is to look into the rationale of defaulting to YCbCr444 on HDMI when the monitor also supports RGB. For reference although on my kit I see no detrimental effects from YCbCr, I'm using onboard graphics with a motherboard that has just D-sub and HDMI -- so DisplayPort's not an option.] -James ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/dc: Pixel encoding DRM property and module parameter
On 22/09/2020 21:33, Alex Deucher wrote: >> +/** >> + * DOC: pixel_encoding (string) >> + * Specify the initial pixel encoding used by a connector. >> + */ >> +static char amdgpu_pixel_encoding[MAX_INPUT]; >> +MODULE_PARM_DESC(pixel_encoding, "Override pixel encoding"); >> +module_param_string(pixel_encoding, amdgpu_pixel_encoding, >> sizeof(amdgpu_pixel_encoding), 0444); > > You can drop this part. We don't need a module parameter if we have a > kms property. > > Alex OK, but is there then an alternative means of setting the pixel encoding to be used immediately on boot or when amdgpu loads? Also are there user tools other than xrandr to change a KMS property, for Wayland and console users? -James ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu/dc: Pixel encoding DRM property and module parameter
Exposes the pixel encoding as the DRM property "pixel encoding" and the module parameter pixel_encoding. The DRM property may take values "auto" (current behaviour when a display is plugged in), "rgb" for RGB, or "ycbcr444" for YCbCr 4:4:4. The module parameter may only be set on boot and is of the format [connector-name:]encoding[,...] where encoding is one of the same values as are valid for the DRM property. https://gitlab.freedesktop.org/drm/amd/-/issues/476 --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 33 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_display.h | 4 + drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 8 + .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 247 -- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + drivers/gpu/drm/amd/display/dc/core/dc.c | 12 +- drivers/gpu/drm/amd/display/dc/dc_stream.h| 2 + 7 files changed, 289 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index d76172965199..37748f35c52b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -614,6 +614,31 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = { AMDGPU_FMT_DITHER_ENABLE, "on" }, }; +static const struct drm_prop_enum_list amdgpu_user_pixenc_list[] = +{ { AMDGPU_USER_PIXENC_AUTO, "auto" }, + { AMDGPU_USER_PIXENC_RGB, "rgb" }, + { AMDGPU_USER_PIXENC_YCBCR444, "ycbcr444" }, +}; + +bool amdgpu_user_pixenc_from_name( + enum amdgpu_user_pixenc *user_pixenc, + const char *pixenc_name) +{ + /* user_pixenc only modified if name found */ + bool found = false; + if (pixenc_name && (*pixenc_name != '\0')) { + const int sz = ARRAY_SIZE(amdgpu_user_pixenc_list); + int i; + for (i = 0; !found && i < sz; ++i) { + if (strcmp(pixenc_name, amdgpu_user_pixenc_list[i].name) == 0) { + *user_pixenc = amdgpu_user_pixenc_list[i].type; + found = true; + } + } + } + return found; +} + int amdgpu_display_modeset_create_props(struct amdgpu_device *adev) { int sz; @@ -666,6 +691,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev) "abm level", 0, 4); if (!adev->mode_info.abm_level_property) return -ENOMEM; + + sz = ARRAY_SIZE(amdgpu_user_pixenc_list); + adev->mode_info.pixel_encoding_property = + drm_property_create_enum(adev->ddev, 0, + "pixel encoding", + amdgpu_user_pixenc_list, sz); + if (!adev->mode_info.pixel_encoding_property) + return -ENOMEM; } return 0; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h index 3620b24785e1..72f82e3f4e32 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h @@ -45,4 +45,8 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev, struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd); +bool amdgpu_user_pixenc_from_name( + enum amdgpu_user_pixenc *user_pixenc, + const char *pixenc_name); + #endif diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h index 37ba07e2feb5..3c21ecf3d259 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h @@ -123,6 +123,12 @@ enum amdgpu_flip_status { AMDGPU_FLIP_SUBMITTED }; +enum amdgpu_user_pixenc { + AMDGPU_USER_PIXENC_AUTO, + AMDGPU_USER_PIXENC_RGB, + AMDGPU_USER_PIXENC_YCBCR444 +}; + #define AMDGPU_MAX_I2C_BUS 16 /* amdgpu gpio-based i2c @@ -333,6 +339,8 @@ struct amdgpu_mode_info { struct drm_property *dither_property; /* Adaptive Backlight Modulation (power feature) */ struct drm_property *abm_level_property; + /* User HDMI pixel encoding override */ + struct drm_property *pixel_encoding_property; /* hardcoded DFP edid from BIOS */ struct edid *bios_hardcoded_edid; int bios_hardcoded_edid_size; diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index b51c527a3f0d..8901271d1902 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -158,7 +158,6 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector); static int amdgpu_dm_atomic_commit(struct drm_device *dev, struct