| Message ID | 20230213085612.1026538-4-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: lcdif: Add i.MX93 LCDIF support | expand |
Hi Liu, thanks for the update. 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@nxp.com> > --- > 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. > + 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_state- >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. > 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(). > + 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'? 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. Best regards, Alexander > + new = kzalloc(sizeof(*new), GFP_KERNEL); > + if (!new) > + return NULL; > + > + __drm_atomic_helper_crtc_duplicate_state(crtc, &new->base); > + > + new->bus_format = old->bus_format; > + new->bus_flags = old->bus_flags; > + > + return &new->base; > +} > + > +static void lcdif_crtc_atomic_destroy_state(struct drm_crtc *crtc, > + struct drm_crtc_state *state) > +{ > + __drm_atomic_helper_crtc_destroy_state(state); > + kfree(to_lcdif_crtc_state(state)); > +} > + > static int lcdif_crtc_enable_vblank(struct drm_crtc *crtc) > { > struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev); > @@ -548,12 +610,12 @@ static const struct drm_crtc_helper_funcs > lcdif_crtc_helper_funcs = { }; > > static const struct drm_crtc_funcs lcdif_crtc_funcs = { > - .reset = drm_atomic_helper_crtc_reset, > + .reset = lcdif_crtc_reset, > .destroy = drm_crtc_cleanup, > .set_config = drm_atomic_helper_set_config, > .page_flip = drm_atomic_helper_page_flip, > - .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > + .atomic_duplicate_state = lcdif_crtc_atomic_duplicate_state, > + .atomic_destroy_state = lcdif_crtc_atomic_destroy_state, > .enable_vblank = lcdif_crtc_enable_vblank, > .disable_vblank = lcdif_crtc_disable_vblank, > };
On Tue, Feb 14, 2023 at 03:12:55PM +0100, Alexander Stein wrote: > Hi Liu, > > thanks for the update. > > 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@nxp.com> > > --- > > 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. NULL pointers is where these things go bad if it't not at offset 0. Unless you're willing to always handle those explicitly.
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@nxp.com> > > --- > > 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
Hi Liu, Am Mittwoch, 15. Februar 2023, 04:44:15 CET schrieb Liu Ying: > 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@nxp.com> > > > --- > > > 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. Nice, this makes totally sense. I was just curious about the reasoning. > > > > + 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. You can do a rename, but I would rather put that into a dedicated commit. Up to you. > > > 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. Agreed, both ways have their advantages. I tried to match you changes to the previously used helper functions. > > > > + 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. Oh, that's for sure. It was late :) > > > 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. Sounds reasonable. Thanks > Regards, > Liu Ying Best regards, Alexander
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 */ + 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_state->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); 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)); + crtc->state = NULL; + + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (state) + __drm_atomic_helper_crtc_reset(crtc, &state->base); +} + +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; + + new = kzalloc(sizeof(*new), GFP_KERNEL); + if (!new) + return NULL; + + __drm_atomic_helper_crtc_duplicate_state(crtc, &new->base); + + new->bus_format = old->bus_format; + new->bus_flags = old->bus_flags; + + return &new->base; +} + +static void lcdif_crtc_atomic_destroy_state(struct drm_crtc *crtc, + struct drm_crtc_state *state) +{ + __drm_atomic_helper_crtc_destroy_state(state); + kfree(to_lcdif_crtc_state(state)); +} + static int lcdif_crtc_enable_vblank(struct drm_crtc *crtc) { struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev); @@ -548,12 +610,12 @@ static const struct drm_crtc_helper_funcs lcdif_crtc_helper_funcs = { }; static const struct drm_crtc_funcs lcdif_crtc_funcs = { - .reset = drm_atomic_helper_crtc_reset, + .reset = lcdif_crtc_reset, .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, - .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, + .atomic_duplicate_state = lcdif_crtc_atomic_duplicate_state, + .atomic_destroy_state = lcdif_crtc_atomic_destroy_state, .enable_vblank = lcdif_crtc_enable_vblank, .disable_vblank = lcdif_crtc_disable_vblank, };
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@nxp.com> --- 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(-)