[PATCH] drm/core: Change declaration for gamma_set.
Op 06-06-16 om 18:25 schreef Emil Velikov: > On 6 June 2016 at 17:15, Maarten Lankhorst > wrote: >> Op 06-06-16 om 18:11 schreef Emil Velikov: >>> On 6 June 2016 at 17:00, Daniel Vetter wrote: On Mon, Jun 06, 2016 at 11:18:09AM +0200, Maarten Lankhorst wrote: > Change return value to int to propagate errors from gamma_set, > and remove start parameter. Updates always use the full size, > and some drivers even ignore the start parameter altogether. Commit message should explain why we suddenly need to pass up the error code, too: "This is needed for atomic drivers, where an atomic commit can fail with EAGAIN and should be restarted." >>> Since that can happen only on the DRM core side, shouldn't one refrain >>> from adding the "always returns 0" return status for the drivers' >>> .gamma_set hook ? Or there's is something in the works that would lead >>> to drivers being able to return a non 0 value ? >> Look at drm_atomic_helper.c :-) >> >> Being implemented as an atomic update means any errno can be returned. >> > Ouch. Looks like only the i915 driver uses the helper while I was > looking at other ATOMIC drivers. So things short circuited on my > noggin'. I guess it's up-to the respective drivers to be updated and > use the drm_atomic_helper_legacy_gamma_set helper. > > Thanks for sticking with my silly question :-) Feel free to convert some drivers over, would be nice to have more widespread support for changing gamma through atomic properties. :-)
[PATCH] drm/core: Change declaration for gamma_set.
Op 06-06-16 om 18:17 schreef Emil Velikov: > Hi Maarten, > > On 6 June 2016 at 10:18, Maarten Lankhorst > wrote: > >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -1076,8 +1076,6 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, >> u16 green, >> WARN_ON(fb->bits_per_pixel != 8); >> >> fb_helper->funcs->gamma_set(crtc, red, green, blue, regno); >> - >> - return 0; >> } > This seems like unrelated change for which the compiler should give > you a nice big warning :-) > > Thanks > Emil Oops, was a leftover. Ignore this hunk. I originally added a return, but fb_helper is left unchanged for now. ~Maarten
[PATCH] drm/core: Change declaration for gamma_set.
Op 06-06-16 om 18:11 schreef Emil Velikov: > On 6 June 2016 at 17:00, Daniel Vetter wrote: >> On Mon, Jun 06, 2016 at 11:18:09AM +0200, Maarten Lankhorst wrote: >>> Change return value to int to propagate errors from gamma_set, >>> and remove start parameter. Updates always use the full size, >>> and some drivers even ignore the start parameter altogether. >> Commit message should explain why we suddenly need to pass up the error >> code, too: >> >> "This is needed for atomic drivers, where an atomic commit can fail with >> EAGAIN and should be restarted." >> > Since that can happen only on the DRM core side, shouldn't one refrain > from adding the "always returns 0" return status for the drivers' > .gamma_set hook ? Or there's is something in the works that would lead > to drivers being able to return a non 0 value ? Look at drm_atomic_helper.c :-) Being implemented as an atomic update means any errno can be returned. ~Maarten
[PATCH] drm/core: Change declaration for gamma_set.
On Mon, Jun 06, 2016 at 11:18:09AM +0200, Maarten Lankhorst wrote:
> Change return value to int to propagate errors from gamma_set,
> and remove start parameter. Updates always use the full size,
> and some drivers even ignore the start parameter altogether.
Commit message should explain why we suddenly need to pass up the error
code, too:
"This is needed for atomic drivers, where an atomic commit can fail with
EAGAIN and should be restarted."
I'll add that when merging, but will wait for a bit more acks/r-b first.
-Daniel
>
> Cc: Alex Deucher
> Cc: Christian König
> Cc: David Airlie
> Cc: Patrik Jakobsson
> Cc: Ben Skeggs
> Cc: Eric Anholt
> Cc: VMware Graphics
> Cc: Mathieu Larouche
> Cc: Thierry Reding
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 10 ++
> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 10 ++
> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 10 ++
> drivers/gpu/drm/ast/ast_mode.c | 10 ++
> drivers/gpu/drm/cirrus/cirrus_mode.c| 8 +---
> drivers/gpu/drm/drm_atomic_helper.c | 13 ++---
> drivers/gpu/drm/drm_crtc.c | 2 +-
> drivers/gpu/drm/drm_fb_helper.c | 4 +---
> drivers/gpu/drm/gma500/gma_display.c| 9 +
> drivers/gpu/drm/gma500/gma_display.h| 4 ++--
> drivers/gpu/drm/mgag200/mgag200_mode.c | 9 +
> drivers/gpu/drm/nouveau/dispnv04/crtc.c | 12 +++-
> drivers/gpu/drm/nouveau/nv50_display.c | 9 +
> drivers/gpu/drm/radeon/radeon_display.c | 11 +++
> drivers/gpu/drm/vc4/vc4_crtc.c | 8 +---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 +---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 4 ++--
> include/drm/drm_atomic_helper.h | 6 +++---
> include/drm/drm_crtc.h | 4 ++--
> 19 files changed, 85 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index 8227344d2ff6..16de862ea51f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2667,19 +2667,21 @@ static void dce_v10_0_cursor_reset(struct drm_crtc
> *crtc)
> }
> }
>
> -static void dce_v10_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
> *green,
> - u16 *blue, uint32_t start, uint32_t size)
> +static int dce_v10_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
> *green,
> + u16 *blue, uint32_t size)
> {
> struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> - int end = (start + size > 256) ? 256 : start + size, i;
> + int i;
>
> /* userspace palettes are always correct as is */
> - for (i = start; i < end; i++) {
> + for (i = 0; i < size; i++) {
> amdgpu_crtc->lut_r[i] = red[i] >> 6;
> amdgpu_crtc->lut_g[i] = green[i] >> 6;
> amdgpu_crtc->lut_b[i] = blue[i] >> 6;
> }
> dce_v10_0_crtc_load_lut(crtc);
> +
> + return 0;
> }
>
> static void dce_v10_0_crtc_destroy(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index af26ec0bc59d..6e0ae8e2e65c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2678,19 +2678,21 @@ static void dce_v11_0_cursor_reset(struct drm_crtc
> *crtc)
> }
> }
>
> -static void dce_v11_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
> *green,
> - u16 *blue, uint32_t start, uint32_t size)
> +static int dce_v11_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
> *green,
> + u16 *blue, uint32_t size)
> {
> struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> - int end = (start + size > 256) ? 256 : start + size, i;
> + int i;
>
> /* userspace palettes are always correct as is */
> - for (i = start; i < end; i++) {
> + for (i = 0; i < size; i++) {
> amdgpu_crtc->lut_r[i] = red[i] >> 6;
> amdgpu_crtc->lut_g[i] = green[i] >> 6;
> amdgpu_crtc->lut_b[i] = blue[i] >> 6;
> }
> dce_v11_0_crtc_load_lut(crtc);
> +
> + return 0;
> }
>
> static void dce_v11_0_crtc_destroy(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> index 3fb65e41a6ef..7b5fae4c74f1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -2574,19 +2574,21 @@ static void dce_v8_0_cursor_reset(struct drm_crtc
> *crtc)
> }
> }
>
> -static void dce_v8_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
> *green,
> - u16 *blue, uint32_t start, uint32_t size)
> +static int dce_v8_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
> *green,
> +
[PATCH] drm/core: Change declaration for gamma_set.
On 6 June 2016 at 17:15, Maarten Lankhorst wrote: > Op 06-06-16 om 18:11 schreef Emil Velikov: >> On 6 June 2016 at 17:00, Daniel Vetter wrote: >>> On Mon, Jun 06, 2016 at 11:18:09AM +0200, Maarten Lankhorst wrote: Change return value to int to propagate errors from gamma_set, and remove start parameter. Updates always use the full size, and some drivers even ignore the start parameter altogether. >>> Commit message should explain why we suddenly need to pass up the error >>> code, too: >>> >>> "This is needed for atomic drivers, where an atomic commit can fail with >>> EAGAIN and should be restarted." >>> >> Since that can happen only on the DRM core side, shouldn't one refrain >> from adding the "always returns 0" return status for the drivers' >> .gamma_set hook ? Or there's is something in the works that would lead >> to drivers being able to return a non 0 value ? > Look at drm_atomic_helper.c :-) > > Being implemented as an atomic update means any errno can be returned. > Ouch. Looks like only the i915 driver uses the helper while I was looking at other ATOMIC drivers. So things short circuited on my noggin'. I guess it's up-to the respective drivers to be updated and use the drm_atomic_helper_legacy_gamma_set helper. Thanks for sticking with my silly question :-) Emil
[PATCH] drm/core: Change declaration for gamma_set.
Hi Maarten, On 6 June 2016 at 10:18, Maarten Lankhorst wrote: > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1076,8 +1076,6 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, > u16 green, > WARN_ON(fb->bits_per_pixel != 8); > > fb_helper->funcs->gamma_set(crtc, red, green, blue, regno); > - > - return 0; > } This seems like unrelated change for which the compiler should give you a nice big warning :-) Thanks Emil
[PATCH] drm/core: Change declaration for gamma_set.
On 6 June 2016 at 17:00, Daniel Vetter wrote: > On Mon, Jun 06, 2016 at 11:18:09AM +0200, Maarten Lankhorst wrote: >> Change return value to int to propagate errors from gamma_set, >> and remove start parameter. Updates always use the full size, >> and some drivers even ignore the start parameter altogether. > > Commit message should explain why we suddenly need to pass up the error > code, too: > > "This is needed for atomic drivers, where an atomic commit can fail with > EAGAIN and should be restarted." > Since that can happen only on the DRM core side, shouldn't one refrain from adding the "always returns 0" return status for the drivers' .gamma_set hook ? Or there's is something in the works that would lead to drivers being able to return a non 0 value ? Thanks Emil
[PATCH] drm/core: Change declaration for gamma_set.
> -Original Message-
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, June 06, 2016 12:00 PM
> To: Maarten Lankhorst
> Cc: dri-devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org;
> Mathieu Larouche; VMware Graphics; Ben Skeggs; Deucher, Alexander;
> Thierry Reding; Koenig, Christian
> Subject: Re: [PATCH] drm/core: Change declaration for gamma_set.
>
> On Mon, Jun 06, 2016 at 11:18:09AM +0200, Maarten Lankhorst wrote:
> > Change return value to int to propagate errors from gamma_set,
> > and remove start parameter. Updates always use the full size,
> > and some drivers even ignore the start parameter altogether.
>
> Commit message should explain why we suddenly need to pass up the error
> code, too:
>
> "This is needed for atomic drivers, where an atomic commit can fail with
> EAGAIN and should be restarted."
>
> I'll add that when merging, but will wait for a bit more acks/r-b first.
Thanks. I was just about to ask why this was necessary. Patch is:
Acked-by: Alex Deucher
> -Daniel
> >
> > Cc: Alex Deucher
> > Cc: Christian König
> > Cc: David Airlie
> > Cc: Patrik Jakobsson
> > Cc: Ben Skeggs
> > Cc: Eric Anholt
> > Cc: VMware Graphics
> > Cc: Mathieu Larouche
> > Cc: Thierry Reding
> > Signed-off-by: Maarten Lankhorst
> > ---
> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 10 ++
> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 10 ++
> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 10 ++
> > drivers/gpu/drm/ast/ast_mode.c | 10 ++
> > drivers/gpu/drm/cirrus/cirrus_mode.c| 8 +---
> > drivers/gpu/drm/drm_atomic_helper.c | 13 ++---
> > drivers/gpu/drm/drm_crtc.c | 2 +-
> > drivers/gpu/drm/drm_fb_helper.c | 4 +---
> > drivers/gpu/drm/gma500/gma_display.c| 9 +
> > drivers/gpu/drm/gma500/gma_display.h| 4 ++--
> > drivers/gpu/drm/mgag200/mgag200_mode.c | 9 +
> > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 12 +++-
> > drivers/gpu/drm/nouveau/nv50_display.c | 9 +
> > drivers/gpu/drm/radeon/radeon_display.c | 11 +++
> > drivers/gpu/drm/vc4/vc4_crtc.c | 8 +---
> > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 +---
> > drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 4 ++--
> > include/drm/drm_atomic_helper.h | 6 +++---
> > include/drm/drm_crtc.h | 4 ++--
> > 19 files changed, 85 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > index 8227344d2ff6..16de862ea51f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > @@ -2667,19 +2667,21 @@ static void dce_v10_0_cursor_reset(struct
> drm_crtc *crtc)
> > }
> > }
> >
> > -static void dce_v10_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red,
> u16 *green,
> > - u16 *blue, uint32_t start, uint32_t size)
> > +static int dce_v10_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
> *green,
> > + u16 *blue, uint32_t size)
> > {
> > struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> > - int end = (start + size > 256) ? 256 : start + size, i;
> > + int i;
> >
> > /* userspace palettes are always correct as is */
> > - for (i = start; i < end; i++) {
> > + for (i = 0; i < size; i++) {
> > amdgpu_crtc->lut_r[i] = red[i] >> 6;
> > amdgpu_crtc->lut_g[i] = green[i] >> 6;
> > amdgpu_crtc->lut_b[i] = blue[i] >> 6;
> > }
> > dce_v10_0_crtc_load_lut(crtc);
> > +
> > + return 0;
> > }
> >
> > static void dce_v10_0_crtc_destroy(struct drm_crtc *crtc)
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > index af26ec0bc59d..6e0ae8e2e65c 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > @@ -2678,19 +2678,21 @@ static void dce_v11_0_cursor_reset(struct
> drm_crtc *crtc)
> > }
> > }
> >
> > -static void dce_v11_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red,
> u16 *green,
> > - u16 *blue, uint32_t start, uint32_t size)
> > +static int dce_v11_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
>
[PATCH] drm/core: Change declaration for gamma_set.
Change return value to int to propagate errors from gamma_set,
and remove start parameter. Updates always use the full size,
and some drivers even ignore the start parameter altogether.
Cc: Alex Deucher
Cc: Christian König
Cc: David Airlie
Cc: Patrik Jakobsson
Cc: Ben Skeggs
Cc: Eric Anholt
Cc: VMware Graphics
Cc: Mathieu Larouche
Cc: Thierry Reding
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 10 ++
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 10 ++
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 10 ++
drivers/gpu/drm/ast/ast_mode.c | 10 ++
drivers/gpu/drm/cirrus/cirrus_mode.c| 8 +---
drivers/gpu/drm/drm_atomic_helper.c | 13 ++---
drivers/gpu/drm/drm_crtc.c | 2 +-
drivers/gpu/drm/drm_fb_helper.c | 4 +---
drivers/gpu/drm/gma500/gma_display.c| 9 +
drivers/gpu/drm/gma500/gma_display.h| 4 ++--
drivers/gpu/drm/mgag200/mgag200_mode.c | 9 +
drivers/gpu/drm/nouveau/dispnv04/crtc.c | 12 +++-
drivers/gpu/drm/nouveau/nv50_display.c | 9 +
drivers/gpu/drm/radeon/radeon_display.c | 11 +++
drivers/gpu/drm/vc4/vc4_crtc.c | 8 +---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 +---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 4 ++--
include/drm/drm_atomic_helper.h | 6 +++---
include/drm/drm_crtc.h | 4 ++--
19 files changed, 85 insertions(+), 66 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 8227344d2ff6..16de862ea51f 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2667,19 +2667,21 @@ static void dce_v10_0_cursor_reset(struct drm_crtc
*crtc)
}
}
-static void dce_v10_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
*green,
- u16 *blue, uint32_t start, uint32_t size)
+static int dce_v10_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
*green,
+ u16 *blue, uint32_t size)
{
struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
- int end = (start + size > 256) ? 256 : start + size, i;
+ int i;
/* userspace palettes are always correct as is */
- for (i = start; i < end; i++) {
+ for (i = 0; i < size; i++) {
amdgpu_crtc->lut_r[i] = red[i] >> 6;
amdgpu_crtc->lut_g[i] = green[i] >> 6;
amdgpu_crtc->lut_b[i] = blue[i] >> 6;
}
dce_v10_0_crtc_load_lut(crtc);
+
+ return 0;
}
static void dce_v10_0_crtc_destroy(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index af26ec0bc59d..6e0ae8e2e65c 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2678,19 +2678,21 @@ static void dce_v11_0_cursor_reset(struct drm_crtc
*crtc)
}
}
-static void dce_v11_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
*green,
- u16 *blue, uint32_t start, uint32_t size)
+static int dce_v11_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
*green,
+ u16 *blue, uint32_t size)
{
struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
- int end = (start + size > 256) ? 256 : start + size, i;
+ int i;
/* userspace palettes are always correct as is */
- for (i = start; i < end; i++) {
+ for (i = 0; i < size; i++) {
amdgpu_crtc->lut_r[i] = red[i] >> 6;
amdgpu_crtc->lut_g[i] = green[i] >> 6;
amdgpu_crtc->lut_b[i] = blue[i] >> 6;
}
dce_v11_0_crtc_load_lut(crtc);
+
+ return 0;
}
static void dce_v11_0_crtc_destroy(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 3fb65e41a6ef..7b5fae4c74f1 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -2574,19 +2574,21 @@ static void dce_v8_0_cursor_reset(struct drm_crtc *crtc)
}
}
-static void dce_v8_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16
*green,
- u16 *blue, uint32_t start, uint32_t size)
+static int dce_v8_0_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
+ u16 *blue, uint32_t size)
{
struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
- int end = (start + size > 256) ? 256 : start + size, i;
+ int i;
/* userspace palettes are always correct as is */
- for (i = start; i < end; i++) {
+ for (i = 0; i < size; i++) {
amdgpu_crtc->lut_r[i] = red[i] >> 6;
amdgpu_crtc->lut_g[i] = green[i] >> 6;
amdgpu_crtc->lut_b[i] = blue[i] >> 6;
}
