drm/nouveau/kms/nv50-: Always validate LUTs in nv50_head_atomic_check_lut()

When it comes to gamma or degamma luts, nouveau will actually skip the
calculation of certain LUTs depending on the head and plane states. For
instance, when the head is disabled we don't perform any error checking on
the gamma LUT, and likewise if no planes are present and enabled in our
atomic state we will skip error checking the degamma LUT. This is a bit of
a problem though, since the per-head gamma and degamma props in DRM can be
changed even while a head is disabled - a situation which can be triggered
by the igt testcase mentioned down below.

Originally I thought this was a bit silly and was tempted to just fix the
igt test to only set gamma/degamma with the head enabled. After a bit of
thinking though I realized we should fix this in nouveau. This is because
if a program decides to set an invalid LUT for a head before enabling the
head, such a property change would succeed while also making it impossible
to turn the head back on until the LUT is removed or corrected - something
that could be painful for a user to figure out.

So, fix this checking both degamma and gamma LUTs unconditionally during
atomic checks. We start by calling nv50_head_atomic_check_lut() regardless
of whether the head is active or not in nv50_head_atomic_check(). Then we
move the ilut error checking into nv50_head_atomic_check_lut() and add a
per-head hook for it, primarily because as a per-CRTC property DRM we want
the LUT to be error checked by the head any time it's included in an atomic
state. Of course though, actual programming of the degamma lut to hardware
is still handled in each plane's atomic check and commit.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Testcase: igt/kms_color/pipe-invalid-*-lut-sizes
Signed-off-by: Ben Skeggs <bskeggs@redhat.com>
Reviewed-by: Karol Herbst <kherbst@redhat.com>
Signed-off-by: Karol Herbst <kherbst@redhat.com>
Link: https://gitlab.freedesktop.org/drm/nouveau/-/merge_requests/10

diff --git a/drivers/gpu/drm/nouveau/dispnv50/base907c.c b/drivers/gpu/drm/nouveau/dispnv50/base907c.c
index 5396e37..e6b0417 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/base907c.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/base907c.c
@@ -103,12 +103,9 @@ base907c_xlut_set(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw)
        return 0;
 }
 
        return 0;
 }
 

-static bool
+static void

 base907c_ilut(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw, int size)
 {
 base907c_ilut(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw, int size)
 {

-       if (size != 256 && size != 1024)
-               return false;
-

        if (size == 1024)
                asyw->xlut.i.mode = NV907C_SET_BASE_LUT_LO_MODE_INTERPOLATE_1025_UNITY_RANGE;
        else
        if (size == 1024)
                asyw->xlut.i.mode = NV907C_SET_BASE_LUT_LO_MODE_INTERPOLATE_1025_UNITY_RANGE;
        else


@@ -116,7 +113,6 @@ base907c_ilut(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw, int size)
 
        asyw->xlut.i.enable = NV907C_SET_BASE_LUT_LO_ENABLE_ENABLE;
        asyw->xlut.i.load = head907d_olut_load;
 
        asyw->xlut.i.enable = NV907C_SET_BASE_LUT_LO_ENABLE_ENABLE;
        asyw->xlut.i.load = head907d_olut_load;

-       return true;

 }
 
 static inline u32
 }
 
 static inline u32

diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c
index ca8b292..f8438a8 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/head.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
@@ -230,9 +230,20 @@ nv50_head_atomic_check_lut(struct nv50_head *head,
        struct drm_crtc *crtc = &head->base.base;
        struct nv50_disp *disp = nv50_disp(dev);
        struct nouveau_drm *drm = nouveau_drm(dev);
        struct drm_crtc *crtc = &head->base.base;
        struct nv50_disp *disp = nv50_disp(dev);
        struct nouveau_drm *drm = nouveau_drm(dev);

-       struct drm_property_blob *olut = asyh->state.gamma_lut;
+       struct drm_property_blob *olut = asyh->state.gamma_lut,
+                                *ilut = asyh->state.degamma_lut;

        int size;
 
        int size;
 

+       /* Ensure that the ilut is valid */
+       if (ilut) {
+               size = drm_color_lut_size(ilut);
+               if (!head->func->ilut_check(size)) {
+                       NV_ATOMIC(drm, "Invalid size %d for degamma on [CRTC:%d:%s]\n",
+                                 size, crtc->base.id, crtc->name);
+                       return -EINVAL;
+               }
+       }
+

        /* Determine whether core output LUT should be enabled. */
        if (olut) {
                /* Check if any window(s) have stolen the core output LUT
        /* Determine whether core output LUT should be enabled. */
        if (olut) {
                /* Check if any window(s) have stolen the core output LUT


@@ -334,8 +345,17 @@ nv50_head_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
        struct drm_connector_state *conns;
        struct drm_connector *conn;
        int i, ret;
        struct drm_connector_state *conns;
        struct drm_connector *conn;
        int i, ret;

+       bool check_lut = asyh->state.color_mgmt_changed ||
+                        memcmp(&armh->wndw, &asyh->wndw, sizeof(asyh->wndw));

 
        NV_ATOMIC(drm, "%s atomic_check %d\n", crtc->name, asyh->state.active);
 
        NV_ATOMIC(drm, "%s atomic_check %d\n", crtc->name, asyh->state.active);

+
+       if (check_lut) {
+               ret = nv50_head_atomic_check_lut(head, asyh);
+               if (ret)
+                       return ret;
+       }
+

        if (asyh->state.active) {
                for_each_new_connector_in_state(asyh->state.state, conn, conns, i) {
                        if (conns->crtc == crtc) {
        if (asyh->state.active) {
                for_each_new_connector_in_state(asyh->state.state, conn, conns, i) {
                        if (conns->crtc == crtc) {


@@ -361,14 +381,8 @@ nv50_head_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
                if (asyh->state.mode_changed || asyh->state.connectors_changed)
                        nv50_head_atomic_check_mode(head, asyh);
 
                if (asyh->state.mode_changed || asyh->state.connectors_changed)
                        nv50_head_atomic_check_mode(head, asyh);
 

-               if (asyh->state.color_mgmt_changed ||
-                   memcmp(&armh->wndw, &asyh->wndw, sizeof(asyh->wndw))) {
-                       int ret = nv50_head_atomic_check_lut(head, asyh);
-                       if (ret)
-                               return ret;
-
+               if (check_lut)

                        asyh->olut.visible = asyh->olut.handle != 0;
                        asyh->olut.visible = asyh->olut.handle != 0;

-               }

 
                if (asyc) {
                        if (asyc->set.scaler)
 
                if (asyc) {
                        if (asyc->set.scaler)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.h b/drivers/gpu/drm/nouveau/dispnv50/head.h
index 0bac6be..41c8788 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/head.h
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.h
@@ -29,6 +29,7 @@ struct nv50_head_func {
        int (*view)(struct nv50_head *, struct nv50_head_atom *);
        int (*mode)(struct nv50_head *, struct nv50_head_atom *);
        bool (*olut)(struct nv50_head *, struct nv50_head_atom *, int);
        int (*view)(struct nv50_head *, struct nv50_head_atom *);
        int (*mode)(struct nv50_head *, struct nv50_head_atom *);
        bool (*olut)(struct nv50_head *, struct nv50_head_atom *, int);

+       bool (*ilut_check)(int size);

        bool olut_identity;
        int  olut_size;
        int (*olut_set)(struct nv50_head *, struct nv50_head_atom *);
        bool olut_identity;
        int  olut_size;
        int (*olut_set)(struct nv50_head *, struct nv50_head_atom *);


@@ -71,6 +72,7 @@ extern const struct nv50_head_func head907d;
 int head907d_view(struct nv50_head *, struct nv50_head_atom *);
 int head907d_mode(struct nv50_head *, struct nv50_head_atom *);
 bool head907d_olut(struct nv50_head *, struct nv50_head_atom *, int);
 int head907d_view(struct nv50_head *, struct nv50_head_atom *);
 int head907d_mode(struct nv50_head *, struct nv50_head_atom *);
 bool head907d_olut(struct nv50_head *, struct nv50_head_atom *, int);

+bool head907d_ilut_check(int size);

 int head907d_olut_set(struct nv50_head *, struct nv50_head_atom *);
 int head907d_olut_clr(struct nv50_head *);
 int head907d_core_set(struct nv50_head *, struct nv50_head_atom *);
 int head907d_olut_set(struct nv50_head *, struct nv50_head_atom *);
 int head907d_olut_clr(struct nv50_head *);
 int head907d_core_set(struct nv50_head *, struct nv50_head_atom *);

diff --git a/drivers/gpu/drm/nouveau/dispnv50/head907d.c b/drivers/gpu/drm/nouveau/dispnv50/head907d.c
index 85648d7..18fe4c1 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/head907d.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head907d.c
@@ -314,6 +314,11 @@ head907d_olut(struct nv50_head *head, struct nv50_head_atom *asyh, int size)
        return true;
 }
 
        return true;
 }
 

+bool head907d_ilut_check(int size)
+{
+       return size == 256 || size == 1024;
+}
+

 int
 head907d_mode(struct nv50_head *head, struct nv50_head_atom *asyh)
 {
 int
 head907d_mode(struct nv50_head *head, struct nv50_head_atom *asyh)
 {


@@ -409,6 +414,7 @@ head907d = {
        .view = head907d_view,
        .mode = head907d_mode,
        .olut = head907d_olut,
        .view = head907d_view,
        .mode = head907d_mode,
        .olut = head907d_olut,

+       .ilut_check = head907d_ilut_check,

        .olut_size = 1024,
        .olut_set = head907d_olut_set,
        .olut_clr = head907d_olut_clr,
        .olut_size = 1024,
        .olut_set = head907d_olut_set,
        .olut_clr = head907d_olut_clr,

diff --git a/drivers/gpu/drm/nouveau/dispnv50/head917d.c b/drivers/gpu/drm/nouveau/dispnv50/head917d.c
index ea9f866..4ce47b5 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/head917d.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head917d.c
@@ -119,6 +119,7 @@ head917d = {
        .view = head907d_view,
        .mode = head907d_mode,
        .olut = head907d_olut,
        .view = head907d_view,
        .mode = head907d_mode,
        .olut = head907d_olut,

+       .ilut_check = head907d_ilut_check,

        .olut_size = 1024,
        .olut_set = head907d_olut_set,
        .olut_clr = head907d_olut_clr,
        .olut_size = 1024,
        .olut_set = head907d_olut_set,
        .olut_clr = head907d_olut_clr,

diff --git a/drivers/gpu/drm/nouveau/dispnv50/headc37d.c b/drivers/gpu/drm/nouveau/dispnv50/headc37d.c
index 63adfeb..a4a3b78 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/headc37d.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/headc37d.c
@@ -285,6 +285,7 @@ headc37d = {
        .view = headc37d_view,
        .mode = headc37d_mode,
        .olut = headc37d_olut,
        .view = headc37d_view,
        .mode = headc37d_mode,
        .olut = headc37d_olut,

+       .ilut_check = head907d_ilut_check,

        .olut_size = 1024,
        .olut_set = headc37d_olut_set,
        .olut_clr = headc37d_olut_clr,
        .olut_size = 1024,
        .olut_set = headc37d_olut_set,
        .olut_clr = headc37d_olut_clr,

diff --git a/drivers/gpu/drm/nouveau/dispnv50/headc57d.c b/drivers/gpu/drm/nouveau/dispnv50/headc57d.c
index fd51527..fd624eb 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/headc57d.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/headc57d.c
@@ -236,6 +236,7 @@ headc57d = {
        .view = headc37d_view,
        .mode = headc57d_mode,
        .olut = headc57d_olut,
        .view = headc37d_view,
        .mode = headc57d_mode,
        .olut = headc57d_olut,

+       .ilut_check = head907d_ilut_check,

        .olut_identity = true,
        .olut_size = 1024,
        .olut_set = headc57d_olut_set,
        .olut_identity = true,
        .olut_size = 1024,
        .olut_set = headc57d_olut_set,

diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
index 30712a6..133c873 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
@@ -403,10 +403,7 @@ nv50_wndw_atomic_check_lut(struct nv50_wndw *wndw,
        /* Recalculate LUT state. */
        memset(&asyw->xlut, 0x00, sizeof(asyw->xlut));
        if ((asyw->ilut = wndw->func->ilut ? ilut : NULL)) {
        /* Recalculate LUT state. */
        memset(&asyw->xlut, 0x00, sizeof(asyw->xlut));
        if ((asyw->ilut = wndw->func->ilut ? ilut : NULL)) {

-               if (!wndw->func->ilut(wndw, asyw, drm_color_lut_size(ilut))) {
-                       DRM_DEBUG_KMS("Invalid ilut\n");
-                       return -EINVAL;
-               }
+               wndw->func->ilut(wndw, asyw, drm_color_lut_size(ilut));

                asyw->xlut.handle = wndw->wndw.vram.handle;
                asyw->xlut.i.buffer = !asyw->xlut.i.buffer;
                asyw->set.xlut = true;
                asyw->xlut.handle = wndw->wndw.vram.handle;
                asyw->xlut.i.buffer = !asyw->xlut.i.buffer;
                asyw->set.xlut = true;

diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.h b/drivers/gpu/drm/nouveau/dispnv50/wndw.h
index f4e0c50..9c9f2c2 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/wndw.h
+++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.h
@@ -64,7 +64,7 @@ struct nv50_wndw_func {
        int (*ntfy_clr)(struct nv50_wndw *);
        int (*ntfy_wait_begun)(struct nouveau_bo *, u32 offset,
                               struct nvif_device *);
        int (*ntfy_clr)(struct nv50_wndw *);
        int (*ntfy_wait_begun)(struct nouveau_bo *, u32 offset,
                               struct nvif_device *);

-       bool (*ilut)(struct nv50_wndw *, struct nv50_wndw_atom *, int);
+       void (*ilut)(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyh, int size);

        void (*csc)(struct nv50_wndw *, struct nv50_wndw_atom *,
                    const struct drm_color_ctm *);
        int (*csc_set)(struct nv50_wndw *, struct nv50_wndw_atom *);
        void (*csc)(struct nv50_wndw *, struct nv50_wndw_atom *,
                    const struct drm_color_ctm *);
        int (*csc_set)(struct nv50_wndw *, struct nv50_wndw_atom *);


@@ -129,7 +129,7 @@ int wndwc37e_update(struct nv50_wndw *, u32 *);
 
 int wndwc57e_new(struct nouveau_drm *, enum drm_plane_type, int, s32,
                 struct nv50_wndw **);
 
 int wndwc57e_new(struct nouveau_drm *, enum drm_plane_type, int, s32,
                 struct nv50_wndw **);

-bool wndwc57e_ilut(struct nv50_wndw *, struct nv50_wndw_atom *, int);
+void wndwc57e_ilut(struct nv50_wndw *, struct nv50_wndw_atom *, int);

 int wndwc57e_ilut_set(struct nv50_wndw *, struct nv50_wndw_atom *);
 int wndwc57e_ilut_clr(struct nv50_wndw *);
 int wndwc57e_csc_set(struct nv50_wndw *, struct nv50_wndw_atom *);
 int wndwc57e_ilut_set(struct nv50_wndw *, struct nv50_wndw_atom *);
 int wndwc57e_ilut_clr(struct nv50_wndw *);
 int wndwc57e_csc_set(struct nv50_wndw *, struct nv50_wndw_atom *);

diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndwc37e.c b/drivers/gpu/drm/nouveau/dispnv50/wndwc37e.c
index 57df997..183d2c0 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/wndwc37e.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/wndwc37e.c
@@ -82,18 +82,14 @@ wndwc37e_ilut_set(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw)
        return 0;
 }
 
        return 0;
 }
 

-static bool
+static void

 wndwc37e_ilut(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw, int size)
 {
 wndwc37e_ilut(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw, int size)
 {

-       if (size != 256 && size != 1024)
-               return false;
-

        asyw->xlut.i.size = size == 1024 ? NVC37E_SET_CONTROL_INPUT_LUT_SIZE_SIZE_1025 :
                                           NVC37E_SET_CONTROL_INPUT_LUT_SIZE_SIZE_257;
        asyw->xlut.i.range = NVC37E_SET_CONTROL_INPUT_LUT_RANGE_UNITY;
        asyw->xlut.i.output_mode = NVC37E_SET_CONTROL_INPUT_LUT_OUTPUT_MODE_INTERPOLATE;
        asyw->xlut.i.load = head907d_olut_load;
        asyw->xlut.i.size = size == 1024 ? NVC37E_SET_CONTROL_INPUT_LUT_SIZE_SIZE_1025 :
                                           NVC37E_SET_CONTROL_INPUT_LUT_SIZE_SIZE_257;
        asyw->xlut.i.range = NVC37E_SET_CONTROL_INPUT_LUT_RANGE_UNITY;
        asyw->xlut.i.output_mode = NVC37E_SET_CONTROL_INPUT_LUT_OUTPUT_MODE_INTERPOLATE;
        asyw->xlut.i.load = head907d_olut_load;

-       return true;

 }
 
 int
 }
 
 int

diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndwc57e.c b/drivers/gpu/drm/nouveau/dispnv50/wndwc57e.c
index abdd3bb..37f6da8 100644 (file)
--- a/drivers/gpu/drm/nouveau/dispnv50/wndwc57e.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/wndwc57e.c
@@ -179,11 +179,11 @@ wndwc57e_ilut_load(struct drm_color_lut *in, int size, void __iomem *mem)
        writew(readw(mem - 4), mem + 4);
 }
 
        writew(readw(mem - 4), mem + 4);
 }
 

-bool
+void

 wndwc57e_ilut(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw, int size)
 {
 wndwc57e_ilut(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw, int size)
 {

-       if (size = size ? size : 1024, size != 256 && size != 1024)
-               return false;
+       if (!size)
+               size = 1024;

 
        if (size == 256)
                asyw->xlut.i.mode = NVC57E_SET_ILUT_CONTROL_MODE_DIRECT8;
 
        if (size == 256)
                asyw->xlut.i.mode = NVC57E_SET_ILUT_CONTROL_MODE_DIRECT8;


@@ -193,7 +193,6 @@ wndwc57e_ilut(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw, int size)
        asyw->xlut.i.size = 4 /* VSS header. */ + size + 1 /* Entries. */;
        asyw->xlut.i.output_mode = NVC57E_SET_ILUT_CONTROL_INTERPOLATE_DISABLE;
        asyw->xlut.i.load = wndwc57e_ilut_load;
        asyw->xlut.i.size = 4 /* VSS header. */ + size + 1 /* Entries. */;
        asyw->xlut.i.output_mode = NVC57E_SET_ILUT_CONTROL_INTERPOLATE_DISABLE;
        asyw->xlut.i.load = wndwc57e_ilut_load;

-       return true;

 }
 
 /****************************************************************
 }
 
 /****************************************************************