[PATCH 1/4] drm/fb-helper: Do not assume drm_mode_create_from_cmdline_mode() can't fail

[PATCH 1/4] drm/fb-helper: Do not assume drm_mode_create_from_cmdline_mode() can't fail

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm_mode_create_from_cmdline_mode() can return NULL, so the caller
should check for that.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index b9b7c06cbc4f..bdfa14cd7f6d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2205,7 +2205,9 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f
 create_mode:
 	mode = drm_mode_create_from_cmdline_mode(fb_helper_conn->connector->dev,
 						 cmdline_mode);
-	list_add(&mode->head, &fb_helper_conn->connector->modes);
+	if (mode)
+		list_add(&mode->head, &fb_helper_conn->connector->modes);
+
 	return mode;
 }
 EXPORT_SYMBOL(drm_pick_cmdline_mode);
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm: Refuse to create zero width/height cmdline modes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If the user specifies zero width/height cmdline mode i915 will
blow up as the fbdev path will bypass the regular fb sanity
check that would otherwise have refused to create a framebuffer
with zero width/height.

The reason I thought to try this is so that I can force a specific
depth for fbdev without actually having to hardcode the mode
on the kernel cmdline. Eg. if I pass video=0x0-8 I will get an
8bpp framebuffer at my monitor's native resolution.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 5a07a28fec6d..b36248a5d826 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1593,6 +1593,9 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
 {
 	struct drm_display_mode *mode;
 
+	if (cmd->xres == 0 || cmd->yres == 0)
+		return NULL;
+
 	if (cmd->cvt)
 		mode = drm_cvt_mode(dev,
 				    cmd->xres, cmd->yres,
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm: Refuse to create zero width/height cmdline modes

[Daniel Vetter]

On Fri, Jun 07, 2019 at 07:26:09PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If the user specifies zero width/height cmdline mode i915 will
> blow up as the fbdev path will bypass the regular fb sanity
> check that would otherwise have refused to create a framebuffer
> with zero width/height.
> 
> The reason I thought to try this is so that I can force a specific
> depth for fbdev without actually having to hardcode the mode
> on the kernel cmdline. Eg. if I pass video=0x0-8 I will get an
> 8bpp framebuffer at my monitor's native resolution.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Makes sense. Maybe we should fix the fbdev helper code and stop it from
bypassing normal drm_fb sanity checks too, but that's another story.
Probably would mean rebasing somehow into Noralf's generic fbdev stuff,
which doesn't use magic direct access to the driver anymore.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_modes.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 5a07a28fec6d..b36248a5d826 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1593,6 +1593,9 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
>  {
>  	struct drm_display_mode *mode;
>  
> +	if (cmd->xres == 0 || cmd->yres == 0)
> +		return NULL;
> +
>  	if (cmd->cvt)
>  		mode = drm_cvt_mode(dev,
>  				    cmd->xres, cmd->yres,
> -- 
> 2.21.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/fb-helper: Set up gamma_lut during restore_fbdev_mode_atomic()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

i915 will now refuse to enable a C8 plane unless the gamma_lut
is already enabled (to avoid scanning out whatever garbage got
left in the hw LUT previously). So in order to make the fbdev
code work with C8 we need to program the gamma_lut already
during restore_fbdev_mode_atomic().

To avoid having to update the hw LUT every time
restore_fbdev_mode_atomic() is called we hang on to the
current gamma_lut blob. Note that the first crtc to get
enabled will dictate the size of the gamma_lut, so this
approach isn't 100% great for hardware with non-uniform
crtcs. But that problem already exists in setcmap_atomic()
so we'll just keep ignoring it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 165 ++++++++++++++++++++------------
 include/drm/drm_fb_helper.h     |   7 ++
 2 files changed, 113 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index bdfa14cd7f6d..0ecec763789f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -436,10 +436,76 @@ static bool drm_fb_helper_panel_rotation(struct drm_mode_set *modeset,
 	return true;
 }
 
+static int setcmap_crtc_atomic(struct drm_atomic_state *state,
+			       struct drm_crtc *crtc,
+			       struct drm_property_blob *gamma_lut)
+{
+	struct drm_crtc_state *crtc_state;
+	bool replaced;
+
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
+					      NULL);
+	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
+	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
+					      gamma_lut);
+	crtc_state->color_mgmt_changed |= replaced;
+
+	return 0;
+}
+
+static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
+						       struct fb_cmap *cmap)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_property_blob *gamma_lut;
+	struct drm_color_lut *lut;
+	int size = crtc->gamma_size;
+	int i;
+
+	if (!size || cmap->start + cmap->len > size)
+		return ERR_PTR(-EINVAL);
+
+	gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
+	if (IS_ERR(gamma_lut))
+		return gamma_lut;
+
+	lut = gamma_lut->data;
+	if (cmap->start || cmap->len != size) {
+		u16 *r = crtc->gamma_store;
+		u16 *g = r + crtc->gamma_size;
+		u16 *b = g + crtc->gamma_size;
+
+		for (i = 0; i < cmap->start; i++) {
+			lut[i].red = r[i];
+			lut[i].green = g[i];
+			lut[i].blue = b[i];
+		}
+		for (i = cmap->start + cmap->len; i < size; i++) {
+			lut[i].red = r[i];
+			lut[i].green = g[i];
+			lut[i].blue = b[i];
+		}
+	}
+
+	for (i = 0; i < cmap->len; i++) {
+		lut[cmap->start + i].red = cmap->red[i];
+		lut[cmap->start + i].green = cmap->green[i];
+		lut[cmap->start + i].blue = cmap->blue[i];
+	}
+
+	return gamma_lut;
+}
+
 static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
 {
+	struct fb_info *info = fb_helper->fbdev;
 	struct drm_client_dev *client = &fb_helper->client;
 	struct drm_device *dev = fb_helper->dev;
+	struct drm_property_blob *gamma_lut;
 	struct drm_plane_state *plane_state;
 	struct drm_plane *plane;
 	struct drm_atomic_state *state;
@@ -455,6 +521,10 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
 		goto out_ctx;
 	}
 
+	gamma_lut = fb_helper->gamma_lut;
+	if (gamma_lut)
+		drm_property_blob_get(gamma_lut);
+
 	state->acquire_ctx = &ctx;
 retry:
 	drm_for_each_plane(plane, dev) {
@@ -476,7 +546,8 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
 	}
 
 	drm_client_for_each_modeset(mode_set, client) {
-		struct drm_plane *primary = mode_set->crtc->primary;
+		struct drm_crtc *crtc = mode_set->crtc;
+		struct drm_plane *primary = crtc->primary;
 		unsigned int rotation;
 
 		if (drm_fb_helper_panel_rotation(mode_set, &rotation)) {
@@ -489,24 +560,46 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
 		if (ret != 0)
 			goto out_state;
 
+		if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
+			if (!gamma_lut)
+				gamma_lut = setcmap_new_gamma_lut(crtc, &info->cmap);
+			if (IS_ERR(gamma_lut)) {
+				ret = PTR_ERR(gamma_lut);
+				gamma_lut = NULL;
+				goto out_state;
+			}
+
+			ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
+			if (ret)
+				goto out_state;
+		}
+
 		/*
 		 * __drm_atomic_helper_set_config() sets active when a
 		 * mode is set, unconditionally clear it if we force DPMS off
 		 */
 		if (!active) {
-			struct drm_crtc *crtc = mode_set->crtc;
-			struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+			struct drm_crtc_state *crtc_state =
+				drm_atomic_get_new_crtc_state(state, crtc);
 
 			crtc_state->active = false;
 		}
 	}
 
 	ret = drm_atomic_commit(state);
+	if (ret)
+		goto out_state;
+
+	if (gamma_lut)
+		drm_property_blob_get(gamma_lut);
+	drm_property_blob_put(fb_helper->gamma_lut);
+	fb_helper->gamma_lut = gamma_lut;
 
 out_state:
 	if (ret == -EDEADLK)
 		goto backoff;
 
+	drm_property_blob_put(gamma_lut);
 	drm_atomic_state_put(state);
 out_ctx:
 	drm_modeset_drop_locks(&ctx);
@@ -996,6 +1089,9 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	}
 	fb_helper->fbdev = NULL;
 
+	drm_property_blob_put(fb_helper->gamma_lut);
+	fb_helper->gamma_lut = NULL;
+
 	mutex_lock(&kernel_fb_helper_lock);
 	if (!list_empty(&fb_helper->kernel_fb_list)) {
 		list_del(&fb_helper->kernel_fb_list);
@@ -1390,61 +1486,16 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
 	return ret;
 }
 
-static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
-						       struct fb_cmap *cmap)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_property_blob *gamma_lut;
-	struct drm_color_lut *lut;
-	int size = crtc->gamma_size;
-	int i;
-
-	if (!size || cmap->start + cmap->len > size)
-		return ERR_PTR(-EINVAL);
-
-	gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
-	if (IS_ERR(gamma_lut))
-		return gamma_lut;
-
-	lut = gamma_lut->data;
-	if (cmap->start || cmap->len != size) {
-		u16 *r = crtc->gamma_store;
-		u16 *g = r + crtc->gamma_size;
-		u16 *b = g + crtc->gamma_size;
-
-		for (i = 0; i < cmap->start; i++) {
-			lut[i].red = r[i];
-			lut[i].green = g[i];
-			lut[i].blue = b[i];
-		}
-		for (i = cmap->start + cmap->len; i < size; i++) {
-			lut[i].red = r[i];
-			lut[i].green = g[i];
-			lut[i].blue = b[i];
-		}
-	}
-
-	for (i = 0; i < cmap->len; i++) {
-		lut[cmap->start + i].red = cmap->red[i];
-		lut[cmap->start + i].green = cmap->green[i];
-		lut[cmap->start + i].blue = cmap->blue[i];
-	}
-
-	return gamma_lut;
-}
-
 static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_property_blob *gamma_lut = NULL;
 	struct drm_modeset_acquire_ctx ctx;
-	struct drm_crtc_state *crtc_state;
 	struct drm_atomic_state *state;
 	struct drm_mode_set *modeset;
 	struct drm_crtc *crtc;
 	u16 *r, *g, *b;
-	bool replaced;
 	int ret = 0;
 
 	drm_modeset_acquire_init(&ctx, 0);
@@ -1468,18 +1519,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
 			goto out_state;
 		}
 
-		crtc_state = drm_atomic_get_crtc_state(state, crtc);
-		if (IS_ERR(crtc_state)) {
-			ret = PTR_ERR(crtc_state);
+		ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
+		if (ret)
 			goto out_state;
-		}
-
-		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
-						      NULL);
-		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
-		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
-						      gamma_lut);
-		crtc_state->color_mgmt_changed |= replaced;
 	}
 
 	ret = drm_atomic_commit(state);
@@ -1498,6 +1540,11 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
 		memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
 	}
 
+	if (gamma_lut)
+		drm_property_blob_get(gamma_lut);
+	drm_property_blob_put(fb_helper->gamma_lut);
+	fb_helper->gamma_lut = gamma_lut;
+
 out_state:
 	if (ret == -EDEADLK)
 		goto backoff;
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 6b334f4d8a22..9c797346ede4 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -155,6 +155,13 @@ struct drm_fb_helper {
 	struct work_struct dirty_work;
 	struct work_struct resume_work;
 
+	/**
+	 * @gamma_lut:
+	 *
+	 * Current gamma_lut.
+	 */
+	struct drm_property_blob *gamma_lut;
+
 	/**
 	 * @lock:
 	 *
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/fb-helper: Set up gamma_lut during restore_fbdev_mode_atomic()

[Daniel Vetter]

On Fri, Jun 07, 2019 at 07:26:10PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> i915 will now refuse to enable a C8 plane unless the gamma_lut
> is already enabled (to avoid scanning out whatever garbage got
> left in the hw LUT previously). So in order to make the fbdev
> code work with C8 we need to program the gamma_lut already
> during restore_fbdev_mode_atomic().
> 
> To avoid having to update the hw LUT every time
> restore_fbdev_mode_atomic() is called we hang on to the
> current gamma_lut blob. Note that the first crtc to get
> enabled will dictate the size of the gamma_lut, so this
> approach isn't 100% great for hardware with non-uniform
> crtcs. But that problem already exists in setcmap_atomic()
> so we'll just keep ignoring it.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 165 ++++++++++++++++++++------------
>  include/drm/drm_fb_helper.h     |   7 ++
>  2 files changed, 113 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index bdfa14cd7f6d..0ecec763789f 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -436,10 +436,76 @@ static bool drm_fb_helper_panel_rotation(struct drm_mode_set *modeset,
>  	return true;
>  }
>  
> +static int setcmap_crtc_atomic(struct drm_atomic_state *state,
> +			       struct drm_crtc *crtc,
> +			       struct drm_property_blob *gamma_lut)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	bool replaced;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> +					      NULL);
> +	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> +	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> +					      gamma_lut);
> +	crtc_state->color_mgmt_changed |= replaced;
> +
> +	return 0;
> +}
> +
> +static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> +						       struct fb_cmap *cmap)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_property_blob *gamma_lut;
> +	struct drm_color_lut *lut;
> +	int size = crtc->gamma_size;
> +	int i;
> +
> +	if (!size || cmap->start + cmap->len > size)
> +		return ERR_PTR(-EINVAL);
> +
> +	gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> +	if (IS_ERR(gamma_lut))
> +		return gamma_lut;
> +
> +	lut = gamma_lut->data;
> +	if (cmap->start || cmap->len != size) {
> +		u16 *r = crtc->gamma_store;
> +		u16 *g = r + crtc->gamma_size;
> +		u16 *b = g + crtc->gamma_size;
> +
> +		for (i = 0; i < cmap->start; i++) {
> +			lut[i].red = r[i];
> +			lut[i].green = g[i];
> +			lut[i].blue = b[i];
> +		}
> +		for (i = cmap->start + cmap->len; i < size; i++) {
> +			lut[i].red = r[i];
> +			lut[i].green = g[i];
> +			lut[i].blue = b[i];
> +		}
> +	}
> +
> +	for (i = 0; i < cmap->len; i++) {
> +		lut[cmap->start + i].red = cmap->red[i];
> +		lut[cmap->start + i].green = cmap->green[i];
> +		lut[cmap->start + i].blue = cmap->blue[i];
> +	}
> +
> +	return gamma_lut;
> +}
> +
>  static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
>  {
> +	struct fb_info *info = fb_helper->fbdev;
>  	struct drm_client_dev *client = &fb_helper->client;
>  	struct drm_device *dev = fb_helper->dev;
> +	struct drm_property_blob *gamma_lut;
>  	struct drm_plane_state *plane_state;
>  	struct drm_plane *plane;
>  	struct drm_atomic_state *state;
> @@ -455,6 +521,10 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>  		goto out_ctx;
>  	}
>  
> +	gamma_lut = fb_helper->gamma_lut;
> +	if (gamma_lut)
> +		drm_property_blob_get(gamma_lut);

Why the get/put stuff here?

> +
>  	state->acquire_ctx = &ctx;
>  retry:
>  	drm_for_each_plane(plane, dev) {
> @@ -476,7 +546,8 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>  	}
>  
>  	drm_client_for_each_modeset(mode_set, client) {
> -		struct drm_plane *primary = mode_set->crtc->primary;
> +		struct drm_crtc *crtc = mode_set->crtc;
> +		struct drm_plane *primary = crtc->primary;
>  		unsigned int rotation;
>  
>  		if (drm_fb_helper_panel_rotation(mode_set, &rotation)) {
> @@ -489,24 +560,46 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>  		if (ret != 0)
>  			goto out_state;
>  
> +		if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
> +			if (!gamma_lut)
> +				gamma_lut = setcmap_new_gamma_lut(crtc, &info->cmap);
> +			if (IS_ERR(gamma_lut)) {
> +				ret = PTR_ERR(gamma_lut);
> +				gamma_lut = NULL;
> +				goto out_state;
> +			}
> +
> +			ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> +			if (ret)
> +				goto out_state;
> +		}
> +
>  		/*
>  		 * __drm_atomic_helper_set_config() sets active when a
>  		 * mode is set, unconditionally clear it if we force DPMS off
>  		 */
>  		if (!active) {
> -			struct drm_crtc *crtc = mode_set->crtc;
> -			struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +			struct drm_crtc_state *crtc_state =
> +				drm_atomic_get_new_crtc_state(state, crtc);
>  
>  			crtc_state->active = false;
>  		}
>  	}
>  
>  	ret = drm_atomic_commit(state);
> +	if (ret)
> +		goto out_state;
> +
> +	if (gamma_lut)
> +		drm_property_blob_get(gamma_lut);
> +	drm_property_blob_put(fb_helper->gamma_lut);
> +	fb_helper->gamma_lut = gamma_lut;

Can't we simplify using drm_property_replace_blob here and below?

>  
>  out_state:
>  	if (ret == -EDEADLK)
>  		goto backoff;
>  
> +	drm_property_blob_put(gamma_lut);
>  	drm_atomic_state_put(state);
>  out_ctx:
>  	drm_modeset_drop_locks(&ctx);
> @@ -996,6 +1089,9 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>  	}
>  	fb_helper->fbdev = NULL;
>  
> +	drm_property_blob_put(fb_helper->gamma_lut);
> +	fb_helper->gamma_lut = NULL;
> +
>  	mutex_lock(&kernel_fb_helper_lock);
>  	if (!list_empty(&fb_helper->kernel_fb_list)) {
>  		list_del(&fb_helper->kernel_fb_list);
> @@ -1390,61 +1486,16 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
>  	return ret;
>  }
>  
> -static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> -						       struct fb_cmap *cmap)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_property_blob *gamma_lut;
> -	struct drm_color_lut *lut;
> -	int size = crtc->gamma_size;
> -	int i;
> -
> -	if (!size || cmap->start + cmap->len > size)
> -		return ERR_PTR(-EINVAL);
> -
> -	gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> -	if (IS_ERR(gamma_lut))
> -		return gamma_lut;
> -
> -	lut = gamma_lut->data;
> -	if (cmap->start || cmap->len != size) {
> -		u16 *r = crtc->gamma_store;
> -		u16 *g = r + crtc->gamma_size;
> -		u16 *b = g + crtc->gamma_size;
> -
> -		for (i = 0; i < cmap->start; i++) {
> -			lut[i].red = r[i];
> -			lut[i].green = g[i];
> -			lut[i].blue = b[i];
> -		}
> -		for (i = cmap->start + cmap->len; i < size; i++) {
> -			lut[i].red = r[i];
> -			lut[i].green = g[i];
> -			lut[i].blue = b[i];
> -		}
> -	}
> -
> -	for (i = 0; i < cmap->len; i++) {
> -		lut[cmap->start + i].red = cmap->red[i];
> -		lut[cmap->start + i].green = cmap->green[i];
> -		lut[cmap->start + i].blue = cmap->blue[i];
> -	}
> -
> -	return gamma_lut;
> -}
> -
>  static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
>  	struct drm_device *dev = fb_helper->dev;
>  	struct drm_property_blob *gamma_lut = NULL;
>  	struct drm_modeset_acquire_ctx ctx;
> -	struct drm_crtc_state *crtc_state;
>  	struct drm_atomic_state *state;
>  	struct drm_mode_set *modeset;
>  	struct drm_crtc *crtc;
>  	u16 *r, *g, *b;
> -	bool replaced;
>  	int ret = 0;
>  
>  	drm_modeset_acquire_init(&ctx, 0);
> @@ -1468,18 +1519,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>  			goto out_state;
>  		}
>  
> -		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -		if (IS_ERR(crtc_state)) {
> -			ret = PTR_ERR(crtc_state);
> +		ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> +		if (ret)
>  			goto out_state;
> -		}
> -
> -		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> -						      NULL);
> -		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> -		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> -						      gamma_lut);
> -		crtc_state->color_mgmt_changed |= replaced;
>  	}
>  
>  	ret = drm_atomic_commit(state);
> @@ -1498,6 +1540,11 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>  		memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
>  	}

The above "update legacy color table" stuff is missing from the
restore_fbdev_mode_atomic version. Unify more, i.e. maybe convert
setcmap_atomic to just update the property in fb_helper->gamma_lut, and
then call into restore_fbdev_mode_atomic to do all of the heavy lifting?

Or maybe we should push this into
drm_atomic_helper_update_legacy_modeset_state(), but that doesn't quite
work because locking rules are silly :-/ Or teach the gamma_get ioctl to
look at atomic and ditch this, dunno.
-Daniel

>  
> +	if (gamma_lut)
> +		drm_property_blob_get(gamma_lut);
> +	drm_property_blob_put(fb_helper->gamma_lut);
> +	fb_helper->gamma_lut = gamma_lut;
> +
>  out_state:
>  	if (ret == -EDEADLK)
>  		goto backoff;
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 6b334f4d8a22..9c797346ede4 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -155,6 +155,13 @@ struct drm_fb_helper {
>  	struct work_struct dirty_work;
>  	struct work_struct resume_work;
>  
> +	/**
> +	 * @gamma_lut:
> +	 *
> +	 * Current gamma_lut.
> +	 */
> +	struct drm_property_blob *gamma_lut;
> +
>  	/**
>  	 * @lock:
>  	 *
> -- 
> 2.21.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/4] drm/fb-helper: Set up gamma_lut during restore_fbdev_mode_atomic()

[Ville Syrjälä]

On Fri, Jun 07, 2019 at 08:40:15PM +0200, Daniel Vetter wrote:
> On Fri, Jun 07, 2019 at 07:26:10PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > i915 will now refuse to enable a C8 plane unless the gamma_lut
> > is already enabled (to avoid scanning out whatever garbage got
> > left in the hw LUT previously). So in order to make the fbdev
> > code work with C8 we need to program the gamma_lut already
> > during restore_fbdev_mode_atomic().
> > 
> > To avoid having to update the hw LUT every time
> > restore_fbdev_mode_atomic() is called we hang on to the
> > current gamma_lut blob. Note that the first crtc to get
> > enabled will dictate the size of the gamma_lut, so this
> > approach isn't 100% great for hardware with non-uniform
> > crtcs. But that problem already exists in setcmap_atomic()
> > so we'll just keep ignoring it.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 165 ++++++++++++++++++++------------
> >  include/drm/drm_fb_helper.h     |   7 ++
> >  2 files changed, 113 insertions(+), 59 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index bdfa14cd7f6d..0ecec763789f 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -436,10 +436,76 @@ static bool drm_fb_helper_panel_rotation(struct drm_mode_set *modeset,
> >  	return true;
> >  }
> >  
> > +static int setcmap_crtc_atomic(struct drm_atomic_state *state,
> > +			       struct drm_crtc *crtc,
> > +			       struct drm_property_blob *gamma_lut)
> > +{
> > +	struct drm_crtc_state *crtc_state;
> > +	bool replaced;
> > +
> > +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > +	if (IS_ERR(crtc_state))
> > +		return PTR_ERR(crtc_state);
> > +
> > +	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > +					      NULL);
> > +	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> > +	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > +					      gamma_lut);
> > +	crtc_state->color_mgmt_changed |= replaced;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> > +						       struct fb_cmap *cmap)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_property_blob *gamma_lut;
> > +	struct drm_color_lut *lut;
> > +	int size = crtc->gamma_size;
> > +	int i;
> > +
> > +	if (!size || cmap->start + cmap->len > size)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> > +	if (IS_ERR(gamma_lut))
> > +		return gamma_lut;
> > +
> > +	lut = gamma_lut->data;
> > +	if (cmap->start || cmap->len != size) {
> > +		u16 *r = crtc->gamma_store;
> > +		u16 *g = r + crtc->gamma_size;
> > +		u16 *b = g + crtc->gamma_size;
> > +
> > +		for (i = 0; i < cmap->start; i++) {
> > +			lut[i].red = r[i];
> > +			lut[i].green = g[i];
> > +			lut[i].blue = b[i];
> > +		}
> > +		for (i = cmap->start + cmap->len; i < size; i++) {
> > +			lut[i].red = r[i];
> > +			lut[i].green = g[i];
> > +			lut[i].blue = b[i];
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < cmap->len; i++) {
> > +		lut[cmap->start + i].red = cmap->red[i];
> > +		lut[cmap->start + i].green = cmap->green[i];
> > +		lut[cmap->start + i].blue = cmap->blue[i];
> > +	}
> > +
> > +	return gamma_lut;
> > +}
> > +
> >  static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
> >  {
> > +	struct fb_info *info = fb_helper->fbdev;
> >  	struct drm_client_dev *client = &fb_helper->client;
> >  	struct drm_device *dev = fb_helper->dev;
> > +	struct drm_property_blob *gamma_lut;
> >  	struct drm_plane_state *plane_state;
> >  	struct drm_plane *plane;
> >  	struct drm_atomic_state *state;
> > @@ -455,6 +521,10 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> >  		goto out_ctx;
> >  	}
> >  
> > +	gamma_lut = fb_helper->gamma_lut;
> > +	if (gamma_lut)
> > +		drm_property_blob_get(gamma_lut);
> 
> Why the get/put stuff here?

So we don't free this guy during the cleanup.

> 
> > +
> >  	state->acquire_ctx = &ctx;
> >  retry:
> >  	drm_for_each_plane(plane, dev) {
> > @@ -476,7 +546,8 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> >  	}
> >  
> >  	drm_client_for_each_modeset(mode_set, client) {
> > -		struct drm_plane *primary = mode_set->crtc->primary;
> > +		struct drm_crtc *crtc = mode_set->crtc;
> > +		struct drm_plane *primary = crtc->primary;
> >  		unsigned int rotation;
> >  
> >  		if (drm_fb_helper_panel_rotation(mode_set, &rotation)) {
> > @@ -489,24 +560,46 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> >  		if (ret != 0)
> >  			goto out_state;
> >  
> > +		if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
> > +			if (!gamma_lut)
> > +				gamma_lut = setcmap_new_gamma_lut(crtc, &info->cmap);
> > +			if (IS_ERR(gamma_lut)) {
> > +				ret = PTR_ERR(gamma_lut);
> > +				gamma_lut = NULL;
> > +				goto out_state;
> > +			}
> > +
> > +			ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> > +			if (ret)
> > +				goto out_state;
> > +		}
> > +
> >  		/*
> >  		 * __drm_atomic_helper_set_config() sets active when a
> >  		 * mode is set, unconditionally clear it if we force DPMS off
> >  		 */
> >  		if (!active) {
> > -			struct drm_crtc *crtc = mode_set->crtc;
> > -			struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > +			struct drm_crtc_state *crtc_state =
> > +				drm_atomic_get_new_crtc_state(state, crtc);
> >  
> >  			crtc_state->active = false;
> >  		}
> >  	}
> >  
> >  	ret = drm_atomic_commit(state);
> > +	if (ret)
> > +		goto out_state;
> > +
> > +	if (gamma_lut)
> > +		drm_property_blob_get(gamma_lut);
> > +	drm_property_blob_put(fb_helper->gamma_lut);
> > +	fb_helper->gamma_lut = gamma_lut;
> 
> Can't we simplify using drm_property_replace_blob here and below?

I guess.

> 
> >  
> >  out_state:
> >  	if (ret == -EDEADLK)
> >  		goto backoff;
> >  
> > +	drm_property_blob_put(gamma_lut);
> >  	drm_atomic_state_put(state);
> >  out_ctx:
> >  	drm_modeset_drop_locks(&ctx);
> > @@ -996,6 +1089,9 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> >  	}
> >  	fb_helper->fbdev = NULL;
> >  
> > +	drm_property_blob_put(fb_helper->gamma_lut);
> > +	fb_helper->gamma_lut = NULL;
> > +
> >  	mutex_lock(&kernel_fb_helper_lock);
> >  	if (!list_empty(&fb_helper->kernel_fb_list)) {
> >  		list_del(&fb_helper->kernel_fb_list);
> > @@ -1390,61 +1486,16 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
> >  	return ret;
> >  }
> >  
> > -static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> > -						       struct fb_cmap *cmap)
> > -{
> > -	struct drm_device *dev = crtc->dev;
> > -	struct drm_property_blob *gamma_lut;
> > -	struct drm_color_lut *lut;
> > -	int size = crtc->gamma_size;
> > -	int i;
> > -
> > -	if (!size || cmap->start + cmap->len > size)
> > -		return ERR_PTR(-EINVAL);
> > -
> > -	gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> > -	if (IS_ERR(gamma_lut))
> > -		return gamma_lut;
> > -
> > -	lut = gamma_lut->data;
> > -	if (cmap->start || cmap->len != size) {
> > -		u16 *r = crtc->gamma_store;
> > -		u16 *g = r + crtc->gamma_size;
> > -		u16 *b = g + crtc->gamma_size;
> > -
> > -		for (i = 0; i < cmap->start; i++) {
> > -			lut[i].red = r[i];
> > -			lut[i].green = g[i];
> > -			lut[i].blue = b[i];
> > -		}
> > -		for (i = cmap->start + cmap->len; i < size; i++) {
> > -			lut[i].red = r[i];
> > -			lut[i].green = g[i];
> > -			lut[i].blue = b[i];
> > -		}
> > -	}
> > -
> > -	for (i = 0; i < cmap->len; i++) {
> > -		lut[cmap->start + i].red = cmap->red[i];
> > -		lut[cmap->start + i].green = cmap->green[i];
> > -		lut[cmap->start + i].blue = cmap->blue[i];
> > -	}
> > -
> > -	return gamma_lut;
> > -}
> > -
> >  static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> >  {
> >  	struct drm_fb_helper *fb_helper = info->par;
> >  	struct drm_device *dev = fb_helper->dev;
> >  	struct drm_property_blob *gamma_lut = NULL;
> >  	struct drm_modeset_acquire_ctx ctx;
> > -	struct drm_crtc_state *crtc_state;
> >  	struct drm_atomic_state *state;
> >  	struct drm_mode_set *modeset;
> >  	struct drm_crtc *crtc;
> >  	u16 *r, *g, *b;
> > -	bool replaced;
> >  	int ret = 0;
> >  
> >  	drm_modeset_acquire_init(&ctx, 0);
> > @@ -1468,18 +1519,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> >  			goto out_state;
> >  		}
> >  
> > -		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > -		if (IS_ERR(crtc_state)) {
> > -			ret = PTR_ERR(crtc_state);
> > +		ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> > +		if (ret)
> >  			goto out_state;
> > -		}
> > -
> > -		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > -						      NULL);
> > -		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> > -		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > -						      gamma_lut);
> > -		crtc_state->color_mgmt_changed |= replaced;
> >  	}
> >  
> >  	ret = drm_atomic_commit(state);
> > @@ -1498,6 +1540,11 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> >  		memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
> >  	}
> 
> The above "update legacy color table" stuff is missing from the
> restore_fbdev_mode_atomic version.

If the cmap didn't change in the meantime there should be nothing to
update, maybe. Though I suppose if someone did a gamma_set in the
meantime we might be out of sync a bit again.

I was actually thinking of nuking this legacy gamma_store stuff
entirely for atomic drivers. It's already out of sync with the
GAMMA_LUT prop anyway so seems mostly dead weight.

> Unify more, i.e. maybe convert
> setcmap_atomic to just update the property in fb_helper->gamma_lut, and
> then call into restore_fbdev_mode_atomic to do all of the heavy lifting?

Hmm. Yeah, that seems like it could work.

> 
> Or maybe we should push this into
> drm_atomic_helper_update_legacy_modeset_state(), but that doesn't quite
> work because locking rules are silly :-/ Or teach the gamma_get ioctl to
> look at atomic and ditch this, dunno.

Yeah, that was my thinking for getting rid of gamma_store. It would
also fix the out of sync issue between get_gamma and GAMMA_LUT.
I suppose I should just do it instead of procrastinating.

There are a few open questions though: 
- what if the current LUT is the wrong size?
- what if there is no LUT?

I see two options:
1) Lie and always return something with crtc->gamma_size
   entries, ie. just duplicate/drop entries for the wrong size,
   and just return a linear LUT when there is no LUT actually
   loaded
2) Expose the truth and figure out if existing userspace
   could actually handle it. Quick scan shows that most users
   would just ignore the existing LUT in this case.

Option 1 seems safer.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/fb-helper: Set up gamma_lut during restore_fbdev_mode_atomic()

[Daniel Vetter]

On Tue, Jun 11, 2019 at 7:50 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Jun 07, 2019 at 08:40:15PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 07, 2019 at 07:26:10PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > i915 will now refuse to enable a C8 plane unless the gamma_lut
> > > is already enabled (to avoid scanning out whatever garbage got
> > > left in the hw LUT previously). So in order to make the fbdev
> > > code work with C8 we need to program the gamma_lut already
> > > during restore_fbdev_mode_atomic().
> > >
> > > To avoid having to update the hw LUT every time
> > > restore_fbdev_mode_atomic() is called we hang on to the
> > > current gamma_lut blob. Note that the first crtc to get
> > > enabled will dictate the size of the gamma_lut, so this
> > > approach isn't 100% great for hardware with non-uniform
> > > crtcs. But that problem already exists in setcmap_atomic()
> > > so we'll just keep ignoring it.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_fb_helper.c | 165 ++++++++++++++++++++------------
> > >  include/drm/drm_fb_helper.h     |   7 ++
> > >  2 files changed, 113 insertions(+), 59 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index bdfa14cd7f6d..0ecec763789f 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -436,10 +436,76 @@ static bool drm_fb_helper_panel_rotation(struct drm_mode_set *modeset,
> > >     return true;
> > >  }
> > >
> > > +static int setcmap_crtc_atomic(struct drm_atomic_state *state,
> > > +                          struct drm_crtc *crtc,
> > > +                          struct drm_property_blob *gamma_lut)
> > > +{
> > > +   struct drm_crtc_state *crtc_state;
> > > +   bool replaced;
> > > +
> > > +   crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > > +   if (IS_ERR(crtc_state))
> > > +           return PTR_ERR(crtc_state);
> > > +
> > > +   replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > > +                                         NULL);
> > > +   replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> > > +   replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > > +                                         gamma_lut);
> > > +   crtc_state->color_mgmt_changed |= replaced;
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> > > +                                                  struct fb_cmap *cmap)
> > > +{
> > > +   struct drm_device *dev = crtc->dev;
> > > +   struct drm_property_blob *gamma_lut;
> > > +   struct drm_color_lut *lut;
> > > +   int size = crtc->gamma_size;
> > > +   int i;
> > > +
> > > +   if (!size || cmap->start + cmap->len > size)
> > > +           return ERR_PTR(-EINVAL);
> > > +
> > > +   gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> > > +   if (IS_ERR(gamma_lut))
> > > +           return gamma_lut;
> > > +
> > > +   lut = gamma_lut->data;
> > > +   if (cmap->start || cmap->len != size) {
> > > +           u16 *r = crtc->gamma_store;
> > > +           u16 *g = r + crtc->gamma_size;
> > > +           u16 *b = g + crtc->gamma_size;
> > > +
> > > +           for (i = 0; i < cmap->start; i++) {
> > > +                   lut[i].red = r[i];
> > > +                   lut[i].green = g[i];
> > > +                   lut[i].blue = b[i];
> > > +           }
> > > +           for (i = cmap->start + cmap->len; i < size; i++) {
> > > +                   lut[i].red = r[i];
> > > +                   lut[i].green = g[i];
> > > +                   lut[i].blue = b[i];
> > > +           }
> > > +   }
> > > +
> > > +   for (i = 0; i < cmap->len; i++) {
> > > +           lut[cmap->start + i].red = cmap->red[i];
> > > +           lut[cmap->start + i].green = cmap->green[i];
> > > +           lut[cmap->start + i].blue = cmap->blue[i];
> > > +   }
> > > +
> > > +   return gamma_lut;
> > > +}
> > > +
> > >  static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
> > >  {
> > > +   struct fb_info *info = fb_helper->fbdev;
> > >     struct drm_client_dev *client = &fb_helper->client;
> > >     struct drm_device *dev = fb_helper->dev;
> > > +   struct drm_property_blob *gamma_lut;
> > >     struct drm_plane_state *plane_state;
> > >     struct drm_plane *plane;
> > >     struct drm_atomic_state *state;
> > > @@ -455,6 +521,10 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > >             goto out_ctx;
> > >     }
> > >
> > > +   gamma_lut = fb_helper->gamma_lut;
> > > +   if (gamma_lut)
> > > +           drm_property_blob_get(gamma_lut);
> >
> > Why the get/put stuff here?
>
> So we don't free this guy during the cleanup.
>
> >
> > > +
> > >     state->acquire_ctx = &ctx;
> > >  retry:
> > >     drm_for_each_plane(plane, dev) {
> > > @@ -476,7 +546,8 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > >     }
> > >
> > >     drm_client_for_each_modeset(mode_set, client) {
> > > -           struct drm_plane *primary = mode_set->crtc->primary;
> > > +           struct drm_crtc *crtc = mode_set->crtc;
> > > +           struct drm_plane *primary = crtc->primary;
> > >             unsigned int rotation;
> > >
> > >             if (drm_fb_helper_panel_rotation(mode_set, &rotation)) {
> > > @@ -489,24 +560,46 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > >             if (ret != 0)
> > >                     goto out_state;
> > >
> > > +           if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
> > > +                   if (!gamma_lut)
> > > +                           gamma_lut = setcmap_new_gamma_lut(crtc, &info->cmap);
> > > +                   if (IS_ERR(gamma_lut)) {
> > > +                           ret = PTR_ERR(gamma_lut);
> > > +                           gamma_lut = NULL;
> > > +                           goto out_state;
> > > +                   }
> > > +
> > > +                   ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> > > +                   if (ret)
> > > +                           goto out_state;
> > > +           }
> > > +
> > >             /*
> > >              * __drm_atomic_helper_set_config() sets active when a
> > >              * mode is set, unconditionally clear it if we force DPMS off
> > >              */
> > >             if (!active) {
> > > -                   struct drm_crtc *crtc = mode_set->crtc;
> > > -                   struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > +                   struct drm_crtc_state *crtc_state =
> > > +                           drm_atomic_get_new_crtc_state(state, crtc);
> > >
> > >                     crtc_state->active = false;
> > >             }
> > >     }
> > >
> > >     ret = drm_atomic_commit(state);
> > > +   if (ret)
> > > +           goto out_state;
> > > +
> > > +   if (gamma_lut)
> > > +           drm_property_blob_get(gamma_lut);
> > > +   drm_property_blob_put(fb_helper->gamma_lut);
> > > +   fb_helper->gamma_lut = gamma_lut;
> >
> > Can't we simplify using drm_property_replace_blob here and below?
>
> I guess.
>
> >
> > >
> > >  out_state:
> > >     if (ret == -EDEADLK)
> > >             goto backoff;
> > >
> > > +   drm_property_blob_put(gamma_lut);
> > >     drm_atomic_state_put(state);
> > >  out_ctx:
> > >     drm_modeset_drop_locks(&ctx);
> > > @@ -996,6 +1089,9 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> > >     }
> > >     fb_helper->fbdev = NULL;
> > >
> > > +   drm_property_blob_put(fb_helper->gamma_lut);
> > > +   fb_helper->gamma_lut = NULL;
> > > +
> > >     mutex_lock(&kernel_fb_helper_lock);
> > >     if (!list_empty(&fb_helper->kernel_fb_list)) {
> > >             list_del(&fb_helper->kernel_fb_list);
> > > @@ -1390,61 +1486,16 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
> > >     return ret;
> > >  }
> > >
> > > -static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> > > -                                                  struct fb_cmap *cmap)
> > > -{
> > > -   struct drm_device *dev = crtc->dev;
> > > -   struct drm_property_blob *gamma_lut;
> > > -   struct drm_color_lut *lut;
> > > -   int size = crtc->gamma_size;
> > > -   int i;
> > > -
> > > -   if (!size || cmap->start + cmap->len > size)
> > > -           return ERR_PTR(-EINVAL);
> > > -
> > > -   gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> > > -   if (IS_ERR(gamma_lut))
> > > -           return gamma_lut;
> > > -
> > > -   lut = gamma_lut->data;
> > > -   if (cmap->start || cmap->len != size) {
> > > -           u16 *r = crtc->gamma_store;
> > > -           u16 *g = r + crtc->gamma_size;
> > > -           u16 *b = g + crtc->gamma_size;
> > > -
> > > -           for (i = 0; i < cmap->start; i++) {
> > > -                   lut[i].red = r[i];
> > > -                   lut[i].green = g[i];
> > > -                   lut[i].blue = b[i];
> > > -           }
> > > -           for (i = cmap->start + cmap->len; i < size; i++) {
> > > -                   lut[i].red = r[i];
> > > -                   lut[i].green = g[i];
> > > -                   lut[i].blue = b[i];
> > > -           }
> > > -   }
> > > -
> > > -   for (i = 0; i < cmap->len; i++) {
> > > -           lut[cmap->start + i].red = cmap->red[i];
> > > -           lut[cmap->start + i].green = cmap->green[i];
> > > -           lut[cmap->start + i].blue = cmap->blue[i];
> > > -   }
> > > -
> > > -   return gamma_lut;
> > > -}
> > > -
> > >  static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> > >  {
> > >     struct drm_fb_helper *fb_helper = info->par;
> > >     struct drm_device *dev = fb_helper->dev;
> > >     struct drm_property_blob *gamma_lut = NULL;
> > >     struct drm_modeset_acquire_ctx ctx;
> > > -   struct drm_crtc_state *crtc_state;
> > >     struct drm_atomic_state *state;
> > >     struct drm_mode_set *modeset;
> > >     struct drm_crtc *crtc;
> > >     u16 *r, *g, *b;
> > > -   bool replaced;
> > >     int ret = 0;
> > >
> > >     drm_modeset_acquire_init(&ctx, 0);
> > > @@ -1468,18 +1519,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> > >                     goto out_state;
> > >             }
> > >
> > > -           crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > > -           if (IS_ERR(crtc_state)) {
> > > -                   ret = PTR_ERR(crtc_state);
> > > +           ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> > > +           if (ret)
> > >                     goto out_state;
> > > -           }
> > > -
> > > -           replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > > -                                                 NULL);
> > > -           replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> > > -           replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > > -                                                 gamma_lut);
> > > -           crtc_state->color_mgmt_changed |= replaced;
> > >     }
> > >
> > >     ret = drm_atomic_commit(state);
> > > @@ -1498,6 +1540,11 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> > >             memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
> > >     }
> >
> > The above "update legacy color table" stuff is missing from the
> > restore_fbdev_mode_atomic version.
>
> If the cmap didn't change in the meantime there should be nothing to
> update, maybe. Though I suppose if someone did a gamma_set in the
> meantime we might be out of sync a bit again.
>
> I was actually thinking of nuking this legacy gamma_store stuff
> entirely for atomic drivers. It's already out of sync with the
> GAMMA_LUT prop anyway so seems mostly dead weight.
>
> > Unify more, i.e. maybe convert
> > setcmap_atomic to just update the property in fb_helper->gamma_lut, and
> > then call into restore_fbdev_mode_atomic to do all of the heavy lifting?
>
> Hmm. Yeah, that seems like it could work.
>
> >
> > Or maybe we should push this into
> > drm_atomic_helper_update_legacy_modeset_state(), but that doesn't quite
> > work because locking rules are silly :-/ Or teach the gamma_get ioctl to
> > look at atomic and ditch this, dunno.
>
> Yeah, that was my thinking for getting rid of gamma_store. It would
> also fix the out of sync issue between get_gamma and GAMMA_LUT.
> I suppose I should just do it instead of procrastinating.
>
> There are a few open questions though:
> - what if the current LUT is the wrong size?
> - what if there is no LUT?
>
> I see two options:
> 1) Lie and always return something with crtc->gamma_size
>    entries, ie. just duplicate/drop entries for the wrong size,
>    and just return a linear LUT when there is no LUT actually
>    loaded
> 2) Expose the truth and figure out if existing userspace
>    could actually handle it. Quick scan shows that most users
>    would just ignore the existing LUT in this case.

I guess in practice this would amount to

if (crtc_lut->gamma_size == crtc->gamma_size == crtc->state->gamma_blob.size)
return atomic gamma table convert to legacy formate
else
return EINVAL;

I think this could work. I think in practice the only case we need to
make sure keeps working is when userspace uses legacy gamma
exclusively. If you have a mixed world it's a mess already anyway.
-Daniel

>
> Option 1 seems safer.
>
> --
> Ville Syrjälä
> Intel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/fb-helper: Set up gamma_lut during restore_fbdev_mode_atomic()

[Ville Syrjälä]

On Tue, Jun 11, 2019 at 07:55:45PM +0200, Daniel Vetter wrote:
> On Tue, Jun 11, 2019 at 7:50 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Fri, Jun 07, 2019 at 08:40:15PM +0200, Daniel Vetter wrote:
> > > On Fri, Jun 07, 2019 at 07:26:10PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > i915 will now refuse to enable a C8 plane unless the gamma_lut
> > > > is already enabled (to avoid scanning out whatever garbage got
> > > > left in the hw LUT previously). So in order to make the fbdev
> > > > code work with C8 we need to program the gamma_lut already
> > > > during restore_fbdev_mode_atomic().
> > > >
> > > > To avoid having to update the hw LUT every time
> > > > restore_fbdev_mode_atomic() is called we hang on to the
> > > > current gamma_lut blob. Note that the first crtc to get
> > > > enabled will dictate the size of the gamma_lut, so this
> > > > approach isn't 100% great for hardware with non-uniform
> > > > crtcs. But that problem already exists in setcmap_atomic()
> > > > so we'll just keep ignoring it.
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_fb_helper.c | 165 ++++++++++++++++++++------------
> > > >  include/drm/drm_fb_helper.h     |   7 ++
> > > >  2 files changed, 113 insertions(+), 59 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > index bdfa14cd7f6d..0ecec763789f 100644
> > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > @@ -436,10 +436,76 @@ static bool drm_fb_helper_panel_rotation(struct drm_mode_set *modeset,
> > > >     return true;
> > > >  }
> > > >
> > > > +static int setcmap_crtc_atomic(struct drm_atomic_state *state,
> > > > +                          struct drm_crtc *crtc,
> > > > +                          struct drm_property_blob *gamma_lut)
> > > > +{
> > > > +   struct drm_crtc_state *crtc_state;
> > > > +   bool replaced;
> > > > +
> > > > +   crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > > > +   if (IS_ERR(crtc_state))
> > > > +           return PTR_ERR(crtc_state);
> > > > +
> > > > +   replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > > > +                                         NULL);
> > > > +   replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> > > > +   replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > > > +                                         gamma_lut);
> > > > +   crtc_state->color_mgmt_changed |= replaced;
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> > > > +                                                  struct fb_cmap *cmap)
> > > > +{
> > > > +   struct drm_device *dev = crtc->dev;
> > > > +   struct drm_property_blob *gamma_lut;
> > > > +   struct drm_color_lut *lut;
> > > > +   int size = crtc->gamma_size;
> > > > +   int i;
> > > > +
> > > > +   if (!size || cmap->start + cmap->len > size)
> > > > +           return ERR_PTR(-EINVAL);
> > > > +
> > > > +   gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> > > > +   if (IS_ERR(gamma_lut))
> > > > +           return gamma_lut;
> > > > +
> > > > +   lut = gamma_lut->data;
> > > > +   if (cmap->start || cmap->len != size) {
> > > > +           u16 *r = crtc->gamma_store;
> > > > +           u16 *g = r + crtc->gamma_size;
> > > > +           u16 *b = g + crtc->gamma_size;
> > > > +
> > > > +           for (i = 0; i < cmap->start; i++) {
> > > > +                   lut[i].red = r[i];
> > > > +                   lut[i].green = g[i];
> > > > +                   lut[i].blue = b[i];
> > > > +           }
> > > > +           for (i = cmap->start + cmap->len; i < size; i++) {
> > > > +                   lut[i].red = r[i];
> > > > +                   lut[i].green = g[i];
> > > > +                   lut[i].blue = b[i];
> > > > +           }
> > > > +   }
> > > > +
> > > > +   for (i = 0; i < cmap->len; i++) {
> > > > +           lut[cmap->start + i].red = cmap->red[i];
> > > > +           lut[cmap->start + i].green = cmap->green[i];
> > > > +           lut[cmap->start + i].blue = cmap->blue[i];
> > > > +   }
> > > > +
> > > > +   return gamma_lut;
> > > > +}
> > > > +
> > > >  static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
> > > >  {
> > > > +   struct fb_info *info = fb_helper->fbdev;
> > > >     struct drm_client_dev *client = &fb_helper->client;
> > > >     struct drm_device *dev = fb_helper->dev;
> > > > +   struct drm_property_blob *gamma_lut;
> > > >     struct drm_plane_state *plane_state;
> > > >     struct drm_plane *plane;
> > > >     struct drm_atomic_state *state;
> > > > @@ -455,6 +521,10 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > > >             goto out_ctx;
> > > >     }
> > > >
> > > > +   gamma_lut = fb_helper->gamma_lut;
> > > > +   if (gamma_lut)
> > > > +           drm_property_blob_get(gamma_lut);
> > >
> > > Why the get/put stuff here?
> >
> > So we don't free this guy during the cleanup.
> >
> > >
> > > > +
> > > >     state->acquire_ctx = &ctx;
> > > >  retry:
> > > >     drm_for_each_plane(plane, dev) {
> > > > @@ -476,7 +546,8 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > > >     }
> > > >
> > > >     drm_client_for_each_modeset(mode_set, client) {
> > > > -           struct drm_plane *primary = mode_set->crtc->primary;
> > > > +           struct drm_crtc *crtc = mode_set->crtc;
> > > > +           struct drm_plane *primary = crtc->primary;
> > > >             unsigned int rotation;
> > > >
> > > >             if (drm_fb_helper_panel_rotation(mode_set, &rotation)) {
> > > > @@ -489,24 +560,46 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > > >             if (ret != 0)
> > > >                     goto out_state;
> > > >
> > > > +           if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
> > > > +                   if (!gamma_lut)
> > > > +                           gamma_lut = setcmap_new_gamma_lut(crtc, &info->cmap);
> > > > +                   if (IS_ERR(gamma_lut)) {
> > > > +                           ret = PTR_ERR(gamma_lut);
> > > > +                           gamma_lut = NULL;
> > > > +                           goto out_state;
> > > > +                   }
> > > > +
> > > > +                   ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> > > > +                   if (ret)
> > > > +                           goto out_state;
> > > > +           }
> > > > +
> > > >             /*
> > > >              * __drm_atomic_helper_set_config() sets active when a
> > > >              * mode is set, unconditionally clear it if we force DPMS off
> > > >              */
> > > >             if (!active) {
> > > > -                   struct drm_crtc *crtc = mode_set->crtc;
> > > > -                   struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > > +                   struct drm_crtc_state *crtc_state =
> > > > +                           drm_atomic_get_new_crtc_state(state, crtc);
> > > >
> > > >                     crtc_state->active = false;
> > > >             }
> > > >     }
> > > >
> > > >     ret = drm_atomic_commit(state);
> > > > +   if (ret)
> > > > +           goto out_state;
> > > > +
> > > > +   if (gamma_lut)
> > > > +           drm_property_blob_get(gamma_lut);
> > > > +   drm_property_blob_put(fb_helper->gamma_lut);
> > > > +   fb_helper->gamma_lut = gamma_lut;
> > >
> > > Can't we simplify using drm_property_replace_blob here and below?
> >
> > I guess.
> >
> > >
> > > >
> > > >  out_state:
> > > >     if (ret == -EDEADLK)
> > > >             goto backoff;
> > > >
> > > > +   drm_property_blob_put(gamma_lut);
> > > >     drm_atomic_state_put(state);
> > > >  out_ctx:
> > > >     drm_modeset_drop_locks(&ctx);
> > > > @@ -996,6 +1089,9 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> > > >     }
> > > >     fb_helper->fbdev = NULL;
> > > >
> > > > +   drm_property_blob_put(fb_helper->gamma_lut);
> > > > +   fb_helper->gamma_lut = NULL;
> > > > +
> > > >     mutex_lock(&kernel_fb_helper_lock);
> > > >     if (!list_empty(&fb_helper->kernel_fb_list)) {
> > > >             list_del(&fb_helper->kernel_fb_list);
> > > > @@ -1390,61 +1486,16 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
> > > >     return ret;
> > > >  }
> > > >
> > > > -static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> > > > -                                                  struct fb_cmap *cmap)
> > > > -{
> > > > -   struct drm_device *dev = crtc->dev;
> > > > -   struct drm_property_blob *gamma_lut;
> > > > -   struct drm_color_lut *lut;
> > > > -   int size = crtc->gamma_size;
> > > > -   int i;
> > > > -
> > > > -   if (!size || cmap->start + cmap->len > size)
> > > > -           return ERR_PTR(-EINVAL);
> > > > -
> > > > -   gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> > > > -   if (IS_ERR(gamma_lut))
> > > > -           return gamma_lut;
> > > > -
> > > > -   lut = gamma_lut->data;
> > > > -   if (cmap->start || cmap->len != size) {
> > > > -           u16 *r = crtc->gamma_store;
> > > > -           u16 *g = r + crtc->gamma_size;
> > > > -           u16 *b = g + crtc->gamma_size;
> > > > -
> > > > -           for (i = 0; i < cmap->start; i++) {
> > > > -                   lut[i].red = r[i];
> > > > -                   lut[i].green = g[i];
> > > > -                   lut[i].blue = b[i];
> > > > -           }
> > > > -           for (i = cmap->start + cmap->len; i < size; i++) {
> > > > -                   lut[i].red = r[i];
> > > > -                   lut[i].green = g[i];
> > > > -                   lut[i].blue = b[i];
> > > > -           }
> > > > -   }
> > > > -
> > > > -   for (i = 0; i < cmap->len; i++) {
> > > > -           lut[cmap->start + i].red = cmap->red[i];
> > > > -           lut[cmap->start + i].green = cmap->green[i];
> > > > -           lut[cmap->start + i].blue = cmap->blue[i];
> > > > -   }
> > > > -
> > > > -   return gamma_lut;
> > > > -}
> > > > -
> > > >  static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> > > >  {
> > > >     struct drm_fb_helper *fb_helper = info->par;
> > > >     struct drm_device *dev = fb_helper->dev;
> > > >     struct drm_property_blob *gamma_lut = NULL;
> > > >     struct drm_modeset_acquire_ctx ctx;
> > > > -   struct drm_crtc_state *crtc_state;
> > > >     struct drm_atomic_state *state;
> > > >     struct drm_mode_set *modeset;
> > > >     struct drm_crtc *crtc;
> > > >     u16 *r, *g, *b;
> > > > -   bool replaced;
> > > >     int ret = 0;
> > > >
> > > >     drm_modeset_acquire_init(&ctx, 0);
> > > > @@ -1468,18 +1519,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> > > >                     goto out_state;
> > > >             }
> > > >
> > > > -           crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > > > -           if (IS_ERR(crtc_state)) {
> > > > -                   ret = PTR_ERR(crtc_state);
> > > > +           ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> > > > +           if (ret)
> > > >                     goto out_state;
> > > > -           }
> > > > -
> > > > -           replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > > > -                                                 NULL);
> > > > -           replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> > > > -           replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > > > -                                                 gamma_lut);
> > > > -           crtc_state->color_mgmt_changed |= replaced;
> > > >     }
> > > >
> > > >     ret = drm_atomic_commit(state);
> > > > @@ -1498,6 +1540,11 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> > > >             memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
> > > >     }
> > >
> > > The above "update legacy color table" stuff is missing from the
> > > restore_fbdev_mode_atomic version.
> >
> > If the cmap didn't change in the meantime there should be nothing to
> > update, maybe. Though I suppose if someone did a gamma_set in the
> > meantime we might be out of sync a bit again.
> >
> > I was actually thinking of nuking this legacy gamma_store stuff
> > entirely for atomic drivers. It's already out of sync with the
> > GAMMA_LUT prop anyway so seems mostly dead weight.
> >
> > > Unify more, i.e. maybe convert
> > > setcmap_atomic to just update the property in fb_helper->gamma_lut, and
> > > then call into restore_fbdev_mode_atomic to do all of the heavy lifting?
> >
> > Hmm. Yeah, that seems like it could work.
> >
> > >
> > > Or maybe we should push this into
> > > drm_atomic_helper_update_legacy_modeset_state(), but that doesn't quite
> > > work because locking rules are silly :-/ Or teach the gamma_get ioctl to
> > > look at atomic and ditch this, dunno.
> >
> > Yeah, that was my thinking for getting rid of gamma_store. It would
> > also fix the out of sync issue between get_gamma and GAMMA_LUT.
> > I suppose I should just do it instead of procrastinating.
> >
> > There are a few open questions though:
> > - what if the current LUT is the wrong size?
> > - what if there is no LUT?
> >
> > I see two options:
> > 1) Lie and always return something with crtc->gamma_size
> >    entries, ie. just duplicate/drop entries for the wrong size,
> >    and just return a linear LUT when there is no LUT actually
> >    loaded
> > 2) Expose the truth and figure out if existing userspace
> >    could actually handle it. Quick scan shows that most users
> >    would just ignore the existing LUT in this case.
> 
> I guess in practice this would amount to
> 
> if (crtc_lut->gamma_size == crtc->gamma_size == crtc->state->gamma_blob.size)
> return atomic gamma table convert to legacy formate
> else
> return EINVAL;
> 
> I think this could work. I think in practice the only case we need to
> make sure keeps working is when userspace uses legacy gamma
> exclusively. If you have a mixed world it's a mess already anyway.

Yeah, I think everyone would just pass the size from getcrtc and so
we'd end up rejecting anything else.

Ah yes, that is pretty much the only way to use the getgamma ioctl
since it doesn't even support the 'size=0 -> just query the actual
size without returning any other data' approach used in other places.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Throw away the BIOS fb if has the wrong depth/bpp

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Respect the user's choice of depth/bpp for the fbdev framebuffer
and throw out the fb we inherited from the BIOS if it doesn't
match.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 0d3a6fa674e6..1a935dc74d23 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -199,6 +199,17 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		drm_framebuffer_put(&intel_fb->base);
 		intel_fb = ifbdev->fb = NULL;
 	}
+	if (intel_fb &&
+	    (sizes->surface_depth != intel_fb->base.format->depth ||
+	     sizes->surface_bpp != intel_fb->base.format->cpp[0] * 8)) {
+		DRM_DEBUG_KMS("BIOS fb using wrong depth/bpp (%d/%d), we require (%d/%d),"
+			      " releasing it\n",
+			      intel_fb->base.format->depth,
+			      intel_fb->base.format->cpp[0] * 8,
+			      sizes->surface_depth, sizes->surface_bpp);
+		drm_framebuffer_put(&intel_fb->base);
+		intel_fb = ifbdev->fb = NULL;
+	}
 	if (!intel_fb || WARN_ON(!intel_fb_obj(&intel_fb->base))) {
 		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
 		ret = intelfb_alloc(helper, sizes);
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/i915: Throw away the BIOS fb if has the wrong depth/bpp

[Daniel Vetter]

On Fri, Jun 07, 2019 at 07:26:11PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Respect the user's choice of depth/bpp for the fbdev framebuffer
> and throw out the fb we inherited from the BIOS if it doesn't
> match.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I guess we're going boom right now, which is maybe a bit much? i.e.
Cc: stable@vger.kernel.org

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 0d3a6fa674e6..1a935dc74d23 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -199,6 +199,17 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  		drm_framebuffer_put(&intel_fb->base);
>  		intel_fb = ifbdev->fb = NULL;
>  	}
> +	if (intel_fb &&
> +	    (sizes->surface_depth != intel_fb->base.format->depth ||
> +	     sizes->surface_bpp != intel_fb->base.format->cpp[0] * 8)) {

Bikeshed: A little helper that does all these checks with debug output,
and just one "throw bios fb away" path would look a lot neater.
-Daniel

> +		DRM_DEBUG_KMS("BIOS fb using wrong depth/bpp (%d/%d), we require (%d/%d),"
> +			      " releasing it\n",
> +			      intel_fb->base.format->depth,
> +			      intel_fb->base.format->cpp[0] * 8,
> +			      sizes->surface_depth, sizes->surface_bpp);
> +		drm_framebuffer_put(&intel_fb->base);
> +		intel_fb = ifbdev->fb = NULL;
> +	}
>  	if (!intel_fb || WARN_ON(!intel_fb_obj(&intel_fb->base))) {
>  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
>  		ret = intelfb_alloc(helper, sizes);
> -- 
> 2.21.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Throw away the BIOS fb if has the wrong depth/bpp

[Ville Syrjälä]

On Fri, Jun 07, 2019 at 08:43:56PM +0200, Daniel Vetter wrote:
> On Fri, Jun 07, 2019 at 07:26:11PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Respect the user's choice of depth/bpp for the fbdev framebuffer
> > and throw out the fb we inherited from the BIOS if it doesn't
> > match.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I guess we're going boom right now, which is maybe a bit much? i.e.
> Cc: stable@vger.kernel.org

I think currently it just ignores whatever the user said.
I didn't see any explosions.

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 0d3a6fa674e6..1a935dc74d23 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -199,6 +199,17 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  		drm_framebuffer_put(&intel_fb->base);
> >  		intel_fb = ifbdev->fb = NULL;
> >  	}
> > +	if (intel_fb &&
> > +	    (sizes->surface_depth != intel_fb->base.format->depth ||
> > +	     sizes->surface_bpp != intel_fb->base.format->cpp[0] * 8)) {
> 
> Bikeshed: A little helper that does all these checks with debug output,
> and just one "throw bios fb away" path would look a lot neater.
> -Daniel
> 
> > +		DRM_DEBUG_KMS("BIOS fb using wrong depth/bpp (%d/%d), we require (%d/%d),"
> > +			      " releasing it\n",
> > +			      intel_fb->base.format->depth,
> > +			      intel_fb->base.format->cpp[0] * 8,
> > +			      sizes->surface_depth, sizes->surface_bpp);
> > +		drm_framebuffer_put(&intel_fb->base);
> > +		intel_fb = ifbdev->fb = NULL;
> > +	}
> >  	if (!intel_fb || WARN_ON(!intel_fb_obj(&intel_fb->base))) {
> >  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
> >  		ret = intelfb_alloc(helper, sizes);
> > -- 
> > 2.21.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/fb-helper: Do not assume drm_mode_create_from_cmdline_mode() can't fail

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/fb-helper: Do not assume drm_mode_create_from_cmdline_mode() can't fail
URL   : https://patchwork.freedesktop.org/series/61781/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
33c52564377d drm/fb-helper: Do not assume drm_mode_create_from_cmdline_mode() can't fail
a0347d8f0f17 drm: Refuse to create zero width/height cmdline modes
27c68a21f439 drm/fb-helper: Set up gamma_lut during restore_fbdev_mode_atomic()
b59d23f65edd drm/i915: Throw away the BIOS fb if has the wrong depth/bpp
-:32: CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#32: FILE: drivers/gpu/drm/i915/intel_fbdev.c:211:
+		intel_fb = ifbdev->fb = NULL;

total: 0 errors, 0 warnings, 1 checks, 17 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/4] drm/fb-helper: Do not assume drm_mode_create_from_cmdline_mode() can't fail

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/fb-helper: Do not assume drm_mode_create_from_cmdline_mode() can't fail
URL   : https://patchwork.freedesktop.org/series/61781/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6221 -> Patchwork_13208
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_13208 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_13208, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13208/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13208:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-gdg-551:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6221/fi-gdg-551/igt@i915_module_load@reload-with-fault-injection.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13208/fi-gdg-551/igt@i915_module_load@reload-with-fault-injection.html

  
Known issues
------------

  Here are the changes found in Patchwork_13208 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       [PASS][3] -> [INCOMPLETE][4] ([fdo#107718])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6221/fi-blb-e6850/igt@i915_module_load@reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13208/fi-blb-e6850/igt@i915_module_load@reload.html

  
#### Possible fixes ####

  * igt@gem_ctx_switch@basic-default:
    - {fi-icl-guc}:       [INCOMPLETE][5] ([fdo#107713] / [fdo#108569]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6221/fi-icl-guc/igt@gem_ctx_switch@basic-default.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13208/fi-icl-guc/igt@gem_ctx_switch@basic-default.html

  * {igt@i915_selftest@live_blt}:
    - fi-skl-iommu:       [INCOMPLETE][7] ([fdo#108602]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6221/fi-skl-iommu/igt@i915_selftest@live_blt.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13208/fi-skl-iommu/igt@i915_selftest@live_blt.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size:
    - fi-icl-u3:          [DMESG-WARN][9] ([fdo#107724]) -> [PASS][10] +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6221/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13208/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602


Participating hosts (54 -> 39)
------------------------------

  Missing    (15): fi-ilk-m540 fi-skl-gvtdvm fi-hsw-4200u fi-bdw-gvtdvm fi-byt-squawks fi-icl-u2 fi-bwr-2160 fi-ilk-650 fi-kbl-7500u fi-bsw-cyan fi-icl-y fi-skl-lmem fi-kbl-7560u fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6221 -> Patchwork_13208

  CI_DRM_6221: 3f71d971d89a60348dbb11c40459aeb9d69c18f9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5048: 1a34b94f1ce07ac5978fe7893a17e8732d467868 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13208: b59d23f65eddbcfa3b055896ce38184cdeededa5 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b59d23f65edd drm/i915: Throw away the BIOS fb if has the wrong depth/bpp
27c68a21f439 drm/fb-helper: Set up gamma_lut during restore_fbdev_mode_atomic()
a0347d8f0f17 drm: Refuse to create zero width/height cmdline modes
33c52564377d drm/fb-helper: Do not assume drm_mode_create_from_cmdline_mode() can't fail

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13208/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/fb-helper: Do not assume drm_mode_create_from_cmdline_mode() can't fail

[Daniel Vetter]

On Fri, Jun 07, 2019 at 07:26:08PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> drm_mode_create_from_cmdline_mode() can return NULL, so the caller
> should check for that.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Aside I noticed that we can drop a few EXPORT_SYMBOL here, I'll type a
patch.
-Daniel
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index b9b7c06cbc4f..bdfa14cd7f6d 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2205,7 +2205,9 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f
>  create_mode:
>  	mode = drm_mode_create_from_cmdline_mode(fb_helper_conn->connector->dev,
>  						 cmdline_mode);
> -	list_add(&mode->head, &fb_helper_conn->connector->modes);
> +	if (mode)
> +		list_add(&mode->head, &fb_helper_conn->connector->modes);
> +
>  	return mode;
>  }
>  EXPORT_SYMBOL(drm_pick_cmdline_mode);
> -- 
> 2.21.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx