* [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code @ 2020-09-23 8:40 ` Maxime Ripard 0 siblings, 0 replies; 16+ messages in thread From: Maxime Ripard @ 2020-09-23 8:40 UTC (permalink / raw) To: Eric Anholt Cc: Tim Gover, Dave Stevenson, dri-devel, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel, Maxime Ripard The current CRTC state reset hook in vc4 allocates a vc4_crtc_state structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state embeds drm_crtc_state as its first member, and therefore can be safely casted. However, this is pretty fragile especially since there's no check for this in place, and we're going to need to access vc4_crtc_state member at reset so this looks like a good occasion to make it more robust. Signed-off-by: Maxime Ripard <maxime@cerno.tech> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> --- Changes from v1: - new patch --- drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index a393f93390a2..7ef20adedee5 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc, void vc4_crtc_reset(struct drm_crtc *crtc) { + struct vc4_crtc_state *vc4_crtc_state; + if (crtc->state) vc4_crtc_destroy_state(crtc, crtc->state); - crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL); - if (crtc->state) - __drm_atomic_helper_crtc_reset(crtc, crtc->state); + + vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL); + if (!vc4_crtc_state) { + crtc->state = NULL; + return; + } + + __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); } static const struct drm_crtc_funcs vc4_crtc_funcs = { -- 2.26.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code @ 2020-09-23 8:40 ` Maxime Ripard 0 siblings, 0 replies; 16+ messages in thread From: Maxime Ripard @ 2020-09-23 8:40 UTC (permalink / raw) To: Eric Anholt Cc: Tim Gover, Dave Stevenson, dri-devel, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel, Maxime Ripard The current CRTC state reset hook in vc4 allocates a vc4_crtc_state structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state embeds drm_crtc_state as its first member, and therefore can be safely casted. However, this is pretty fragile especially since there's no check for this in place, and we're going to need to access vc4_crtc_state member at reset so this looks like a good occasion to make it more robust. Signed-off-by: Maxime Ripard <maxime@cerno.tech> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> --- Changes from v1: - new patch --- drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index a393f93390a2..7ef20adedee5 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc, void vc4_crtc_reset(struct drm_crtc *crtc) { + struct vc4_crtc_state *vc4_crtc_state; + if (crtc->state) vc4_crtc_destroy_state(crtc, crtc->state); - crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL); - if (crtc->state) - __drm_atomic_helper_crtc_reset(crtc, crtc->state); + + vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL); + if (!vc4_crtc_state) { + crtc->state = NULL; + return; + } + + __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); } static const struct drm_crtc_funcs vc4_crtc_funcs = { -- 2.26.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] drm/vc4: crtc: Keep the previously assigned HVS FIFO 2020-09-23 8:40 ` Maxime Ripard @ 2020-09-23 8:40 ` Maxime Ripard -1 siblings, 0 replies; 16+ messages in thread From: Maxime Ripard @ 2020-09-23 8:40 UTC (permalink / raw) To: Eric Anholt Cc: Tim Gover, Dave Stevenson, dri-devel, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel, Maxime Ripard The HVS FIFOs are currently assigned each time we have an atomic_check for all the enabled CRTCs. However, if we are running multiple outputs in parallel and we happen to disable the first (by index) CRTC, we end up changing the assigned FIFO of the second CRTC without disabling and reenabling the pixelvalve which ends up in a stall and eventually a VBLANK timeout. In order to fix this, we can create a special value for our assigned channel to mark it as disabled, and if our CRTC already had an assigned channel in its previous state, we keep on using it. Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard <maxime@cerno.tech> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> --- Changes from v1: - Split away the crtc state reset refactoring - Fixed the checkpatch warnings --- drivers/gpu/drm/vc4/vc4_crtc.c | 1 + drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ drivers/gpu/drm/vc4/vc4_kms.c | 22 ++++++++++++++++------ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 7ef20adedee5..482219fb4db2 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -863,6 +863,7 @@ void vc4_crtc_reset(struct drm_crtc *crtc) return; } + vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); } diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 8c8d96b6289f..90b911fd2a7f 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -532,6 +532,8 @@ struct vc4_crtc_state { } margins; }; +#define VC4_HVS_CHANNEL_DISABLED ((unsigned int)-1) + static inline struct vc4_crtc_state * to_vc4_crtc_state(struct drm_crtc_state *crtc_state) { diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 01fa60844695..149825ff5df8 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -616,7 +616,7 @@ static int vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0); - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_crtc *crtc; int i, ret; @@ -629,6 +629,8 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) * modified. */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + struct drm_crtc_state *crtc_state; + if (!crtc->state->enable) continue; @@ -637,15 +639,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) return PTR_ERR(crtc_state); } - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { - struct vc4_crtc_state *vc4_crtc_state = - to_vc4_crtc_state(crtc_state); + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { + struct vc4_crtc_state *new_vc4_crtc_state = + to_vc4_crtc_state(new_crtc_state); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); unsigned int matching_channels; - if (!crtc_state->enable) + if (old_crtc_state->enable && !new_crtc_state->enable) + new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; + + if (!new_crtc_state->enable) continue; + if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) { + unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel); + continue; + } + /* * The problem we have to solve here is that we have * up to 7 encoders, connected to up to 6 CRTCs. @@ -674,7 +684,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) if (matching_channels) { unsigned int channel = ffs(matching_channels) - 1; - vc4_crtc_state->assigned_channel = channel; + new_vc4_crtc_state->assigned_channel = channel; unassigned_channels &= ~BIT(channel); } else { return -EINVAL; -- 2.26.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] drm/vc4: crtc: Keep the previously assigned HVS FIFO @ 2020-09-23 8:40 ` Maxime Ripard 0 siblings, 0 replies; 16+ messages in thread From: Maxime Ripard @ 2020-09-23 8:40 UTC (permalink / raw) To: Eric Anholt Cc: Tim Gover, Dave Stevenson, dri-devel, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel, Maxime Ripard The HVS FIFOs are currently assigned each time we have an atomic_check for all the enabled CRTCs. However, if we are running multiple outputs in parallel and we happen to disable the first (by index) CRTC, we end up changing the assigned FIFO of the second CRTC without disabling and reenabling the pixelvalve which ends up in a stall and eventually a VBLANK timeout. In order to fix this, we can create a special value for our assigned channel to mark it as disabled, and if our CRTC already had an assigned channel in its previous state, we keep on using it. Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard <maxime@cerno.tech> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> --- Changes from v1: - Split away the crtc state reset refactoring - Fixed the checkpatch warnings --- drivers/gpu/drm/vc4/vc4_crtc.c | 1 + drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ drivers/gpu/drm/vc4/vc4_kms.c | 22 ++++++++++++++++------ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 7ef20adedee5..482219fb4db2 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -863,6 +863,7 @@ void vc4_crtc_reset(struct drm_crtc *crtc) return; } + vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); } diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 8c8d96b6289f..90b911fd2a7f 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -532,6 +532,8 @@ struct vc4_crtc_state { } margins; }; +#define VC4_HVS_CHANNEL_DISABLED ((unsigned int)-1) + static inline struct vc4_crtc_state * to_vc4_crtc_state(struct drm_crtc_state *crtc_state) { diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 01fa60844695..149825ff5df8 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -616,7 +616,7 @@ static int vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) { unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0); - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_crtc *crtc; int i, ret; @@ -629,6 +629,8 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) * modified. */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + struct drm_crtc_state *crtc_state; + if (!crtc->state->enable) continue; @@ -637,15 +639,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) return PTR_ERR(crtc_state); } - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { - struct vc4_crtc_state *vc4_crtc_state = - to_vc4_crtc_state(crtc_state); + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { + struct vc4_crtc_state *new_vc4_crtc_state = + to_vc4_crtc_state(new_crtc_state); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); unsigned int matching_channels; - if (!crtc_state->enable) + if (old_crtc_state->enable && !new_crtc_state->enable) + new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; + + if (!new_crtc_state->enable) continue; + if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) { + unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel); + continue; + } + /* * The problem we have to solve here is that we have * up to 7 encoders, connected to up to 6 CRTCs. @@ -674,7 +684,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) if (matching_channels) { unsigned int channel = ffs(matching_channels) - 1; - vc4_crtc_state->assigned_channel = channel; + new_vc4_crtc_state->assigned_channel = channel; unassigned_channels &= ~BIT(channel); } else { return -EINVAL; -- 2.26.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] drm/vc4: crtc: Keep the previously assigned HVS FIFO 2020-09-23 8:40 ` Maxime Ripard @ 2020-09-25 12:05 ` Dave Stevenson -1 siblings, 0 replies; 16+ messages in thread From: Dave Stevenson @ 2020-09-25 12:05 UTC (permalink / raw) To: Maxime Ripard Cc: Tim Gover, DRI Development, Eric Anholt, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel Hi Maxime Sorry for the delay. On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > The HVS FIFOs are currently assigned each time we have an atomic_check > for all the enabled CRTCs. > > However, if we are running multiple outputs in parallel and we happen to > disable the first (by index) CRTC, we end up changing the assigned FIFO > of the second CRTC without disabling and reenabling the pixelvalve which > ends up in a stall and eventually a VBLANK timeout. > > In order to fix this, we can create a special value for our assigned > channel to mark it as disabled, and if our CRTC already had an assigned > channel in its previous state, we keep on using it. > > Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> I retested too and had triple output (HDMI-0, HDMI-1, and DPI) working happily! Switching around resolutions on the HDMI outputs was working fine. DPI was an Adafruit Kippah and 800x480 LCD, so no options on resolution. We may finally have this muxing nailed. Dave > --- > > Changes from v1: > - Split away the crtc state reset refactoring > - Fixed the checkpatch warnings > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 1 + > drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ > drivers/gpu/drm/vc4/vc4_kms.c | 22 ++++++++++++++++------ > 3 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > index 7ef20adedee5..482219fb4db2 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -863,6 +863,7 @@ void vc4_crtc_reset(struct drm_crtc *crtc) > return; > } > > + vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; > __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); > } > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > index 8c8d96b6289f..90b911fd2a7f 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.h > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > @@ -532,6 +532,8 @@ struct vc4_crtc_state { > } margins; > }; > > +#define VC4_HVS_CHANNEL_DISABLED ((unsigned int)-1) > + > static inline struct vc4_crtc_state * > to_vc4_crtc_state(struct drm_crtc_state *crtc_state) > { > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 01fa60844695..149825ff5df8 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -616,7 +616,7 @@ static int > vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > { > unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0); > - struct drm_crtc_state *crtc_state; > + struct drm_crtc_state *old_crtc_state, *new_crtc_state; > struct drm_crtc *crtc; > int i, ret; > > @@ -629,6 +629,8 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > * modified. > */ > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + struct drm_crtc_state *crtc_state; > + > if (!crtc->state->enable) > continue; > > @@ -637,15 +639,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > return PTR_ERR(crtc_state); > } > > - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > - struct vc4_crtc_state *vc4_crtc_state = > - to_vc4_crtc_state(crtc_state); > + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { > + struct vc4_crtc_state *new_vc4_crtc_state = > + to_vc4_crtc_state(new_crtc_state); > struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); > unsigned int matching_channels; > > - if (!crtc_state->enable) > + if (old_crtc_state->enable && !new_crtc_state->enable) > + new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; > + > + if (!new_crtc_state->enable) > continue; > > + if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) { > + unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel); > + continue; > + } > + > /* > * The problem we have to solve here is that we have > * up to 7 encoders, connected to up to 6 CRTCs. > @@ -674,7 +684,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > if (matching_channels) { > unsigned int channel = ffs(matching_channels) - 1; > > - vc4_crtc_state->assigned_channel = channel; > + new_vc4_crtc_state->assigned_channel = channel; > unassigned_channels &= ~BIT(channel); > } else { > return -EINVAL; > -- > 2.26.2 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] drm/vc4: crtc: Keep the previously assigned HVS FIFO @ 2020-09-25 12:05 ` Dave Stevenson 0 siblings, 0 replies; 16+ messages in thread From: Dave Stevenson @ 2020-09-25 12:05 UTC (permalink / raw) To: Maxime Ripard Cc: Tim Gover, DRI Development, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel Hi Maxime Sorry for the delay. On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > The HVS FIFOs are currently assigned each time we have an atomic_check > for all the enabled CRTCs. > > However, if we are running multiple outputs in parallel and we happen to > disable the first (by index) CRTC, we end up changing the assigned FIFO > of the second CRTC without disabling and reenabling the pixelvalve which > ends up in a stall and eventually a VBLANK timeout. > > In order to fix this, we can create a special value for our assigned > channel to mark it as disabled, and if our CRTC already had an assigned > channel in its previous state, we keep on using it. > > Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> I retested too and had triple output (HDMI-0, HDMI-1, and DPI) working happily! Switching around resolutions on the HDMI outputs was working fine. DPI was an Adafruit Kippah and 800x480 LCD, so no options on resolution. We may finally have this muxing nailed. Dave > --- > > Changes from v1: > - Split away the crtc state reset refactoring > - Fixed the checkpatch warnings > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 1 + > drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ > drivers/gpu/drm/vc4/vc4_kms.c | 22 ++++++++++++++++------ > 3 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > index 7ef20adedee5..482219fb4db2 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -863,6 +863,7 @@ void vc4_crtc_reset(struct drm_crtc *crtc) > return; > } > > + vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; > __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); > } > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > index 8c8d96b6289f..90b911fd2a7f 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.h > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > @@ -532,6 +532,8 @@ struct vc4_crtc_state { > } margins; > }; > > +#define VC4_HVS_CHANNEL_DISABLED ((unsigned int)-1) > + > static inline struct vc4_crtc_state * > to_vc4_crtc_state(struct drm_crtc_state *crtc_state) > { > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 01fa60844695..149825ff5df8 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -616,7 +616,7 @@ static int > vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > { > unsigned long unassigned_channels = GENMASK(NUM_CHANNELS - 1, 0); > - struct drm_crtc_state *crtc_state; > + struct drm_crtc_state *old_crtc_state, *new_crtc_state; > struct drm_crtc *crtc; > int i, ret; > > @@ -629,6 +629,8 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > * modified. > */ > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + struct drm_crtc_state *crtc_state; > + > if (!crtc->state->enable) > continue; > > @@ -637,15 +639,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > return PTR_ERR(crtc_state); > } > > - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > - struct vc4_crtc_state *vc4_crtc_state = > - to_vc4_crtc_state(crtc_state); > + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { > + struct vc4_crtc_state *new_vc4_crtc_state = > + to_vc4_crtc_state(new_crtc_state); > struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); > unsigned int matching_channels; > > - if (!crtc_state->enable) > + if (old_crtc_state->enable && !new_crtc_state->enable) > + new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED; > + > + if (!new_crtc_state->enable) > continue; > > + if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) { > + unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel); > + continue; > + } > + > /* > * The problem we have to solve here is that we have > * up to 7 encoders, connected to up to 6 CRTCs. > @@ -674,7 +684,7 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > if (matching_channels) { > unsigned int channel = ffs(matching_channels) - 1; > > - vc4_crtc_state->assigned_channel = channel; > + new_vc4_crtc_state->assigned_channel = channel; > unassigned_channels &= ~BIT(channel); > } else { > return -EINVAL; > -- > 2.26.2 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code 2020-09-23 8:40 ` Maxime Ripard @ 2020-09-23 14:59 ` Dave Stevenson -1 siblings, 0 replies; 16+ messages in thread From: Dave Stevenson @ 2020-09-23 14:59 UTC (permalink / raw) To: Maxime Ripard Cc: Tim Gover, DRI Development, Eric Anholt, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel Hi Maxime On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state > embeds drm_crtc_state as its first member, and therefore can be safely > casted. s/casted/cast > However, this is pretty fragile especially since there's no check for this > in place, and we're going to need to access vc4_crtc_state member at reset > so this looks like a good occasion to make it more robust. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Based on the issue I perceived with the previous patch, I'm happy. I haven't thought about how the framework handles losing the state, but that's not the driver's problem. There is still an implicit assumption that drm_crtc_state is the first member from the implementation of to_vc4_crtc_state in vc4_drv.h. To make it even more robust that could be a container_of instead. I haven't checked for any other places that make the assumption though. Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > > Changes from v1: > - new patch > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > index a393f93390a2..7ef20adedee5 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc, > > void vc4_crtc_reset(struct drm_crtc *crtc) > { > + struct vc4_crtc_state *vc4_crtc_state; > + > if (crtc->state) > vc4_crtc_destroy_state(crtc, crtc->state); > - crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL); > - if (crtc->state) > - __drm_atomic_helper_crtc_reset(crtc, crtc->state); > + > + vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL); > + if (!vc4_crtc_state) { > + crtc->state = NULL; > + return; > + } > + > + __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); > } > > static const struct drm_crtc_funcs vc4_crtc_funcs = { > -- > 2.26.2 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code @ 2020-09-23 14:59 ` Dave Stevenson 0 siblings, 0 replies; 16+ messages in thread From: Dave Stevenson @ 2020-09-23 14:59 UTC (permalink / raw) To: Maxime Ripard Cc: Tim Gover, DRI Development, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel Hi Maxime On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state > embeds drm_crtc_state as its first member, and therefore can be safely > casted. s/casted/cast > However, this is pretty fragile especially since there's no check for this > in place, and we're going to need to access vc4_crtc_state member at reset > so this looks like a good occasion to make it more robust. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Based on the issue I perceived with the previous patch, I'm happy. I haven't thought about how the framework handles losing the state, but that's not the driver's problem. There is still an implicit assumption that drm_crtc_state is the first member from the implementation of to_vc4_crtc_state in vc4_drv.h. To make it even more robust that could be a container_of instead. I haven't checked for any other places that make the assumption though. Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > > Changes from v1: > - new patch > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > index a393f93390a2..7ef20adedee5 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc, > > void vc4_crtc_reset(struct drm_crtc *crtc) > { > + struct vc4_crtc_state *vc4_crtc_state; > + > if (crtc->state) > vc4_crtc_destroy_state(crtc, crtc->state); > - crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL); > - if (crtc->state) > - __drm_atomic_helper_crtc_reset(crtc, crtc->state); > + > + vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL); > + if (!vc4_crtc_state) { > + crtc->state = NULL; > + return; > + } > + > + __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); > } > > static const struct drm_crtc_funcs vc4_crtc_funcs = { > -- > 2.26.2 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code 2020-09-23 14:59 ` Dave Stevenson @ 2020-09-23 15:41 ` Daniel Vetter -1 siblings, 0 replies; 16+ messages in thread From: Daniel Vetter @ 2020-09-23 15:41 UTC (permalink / raw) To: Dave Stevenson Cc: Tim Gover, DRI Development, bcm-kernel-feedback-list, Maxime Ripard, Phil Elwell, linux-arm-kernel, linux-rpi-kernel On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote: > Hi Maxime > > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state > > embeds drm_crtc_state as its first member, and therefore can be safely > > casted. > > s/casted/cast > > > However, this is pretty fragile especially since there's no check for this > > in place, and we're going to need to access vc4_crtc_state member at reset > > so this looks like a good occasion to make it more robust. Also your next atomic_check will probably look at this, most definitely in the memcpy to take over the old state. So yeah KASAN should complain if you get this wrong :-) Probably want a Fixes: line for this too. -Daniel > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Based on the issue I perceived with the previous patch, I'm happy. I > haven't thought about how the framework handles losing the state, but > that's not the driver's problem. > > There is still an implicit assumption that drm_crtc_state is the first > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To > make it even more robust that could be a container_of instead. I > haven't checked for any other places that make the assumption though. > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > --- > > > > Changes from v1: > > - new patch > > --- > > drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > > index a393f93390a2..7ef20adedee5 100644 > > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > > @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc, > > > > void vc4_crtc_reset(struct drm_crtc *crtc) > > { > > + struct vc4_crtc_state *vc4_crtc_state; > > + > > if (crtc->state) > > vc4_crtc_destroy_state(crtc, crtc->state); > > - crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL); > > - if (crtc->state) > > - __drm_atomic_helper_crtc_reset(crtc, crtc->state); > > + > > + vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL); > > + if (!vc4_crtc_state) { > > + crtc->state = NULL; > > + return; > > + } > > + > > + __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); > > } > > > > static const struct drm_crtc_funcs vc4_crtc_funcs = { > > -- > > 2.26.2 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code @ 2020-09-23 15:41 ` Daniel Vetter 0 siblings, 0 replies; 16+ messages in thread From: Daniel Vetter @ 2020-09-23 15:41 UTC (permalink / raw) To: Dave Stevenson Cc: Tim Gover, DRI Development, bcm-kernel-feedback-list, Maxime Ripard, Phil Elwell, linux-arm-kernel, linux-rpi-kernel On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote: > Hi Maxime > > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state > > embeds drm_crtc_state as its first member, and therefore can be safely > > casted. > > s/casted/cast > > > However, this is pretty fragile especially since there's no check for this > > in place, and we're going to need to access vc4_crtc_state member at reset > > so this looks like a good occasion to make it more robust. Also your next atomic_check will probably look at this, most definitely in the memcpy to take over the old state. So yeah KASAN should complain if you get this wrong :-) Probably want a Fixes: line for this too. -Daniel > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Based on the issue I perceived with the previous patch, I'm happy. I > haven't thought about how the framework handles losing the state, but > that's not the driver's problem. > > There is still an implicit assumption that drm_crtc_state is the first > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To > make it even more robust that could be a container_of instead. I > haven't checked for any other places that make the assumption though. > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > --- > > > > Changes from v1: > > - new patch > > --- > > drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > > index a393f93390a2..7ef20adedee5 100644 > > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > > @@ -852,11 +852,18 @@ void vc4_crtc_destroy_state(struct drm_crtc *crtc, > > > > void vc4_crtc_reset(struct drm_crtc *crtc) > > { > > + struct vc4_crtc_state *vc4_crtc_state; > > + > > if (crtc->state) > > vc4_crtc_destroy_state(crtc, crtc->state); > > - crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL); > > - if (crtc->state) > > - __drm_atomic_helper_crtc_reset(crtc, crtc->state); > > + > > + vc4_crtc_state = kzalloc(sizeof(*vc4_crtc_state), GFP_KERNEL); > > + if (!vc4_crtc_state) { > > + crtc->state = NULL; > > + return; > > + } > > + > > + __drm_atomic_helper_crtc_reset(crtc, &vc4_crtc_state->base); > > } > > > > static const struct drm_crtc_funcs vc4_crtc_funcs = { > > -- > > 2.26.2 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code 2020-09-23 14:59 ` Dave Stevenson @ 2020-09-25 11:38 ` Maxime Ripard -1 siblings, 0 replies; 16+ messages in thread From: Maxime Ripard @ 2020-09-25 11:38 UTC (permalink / raw) To: Dave Stevenson Cc: Tim Gover, DRI Development, Eric Anholt, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel Hi Dave, On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote: > Hi Maxime > > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state > > embeds drm_crtc_state as its first member, and therefore can be safely > > casted. > > s/casted/cast > > > However, this is pretty fragile especially since there's no check for this > > in place, and we're going to need to access vc4_crtc_state member at reset > > so this looks like a good occasion to make it more robust. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Based on the issue I perceived with the previous patch, I'm happy. I > haven't thought about how the framework handles losing the state, but > that's not the driver's problem. > > There is still an implicit assumption that drm_crtc_state is the first > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To > make it even more robust that could be a container_of instead. I > haven't checked for any other places that make the assumption though. Good catch, I'll send another patch to fix it > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Does it apply to the second patch as well? Thanks! Maxime _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code @ 2020-09-25 11:38 ` Maxime Ripard 0 siblings, 0 replies; 16+ messages in thread From: Maxime Ripard @ 2020-09-25 11:38 UTC (permalink / raw) To: Dave Stevenson Cc: Tim Gover, DRI Development, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel Hi Dave, On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote: > Hi Maxime > > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state > > embeds drm_crtc_state as its first member, and therefore can be safely > > casted. > > s/casted/cast > > > However, this is pretty fragile especially since there's no check for this > > in place, and we're going to need to access vc4_crtc_state member at reset > > so this looks like a good occasion to make it more robust. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Based on the issue I perceived with the previous patch, I'm happy. I > haven't thought about how the framework handles losing the state, but > that's not the driver's problem. > > There is still an implicit assumption that drm_crtc_state is the first > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To > make it even more robust that could be a container_of instead. I > haven't checked for any other places that make the assumption though. Good catch, I'll send another patch to fix it > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Does it apply to the second patch as well? Thanks! Maxime _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code 2020-09-25 11:38 ` Maxime Ripard @ 2020-09-25 11:47 ` Dave Stevenson -1 siblings, 0 replies; 16+ messages in thread From: Dave Stevenson @ 2020-09-25 11:47 UTC (permalink / raw) To: Maxime Ripard Cc: Tim Gover, DRI Development, Eric Anholt, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel On Fri, 25 Sep 2020 at 12:38, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Dave, > > On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote: > > Hi Maxime > > > > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state > > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state > > > embeds drm_crtc_state as its first member, and therefore can be safely > > > casted. > > > > s/casted/cast > > > > > However, this is pretty fragile especially since there's no check for this > > > in place, and we're going to need to access vc4_crtc_state member at reset > > > so this looks like a good occasion to make it more robust. > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > Based on the issue I perceived with the previous patch, I'm happy. I > > haven't thought about how the framework handles losing the state, but > > that's not the driver's problem. > > > > There is still an implicit assumption that drm_crtc_state is the first > > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To > > make it even more robust that could be a container_of instead. I > > haven't checked for any other places that make the assumption though. > > Good catch, I'll send another patch to fix it > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Does it apply to the second patch as well? No. I got another interrupt before I'd refreshed my memory over what the second patch was doing and responding to it. I'll do that now (I don't think it changed much from v1) Dave. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code @ 2020-09-25 11:47 ` Dave Stevenson 0 siblings, 0 replies; 16+ messages in thread From: Dave Stevenson @ 2020-09-25 11:47 UTC (permalink / raw) To: Maxime Ripard Cc: Tim Gover, DRI Development, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel On Fri, 25 Sep 2020 at 12:38, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Dave, > > On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote: > > Hi Maxime > > > > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state > > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state > > > embeds drm_crtc_state as its first member, and therefore can be safely > > > casted. > > > > s/casted/cast > > > > > However, this is pretty fragile especially since there's no check for this > > > in place, and we're going to need to access vc4_crtc_state member at reset > > > so this looks like a good occasion to make it more robust. > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > Based on the issue I perceived with the previous patch, I'm happy. I > > haven't thought about how the framework handles losing the state, but > > that's not the driver's problem. > > > > There is still an implicit assumption that drm_crtc_state is the first > > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To > > make it even more robust that could be a container_of instead. I > > haven't checked for any other places that make the assumption though. > > Good catch, I'll send another patch to fix it > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Does it apply to the second patch as well? No. I got another interrupt before I'd refreshed my memory over what the second patch was doing and responding to it. I'll do that now (I don't think it changed much from v1) Dave. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code 2020-09-23 14:59 ` Dave Stevenson @ 2020-09-25 14:57 ` Maxime Ripard -1 siblings, 0 replies; 16+ messages in thread From: Maxime Ripard @ 2020-09-25 14:57 UTC (permalink / raw) To: Dave Stevenson Cc: Tim Gover, DRI Development, Eric Anholt, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 1415 bytes --] Hi, On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote: > Hi Maxime > > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state > > embeds drm_crtc_state as its first member, and therefore can be safely > > casted. > > s/casted/cast > > > However, this is pretty fragile especially since there's no check for this > > in place, and we're going to need to access vc4_crtc_state member at reset > > so this looks like a good occasion to make it more robust. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Based on the issue I perceived with the previous patch, I'm happy. I > haven't thought about how the framework handles losing the state, but > that's not the driver's problem. > > There is still an implicit assumption that drm_crtc_state is the first > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To > make it even more robust that could be a container_of instead. I > haven't checked for any other places that make the assumption though. > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Applied 1 and 2, with the typo fixed (and the fixes tag suggested by Daniel) Maxime [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code @ 2020-09-25 14:57 ` Maxime Ripard 0 siblings, 0 replies; 16+ messages in thread From: Maxime Ripard @ 2020-09-25 14:57 UTC (permalink / raw) To: Dave Stevenson Cc: Tim Gover, DRI Development, bcm-kernel-feedback-list, linux-rpi-kernel, Phil Elwell, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 1415 bytes --] Hi, On Wed, Sep 23, 2020 at 03:59:04PM +0100, Dave Stevenson wrote: > Hi Maxime > > On Wed, 23 Sep 2020 at 09:40, Maxime Ripard <maxime@cerno.tech> wrote: > > > > The current CRTC state reset hook in vc4 allocates a vc4_crtc_state > > structure as a drm_crtc_state, and relies on the fact that vc4_crtc_state > > embeds drm_crtc_state as its first member, and therefore can be safely > > casted. > > s/casted/cast > > > However, this is pretty fragile especially since there's no check for this > > in place, and we're going to need to access vc4_crtc_state member at reset > > so this looks like a good occasion to make it more robust. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Based on the issue I perceived with the previous patch, I'm happy. I > haven't thought about how the framework handles losing the state, but > that's not the driver's problem. > > There is still an implicit assumption that drm_crtc_state is the first > member from the implementation of to_vc4_crtc_state in vc4_drv.h. To > make it even more robust that could be a container_of instead. I > haven't checked for any other places that make the assumption though. > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Applied 1 and 2, with the typo fixed (and the fixes tag suggested by Daniel) Maxime [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-09-28 7:07 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-23 8:40 [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code Maxime Ripard 2020-09-23 8:40 ` Maxime Ripard 2020-09-23 8:40 ` [PATCH v2 2/2] drm/vc4: crtc: Keep the previously assigned HVS FIFO Maxime Ripard 2020-09-23 8:40 ` Maxime Ripard 2020-09-25 12:05 ` Dave Stevenson 2020-09-25 12:05 ` Dave Stevenson 2020-09-23 14:59 ` [PATCH v2 1/2] drm/vc4: crtc: Rework a bit the CRTC state code Dave Stevenson 2020-09-23 14:59 ` Dave Stevenson 2020-09-23 15:41 ` Daniel Vetter 2020-09-23 15:41 ` Daniel Vetter 2020-09-25 11:38 ` Maxime Ripard 2020-09-25 11:38 ` Maxime Ripard 2020-09-25 11:47 ` Dave Stevenson 2020-09-25 11:47 ` Dave Stevenson 2020-09-25 14:57 ` Maxime Ripard 2020-09-25 14:57 ` Maxime Ripard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.