| Message ID | 20200917121623.42023-1-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: hvs: Pull the state of all the CRTCs prior to PV muxing | expand |
Hi Maxime, On 9/17/20 9:16 PM, Maxime Ripard wrote: > The vc4 display engine has a first controller called the HVS that will > perform the composition of the planes. That HVS has 3 FIFOs and can > therefore compose planes for up to three outputs. The timings part is > generated through a component called the Pixel Valve, and the BCM2711 has 6 > of them. > > Thus, the HVS has some bits to control which FIFO gets output to which > Pixel Valve. The current code supports that muxing by looking at all the > CRTCs in a new DRM atomic state in atomic_check, and given the set of > contraints that we have, assigns FIFOs to CRTCs or reject the mode > entirely. The actual muxing will occur during atomic_commit. > > However, that doesn't work if only a fraction of the CRTCs' state is > updated in that state, since it will ignore the CRTCs that are kept running > unmodified, and will thus unassign its associated FIFO, and later disable > it. > > In order to make the code work as expected, let's pull the CRTC state of > all the enabled CRTC in our atomic_check so that we can operate on all the > running CRTCs, no matter whether they are affected by the new state or not. > > Cc: Hoegeun Kwon <hoegeun.kwon@samsung.com> > Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Thanks for the quick patch and detailed explanation. Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com> Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com> Best regards, Hoegeun > --- > drivers/gpu/drm/vc4/vc4_kms.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 16e233e1406e..af3ee3dcdab6 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -620,6 +620,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > struct drm_crtc *crtc; > int i, ret; > > + /* > + * Since the HVS FIFOs are shared across all the pixelvalves and > + * the TXP (and thus all the CRTCs), we need to pull the current > + * state of all the enabled CRTCs so that an update to a single > + * CRTC still keeps the previous FIFOs enabled and assigned to > + * the same CRTCs, instead of evaluating only the CRTC being > + * modified. > + */ > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + if (!crtc->state->enable) > + continue; > + > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > + if (IS_ERR(crtc_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);
Hi Maxime On Thu, 17 Sep 2020 at 13:16, Maxime Ripard <maxime@cerno.tech> wrote: > > The vc4 display engine has a first controller called the HVS that will > perform the composition of the planes. That HVS has 3 FIFOs and can > therefore compose planes for up to three outputs. The timings part is > generated through a component called the Pixel Valve, and the BCM2711 has 6 > of them. > > Thus, the HVS has some bits to control which FIFO gets output to which > Pixel Valve. The current code supports that muxing by looking at all the > CRTCs in a new DRM atomic state in atomic_check, and given the set of > contraints that we have, assigns FIFOs to CRTCs or reject the mode s/contraints/constraints > entirely. The actual muxing will occur during atomic_commit. > > However, that doesn't work if only a fraction of the CRTCs' state is > updated in that state, since it will ignore the CRTCs that are kept running > unmodified, and will thus unassign its associated FIFO, and later disable > it. Yup, that would surely mess things up :) > In order to make the code work as expected, let's pull the CRTC state of > all the enabled CRTC in our atomic_check so that we can operate on all the > running CRTCs, no matter whether they are affected by the new state or not. > > Cc: Hoegeun Kwon <hoegeun.kwon@samsung.com> > 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> Thanks Dave > --- > drivers/gpu/drm/vc4/vc4_kms.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 16e233e1406e..af3ee3dcdab6 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -620,6 +620,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > struct drm_crtc *crtc; > int i, ret; > > + /* > + * Since the HVS FIFOs are shared across all the pixelvalves and > + * the TXP (and thus all the CRTCs), we need to pull the current > + * state of all the enabled CRTCs so that an update to a single > + * CRTC still keeps the previous FIFOs enabled and assigned to > + * the same CRTCs, instead of evaluating only the CRTC being > + * modified. > + */ > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + if (!crtc->state->enable) > + continue; > + > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > + if (IS_ERR(crtc_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); > -- > 2.26.2 >
On Fri, Sep 18, 2020 at 03:52:55PM +0100, Dave Stevenson wrote: > Hi Maxime > > On Thu, 17 Sep 2020 at 13:16, Maxime Ripard <maxime@cerno.tech> wrote: > > > > The vc4 display engine has a first controller called the HVS that will > > perform the composition of the planes. That HVS has 3 FIFOs and can > > therefore compose planes for up to three outputs. The timings part is > > generated through a component called the Pixel Valve, and the BCM2711 has 6 > > of them. > > > > Thus, the HVS has some bits to control which FIFO gets output to which > > Pixel Valve. The current code supports that muxing by looking at all the > > CRTCs in a new DRM atomic state in atomic_check, and given the set of > > contraints that we have, assigns FIFOs to CRTCs or reject the mode > > s/contraints/constraints Oops, thanks I've fixed it while applying it Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 16e233e1406e..af3ee3dcdab6 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -620,6 +620,23 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) struct drm_crtc *crtc; int i, ret; + /* + * Since the HVS FIFOs are shared across all the pixelvalves and + * the TXP (and thus all the CRTCs), we need to pull the current + * state of all the enabled CRTCs so that an update to a single + * CRTC still keeps the previous FIFOs enabled and assigned to + * the same CRTCs, instead of evaluating only the CRTC being + * modified. + */ + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + if (!crtc->state->enable) + continue; + + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_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);
The vc4 display engine has a first controller called the HVS that will perform the composition of the planes. That HVS has 3 FIFOs and can therefore compose planes for up to three outputs. The timings part is generated through a component called the Pixel Valve, and the BCM2711 has 6 of them. Thus, the HVS has some bits to control which FIFO gets output to which Pixel Valve. The current code supports that muxing by looking at all the CRTCs in a new DRM atomic state in atomic_check, and given the set of contraints that we have, assigns FIFOs to CRTCs or reject the mode entirely. The actual muxing will occur during atomic_commit. However, that doesn't work if only a fraction of the CRTCs' state is updated in that state, since it will ignore the CRTCs that are kept running unmodified, and will thus unassign its associated FIFO, and later disable it. In order to make the code work as expected, let's pull the CRTC state of all the enabled CRTC in our atomic_check so that we can operate on all the running CRTCs, no matter whether they are affected by the new state or not. Cc: Hoegeun Kwon <hoegeun.kwon@samsung.com> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/vc4/vc4_kms.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)