On Tue, 2023-02-14 at 15:12 +0100, Alexander Stein wrote: > Hi Liu, Hi Alexander, > > thanks for the update. Thanks for your review. > > Am Montag, 13. Februar 2023, 09:56:09 CET schrieb Liu Ying: > > Instead of determining LCDIF output bus format and bus flags in > > ->atomic_enable(), do that in ->atomic_check(). This is a > > preparation for the upcoming patch to check consistent bus format > > and bus flags across all first downstream bridges in > > ->atomic_check(). > > New lcdif_crtc_state structure is introduced to cache bus format > > and bus flags states in ->atomic_check() so that they can be read > > in ->atomic_enable(). > > > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx> > > --- > > v2->v3: > > * No change. > > > > v1->v2: > > * Split from patch 2/2 in v1. (Marek, Alexander) > > * Add comment on the 'base' member of lcdif_crtc_state structure to > > note it should always be the first member. (Lothar) > > > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 138 ++++++++++++++++++++++-- > > ------ > > 1 file changed, 100 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c index > > e54200a9fcb9..294cecdf5439 100644 > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > @@ -30,6 +30,18 @@ > > #include "lcdif_drv.h" > > #include "lcdif_regs.h" > > > > +struct lcdif_crtc_state { > > + struct drm_crtc_state base; /* always be the first > > member */ > > Is it strictly necessary that base is the first member? > to_lcdif_crtc_state() > should be able to handle any position within the struct. I mean it's > sensible > to put it in first place. But the comment somehow bugs me. The comment was added in v2 to make sure to_lcdif_crtc_state() work ok when a NULL pointer is handed over to it. The NULL pointer worries Lothar a bit as we can see Lothar's comment for v1. > > > + u32 bus_format; > > + u32 bus_flags; > > +}; > > + > > +static inline struct lcdif_crtc_state * > > +to_lcdif_crtc_state(struct drm_crtc_state *s) > > +{ > > + return container_of(s, struct lcdif_crtc_state, base); > > +} > > + > > /* > > ----------------------------------------------------------------- > > ---------- > > -- * CRTC > > */ > > @@ -385,48 +397,72 @@ static void lcdif_reset_block(struct > > lcdif_drm_private > > *lcdif) readl(lcdif->base + LCDC_V8_CTRL); > > } > > > > -static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private > > *lcdif, > > - struct drm_plane_state > > *plane_state, > > - struct drm_bridge_state > > *bridge_state, > > - const u32 bus_format) > > +static void lcdif_crtc_mode_set_nofb(struct drm_crtc_state > > *crtc_state, > > + struct drm_plane_state > > *plane_state) > > { > > - struct drm_device *drm = lcdif->crtc.dev; > > - struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode; > > - u32 bus_flags = 0; > > - > > - if (lcdif->bridge->timings) > > - bus_flags = lcdif->bridge->timings->input_bus_flags; > > - else if (bridge_state) > > - bus_flags = bridge_state->input_bus_cfg.flags; > > + struct lcdif_crtc_state *lcdif_crtc_state = > > to_lcdif_crtc_state(crtc_state); + struct drm_device *drm = > > crtc_state->crtc->dev; > > + struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm); > > + struct drm_display_mode *m = &crtc_state->adjusted_mode; > > > > DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock: %dkHz (actual: > > %dkHz) > > \n", > > m->crtc_clock, > > (int)(clk_get_rate(lcdif->clk) / 1000)); > > DRM_DEV_DEBUG_DRIVER(drm->dev, "Bridge bus_flags: 0x%08X\n", > > - bus_flags); > > + lcdif_crtc_state->bus_flags); > > DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m- > > >flags); > > > > /* Mandatory eLCDIF reset as per the Reference Manual */ > > lcdif_reset_block(lcdif); > > > > - lcdif_set_formats(lcdif, plane_state, bus_format); > > + lcdif_set_formats(lcdif, plane_state, lcdif_crtc_state- > > >bus_format); > > > > - lcdif_set_mode(lcdif, bus_flags); > > + lcdif_set_mode(lcdif, lcdif_crtc_state->bus_flags); > > } > > > > static int lcdif_crtc_atomic_check(struct drm_crtc *crtc, > > struct drm_atomic_state *state) > > { > > + struct drm_device *drm = crtc->dev; > > + struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm); > > struct drm_crtc_state *crtc_state = > > drm_atomic_get_new_crtc_state(state, > > > > crtc); > > + struct lcdif_crtc_state *lcdif_crtc_state = > > to_lcdif_crtc_state(crtc_state); bool has_primary = crtc_state- > > >plane_mask > > & > > drm_plane_mask(crtc->primary); > > + struct drm_bridge_state *bridge_state; > > + struct drm_bridge *bridge = lcdif->bridge; > > + int ret; > > > > /* The primary plane has to be enabled when the CRTC is active. > > */ > > if (crtc_state->active && !has_primary) > > return -EINVAL; > > > > - return drm_atomic_add_affected_planes(state, crtc); > > + ret = drm_atomic_add_affected_planes(state, crtc); > > + if (ret) > > + return ret; > > + > > + bridge_state = drm_atomic_get_new_bridge_state(state, bridge); > > + if (!bridge_state) > > + lcdif_crtc_state->bus_format = MEDIA_BUS_FMT_FIXED; > > + else > > + lcdif_crtc_state->bus_format = bridge_state- > > input_bus_cfg.format; > > + > > + if (lcdif_crtc_state->bus_format == MEDIA_BUS_FMT_FIXED) { > > + dev_warn_once(drm->dev, > > + "Bridge does not provide bus format, > > assuming > > MEDIA_BUS_FMT_RGB888_1X24.\n" + "Please > > fix > > bridge driver by > > handling atomic_get_input_bus_fmts.\n"); + lcdif_crtc_stat > > e- > > bus_format = > > MEDIA_BUS_FMT_RGB888_1X24; > > + } > > + > > + if (bridge->timings) > > + lcdif_crtc_state->bus_flags = bridge->timings- > > input_bus_flags; > > + else if (bridge_state) > > + lcdif_crtc_state->bus_flags = bridge_state- > > input_bus_cfg.flags; > > + else > > + lcdif_crtc_state->bus_flags = 0; > > + > > + return 0; > > } > > > > static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc, > > @@ -458,35 +494,21 @@ static void lcdif_crtc_atomic_enable(struct > > drm_crtc > > *crtc, struct drm_atomic_state *state) > > { > > struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc- > > >dev); > > - struct drm_plane_state *new_pstate = > > drm_atomic_get_new_plane_state(state, > > - > > crtc->primary); > > + struct drm_crtc_state *new_crtc_state = > > drm_atomic_get_new_crtc_state(state, crtc); + struct > > drm_plane_state > > *new_plane_state = drm_atomic_get_new_plane_state(state, + > > > > > > crtc->primary); > > While the rename to 'new_plane_state' makes sense, this is an > unrelated > change. I'll use 'new_pstate' and 'new_cstate' in next version. > > > struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode; > > - struct drm_bridge_state *bridge_state = NULL; > > struct drm_device *drm = lcdif->drm; > > - u32 bus_format; > > dma_addr_t paddr; > > > > - bridge_state = drm_atomic_get_new_bridge_state(state, lcdif- > > bridge); > > - if (!bridge_state) > > - bus_format = MEDIA_BUS_FMT_FIXED; > > - else > > - bus_format = bridge_state->input_bus_cfg.format; > > - > > - if (bus_format == MEDIA_BUS_FMT_FIXED) { > > - dev_warn_once(drm->dev, > > - "Bridge does not provide bus format, > > assuming > > MEDIA_BUS_FMT_RGB888_1X24.\n" - "Please > > fix > > bridge driver by > > handling atomic_get_input_bus_fmts.\n"); - bus_format = > > MEDIA_BUS_FMT_RGB888_1X24; > > - } > > - > > clk_set_rate(lcdif->clk, m->crtc_clock * 1000); > > > > pm_runtime_get_sync(drm->dev); > > > > - lcdif_crtc_mode_set_nofb(lcdif, new_pstate, bridge_state, > > bus_format); > > + lcdif_crtc_mode_set_nofb(new_crtc_state, new_plane_state); > > > > /* Write cur_buf as well to avoid an initial corrupt frame */ > > - paddr = drm_fb_dma_get_gem_addr(new_pstate->fb, new_pstate, 0); > > + paddr = drm_fb_dma_get_gem_addr(new_plane_state->fb, > > new_plane_state, 0); > > if (paddr) { > > writel(lower_32_bits(paddr), > > lcdif->base + LCDC_V8_CTRLDESCL_LOW0_4); > > @@ -520,6 +542,46 @@ static void lcdif_crtc_atomic_disable(struct > > drm_crtc > > *crtc, pm_runtime_put_sync(drm->dev); > > } > > > > +static void lcdif_crtc_reset(struct drm_crtc *crtc) > > +{ > > + struct lcdif_crtc_state *state; > > + > > + if (crtc->state) > > + __drm_atomic_helper_crtc_destroy_state(crtc->state); > > + > > + kfree(to_lcdif_crtc_state(crtc->state)); > > Shouldn't this be just > if (crtc->state) > crtc->funcs->atomic_destroy_state(crtc, crtc->state); > > similar to what drm_atomic_helper_crtc_reset is doing? This will > eventually > call lcdif_crtc_atomic_destroy_state(). I think you are right. But, instead of calling crtc->funcs- >atomic_destroy_state(), I would call lcdif_crtc_atomic_destroy_state() directly, which looks simpler to me. > > > + crtc->state = NULL; > > + > > + state = kzalloc(sizeof(*state), GFP_KERNEL); > > + if (state) > > + __drm_atomic_helper_crtc_reset(crtc, &state->base); > > Is there a specific reason to not call this helper when > 'state==NULL'? Yes, when 'state==NULL', &state->base would de-reference a NULL pointer. > drm_atomic_helper_crtc_reset() does call this even when passing NULL > for > crtc_state. > > > +} > > + > > +static struct drm_crtc_state * > > +lcdif_crtc_atomic_duplicate_state(struct drm_crtc *crtc) > > +{ > > + struct lcdif_crtc_state *old = to_lcdif_crtc_state(crtc- > > >state); > > + struct lcdif_crtc_state *new; > > + > > drm_atomic_helper_crtc_duplicate_state() has a check for > if (WARN_ON(!crtc->state)) > return NULL; > > Maybe it should be added here as well. But then the call to > to_lcdif_crtc_state() has to be moved down. I'll add the check in next version. But, to_lcdif_crtc_state() doesn't have to be moved down even if crtc->state is NULL, because 'base' is the first member of struct lcdif_crtc_state and it's safe. Regards, Liu Ying