On Fri, 25 Aug 2023 13:29:44 -0100 Melissa Wen <m...@igalia.com> wrote:
> On 08/22, Pekka Paalanen wrote:
> > On Thu, 10 Aug 2023 15:02:59 -0100
> > Melissa Wen <m...@igalia.com> wrote:
> >
> > > The next patch adds pre-blending degamma to AMD color mgmt pipeline, but
> > > pre-blending degamma caps (DPP) is currently in use to provide DRM CRTC
> > > atomic degamma or implict degamma on legacy gamma. Detach degamma usage
> > > regarging CRTC color properties to manage plane and CRTC color
> > > correction combinations.
> > >
> > > Reviewed-by: Harry Wentland <harry.wentl...@amd.com>
> > > Signed-off-by: Melissa Wen <m...@igalia.com>
> > > ---
> > > .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 59 +++++++++++++------
> > > 1 file changed, 41 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> > > index 68e9f2c62f2e..74eb02655d96 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> > > @@ -764,20 +764,9 @@ int amdgpu_dm_update_crtc_color_mgmt(struct
> > > dm_crtc_state *crtc)
> > > return 0;
> > > }
> > >
> > > -/**
> > > - * amdgpu_dm_update_plane_color_mgmt: Maps DRM color management to DC
> > > plane.
> > > - * @crtc: amdgpu_dm crtc state
> > > - * @dc_plane_state: target DC surface
> > > - *
> > > - * Update the underlying dc_stream_state's input transfer function (ITF)
> > > in
> > > - * preparation for hardware commit. The transfer function used depends on
> > > - * the preparation done on the stream for color management.
> > > - *
> > > - * Returns:
> > > - * 0 on success. -ENOMEM if mem allocation fails.
> > > - */
> > > -int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
> > > - struct dc_plane_state *dc_plane_state)
> > > +static int
> > > +map_crtc_degamma_to_dc_plane(struct dm_crtc_state *crtc,
> > > + struct dc_plane_state *dc_plane_state)
> > > {
> > > const struct drm_color_lut *degamma_lut;
> > > enum dc_transfer_func_predefined tf = TRANSFER_FUNCTION_SRGB;
> > > @@ -800,8 +789,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct
> > > dm_crtc_state *crtc,
> > > °amma_size);
> > > ASSERT(degamma_size == MAX_COLOR_LUT_ENTRIES);
> > >
> > > - dc_plane_state->in_transfer_func->type =
> > > - TF_TYPE_DISTRIBUTED_POINTS;
> > > + dc_plane_state->in_transfer_func->type =
> > > TF_TYPE_DISTRIBUTED_POINTS;
> > >
> > > /*
> > > * This case isn't fully correct, but also fairly
> > > @@ -837,7 +825,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct
> > > dm_crtc_state *crtc,
> > > degamma_lut, degamma_size);
> > > if (r)
> > > return r;
> > > - } else if (crtc->cm_is_degamma_srgb) {
> > > + } else {
> > > /*
> > > * For legacy gamma support we need the regamma input
> > > * in linear space. Assume that the input is sRGB.
> > > @@ -847,8 +835,43 @@ int amdgpu_dm_update_plane_color_mgmt(struct
> > > dm_crtc_state *crtc,
> > >
> > > if (tf != TRANSFER_FUNCTION_SRGB &&
> > > !mod_color_calculate_degamma_params(NULL,
> > > - dc_plane_state->in_transfer_func, NULL, false))
> > > +
> > > dc_plane_state->in_transfer_func,
> > > + NULL, false))
> > > return -ENOMEM;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * amdgpu_dm_update_plane_color_mgmt: Maps DRM color management to DC
> > > plane.
> > > + * @crtc: amdgpu_dm crtc state
> > > + * @dc_plane_state: target DC surface
> > > + *
> > > + * Update the underlying dc_stream_state's input transfer function (ITF)
> > > in
> > > + * preparation for hardware commit. The transfer function used depends on
> > > + * the preparation done on the stream for color management.
> > > + *
> > > + * Returns:
> > > + * 0 on success. -ENOMEM if mem allocation fails.
> > > + */
> > > +int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
> > > + struct dc_plane_state *dc_plane_state)
> > > +{
> > > + bool has_crtc_cm_degamma;
> > > + int ret;
> > > +
> > > + has_crtc_cm_degamma = (crtc->cm_has_degamma ||
> > > crtc->cm_is_degamma_srgb);
> > > + if (has_crtc_cm_degamma){
> > > + /* AMD HW doesn't have post-blending degamma caps. When DRM
> > > + * CRTC atomic degamma is set, we maps it to DPP degamma block
> > > + * (pre-blending) or, on legacy gamma, we use DPP degamma to
> > > + * linearize (implicit degamma) from sRGB/BT709 according to
> > > + * the input space.
> >
> > Uhh, you can't just move degamma before blending if KMS userspace
> > wants it after blending. That would be incorrect behaviour. If you
> > can't implement it correctly, reject it.
> >
> > I hope that magical unexpected linearization is not done with atomic,
> > either.
> >
> > Or maybe this is all a lost cause, and only the new color-op pipeline
> > UAPI will actually work across drivers.
>
> I agree that crtc degamma is an optional property and should be not
> exposed if not available. I did something in this line for DCE that has
> no degamma block[1]. Then, AMD DDX driver stopped to advertise atomic
> API for DCE, that was not correct too[2].
Did AMD go through all the trouble of making their Xorg DDX use KMS
atomic, even after the kernel took it away from X due to modesetting
DDX screwing it up? I'm surprised, what did that achieve?
I saw that between 5.15 and 6.1 amdgpu stopped advertising CRTC DEGAMMA
on my card, which seems like the right thing to do if there is no
hardware for it.
> But I see it as a lost cause that will only be fixed in a new generic
> color API. I don't think we should change it using the current DRM CRTC
> API with driver-specific props.
>
> [1] https://lore.kernel.org/amd-gfx/20221103184500.14450-1-m...@igalia.com/
> [2] https://gitlab.freedesktop.org/xorg/driver/xf86-video-amdgpu/-/issues/67
Oh well.
Is the old CRTC GAMMA property still "safe" to use in that it is
definitely always after blending?
Thanks,
pq
> > > + */
> > > + ret = map_crtc_degamma_to_dc_plane(crtc, dc_plane_state);
> > > + if (ret)
> > > + return ret;
> > > } else {
> > > /* ...Otherwise we can just bypass the DGM block. */
> > > dc_plane_state->in_transfer_func->type = TF_TYPE_BYPASS;
> >
pgpPBlIeTrXcr.pgp
Description: OpenPGP digital signature
