| Message ID | 20221212182137.374625-17-harry.wentland@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable Colorspace connector property in amdgpu | expand |
On Mon, 12 Dec 2022 13:21:37 -0500 Harry Wentland <harry.wentland@amd.com> wrote: > This will let us pass kms_hdr.bpc_switch. > > I don't see any good reasons why we still need to > limit bpc to 8 bpc and doing so is problematic when > we enable HDR. > > If I remember correctly there might have been some > displays out there where the advertised link bandwidth > was not large enough to drive the default timing at > max bpc. This would leave to an atomic commit/check > failure which should really be handled in compositors > with some sort of fallback mechanism. > > If this somehow turns out to still be an issue I > suggest we add a module parameter to allow users to > limit the max_bpc to a desired value. > > Signed-off-by: Harry Wentland <harry.wentland@amd.com> > Cc: Pekka Paalanen <ppaalanen@gmail.com> > Cc: Sebastian Wick <sebastian.wick@redhat.com> > Cc: Vitaly.Prosyak@amd.com > Cc: Uma Shankar <uma.shankar@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Joshua Ashton <joshua@froggi.es> > Cc: dri-devel@lists.freedesktop.org > Cc: amd-gfx@lists.freedesktop.org > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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 d0c071000a5d..396e345f5d6b 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -7115,7 +7115,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, > drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16); > > /* This defaults to the max in the range, but we want 8bpc for non-edp. */ > - aconnector->base.state->max_bpc = (connector_type == DRM_MODE_CONNECTOR_eDP) ? 16 : 8; > + aconnector->base.state->max_bpc = 16; Hi, missed to delete the comment. If it really defaults, then can't you just drop the assignment? Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com> Thanks, pq > aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc; > > if (connector_type == DRM_MODE_CONNECTOR_eDP &&
On 12/12/22 19:21, Harry Wentland wrote: > This will let us pass kms_hdr.bpc_switch. > > I don't see any good reasons why we still need to > limit bpc to 8 bpc and doing so is problematic when > we enable HDR. > > If I remember correctly there might have been some > displays out there where the advertised link bandwidth > was not large enough to drive the default timing at > max bpc. This would leave to an atomic commit/check > failure which should really be handled in compositors > with some sort of fallback mechanism. > > If this somehow turns out to still be an issue I > suggest we add a module parameter to allow users to > limit the max_bpc to a desired value. While leaving the fallback for user space to handle makes some sense in theory, in practice most KMS display servers likely won't handle it. Another issue is that if mode validation is based on the maximum bpc value, it may reject modes which would work with lower bpc. What Ville (CC'd) suggested before instead (and what i915 seems to be doing already) is that the driver should do mode validation based on the *minimum* bpc, and automatically make the effective bpc lower than the maximum as needed to make the rest of the atomic state work.
On Tue, 13 Dec 2022 18:20:59 +0100 Michel Dänzer <michel.daenzer@mailbox.org> wrote: > On 12/12/22 19:21, Harry Wentland wrote: > > This will let us pass kms_hdr.bpc_switch. > > > > I don't see any good reasons why we still need to > > limit bpc to 8 bpc and doing so is problematic when > > we enable HDR. > > > > If I remember correctly there might have been some > > displays out there where the advertised link bandwidth > > was not large enough to drive the default timing at > > max bpc. This would leave to an atomic commit/check > > failure which should really be handled in compositors > > with some sort of fallback mechanism. > > > > If this somehow turns out to still be an issue I > > suggest we add a module parameter to allow users to > > limit the max_bpc to a desired value. > > While leaving the fallback for user space to handle makes some sense > in theory, in practice most KMS display servers likely won't handle > it. > > Another issue is that if mode validation is based on the maximum bpc > value, it may reject modes which would work with lower bpc. > > > What Ville (CC'd) suggested before instead (and what i915 seems to be > doing already) is that the driver should do mode validation based on > the *minimum* bpc, and automatically make the effective bpc lower > than the maximum as needed to make the rest of the atomic state work. A driver is always allowed to choose a bpc lower than max_bpc, so it very well should do so when necessary due to *known* hardware etc. limitations. So things like mode validation cannot just look at a single max or min bpc, but it needs to figure out if there is any usable bpc value that makes the mode work. The max_bpc knob exists only for the cases where the sink undetectably malfunctions unless the bpc is artificially limited more than seems necessary. That malfunction requires a human to detect, and reconfigure their system as we don't have a quirk database for this I think. The question of userspace wanting a specific bpc is a different matter and an unsolved one. It also ties to userspace wanting to use the current mode to avoid a mode switch between e.g. hand-off from firmware boot splash to proper userspace. That's also unsolved AFAIK. OTOH, we have the discussion that concluded as https://gitlab.freedesktop.org/wayland/weston/-/issues/612#note_1359898 which really puts userspace in charge of max_bpc, so the driver-chosen default value does not have much impact as long as it makes the firmware-chosen video mode to continue, as requested in https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/995 given that userspace cannot know what the actual bpc currently is nor set the exact bpc to keep it the same. Thanks, pq
On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > On Tue, 13 Dec 2022 18:20:59 +0100 > Michel Dänzer <michel.daenzer@mailbox.org> wrote: > > > On 12/12/22 19:21, Harry Wentland wrote: > > > This will let us pass kms_hdr.bpc_switch. > > > > > > I don't see any good reasons why we still need to > > > limit bpc to 8 bpc and doing so is problematic when > > > we enable HDR. > > > > > > If I remember correctly there might have been some > > > displays out there where the advertised link bandwidth > > > was not large enough to drive the default timing at > > > max bpc. This would leave to an atomic commit/check > > > failure which should really be handled in compositors > > > with some sort of fallback mechanism. > > > > > > If this somehow turns out to still be an issue I > > > suggest we add a module parameter to allow users to > > > limit the max_bpc to a desired value. > > > > While leaving the fallback for user space to handle makes some sense > > in theory, in practice most KMS display servers likely won't handle > > it. > > > > Another issue is that if mode validation is based on the maximum bpc > > value, it may reject modes which would work with lower bpc. > > > > > > What Ville (CC'd) suggested before instead (and what i915 seems to be > > doing already) is that the driver should do mode validation based on > > the *minimum* bpc, and automatically make the effective bpc lower > > than the maximum as needed to make the rest of the atomic state work. > > A driver is always allowed to choose a bpc lower than max_bpc, so it > very well should do so when necessary due to *known* hardware etc. > limitations. > In the amdgpu case, it's more of a preference thing. The driver would enable higher bpcs at the expense of refresh rate and it seemed most users want higher refresh rates than higher bpc. I guess the driver can select a lower bpc at its discretion to produce what it thinks is the best default, but what about users that don't want the default? Alex > So things like mode validation cannot just look at a single max or min > bpc, but it needs to figure out if there is any usable bpc value that > makes the mode work. > > The max_bpc knob exists only for the cases where the sink undetectably > malfunctions unless the bpc is artificially limited more than seems > necessary. That malfunction requires a human to detect, and reconfigure > their system as we don't have a quirk database for this I think. > > The question of userspace wanting a specific bpc is a different matter > and an unsolved one. It also ties to userspace wanting to use the > current mode to avoid a mode switch between e.g. hand-off from firmware > boot splash to proper userspace. That's also unsolved AFAIK. > > OTOH, we have the discussion that concluded as > https://gitlab.freedesktop.org/wayland/weston/-/issues/612#note_1359898 > which really puts userspace in charge of max_bpc, so the driver-chosen > default value does not have much impact as long as it makes the > firmware-chosen video mode to continue, as requested in > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/995 > given that userspace cannot know what the actual bpc currently is nor > set the exact bpc to keep it the same. > > > Thanks, > pq
On 12/14/22 16:46, Alex Deucher wrote: > On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: >> On Tue, 13 Dec 2022 18:20:59 +0100 >> Michel Dänzer <michel.daenzer@mailbox.org> wrote: >>> On 12/12/22 19:21, Harry Wentland wrote: >>>> This will let us pass kms_hdr.bpc_switch. >>>> >>>> I don't see any good reasons why we still need to >>>> limit bpc to 8 bpc and doing so is problematic when >>>> we enable HDR. >>>> >>>> If I remember correctly there might have been some >>>> displays out there where the advertised link bandwidth >>>> was not large enough to drive the default timing at >>>> max bpc. This would leave to an atomic commit/check >>>> failure which should really be handled in compositors >>>> with some sort of fallback mechanism. >>>> >>>> If this somehow turns out to still be an issue I >>>> suggest we add a module parameter to allow users to >>>> limit the max_bpc to a desired value. >>> >>> While leaving the fallback for user space to handle makes some sense >>> in theory, in practice most KMS display servers likely won't handle >>> it. >>> >>> Another issue is that if mode validation is based on the maximum bpc >>> value, it may reject modes which would work with lower bpc. >>> >>> >>> What Ville (CC'd) suggested before instead (and what i915 seems to be >>> doing already) is that the driver should do mode validation based on >>> the *minimum* bpc, and automatically make the effective bpc lower >>> than the maximum as needed to make the rest of the atomic state work. >> >> A driver is always allowed to choose a bpc lower than max_bpc, so it >> very well should do so when necessary due to *known* hardware etc. >> limitations. >> > > In the amdgpu case, it's more of a preference thing. The driver would > enable higher bpcs at the expense of refresh rate and it seemed most > users want higher refresh rates than higher bpc. I wrote the above because I thought that this patch might result in some modes getting pruned because they can't work with the max bpc. However, I see now that cbd14ae7ea93 ("drm/amd/display: Fix incorrectly pruned modes with deep color") should prevent that AFAICT. The question then is: What happens if user space tries to use a mode which doesn't work with the max bpc? Does the driver automatically lower the effective bpc as needed, or does the atomic commit (check) fail? The latter would seem bad. > I guess the driver can select a lower bpc at its discretion to produce > what it thinks is the best default, but what about users that don't want > the default? They can choose the lower refresh rate?
On Wed, 14 Dec 2022 10:46:55 -0500 Alex Deucher <alexdeucher@gmail.com> wrote: > On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > On Tue, 13 Dec 2022 18:20:59 +0100 > > Michel Dänzer <michel.daenzer@mailbox.org> wrote: > > > > > On 12/12/22 19:21, Harry Wentland wrote: > > > > This will let us pass kms_hdr.bpc_switch. > > > > > > > > I don't see any good reasons why we still need to > > > > limit bpc to 8 bpc and doing so is problematic when > > > > we enable HDR. > > > > > > > > If I remember correctly there might have been some > > > > displays out there where the advertised link bandwidth > > > > was not large enough to drive the default timing at > > > > max bpc. This would leave to an atomic commit/check > > > > failure which should really be handled in compositors > > > > with some sort of fallback mechanism. > > > > > > > > If this somehow turns out to still be an issue I > > > > suggest we add a module parameter to allow users to > > > > limit the max_bpc to a desired value. > > > > > > While leaving the fallback for user space to handle makes some sense > > > in theory, in practice most KMS display servers likely won't handle > > > it. > > > > > > Another issue is that if mode validation is based on the maximum bpc > > > value, it may reject modes which would work with lower bpc. > > > > > > > > > What Ville (CC'd) suggested before instead (and what i915 seems to be > > > doing already) is that the driver should do mode validation based on > > > the *minimum* bpc, and automatically make the effective bpc lower > > > than the maximum as needed to make the rest of the atomic state work. > > > > A driver is always allowed to choose a bpc lower than max_bpc, so it > > very well should do so when necessary due to *known* hardware etc. > > limitations. > > > > In the amdgpu case, it's more of a preference thing. The driver would > enable higher bpcs at the expense of refresh rate and it seemed most > users want higher refresh rates than higher bpc. Hi Alex, we already have userspace in explicit control of the refresh rate. Userspace picks the refresh rate first, then the driver silently checks what bpc is possible. Userspace preference wins, so bpc is chosen to produce the desired refresh rate. In what cases does the driver really pick a refresh rate on its own? Or is this about display latency more than throughput, to send a full frame in shorter time while not actually increasing refresh rate? Even for VRR, userspace already explicitly chooses the max refresh rate, I believe. I suppose optimising power consumption by choosing a lower bpc than what would work is a use case, but that's a bit awkward currently because there is no way for userspace to opt-out of that. OTOH, userspace can already opt-in for that by lowering max_bpc. > I guess the driver > can select a lower bpc at its discretion to produce what it thinks is > the best default, but what about users that don't want the default? That's what we need explicit bpc control KMS properties for, e.g. min_bpc to complement max_bpc (I recall old discussions about this). Only userspace could know what a particular end user wants. To ask this differently: in what cases would max_bpc become the wanted bpc in a way that it overrides the explicitly set video mode (refresh rate), or would cause a modeset to fail if a lower bpc would make the modeset succeed? The very definition of max_bpc is that it's the upper limit, and not the desired bpc. The UAPI documentation says: max bpc: This range property is used by userspace to limit the bit depth. When used the driver would limit the bpc in accordance with the valid range supported by the hardware and sink. Drivers to use the function drm_connector_attach_max_bpc_property() to create and attach the property to the connector during initialization. Git archaeology revealed that this property was first intended to work around broken sinks and not for user preferences. Thanks, pq > > Alex > > > > So things like mode validation cannot just look at a single max or min > > bpc, but it needs to figure out if there is any usable bpc value that > > makes the mode work. > > > > The max_bpc knob exists only for the cases where the sink undetectably > > malfunctions unless the bpc is artificially limited more than seems > > necessary. That malfunction requires a human to detect, and reconfigure > > their system as we don't have a quirk database for this I think. > > > > The question of userspace wanting a specific bpc is a different matter > > and an unsolved one. It also ties to userspace wanting to use the > > current mode to avoid a mode switch between e.g. hand-off from firmware > > boot splash to proper userspace. That's also unsolved AFAIK. > > > > OTOH, we have the discussion that concluded as > > https://gitlab.freedesktop.org/wayland/weston/-/issues/612#note_1359898 > > which really puts userspace in charge of max_bpc, so the driver-chosen > > default value does not have much impact as long as it makes the > > firmware-chosen video mode to continue, as requested in > > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/995 > > given that userspace cannot know what the actual bpc currently is nor > > set the exact bpc to keep it the same. > > > > > > Thanks, > > pq
On Thu, Dec 15, 2022 at 4:07 AM Michel Dänzer <michel.daenzer@mailbox.org> wrote: > > On 12/14/22 16:46, Alex Deucher wrote: > > On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > >> On Tue, 13 Dec 2022 18:20:59 +0100 > >> Michel Dänzer <michel.daenzer@mailbox.org> wrote: > >>> On 12/12/22 19:21, Harry Wentland wrote: > >>>> This will let us pass kms_hdr.bpc_switch. > >>>> > >>>> I don't see any good reasons why we still need to > >>>> limit bpc to 8 bpc and doing so is problematic when > >>>> we enable HDR. > >>>> > >>>> If I remember correctly there might have been some > >>>> displays out there where the advertised link bandwidth > >>>> was not large enough to drive the default timing at > >>>> max bpc. This would leave to an atomic commit/check > >>>> failure which should really be handled in compositors > >>>> with some sort of fallback mechanism. > >>>> > >>>> If this somehow turns out to still be an issue I > >>>> suggest we add a module parameter to allow users to > >>>> limit the max_bpc to a desired value. > >>> > >>> While leaving the fallback for user space to handle makes some sense > >>> in theory, in practice most KMS display servers likely won't handle > >>> it. > >>> > >>> Another issue is that if mode validation is based on the maximum bpc > >>> value, it may reject modes which would work with lower bpc. > >>> > >>> > >>> What Ville (CC'd) suggested before instead (and what i915 seems to be > >>> doing already) is that the driver should do mode validation based on > >>> the *minimum* bpc, and automatically make the effective bpc lower > >>> than the maximum as needed to make the rest of the atomic state work. > >> > >> A driver is always allowed to choose a bpc lower than max_bpc, so it > >> very well should do so when necessary due to *known* hardware etc. > >> limitations. > >> > > > > In the amdgpu case, it's more of a preference thing. The driver would > > enable higher bpcs at the expense of refresh rate and it seemed most > > users want higher refresh rates than higher bpc. > > I wrote the above because I thought that this patch might result in some modes getting pruned because they can't work with the max bpc. However, I see now that cbd14ae7ea93 ("drm/amd/display: Fix incorrectly pruned modes with deep color") should prevent that AFAICT. > Yeah, maybe that has been fixed now. IIRC, the max bpc hack was added a long time ago. Alex > The question then is: What happens if user space tries to use a mode which doesn't work with the max bpc? Does the driver automatically lower the effective bpc as needed, or does the atomic commit (check) fail? The latter would seem bad. > > > > I guess the driver can select a lower bpc at its discretion to produce > > what it thinks is the best default, but what about users that don't want > > the default? > > They can choose the lower refresh rate? > > > -- > Earthling Michel Dänzer | https://redhat.com > Libre software enthusiast | Mesa and Xwayland developer >
On Thu, Dec 15, 2022 at 4:17 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > On Wed, 14 Dec 2022 10:46:55 -0500 > Alex Deucher <alexdeucher@gmail.com> wrote: > > > On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > > > On Tue, 13 Dec 2022 18:20:59 +0100 > > > Michel Dänzer <michel.daenzer@mailbox.org> wrote: > > > > > > > On 12/12/22 19:21, Harry Wentland wrote: > > > > > This will let us pass kms_hdr.bpc_switch. > > > > > > > > > > I don't see any good reasons why we still need to > > > > > limit bpc to 8 bpc and doing so is problematic when > > > > > we enable HDR. > > > > > > > > > > If I remember correctly there might have been some > > > > > displays out there where the advertised link bandwidth > > > > > was not large enough to drive the default timing at > > > > > max bpc. This would leave to an atomic commit/check > > > > > failure which should really be handled in compositors > > > > > with some sort of fallback mechanism. > > > > > > > > > > If this somehow turns out to still be an issue I > > > > > suggest we add a module parameter to allow users to > > > > > limit the max_bpc to a desired value. > > > > > > > > While leaving the fallback for user space to handle makes some sense > > > > in theory, in practice most KMS display servers likely won't handle > > > > it. > > > > > > > > Another issue is that if mode validation is based on the maximum bpc > > > > value, it may reject modes which would work with lower bpc. > > > > > > > > > > > > What Ville (CC'd) suggested before instead (and what i915 seems to be > > > > doing already) is that the driver should do mode validation based on > > > > the *minimum* bpc, and automatically make the effective bpc lower > > > > than the maximum as needed to make the rest of the atomic state work. > > > > > > A driver is always allowed to choose a bpc lower than max_bpc, so it > > > very well should do so when necessary due to *known* hardware etc. > > > limitations. > > > > > > > In the amdgpu case, it's more of a preference thing. The driver would > > enable higher bpcs at the expense of refresh rate and it seemed most > > users want higher refresh rates than higher bpc. > > Hi Alex, > > we already have userspace in explicit control of the refresh rate. > Userspace picks the refresh rate first, then the driver silently checks > what bpc is possible. Userspace preference wins, so bpc is chosen to > produce the desired refresh rate. > > In what cases does the driver really pick a refresh rate on its own? It didn't, but IIRC, at the time the driver filtered out modes that it could not support with the max bpc. It looks like that has been fixed now, so maybe this whole discussion is moot. Alex > > Or is this about display latency more than throughput, to send a full > frame in shorter time while not actually increasing refresh rate? > > Even for VRR, userspace already explicitly chooses the max refresh > rate, I believe. > > I suppose optimising power consumption by choosing a lower bpc than > what would work is a use case, but that's a bit awkward currently > because there is no way for userspace to opt-out of that. OTOH, > userspace can already opt-in for that by lowering max_bpc. > > > I guess the driver > > can select a lower bpc at its discretion to produce what it thinks is > > the best default, but what about users that don't want the default? > > That's what we need explicit bpc control KMS properties for, e.g. > min_bpc to complement max_bpc (I recall old discussions about this). > Only userspace could know what a particular end user wants. > > To ask this differently: in what cases would max_bpc become the wanted > bpc in a way that it overrides the explicitly set video mode (refresh > rate), or would cause a modeset to fail if a lower bpc would make the > modeset succeed? > > The very definition of max_bpc is that it's the upper limit, and not the > desired bpc. The UAPI documentation says: > > max bpc: > > This range property is used by userspace to limit the bit depth. > When used the driver would limit the bpc in accordance with the > valid range supported by the hardware and sink. Drivers to use the > function drm_connector_attach_max_bpc_property() to create and > attach the property to the connector during initialization. > > Git archaeology revealed that this property was first intended to work > around broken sinks and not for user preferences. > > > Thanks, > pq > > > > > Alex > > > > > > > So things like mode validation cannot just look at a single max or min > > > bpc, but it needs to figure out if there is any usable bpc value that > > > makes the mode work. > > > > > > The max_bpc knob exists only for the cases where the sink undetectably > > > malfunctions unless the bpc is artificially limited more than seems > > > necessary. That malfunction requires a human to detect, and reconfigure > > > their system as we don't have a quirk database for this I think. > > > > > > The question of userspace wanting a specific bpc is a different matter > > > and an unsolved one. It also ties to userspace wanting to use the > > > current mode to avoid a mode switch between e.g. hand-off from firmware > > > boot splash to proper userspace. That's also unsolved AFAIK. > > > > > > OTOH, we have the discussion that concluded as > > > https://gitlab.freedesktop.org/wayland/weston/-/issues/612#note_1359898 > > > which really puts userspace in charge of max_bpc, so the driver-chosen > > > default value does not have much impact as long as it makes the > > > firmware-chosen video mode to continue, as requested in > > > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/995 > > > given that userspace cannot know what the actual bpc currently is nor > > > set the exact bpc to keep it the same. > > > > > > > > > Thanks, > > > pq >
On 12/15/22 18:33, Alex Deucher wrote: > On Thu, Dec 15, 2022 at 4:17 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: >> >> On Wed, 14 Dec 2022 10:46:55 -0500 >> Alex Deucher <alexdeucher@gmail.com> wrote: >> >>> On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: >>>> >>>> On Tue, 13 Dec 2022 18:20:59 +0100 >>>> Michel Dänzer <michel.daenzer@mailbox.org> wrote: >>>> >>>>> On 12/12/22 19:21, Harry Wentland wrote: >>>>>> This will let us pass kms_hdr.bpc_switch. >>>>>> >>>>>> I don't see any good reasons why we still need to >>>>>> limit bpc to 8 bpc and doing so is problematic when >>>>>> we enable HDR. >>>>>> >>>>>> If I remember correctly there might have been some >>>>>> displays out there where the advertised link bandwidth >>>>>> was not large enough to drive the default timing at >>>>>> max bpc. This would leave to an atomic commit/check >>>>>> failure which should really be handled in compositors >>>>>> with some sort of fallback mechanism. >>>>>> >>>>>> If this somehow turns out to still be an issue I >>>>>> suggest we add a module parameter to allow users to >>>>>> limit the max_bpc to a desired value. >>>>> >>>>> While leaving the fallback for user space to handle makes some sense >>>>> in theory, in practice most KMS display servers likely won't handle >>>>> it. >>>>> >>>>> Another issue is that if mode validation is based on the maximum bpc >>>>> value, it may reject modes which would work with lower bpc. >>>>> >>>>> >>>>> What Ville (CC'd) suggested before instead (and what i915 seems to be >>>>> doing already) is that the driver should do mode validation based on >>>>> the *minimum* bpc, and automatically make the effective bpc lower >>>>> than the maximum as needed to make the rest of the atomic state work. >>>> >>>> A driver is always allowed to choose a bpc lower than max_bpc, so it >>>> very well should do so when necessary due to *known* hardware etc. >>>> limitations. >>>> >>> >>> In the amdgpu case, it's more of a preference thing. The driver would >>> enable higher bpcs at the expense of refresh rate and it seemed most >>> users want higher refresh rates than higher bpc. >> >> Hi Alex, >> >> we already have userspace in explicit control of the refresh rate. >> Userspace picks the refresh rate first, then the driver silently checks >> what bpc is possible. Userspace preference wins, so bpc is chosen to >> produce the desired refresh rate. >> >> In what cases does the driver really pick a refresh rate on its own? > > It didn't, but IIRC, at the time the driver filtered out modes that it > could not support with the max bpc. It looks like that has been fixed > now, so maybe this whole discussion is moot. That depends on the answer to the follow-up question in my previous post.
On 12/15/22 18:42, Michel Dänzer wrote: > On 12/15/22 18:33, Alex Deucher wrote: >> On Thu, Dec 15, 2022 at 4:17 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: >>> >>> On Wed, 14 Dec 2022 10:46:55 -0500 >>> Alex Deucher <alexdeucher@gmail.com> wrote: >>> >>>> On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: >>>>> >>>>> On Tue, 13 Dec 2022 18:20:59 +0100 >>>>> Michel Dänzer <michel.daenzer@mailbox.org> wrote: >>>>> >>>>>> On 12/12/22 19:21, Harry Wentland wrote: >>>>>>> This will let us pass kms_hdr.bpc_switch. >>>>>>> >>>>>>> I don't see any good reasons why we still need to >>>>>>> limit bpc to 8 bpc and doing so is problematic when >>>>>>> we enable HDR. >>>>>>> >>>>>>> If I remember correctly there might have been some >>>>>>> displays out there where the advertised link bandwidth >>>>>>> was not large enough to drive the default timing at >>>>>>> max bpc. This would leave to an atomic commit/check >>>>>>> failure which should really be handled in compositors >>>>>>> with some sort of fallback mechanism. >>>>>>> >>>>>>> If this somehow turns out to still be an issue I >>>>>>> suggest we add a module parameter to allow users to >>>>>>> limit the max_bpc to a desired value. >>>>>> >>>>>> While leaving the fallback for user space to handle makes some sense >>>>>> in theory, in practice most KMS display servers likely won't handle >>>>>> it. >>>>>> >>>>>> Another issue is that if mode validation is based on the maximum bpc >>>>>> value, it may reject modes which would work with lower bpc. >>>>>> >>>>>> >>>>>> What Ville (CC'd) suggested before instead (and what i915 seems to be >>>>>> doing already) is that the driver should do mode validation based on >>>>>> the *minimum* bpc, and automatically make the effective bpc lower >>>>>> than the maximum as needed to make the rest of the atomic state work. >>>>> >>>>> A driver is always allowed to choose a bpc lower than max_bpc, so it >>>>> very well should do so when necessary due to *known* hardware etc. >>>>> limitations. >>>>> >>>> >>>> In the amdgpu case, it's more of a preference thing. The driver would >>>> enable higher bpcs at the expense of refresh rate and it seemed most >>>> users want higher refresh rates than higher bpc. >>> >>> Hi Alex, >>> >>> we already have userspace in explicit control of the refresh rate. >>> Userspace picks the refresh rate first, then the driver silently checks >>> what bpc is possible. Userspace preference wins, so bpc is chosen to >>> produce the desired refresh rate. >>> >>> In what cases does the driver really pick a refresh rate on its own? >> >> It didn't, but IIRC, at the time the driver filtered out modes that it >> could not support with the max bpc. It looks like that has been fixed >> now, so maybe this whole discussion is moot. > > That depends on the answer to the follow-up question in my previous post. Never mind, looks like cbd14ae7ea93 ("drm/amd/display: Fix incorrectly pruned modes with deep color") does lower bpc as needed both for mode validation and atomic commit (check).
On 12/15/22 10:07, Michel Dänzer wrote: > On 12/14/22 16:46, Alex Deucher wrote: >> On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: >>> On Tue, 13 Dec 2022 18:20:59 +0100 >>> Michel Dänzer <michel.daenzer@mailbox.org> wrote: >>>> On 12/12/22 19:21, Harry Wentland wrote: >>>>> This will let us pass kms_hdr.bpc_switch. >>>>> >>>>> I don't see any good reasons why we still need to >>>>> limit bpc to 8 bpc and doing so is problematic when >>>>> we enable HDR. >>>>> >>>>> If I remember correctly there might have been some >>>>> displays out there where the advertised link bandwidth >>>>> was not large enough to drive the default timing at >>>>> max bpc. This would leave to an atomic commit/check >>>>> failure which should really be handled in compositors >>>>> with some sort of fallback mechanism. >>>>> >>>>> If this somehow turns out to still be an issue I >>>>> suggest we add a module parameter to allow users to >>>>> limit the max_bpc to a desired value. >>>> >>>> While leaving the fallback for user space to handle makes some sense >>>> in theory, in practice most KMS display servers likely won't handle >>>> it. >>>> >>>> Another issue is that if mode validation is based on the maximum bpc >>>> value, it may reject modes which would work with lower bpc. >>>> >>>> >>>> What Ville (CC'd) suggested before instead (and what i915 seems to be >>>> doing already) is that the driver should do mode validation based on >>>> the *minimum* bpc, and automatically make the effective bpc lower >>>> than the maximum as needed to make the rest of the atomic state work. >>> >>> A driver is always allowed to choose a bpc lower than max_bpc, so it >>> very well should do so when necessary due to *known* hardware etc. >>> limitations. >>> >> >> In the amdgpu case, it's more of a preference thing. The driver would >> enable higher bpcs at the expense of refresh rate and it seemed most >> users want higher refresh rates than higher bpc. > > I wrote the above because I thought that this patch might result in some modes getting pruned because they can't work with the max bpc. However, I see now that cbd14ae7ea93 ("drm/amd/display: Fix incorrectly pruned modes with deep color") should prevent that AFAICT. > > The question then is: What happens if user space tries to use a mode which doesn't work with the max bpc? Does the driver automatically lower the effective bpc as needed, or does the atomic commit (check) fail? The latter would seem bad. Per my previous post in the other sub-thread, cbd14ae7ea93 ("drm/amd/display: Fix incorrectly pruned modes with deep color") seems to do the former. The commit log of this patch should probably be changed to reflect that.
On 12/14/22 04:01, Pekka Paalanen wrote: > On Tue, 13 Dec 2022 18:20:59 +0100 > Michel Dänzer <michel.daenzer@mailbox.org> wrote: > >> On 12/12/22 19:21, Harry Wentland wrote: >>> This will let us pass kms_hdr.bpc_switch. >>> >>> I don't see any good reasons why we still need to >>> limit bpc to 8 bpc and doing so is problematic when >>> we enable HDR. >>> >>> If I remember correctly there might have been some >>> displays out there where the advertised link bandwidth >>> was not large enough to drive the default timing at >>> max bpc. This would leave to an atomic commit/check >>> failure which should really be handled in compositors >>> with some sort of fallback mechanism. >>> >>> If this somehow turns out to still be an issue I >>> suggest we add a module parameter to allow users to >>> limit the max_bpc to a desired value. >> >> While leaving the fallback for user space to handle makes some sense >> in theory, in practice most KMS display servers likely won't handle >> it. >> >> Another issue is that if mode validation is based on the maximum bpc >> value, it may reject modes which would work with lower bpc. >> >> >> What Ville (CC'd) suggested before instead (and what i915 seems to be >> doing already) is that the driver should do mode validation based on >> the *minimum* bpc, and automatically make the effective bpc lower >> than the maximum as needed to make the rest of the atomic state work. > > A driver is always allowed to choose a bpc lower than max_bpc, so it > very well should do so when necessary due to *known* hardware etc. > limitations. > I spent a bunch of time to figure out how this actually pans out in amdgpu and it looks like we're doing the right thing, i.e. if bandwidth limitations require it we'll downgrade bpc appropriately. These changes happened over the last couple years or so. So while raising the default max_bpc wasn't safe in amdgpu years ago it is completely fine now. As for the relevant code it's mostly handled in create_validate_stream_for_sink in amdgpu_dm.c where we iterate over a stream's mode validation with decreasing bpc if it fails (down to a bpc of 6). For HDMI we also have a separate adjust_colour_depth_from_display_info function that downgrades bpc in order to fit within the max_tmds_clock. So, in short, this change should not lead to displays not lighting up because we no longer force a given bpc. > So things like mode validation cannot just look at a single max or min > bpc, but it needs to figure out if there is any usable bpc value that > makes the mode work. > > The max_bpc knob exists only for the cases where the sink undetectably > malfunctions unless the bpc is artificially limited more than seems > necessary. That malfunction requires a human to detect, and reconfigure > their system as we don't have a quirk database for this I think. > > The question of userspace wanting a specific bpc is a different matter > and an unsolved one. It also ties to userspace wanting to use the > current mode to avoid a mode switch between e.g. hand-off from firmware > boot splash to proper userspace. That's also unsolved AFAIK. > Agreed, the current "max bpc" just sets a max. We'd probably want a "min bpc" if userspace needs a minimum (e.g., for HDR). Harry > OTOH, we have the discussion that concluded as > https://gitlab.freedesktop.org/wayland/weston/-/issues/612#note_1359898 > which really puts userspace in charge of max_bpc, so the driver-chosen > default value does not have much impact as long as it makes the > firmware-chosen video mode to continue, as requested in > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/995 > given that userspace cannot know what the actual bpc currently is nor > set the exact bpc to keep it the same. > > > Thanks, > pq
On 12/23/22 20:10, Harry Wentland wrote: > On 12/14/22 04:01, Pekka Paalanen wrote: >> On Tue, 13 Dec 2022 18:20:59 +0100 >> Michel Dänzer <michel.daenzer@mailbox.org> wrote: >>> On 12/12/22 19:21, Harry Wentland wrote: >>>> This will let us pass kms_hdr.bpc_switch. >>>> >>>> I don't see any good reasons why we still need to >>>> limit bpc to 8 bpc and doing so is problematic when >>>> we enable HDR. >>>> >>>> If I remember correctly there might have been some >>>> displays out there where the advertised link bandwidth >>>> was not large enough to drive the default timing at >>>> max bpc. This would leave to an atomic commit/check >>>> failure which should really be handled in compositors >>>> with some sort of fallback mechanism. >>>> >>>> If this somehow turns out to still be an issue I >>>> suggest we add a module parameter to allow users to >>>> limit the max_bpc to a desired value. >>> >>> While leaving the fallback for user space to handle makes some sense >>> in theory, in practice most KMS display servers likely won't handle >>> it. >>> >>> Another issue is that if mode validation is based on the maximum bpc >>> value, it may reject modes which would work with lower bpc. >>> >>> >>> What Ville (CC'd) suggested before instead (and what i915 seems to be >>> doing already) is that the driver should do mode validation based on >>> the *minimum* bpc, and automatically make the effective bpc lower >>> than the maximum as needed to make the rest of the atomic state work. >> >> A driver is always allowed to choose a bpc lower than max_bpc, so it >> very well should do so when necessary due to *known* hardware etc. >> limitations. >> > > I spent a bunch of time to figure out how this actually pans out in > amdgpu and it looks like we're doing the right thing, i.e. if bandwidth > limitations require it we'll downgrade bpc appropriately. These changes > happened over the last couple years or so. So while raising the default > max_bpc wasn't safe in amdgpu years ago it is completely fine now. > > As for the relevant code it's mostly handled in create_validate_stream_for_sink > in amdgpu_dm.c where we iterate over a stream's mode validation with > decreasing bpc if it fails (down to a bpc of 6). > > For HDMI we also have a separate adjust_colour_depth_from_display_info > function that downgrades bpc in order to fit within the max_tmds_clock. > > So, in short, this change should not lead to displays not lighting up > because we no longer force a given bpc. Thanks for double-checking! This patch is Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
On Fri, Dec 23, 2022 at 8:10 PM Harry Wentland <harry.wentland@amd.com> wrote: > > > > On 12/14/22 04:01, Pekka Paalanen wrote: > > On Tue, 13 Dec 2022 18:20:59 +0100 > > Michel Dänzer <michel.daenzer@mailbox.org> wrote: > > > >> On 12/12/22 19:21, Harry Wentland wrote: > >>> This will let us pass kms_hdr.bpc_switch. > >>> > >>> I don't see any good reasons why we still need to > >>> limit bpc to 8 bpc and doing so is problematic when > >>> we enable HDR. > >>> > >>> If I remember correctly there might have been some > >>> displays out there where the advertised link bandwidth > >>> was not large enough to drive the default timing at > >>> max bpc. This would leave to an atomic commit/check > >>> failure which should really be handled in compositors > >>> with some sort of fallback mechanism. > >>> > >>> If this somehow turns out to still be an issue I > >>> suggest we add a module parameter to allow users to > >>> limit the max_bpc to a desired value. > >> > >> While leaving the fallback for user space to handle makes some sense > >> in theory, in practice most KMS display servers likely won't handle > >> it. > >> > >> Another issue is that if mode validation is based on the maximum bpc > >> value, it may reject modes which would work with lower bpc. > >> > >> > >> What Ville (CC'd) suggested before instead (and what i915 seems to be > >> doing already) is that the driver should do mode validation based on > >> the *minimum* bpc, and automatically make the effective bpc lower > >> than the maximum as needed to make the rest of the atomic state work. > > > > A driver is always allowed to choose a bpc lower than max_bpc, so it > > very well should do so when necessary due to *known* hardware etc. > > limitations. > > > > I spent a bunch of time to figure out how this actually pans out in > amdgpu and it looks like we're doing the right thing, i.e. if bandwidth > limitations require it we'll downgrade bpc appropriately. These changes > happened over the last couple years or so. So while raising the default > max_bpc wasn't safe in amdgpu years ago it is completely fine now. > > As for the relevant code it's mostly handled in create_validate_stream_for_sink > in amdgpu_dm.c where we iterate over a stream's mode validation with > decreasing bpc if it fails (down to a bpc of 6). > > For HDMI we also have a separate adjust_colour_depth_from_display_info > function that downgrades bpc in order to fit within the max_tmds_clock. > > So, in short, this change should not lead to displays not lighting up > because we no longer force a given bpc. Very good! > > > So things like mode validation cannot just look at a single max or min > > bpc, but it needs to figure out if there is any usable bpc value that > > makes the mode work. > > > > The max_bpc knob exists only for the cases where the sink undetectably > > malfunctions unless the bpc is artificially limited more than seems > > necessary. That malfunction requires a human to detect, and reconfigure > > their system as we don't have a quirk database for this I think. > > > > The question of userspace wanting a specific bpc is a different matter > > and an unsolved one. It also ties to userspace wanting to use the > > current mode to avoid a mode switch between e.g. hand-off from firmware > > boot splash to proper userspace. That's also unsolved AFAIK. > > > > Agreed, the current "max bpc" just sets a max. We'd probably want a > "min bpc" if userspace needs a minimum (e.g., for HDR). To be clear: we need this. I've argued before that we need a min bpc setting because we'd rather not enable HDR if we can't get the bit depth required for it and there is no other mechanism to control this. The other remaining kernel problem for HDR is that we still have no control over YCC/RGB selection on the cable and thus can't choose the correct color space infoframe. > > Harry > > > OTOH, we have the discussion that concluded as > > https://gitlab.freedesktop.org/wayland/weston/-/issues/612#note_1359898 > > which really puts userspace in charge of max_bpc, so the driver-chosen > > default value does not have much impact as long as it makes the > > firmware-chosen video mode to continue, as requested in > > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/995 > > given that userspace cannot know what the actual bpc currently is nor > > set the exact bpc to keep it the same. > > > > > > Thanks, > > pq >
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 d0c071000a5d..396e345f5d6b 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -7115,7 +7115,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, drm_connector_attach_max_bpc_property(&aconnector->base, 8, 16); /* This defaults to the max in the range, but we want 8bpc for non-edp. */ - aconnector->base.state->max_bpc = (connector_type == DRM_MODE_CONNECTOR_eDP) ? 16 : 8; + aconnector->base.state->max_bpc = 16; aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc; if (connector_type == DRM_MODE_CONNECTOR_eDP &&
This will let us pass kms_hdr.bpc_switch. I don't see any good reasons why we still need to limit bpc to 8 bpc and doing so is problematic when we enable HDR. If I remember correctly there might have been some displays out there where the advertised link bandwidth was not large enough to drive the default timing at max bpc. This would leave to an atomic commit/check failure which should really be handled in compositors with some sort of fallback mechanism. If this somehow turns out to still be an issue I suggest we add a module parameter to allow users to limit the max_bpc to a desired value. Signed-off-by: Harry Wentland <harry.wentland@amd.com> Cc: Pekka Paalanen <ppaalanen@gmail.com> Cc: Sebastian Wick <sebastian.wick@redhat.com> Cc: Vitaly.Prosyak@amd.com Cc: Uma Shankar <uma.shankar@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Joshua Ashton <joshua@froggi.es> Cc: dri-devel@lists.freedesktop.org Cc: amd-gfx@lists.freedesktop.org --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)