On Fri, Jun 07, 2019 at 07:26:10PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
