| Message ID | 20231025103957.3776-1-keith.zhao@starfivetech.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | DRM driver for verisilicon | expand |
On Wed, Oct 25, 2023 at 06:39:56PM +0800, Keith Zhao wrote: > +static struct drm_crtc_state * > +vs_crtc_atomic_duplicate_state(struct drm_crtc *crtc) > +{ > + struct vs_crtc_state *ori_state; > + struct vs_crtc_state *state; > + > + if (!crtc->state) > + return NULL; > + > + ori_state = to_vs_crtc_state(crtc->state); > + state = kzalloc(sizeof(*state), GFP_KERNEL); > + if (!state) > + return NULL; > + > + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); > + > + state->output_fmt = ori_state->output_fmt; That field is never set in your patch. > + state->encoder_type = ori_state->encoder_type; That isn't either, and it's not clear why you would need the encoder_type stored in the CRTC? > + state->bpp = ori_state->bpp; You seem to derive that from output_fmt, it doesn't need to be in the CRTC state. > + state->underflow = ori_state->underflow; Assuming you're setting this from the interrupt handler, it's unsafe, you shouldn't do that. What are you using it for? > +static const struct drm_prop_enum_list vs_sync_mode_enum_list[] = { > + { VS_SINGLE_DC, "single dc mode" }, > + { VS_MULTI_DC_PRIMARY, "primary dc for multi dc mode" }, > + { VS_MULTI_DC_SECONDARY, "secondary dc for multi dc mode" }, > +}; Custom driver properties are a no-go: https://docs.kernel.org/gpu/drm-kms.html#requirements And https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements > +void vs_dc_enable(struct vs_dc *dc, struct drm_crtc *crtc) > +{ > + struct vs_crtc_state *crtc_state = to_vs_crtc_state(crtc->state); > + struct drm_display_mode *mode = &crtc->state->adjusted_mode; > + struct dc_hw_display display; Why are you rolling your own structure here, if it's exactly equivalent to what drm_display_mode and the crtc_state provide? > +void vs_dc_commit(struct vs_dc *dc) > +{ > + dc_hw_enable_shadow_register(&dc->hw, false); > + > + dc_hw_commit(&dc->hw); > + > + if (dc->first_frame) > + dc->first_frame = false; > + > + dc_hw_enable_shadow_register(&dc->hw, true); > +} It's not clear to me what you're trying to do here, does the hardware have latched registers that are only updated during vblank? > +static int dc_bind(struct device *dev, struct device *master, void *data) > +{ > + struct drm_device *drm_dev = data; > + struct vs_dc *dc = dev_get_drvdata(dev); > + struct device_node *port; > + struct vs_crtc *crtc; > + struct vs_dc_info *dc_info; > + struct vs_plane *plane; > + struct vs_plane_info *plane_info; > + int i, ret; > + u32 ctrc_mask = 0; > + > + if (!drm_dev || !dc) { > + dev_err(dev, "devices are not created.\n"); > + return -ENODEV; > + } > + > + ret = dc_init(dev); > + if (ret < 0) { > + drm_err(drm_dev, "Failed to initialize DC hardware.\n"); > + return ret; > + } > + > + port = of_get_child_by_name(dev->of_node, "port"); > + if (!port) { > + drm_err(drm_dev, "no port node found\n"); > + return -ENODEV; > + } > + of_node_put(port); > + > + dc_info = dc->hw.info; > + > + for (i = 0; i < dc_info->panel_num; i++) { > + crtc = vs_crtc_create(drm_dev, dc_info); > + if (!crtc) { > + drm_err(drm_dev, "Failed to create CRTC.\n"); > + ret = -ENOMEM; > + return ret; > + } > + > + crtc->base.port = port; > + crtc->dev = dev; > + dc->crtc[i] = crtc; > + ctrc_mask |= drm_crtc_mask(&crtc->base); > + } > + > + for (i = 0; i < dc_info->plane_num; i++) { > + plane_info = (struct vs_plane_info *)&dc_info->planes[i]; > + > + if (!strcmp(plane_info->name, "Primary") || !strcmp(plane_info->name, "Cursor")) { > + plane = vs_plane_create(drm_dev, plane_info, dc_info->layer_num, > + drm_crtc_mask(&dc->crtc[0]->base)); > + } else if (!strcmp(plane_info->name, "Primary_1") || > + !strcmp(plane_info->name, "Cursor_1")) { Please use an enum and an id there. > +static int vs_plane_atomic_set_property(struct drm_plane *plane, > + struct drm_plane_state *state, > + struct drm_property *property, > + uint64_t val) > +{ > + struct drm_device *dev = plane->dev; > + struct vs_plane *vs_plane = to_vs_plane(plane); > + struct vs_plane_state *vs_plane_state = to_vs_plane_state(state); > + int ret = 0; > + > + if (property == vs_plane->degamma_mode) { > + if (vs_plane_state->degamma != val) { > + vs_plane_state->degamma = val; > + vs_plane_state->degamma_changed = true; > + } else { > + vs_plane_state->degamma_changed = false; > + } > + } else if (property == vs_plane->watermark_prop) { > + ret = _vs_plane_set_property_blob_from_id(dev, > + &vs_plane_state->watermark, > + val, > + sizeof(struct drm_vs_watermark)); > + return ret; > + } else if (property == vs_plane->color_mgmt_prop) { > + ret = _vs_plane_set_property_blob_from_id(dev, > + &vs_plane_state->color_mgmt, > + val, > + sizeof(struct drm_vs_color_mgmt)); > + return ret; > + } else if (property == vs_plane->roi_prop) { > + ret = _vs_plane_set_property_blob_from_id(dev, > + &vs_plane_state->roi, > + val, > + sizeof(struct drm_vs_roi)); > + return ret; > + } else { > + return -EINVAL; > + } > + > + return 0; > +} Same story than above for properties Honestly, that driver is pretty massive, and you should be simplifying it a lot of you want the initial contribution to be as smooth as possible. Things like all the tiling formats, the underflowing handling, all those properties, etc can (and should) be added in a second step once the foundations are in. Maxime
On Wed, Oct 25, 2023 at 10:28:56PM +0300, Dmitry Baryshkov wrote: > On 25/10/2023 13:39, Keith Zhao wrote: > > add 2 crtcs and 8 planes in vs-drm > > > > Signed-off-by: Keith Zhao <keith.zhao@starfivetech.com> > > --- > > drivers/gpu/drm/verisilicon/Makefile | 8 +- > > drivers/gpu/drm/verisilicon/vs_crtc.c | 257 ++++ > > drivers/gpu/drm/verisilicon/vs_crtc.h | 43 + > > drivers/gpu/drm/verisilicon/vs_dc.c | 1002 ++++++++++++ > > drivers/gpu/drm/verisilicon/vs_dc.h | 80 + > > drivers/gpu/drm/verisilicon/vs_dc_hw.c | 1959 ++++++++++++++++++++++++ > > drivers/gpu/drm/verisilicon/vs_dc_hw.h | 492 ++++++ > > drivers/gpu/drm/verisilicon/vs_drv.c | 2 + > > drivers/gpu/drm/verisilicon/vs_plane.c | 526 +++++++ > > drivers/gpu/drm/verisilicon/vs_plane.h | 58 + > > drivers/gpu/drm/verisilicon/vs_type.h | 69 + > > 11 files changed, 4494 insertions(+), 2 deletions(-) > > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.c > > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.h > > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.c > > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.h > > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.c > > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.h > > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.c > > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.h > > create mode 100644 drivers/gpu/drm/verisilicon/vs_type.h > > > > diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile > > index 7d3be305b..1d48016ca 100644 > > --- a/drivers/gpu/drm/verisilicon/Makefile > > +++ b/drivers/gpu/drm/verisilicon/Makefile > > @@ -1,7 +1,11 @@ > > # SPDX-License-Identifier: GPL-2.0 > > > > -vs_drm-objs := vs_drv.o \ > > - vs_modeset.o > > +vs_drm-objs := vs_dc_hw.o \ > > + vs_dc.o \ > > + vs_crtc.o \ > > + vs_drv.o \ > > + vs_modeset.o \ > > + vs_plane.o > > > > obj-$(CONFIG_DRM_VERISILICON) += vs_drm.o > > > > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c > > new file mode 100644 > > index 000000000..8a658ea77 > > --- /dev/null > > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c > > @@ -0,0 +1,257 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd. > > + * > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/debugfs.h> > > +#include <linux/media-bus-format.h> > > + > > +#include <drm/drm_atomic_helper.h> > > +#include <drm/drm_atomic.h> > > +#include <drm/drm_crtc.h> > > +#include <drm/drm_gem_atomic_helper.h> > > +#include <drm/drm_vblank.h> > > +#include <drm/vs_drm.h> > > + > > +#include "vs_crtc.h" > > +#include "vs_dc.h" > > +#include "vs_drv.h" > > + > > +static void vs_crtc_reset(struct drm_crtc *crtc) > > +{ > > + struct vs_crtc_state *state; > > + > > + if (crtc->state) { > > + __drm_atomic_helper_crtc_destroy_state(crtc->state); > > + > > + state = to_vs_crtc_state(crtc->state); > > + kfree(state); > > + crtc->state = NULL; > > + } > > You can call your crtc_destroy_state function directly here. > > > + > > + state = kzalloc(sizeof(*state), GFP_KERNEL); > > + if (!state) > > + return; > > + > > + __drm_atomic_helper_crtc_reset(crtc, &state->base); > > +} > > + > > +static struct drm_crtc_state * > > +vs_crtc_atomic_duplicate_state(struct drm_crtc *crtc) > > +{ > > + struct vs_crtc_state *ori_state; > > It might be a matter of taste, but it is usually old_state. > > > + struct vs_crtc_state *state; > > + > > + if (!crtc->state) > > + return NULL; > > + > > + ori_state = to_vs_crtc_state(crtc->state); > > + state = kzalloc(sizeof(*state), GFP_KERNEL); > > + if (!state) > > + return NULL; > > + > > + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); > > + > > + state->output_fmt = ori_state->output_fmt; > > + state->encoder_type = ori_state->encoder_type; > > + state->bpp = ori_state->bpp; > > + state->underflow = ori_state->underflow; > > Can you use kmemdup instead? > > > + > > + return &state->base; > > +} > > + > > +static void vs_crtc_atomic_destroy_state(struct drm_crtc *crtc, > > + struct drm_crtc_state *state) > > +{ > > + __drm_atomic_helper_crtc_destroy_state(state); > > + kfree(to_vs_crtc_state(state)); > > +} > > + > > +static int vs_crtc_enable_vblank(struct drm_crtc *crtc) > > +{ > > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); > > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); > > + > > + vs_dc_enable_vblank(dc, true); > > + > > + return 0; > > +} > > + > > +static void vs_crtc_disable_vblank(struct drm_crtc *crtc) > > +{ > > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); > > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); > > + > > + vs_dc_enable_vblank(dc, false); > > +} > > + > > +static const struct drm_crtc_funcs vs_crtc_funcs = { > > + .set_config = drm_atomic_helper_set_config, > > + .page_flip = drm_atomic_helper_page_flip, > > destroy is required, see drm_mode_config_cleanup() > > > + .reset = vs_crtc_reset, > > + .atomic_duplicate_state = vs_crtc_atomic_duplicate_state, > > + .atomic_destroy_state = vs_crtc_atomic_destroy_state, > > please consider adding atomic_print_state to output driver-specific bits. > > > + .enable_vblank = vs_crtc_enable_vblank, > > + .disable_vblank = vs_crtc_disable_vblank, > > +}; > > + > > +static u8 cal_pixel_bits(u32 bus_format) > > This looks like a generic helper code, which can go to a common place. > > > +{ > > + u8 bpp; > > + > > + switch (bus_format) { > > + case MEDIA_BUS_FMT_RGB565_1X16: > > + case MEDIA_BUS_FMT_UYVY8_1X16: > > + bpp = 16; > > + break; > > + case MEDIA_BUS_FMT_RGB666_1X18: > > + case MEDIA_BUS_FMT_RGB666_1X24_CPADHI: > > + bpp = 18; > > + break; > > + case MEDIA_BUS_FMT_UYVY10_1X20: > > + bpp = 20; > > + break; > > + case MEDIA_BUS_FMT_BGR888_1X24: > > + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: > > + case MEDIA_BUS_FMT_YUV8_1X24: > > + bpp = 24; > > + break; > > + case MEDIA_BUS_FMT_RGB101010_1X30: > > + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: > > + case MEDIA_BUS_FMT_YUV10_1X30: > > + bpp = 30; > > + break; > > + default: > > + bpp = 24; > > + break; > > + } > > + > > + return bpp; > > +} > > + > > +static void vs_crtc_atomic_enable(struct drm_crtc *crtc, > > + struct drm_atomic_state *state) > > +{ > > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); > > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); > > + struct vs_crtc_state *vs_crtc_state = to_vs_crtc_state(crtc->state); > > + > > + vs_crtc_state->bpp = cal_pixel_bits(vs_crtc_state->output_fmt); > > + > > + vs_dc_enable(dc, crtc); > > + drm_crtc_vblank_on(crtc); > > +} > > + > > +static void vs_crtc_atomic_disable(struct drm_crtc *crtc, > > + struct drm_atomic_state *state) > > +{ > > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); > > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); > > + > > + drm_crtc_vblank_off(crtc); > > + > > + vs_dc_disable(dc, crtc); > > + > > + if (crtc->state->event && !crtc->state->active) { > > + spin_lock_irq(&crtc->dev->event_lock); > > + drm_crtc_send_vblank_event(crtc, crtc->state->event); > > + spin_unlock_irq(&crtc->dev->event_lock); > > + > > + crtc->state->event = NULL; > > I think even should be cleared within the lock. event_lock doesn't protect anything in the crtc state. But the bigger problem in this code is the prevalent crtc->state usage. That should pretty much never be done, especially in anything that can get called from the actual commit phase where you no longer have the locks held. Instead one should almost always use the get_{new,old}_state() stuff, or the old/new/oldnew state iterators. > > > + } > > +} > > + > > +static void vs_crtc_atomic_begin(struct drm_crtc *crtc, > > + struct drm_atomic_state *state) > > +{ > > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, > > + crtc); > > + > > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); > > + struct device *dev = vs_crtc->dev; > > + struct drm_property_blob *blob = crtc->state->gamma_lut; Eg. here you are using drm_atomic_get_new_crtc_state() correctly, but then proceed to directly access crtc->state anyway. > > + struct drm_color_lut *lut; > > + struct vs_dc *dc = dev_get_drvdata(dev); > > + > > + if (crtc_state->color_mgmt_changed) { > > + if (blob && blob->length) { > > + lut = blob->data; > > + vs_dc_set_gamma(dc, crtc, lut, > > + blob->length / sizeof(*lut)); > > + vs_dc_enable_gamma(dc, crtc, true); > > + } else { > > + vs_dc_enable_gamma(dc, crtc, false); > > + } > > + } > > +}
hi Ville: very glad to receive your feedback Some of them are very good ideas. Some are not very clear and hope to get your further reply! On 2023/10/26 3:49, Ville Syrjälä wrote: > On Wed, Oct 25, 2023 at 10:28:56PM +0300, Dmitry Baryshkov wrote: >> On 25/10/2023 13:39, Keith Zhao wrote: >> > add 2 crtcs and 8 planes in vs-drm >> > >> > Signed-off-by: Keith Zhao <keith.zhao@starfivetech.com> >> > --- >> > drivers/gpu/drm/verisilicon/Makefile | 8 +- >> > drivers/gpu/drm/verisilicon/vs_crtc.c | 257 ++++ >> > drivers/gpu/drm/verisilicon/vs_crtc.h | 43 + >> > drivers/gpu/drm/verisilicon/vs_dc.c | 1002 ++++++++++++ >> > drivers/gpu/drm/verisilicon/vs_dc.h | 80 + >> > drivers/gpu/drm/verisilicon/vs_dc_hw.c | 1959 ++++++++++++++++++++++++ >> > drivers/gpu/drm/verisilicon/vs_dc_hw.h | 492 ++++++ >> > drivers/gpu/drm/verisilicon/vs_drv.c | 2 + >> > drivers/gpu/drm/verisilicon/vs_plane.c | 526 +++++++ >> > drivers/gpu/drm/verisilicon/vs_plane.h | 58 + >> > drivers/gpu/drm/verisilicon/vs_type.h | 69 + >> > 11 files changed, 4494 insertions(+), 2 deletions(-) >> > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.c >> > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.h >> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.c >> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.h >> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.c >> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.h >> > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.c >> > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.h >> > create mode 100644 drivers/gpu/drm/verisilicon/vs_type.h >> > >> > diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile >> > index 7d3be305b..1d48016ca 100644 >> > --- a/drivers/gpu/drm/verisilicon/Makefile >> > +++ b/drivers/gpu/drm/verisilicon/Makefile >> > @@ -1,7 +1,11 @@ >> > # SPDX-License-Identifier: GPL-2.0 >> > >> > -vs_drm-objs := vs_drv.o \ >> > - vs_modeset.o >> > +vs_drm-objs := vs_dc_hw.o \ >> > + vs_dc.o \ >> > + vs_crtc.o \ >> > + vs_drv.o \ >> > + vs_modeset.o \ >> > + vs_plane.o >> > >> > obj-$(CONFIG_DRM_VERISILICON) += vs_drm.o >> > >> > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c >> > new file mode 100644 >> > index 000000000..8a658ea77 >> > --- /dev/null >> > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c >> > @@ -0,0 +1,257 @@ >> > +// SPDX-License-Identifier: GPL-2.0 >> > +/* >> > + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd. >> > + * >> > + */ >> > + >> > +#include <linux/clk.h> >> > +#include <linux/debugfs.h> >> > +#include <linux/media-bus-format.h> >> > + >> > +#include <drm/drm_atomic_helper.h> >> > +#include <drm/drm_atomic.h> >> > +#include <drm/drm_crtc.h> >> > +#include <drm/drm_gem_atomic_helper.h> >> > +#include <drm/drm_vblank.h> >> > +#include <drm/vs_drm.h> >> > + >> > +#include "vs_crtc.h" >> > +#include "vs_dc.h" >> > +#include "vs_drv.h" >> > + >> > +static void vs_crtc_reset(struct drm_crtc *crtc) >> > +{ >> > + struct vs_crtc_state *state; >> > + >> > + if (crtc->state) { >> > + __drm_atomic_helper_crtc_destroy_state(crtc->state); >> > + >> > + state = to_vs_crtc_state(crtc->state); >> > + kfree(state); >> > + crtc->state = NULL; >> > + } >> >> You can call your crtc_destroy_state function directly here. ok got it ! >> >> > + >> > + state = kzalloc(sizeof(*state), GFP_KERNEL); >> > + if (!state) >> > + return; >> > + >> > + __drm_atomic_helper_crtc_reset(crtc, &state->base); >> > +} >> > + >> > +static struct drm_crtc_state * >> > +vs_crtc_atomic_duplicate_state(struct drm_crtc *crtc) >> > +{ >> > + struct vs_crtc_state *ori_state; >> >> It might be a matter of taste, but it is usually old_state. >> >> > + struct vs_crtc_state *state; >> > + >> > + if (!crtc->state) >> > + return NULL; >> > + >> > + ori_state = to_vs_crtc_state(crtc->state); >> > + state = kzalloc(sizeof(*state), GFP_KERNEL); >> > + if (!state) >> > + return NULL; >> > + >> > + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); >> > + >> > + state->output_fmt = ori_state->output_fmt; >> > + state->encoder_type = ori_state->encoder_type; >> > + state->bpp = ori_state->bpp; >> > + state->underflow = ori_state->underflow; >> >> Can you use kmemdup instead? ok >> >> > + >> > + return &state->base; >> > +} >> > + >> > +static void vs_crtc_atomic_destroy_state(struct drm_crtc *crtc, >> > + struct drm_crtc_state *state) >> > +{ >> > + __drm_atomic_helper_crtc_destroy_state(state); >> > + kfree(to_vs_crtc_state(state)); >> > +} >> > + >> > +static int vs_crtc_enable_vblank(struct drm_crtc *crtc) >> > +{ >> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >> > + >> > + vs_dc_enable_vblank(dc, true); >> > + >> > + return 0; >> > +} >> > + >> > +static void vs_crtc_disable_vblank(struct drm_crtc *crtc) >> > +{ >> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >> > + >> > + vs_dc_enable_vblank(dc, false); >> > +} >> > + >> > +static const struct drm_crtc_funcs vs_crtc_funcs = { >> > + .set_config = drm_atomic_helper_set_config, >> > + .page_flip = drm_atomic_helper_page_flip, >> >> destroy is required, see drm_mode_config_cleanup() will add destory >> >> > + .reset = vs_crtc_reset, >> > + .atomic_duplicate_state = vs_crtc_atomic_duplicate_state, >> > + .atomic_destroy_state = vs_crtc_atomic_destroy_state, >> >> please consider adding atomic_print_state to output driver-specific bits. >> will add atomic_print_state to print my private definitions >> > + .enable_vblank = vs_crtc_enable_vblank, >> > + .disable_vblank = vs_crtc_disable_vblank, >> > +}; >> > + >> > +static u8 cal_pixel_bits(u32 bus_format) >> >> This looks like a generic helper code, which can go to a common place. >> >> > +{ >> > + u8 bpp; >> > + >> > + switch (bus_format) { >> > + case MEDIA_BUS_FMT_RGB565_1X16: >> > + case MEDIA_BUS_FMT_UYVY8_1X16: >> > + bpp = 16; >> > + break; >> > + case MEDIA_BUS_FMT_RGB666_1X18: >> > + case MEDIA_BUS_FMT_RGB666_1X24_CPADHI: >> > + bpp = 18; >> > + break; >> > + case MEDIA_BUS_FMT_UYVY10_1X20: >> > + bpp = 20; >> > + break; >> > + case MEDIA_BUS_FMT_BGR888_1X24: >> > + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: >> > + case MEDIA_BUS_FMT_YUV8_1X24: >> > + bpp = 24; >> > + break; >> > + case MEDIA_BUS_FMT_RGB101010_1X30: >> > + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: >> > + case MEDIA_BUS_FMT_YUV10_1X30: >> > + bpp = 30; >> > + break; >> > + default: >> > + bpp = 24; >> > + break; >> > + } >> > + >> > + return bpp; >> > +} >> > + >> > +static void vs_crtc_atomic_enable(struct drm_crtc *crtc, >> > + struct drm_atomic_state *state) >> > +{ >> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >> > + struct vs_crtc_state *vs_crtc_state = to_vs_crtc_state(crtc->state); >> > + >> > + vs_crtc_state->bpp = cal_pixel_bits(vs_crtc_state->output_fmt); >> > + >> > + vs_dc_enable(dc, crtc); >> > + drm_crtc_vblank_on(crtc); >> > +} >> > + >> > +static void vs_crtc_atomic_disable(struct drm_crtc *crtc, >> > + struct drm_atomic_state *state) >> > +{ >> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >> > + >> > + drm_crtc_vblank_off(crtc); >> > + >> > + vs_dc_disable(dc, crtc); >> > + >> > + if (crtc->state->event && !crtc->state->active) { >> > + spin_lock_irq(&crtc->dev->event_lock); >> > + drm_crtc_send_vblank_event(crtc, crtc->state->event); >> > + spin_unlock_irq(&crtc->dev->event_lock); >> > + >> > + crtc->state->event = NULL; >> >> I think even should be cleared within the lock. > > event_lock doesn't protect anything in the crtc state. how about match like this: spin_lock_irq(&crtc->dev->event_lock); if (crtc->state->event) { drm_crtc_send_vblank_event(crtc, crtc->state->event); crtc->state->event = NULL; } spin_unlock_irq(&crtc->dev->event_lock); > > But the bigger problem in this code is the prevalent crtc->state > usage. That should pretty much never be done, especially in anything > that can get called from the actual commit phase where you no longer > have the locks held. Instead one should almost always use the > get_{new,old}_state() stuff, or the old/new/oldnew state iterators. the prevalent crtc->state usage : crtc->state should not be used directly? need replace it by get_{new,old}_state() for example: drm_crtc_send_vblank_event(crtc, crtc->state->event); should do like this : struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); ... drm_crtc_send_vblank_event(crtc, crtc_state->event); ... If there is a problem, help further correct wish give a example Thanks > >> >> > + } >> > +} >> > + >> > +static void vs_crtc_atomic_begin(struct drm_crtc *crtc, >> > + struct drm_atomic_state *state) >> > +{ >> > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, >> > + crtc); >> > + >> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >> > + struct device *dev = vs_crtc->dev; >> > + struct drm_property_blob *blob = crtc->state->gamma_lut; > > Eg. here you are using drm_atomic_get_new_crtc_state() correctly, but > then proceed to directly access crtc->state anyway. > struct drm_property_blob *blob = crtc->state->gamma_lut; change to struct drm_property_blob *blob = crtc_state ->gamma_lut; >> > + struct drm_color_lut *lut; >> > + struct vs_dc *dc = dev_get_drvdata(dev); >> > + >> > + if (crtc_state->color_mgmt_changed) { >> > + if (blob && blob->length) { >> > + lut = blob->data; >> > + vs_dc_set_gamma(dc, crtc, lut, >> > + blob->length / sizeof(*lut)); >> > + vs_dc_enable_gamma(dc, crtc, true); >> > + } else { >> > + vs_dc_enable_gamma(dc, crtc, false); >> > + } >> > + } >> > +} >
sorry Dmitry ,accidentally wrote the wrong name Take no offense On 2023/10/26 17:42, Keith Zhao wrote: > hi Ville: > very glad to receive your feedback > Some of them are very good ideas. > Some are not very clear and hope to get your further reply! > > > On 2023/10/26 3:49, Ville Syrjälä wrote: >> On Wed, Oct 25, 2023 at 10:28:56PM +0300, Dmitry Baryshkov wrote: >>> On 25/10/2023 13:39, Keith Zhao wrote: >>> > add 2 crtcs and 8 planes in vs-drm >>> > >>> > Signed-off-by: Keith Zhao <keith.zhao@starfivetech.com> >>> > --- >>> > drivers/gpu/drm/verisilicon/Makefile | 8 +- >>> > drivers/gpu/drm/verisilicon/vs_crtc.c | 257 ++++ >>> > drivers/gpu/drm/verisilicon/vs_crtc.h | 43 + >>> > drivers/gpu/drm/verisilicon/vs_dc.c | 1002 ++++++++++++ >>> > drivers/gpu/drm/verisilicon/vs_dc.h | 80 + >>> > drivers/gpu/drm/verisilicon/vs_dc_hw.c | 1959 ++++++++++++++++++++++++ >>> > drivers/gpu/drm/verisilicon/vs_dc_hw.h | 492 ++++++ >>> > drivers/gpu/drm/verisilicon/vs_drv.c | 2 + >>> > drivers/gpu/drm/verisilicon/vs_plane.c | 526 +++++++ >>> > drivers/gpu/drm/verisilicon/vs_plane.h | 58 + >>> > drivers/gpu/drm/verisilicon/vs_type.h | 69 + >>> > 11 files changed, 4494 insertions(+), 2 deletions(-) >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.c >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.h >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.c >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.h >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.c >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.h >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.c >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.h >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_type.h >>> > >>> > diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile >>> > index 7d3be305b..1d48016ca 100644 >>> > --- a/drivers/gpu/drm/verisilicon/Makefile >>> > +++ b/drivers/gpu/drm/verisilicon/Makefile >>> > @@ -1,7 +1,11 @@ >>> > # SPDX-License-Identifier: GPL-2.0 >>> > >>> > -vs_drm-objs := vs_drv.o \ >>> > - vs_modeset.o >>> > +vs_drm-objs := vs_dc_hw.o \ >>> > + vs_dc.o \ >>> > + vs_crtc.o \ >>> > + vs_drv.o \ >>> > + vs_modeset.o \ >>> > + vs_plane.o >>> > >>> > obj-$(CONFIG_DRM_VERISILICON) += vs_drm.o >>> > >>> > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c >>> > new file mode 100644 >>> > index 000000000..8a658ea77 >>> > --- /dev/null >>> > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c >>> > @@ -0,0 +1,257 @@ >>> > +// SPDX-License-Identifier: GPL-2.0 >>> > +/* >>> > + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd. >>> > + * >>> > + */ >>> > + >>> > +#include <linux/clk.h> >>> > +#include <linux/debugfs.h> >>> > +#include <linux/media-bus-format.h> >>> > + >>> > +#include <drm/drm_atomic_helper.h> >>> > +#include <drm/drm_atomic.h> >>> > +#include <drm/drm_crtc.h> >>> > +#include <drm/drm_gem_atomic_helper.h> >>> > +#include <drm/drm_vblank.h> >>> > +#include <drm/vs_drm.h> >>> > + >>> > +#include "vs_crtc.h" >>> > +#include "vs_dc.h" >>> > +#include "vs_drv.h" >>> > + >>> > +static void vs_crtc_reset(struct drm_crtc *crtc) >>> > +{ >>> > + struct vs_crtc_state *state; >>> > + >>> > + if (crtc->state) { >>> > + __drm_atomic_helper_crtc_destroy_state(crtc->state); >>> > + >>> > + state = to_vs_crtc_state(crtc->state); >>> > + kfree(state); >>> > + crtc->state = NULL; >>> > + } >>> >>> You can call your crtc_destroy_state function directly here. > ok got it ! >>> >>> > + >>> > + state = kzalloc(sizeof(*state), GFP_KERNEL); >>> > + if (!state) >>> > + return; >>> > + >>> > + __drm_atomic_helper_crtc_reset(crtc, &state->base); >>> > +} >>> > + >>> > +static struct drm_crtc_state * >>> > +vs_crtc_atomic_duplicate_state(struct drm_crtc *crtc) >>> > +{ >>> > + struct vs_crtc_state *ori_state; >>> >>> It might be a matter of taste, but it is usually old_state. >>> >>> > + struct vs_crtc_state *state; >>> > + >>> > + if (!crtc->state) >>> > + return NULL; >>> > + >>> > + ori_state = to_vs_crtc_state(crtc->state); >>> > + state = kzalloc(sizeof(*state), GFP_KERNEL); >>> > + if (!state) >>> > + return NULL; >>> > + >>> > + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); >>> > + >>> > + state->output_fmt = ori_state->output_fmt; >>> > + state->encoder_type = ori_state->encoder_type; >>> > + state->bpp = ori_state->bpp; >>> > + state->underflow = ori_state->underflow; >>> >>> Can you use kmemdup instead? > ok >>> >>> > + >>> > + return &state->base; >>> > +} >>> > + >>> > +static void vs_crtc_atomic_destroy_state(struct drm_crtc *crtc, >>> > + struct drm_crtc_state *state) >>> > +{ >>> > + __drm_atomic_helper_crtc_destroy_state(state); >>> > + kfree(to_vs_crtc_state(state)); >>> > +} >>> > + >>> > +static int vs_crtc_enable_vblank(struct drm_crtc *crtc) >>> > +{ >>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >>> > + >>> > + vs_dc_enable_vblank(dc, true); >>> > + >>> > + return 0; >>> > +} >>> > + >>> > +static void vs_crtc_disable_vblank(struct drm_crtc *crtc) >>> > +{ >>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >>> > + >>> > + vs_dc_enable_vblank(dc, false); >>> > +} >>> > + >>> > +static const struct drm_crtc_funcs vs_crtc_funcs = { >>> > + .set_config = drm_atomic_helper_set_config, >>> > + .page_flip = drm_atomic_helper_page_flip, >>> >>> destroy is required, see drm_mode_config_cleanup() > will add destory >>> >>> > + .reset = vs_crtc_reset, >>> > + .atomic_duplicate_state = vs_crtc_atomic_duplicate_state, >>> > + .atomic_destroy_state = vs_crtc_atomic_destroy_state, >>> >>> please consider adding atomic_print_state to output driver-specific bits. >>> > will add atomic_print_state to print my private definitions >>> > + .enable_vblank = vs_crtc_enable_vblank, >>> > + .disable_vblank = vs_crtc_disable_vblank, >>> > +}; >>> > + >>> > +static u8 cal_pixel_bits(u32 bus_format) >>> >>> This looks like a generic helper code, which can go to a common place. >>> >>> > +{ >>> > + u8 bpp; >>> > + >>> > + switch (bus_format) { >>> > + case MEDIA_BUS_FMT_RGB565_1X16: >>> > + case MEDIA_BUS_FMT_UYVY8_1X16: >>> > + bpp = 16; >>> > + break; >>> > + case MEDIA_BUS_FMT_RGB666_1X18: >>> > + case MEDIA_BUS_FMT_RGB666_1X24_CPADHI: >>> > + bpp = 18; >>> > + break; >>> > + case MEDIA_BUS_FMT_UYVY10_1X20: >>> > + bpp = 20; >>> > + break; >>> > + case MEDIA_BUS_FMT_BGR888_1X24: >>> > + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: >>> > + case MEDIA_BUS_FMT_YUV8_1X24: >>> > + bpp = 24; >>> > + break; >>> > + case MEDIA_BUS_FMT_RGB101010_1X30: >>> > + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: >>> > + case MEDIA_BUS_FMT_YUV10_1X30: >>> > + bpp = 30; >>> > + break; >>> > + default: >>> > + bpp = 24; >>> > + break; >>> > + } >>> > + >>> > + return bpp; >>> > +} >>> > + >>> > +static void vs_crtc_atomic_enable(struct drm_crtc *crtc, >>> > + struct drm_atomic_state *state) >>> > +{ >>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >>> > + struct vs_crtc_state *vs_crtc_state = to_vs_crtc_state(crtc->state); >>> > + >>> > + vs_crtc_state->bpp = cal_pixel_bits(vs_crtc_state->output_fmt); >>> > + >>> > + vs_dc_enable(dc, crtc); >>> > + drm_crtc_vblank_on(crtc); >>> > +} >>> > + >>> > +static void vs_crtc_atomic_disable(struct drm_crtc *crtc, >>> > + struct drm_atomic_state *state) >>> > +{ >>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >>> > + >>> > + drm_crtc_vblank_off(crtc); >>> > + >>> > + vs_dc_disable(dc, crtc); >>> > + >>> > + if (crtc->state->event && !crtc->state->active) { >>> > + spin_lock_irq(&crtc->dev->event_lock); >>> > + drm_crtc_send_vblank_event(crtc, crtc->state->event); >>> > + spin_unlock_irq(&crtc->dev->event_lock); >>> > + >>> > + crtc->state->event = NULL; >>> >>> I think even should be cleared within the lock. >> >> event_lock doesn't protect anything in the crtc state. > > how about match like this: > spin_lock_irq(&crtc->dev->event_lock); > if (crtc->state->event) { > drm_crtc_send_vblank_event(crtc, crtc->state->event); > crtc->state->event = NULL; > } > spin_unlock_irq(&crtc->dev->event_lock); > >> >> But the bigger problem in this code is the prevalent crtc->state >> usage. That should pretty much never be done, especially in anything >> that can get called from the actual commit phase where you no longer >> have the locks held. Instead one should almost always use the >> get_{new,old}_state() stuff, or the old/new/oldnew state iterators. > > the prevalent crtc->state usage : > crtc->state should not be used directly? > need replace it by get_{new,old}_state() > > for example: > drm_crtc_send_vblank_event(crtc, crtc->state->event); > > > should do like this : > struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > ... > drm_crtc_send_vblank_event(crtc, crtc_state->event); > ... > > If there is a problem, help further correct > wish give a example > Thanks > >> >>> >>> > + } >>> > +} >>> > + >>> > +static void vs_crtc_atomic_begin(struct drm_crtc *crtc, >>> > + struct drm_atomic_state *state) >>> > +{ >>> > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, >>> > + crtc); >>> > + >>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >>> > + struct device *dev = vs_crtc->dev; >>> > + struct drm_property_blob *blob = crtc->state->gamma_lut; >> >> Eg. here you are using drm_atomic_get_new_crtc_state() correctly, but >> then proceed to directly access crtc->state anyway. >> > struct drm_property_blob *blob = crtc->state->gamma_lut; > change to > struct drm_property_blob *blob = crtc_state ->gamma_lut; > >>> > + struct drm_color_lut *lut; >>> > + struct vs_dc *dc = dev_get_drvdata(dev); >>> > + >>> > + if (crtc_state->color_mgmt_changed) { >>> > + if (blob && blob->length) { >>> > + lut = blob->data; >>> > + vs_dc_set_gamma(dc, crtc, lut, >>> > + blob->length / sizeof(*lut)); >>> > + vs_dc_enable_gamma(dc, crtc, true); >>> > + } else { >>> > + vs_dc_enable_gamma(dc, crtc, false); >>> > + } >>> > + } >>> > +} >>
On 2023/10/25 21:57, Maxime Ripard wrote: > On Wed, Oct 25, 2023 at 06:39:56PM +0800, Keith Zhao wrote: >> +static struct drm_crtc_state * >> +vs_crtc_atomic_duplicate_state(struct drm_crtc *crtc) >> +{ >> + struct vs_crtc_state *ori_state; >> + struct vs_crtc_state *state; >> + >> + if (!crtc->state) >> + return NULL; >> + >> + ori_state = to_vs_crtc_state(crtc->state); >> + state = kzalloc(sizeof(*state), GFP_KERNEL); >> + if (!state) >> + return NULL; >> + >> + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); >> + >> + state->output_fmt = ori_state->output_fmt; > > That field is never set in your patch. > >> + state->encoder_type = ori_state->encoder_type; > > That isn't either, and it's not clear why you would need the > encoder_type stored in the CRTC? > >> + state->bpp = ori_state->bpp; > > You seem to derive that from output_fmt, it doesn't need to be in the > CRTC state. > >> + state->underflow = ori_state->underflow; > > Assuming you're setting this from the interrupt handler, it's unsafe, > you shouldn't do that. What are you using it for? I am going to use the crtc_debugfs function for printing. crtc_debugfs will use it But now I'd better delete it > >> +static const struct drm_prop_enum_list vs_sync_mode_enum_list[] = { >> + { VS_SINGLE_DC, "single dc mode" }, >> + { VS_MULTI_DC_PRIMARY, "primary dc for multi dc mode" }, >> + { VS_MULTI_DC_SECONDARY, "secondary dc for multi dc mode" }, >> +}; > > Custom driver properties are a no-go: > https://docs.kernel.org/gpu/drm-kms.html#requirements > > And > > https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements > >> +void vs_dc_enable(struct vs_dc *dc, struct drm_crtc *crtc) >> +{ >> + struct vs_crtc_state *crtc_state = to_vs_crtc_state(crtc->state); >> + struct drm_display_mode *mode = &crtc->state->adjusted_mode; >> + struct dc_hw_display display; > > Why are you rolling your own structure here, if it's exactly equivalent > to what drm_display_mode and the crtc_state provide? My original intention was to make the hardware part purer. and want to decouple hardware from drm struct. so I define the own structure between drm and hardware. Maybe doing this will make both the hardware and drm happy > >> +void vs_dc_commit(struct vs_dc *dc) >> +{ >> + dc_hw_enable_shadow_register(&dc->hw, false); >> + >> + dc_hw_commit(&dc->hw); >> + >> + if (dc->first_frame) >> + dc->first_frame = false; >> + >> + dc_hw_enable_shadow_register(&dc->hw, true); >> +} > > It's not clear to me what you're trying to do here, does the hardware > have latched registers that are only updated during vblank? > >> +static int dc_bind(struct device *dev, struct device *master, void *data) >> +{ >> + struct drm_device *drm_dev = data; >> + struct vs_dc *dc = dev_get_drvdata(dev); >> + struct device_node *port; >> + struct vs_crtc *crtc; >> + struct vs_dc_info *dc_info; >> + struct vs_plane *plane; >> + struct vs_plane_info *plane_info; >> + int i, ret; >> + u32 ctrc_mask = 0; >> + >> + if (!drm_dev || !dc) { >> + dev_err(dev, "devices are not created.\n"); >> + return -ENODEV; >> + } >> + >> + ret = dc_init(dev); >> + if (ret < 0) { >> + drm_err(drm_dev, "Failed to initialize DC hardware.\n"); >> + return ret; >> + } >> + >> + port = of_get_child_by_name(dev->of_node, "port"); >> + if (!port) { >> + drm_err(drm_dev, "no port node found\n"); >> + return -ENODEV; >> + } >> + of_node_put(port); >> + >> + dc_info = dc->hw.info; >> + >> + for (i = 0; i < dc_info->panel_num; i++) { >> + crtc = vs_crtc_create(drm_dev, dc_info); >> + if (!crtc) { >> + drm_err(drm_dev, "Failed to create CRTC.\n"); >> + ret = -ENOMEM; >> + return ret; >> + } >> + >> + crtc->base.port = port; >> + crtc->dev = dev; >> + dc->crtc[i] = crtc; >> + ctrc_mask |= drm_crtc_mask(&crtc->base); >> + } >> + >> + for (i = 0; i < dc_info->plane_num; i++) { >> + plane_info = (struct vs_plane_info *)&dc_info->planes[i]; >> + >> + if (!strcmp(plane_info->name, "Primary") || !strcmp(plane_info->name, "Cursor")) { >> + plane = vs_plane_create(drm_dev, plane_info, dc_info->layer_num, >> + drm_crtc_mask(&dc->crtc[0]->base)); >> + } else if (!strcmp(plane_info->name, "Primary_1") || >> + !strcmp(plane_info->name, "Cursor_1")) { > > Please use an enum and an id there. > >> +static int vs_plane_atomic_set_property(struct drm_plane *plane, >> + struct drm_plane_state *state, >> + struct drm_property *property, >> + uint64_t val) >> +{ >> + struct drm_device *dev = plane->dev; >> + struct vs_plane *vs_plane = to_vs_plane(plane); >> + struct vs_plane_state *vs_plane_state = to_vs_plane_state(state); >> + int ret = 0; >> + >> + if (property == vs_plane->degamma_mode) { >> + if (vs_plane_state->degamma != val) { >> + vs_plane_state->degamma = val; >> + vs_plane_state->degamma_changed = true; >> + } else { >> + vs_plane_state->degamma_changed = false; >> + } >> + } else if (property == vs_plane->watermark_prop) { >> + ret = _vs_plane_set_property_blob_from_id(dev, >> + &vs_plane_state->watermark, >> + val, >> + sizeof(struct drm_vs_watermark)); >> + return ret; >> + } else if (property == vs_plane->color_mgmt_prop) { >> + ret = _vs_plane_set_property_blob_from_id(dev, >> + &vs_plane_state->color_mgmt, >> + val, >> + sizeof(struct drm_vs_color_mgmt)); >> + return ret; >> + } else if (property == vs_plane->roi_prop) { >> + ret = _vs_plane_set_property_blob_from_id(dev, >> + &vs_plane_state->roi, >> + val, >> + sizeof(struct drm_vs_roi)); >> + return ret; >> + } else { >> + return -EINVAL; >> + } >> + >> + return 0; >> +} > > Same story than above for properties > > > Honestly, that driver is pretty massive, and you should be simplifying > it a lot of you want the initial contribution to be as smooth as > possible. > > Things like all the tiling formats, the underflowing handling, all those > properties, etc can (and should) be added in a second step once the > foundations are in. > > Maxime ok , Thanks for reminding me. I will clarify my next goal and be more likely to simplify features.