Hi,
This is a follow-up of the series here: https://lore.kernel.org/all/20210924135530.1036564-1-maxime@cerno.tech/
and the discussion that occured here: https://lore.kernel.org/all/YWgteNaNeaS9uWDe@phenom.ffwll.local/
The original series aimed at getting rid of the encoder->crtc pointer usage in the vc4 HDMI driver, some regression was noticed and the following discussion pointed out that we were doing a fair number of KMS state access outside of the mode set path, which is disallowed and unsafe.
We already have a bug that was a result of that access which is fixed in the second patch, but other instabilities might have occured without being reported.
That series thus removes all those accesses, and add some locking in the process.
Sudip, since that series changed very significantly since you last tested it, could you give it a test run on the Codethink farm?
Let me know what you think, Maxime
Maxime Ripard (9): drm/vc4: crtc: Drop feed_txp from state drm/vc4: Fix non-blocking commit getting stuck forever drm/vc4: crtc: Copy assigned channel to the CRTC drm/vc4: hdmi: Add a spinlock to protect register access drm/vc4: hdmi: Use a mutex to prevent concurrent framework access drm/vc4: hdmi: Prevent access to crtc->state outside of KMS drm/vc4: hdmi: Check the device state in prepare() drm/vc4: hdmi: Introduce an output_enabled flag drm/vc4: hdmi: Introduce a scdc_enabled flag
drivers/gpu/drm/vc4/vc4_crtc.c | 12 +- drivers/gpu/drm/vc4/vc4_drv.h | 29 +- drivers/gpu/drm/vc4/vc4_hdmi.c | 415 +++++++++++++++++++++++++--- drivers/gpu/drm/vc4/vc4_hdmi.h | 37 +++ drivers/gpu/drm/vc4/vc4_hdmi_phy.c | 37 +++ drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 2 + drivers/gpu/drm/vc4/vc4_hvs.c | 26 +- drivers/gpu/drm/vc4/vc4_kms.c | 3 +- drivers/gpu/drm/vc4/vc4_txp.c | 4 +- 9 files changed, 509 insertions(+), 56 deletions(-)
Accessing the crtc->state pointer from outside the modesetting context is not allowed. We thus need to copy whatever we need from the KMS state to our structure in order to access it.
In VC4, a number of users of that pointers have crept in over the years, the first one being whether or not the downstream controller of the pixelvalve is our writeback controller.
Fortunately for us, Since commit 39fcb2808376 ("drm/vc4: txp: Turn the TXP into a CRTC of its own") this is no longer something that can change from one commit to the other and is hardcoded.
Let's set this flag in struct vc4_crtc if we happen to be the TXP, and drop the flag from our private state structure.
Link: https://lore.kernel.org/all/YWgteNaNeaS9uWDe@phenom.ffwll.local/ Fixes: 008095e065a8 ("drm/vc4: Add support for the transposer block") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_crtc.c | 3 +-- drivers/gpu/drm/vc4/vc4_drv.h | 6 +++++- drivers/gpu/drm/vc4/vc4_hvs.c | 7 +++---- drivers/gpu/drm/vc4/vc4_kms.c | 3 ++- drivers/gpu/drm/vc4/vc4_txp.c | 3 +-- 5 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index c0df11e5fcf2..b90187d2c819 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -715,7 +715,7 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc) spin_lock_irqsave(&dev->event_lock, flags); if (vc4_crtc->event && (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)) || - vc4_state->feed_txp)) { + vc4_crtc->feeds_txp)) { drm_crtc_send_vblank_event(crtc, vc4_crtc->event); vc4_crtc->event = NULL; drm_crtc_vblank_put(crtc); @@ -893,7 +893,6 @@ struct drm_crtc_state *vc4_crtc_duplicate_state(struct drm_crtc *crtc) return NULL;
old_vc4_state = to_vc4_crtc_state(crtc->state); - vc4_state->feed_txp = old_vc4_state->feed_txp; vc4_state->margins = old_vc4_state->margins; vc4_state->assigned_channel = old_vc4_state->assigned_channel;
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index ef73e0aaf726..3c69b89363cb 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -495,6 +495,11 @@ struct vc4_crtc { struct drm_pending_vblank_event *event;
struct debugfs_regset32 regset; + + /** + * @feeds_txp: True if the CRTC feeds our writeback controller. + */ + bool feeds_txp; };
static inline struct vc4_crtc * @@ -521,7 +526,6 @@ struct vc4_crtc_state { struct drm_crtc_state base; /* Dlist area for this CRTC configuration. */ struct drm_mm_node mm; - bool feed_txp; bool txp_armed; unsigned int assigned_channel;
diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index c239045e05d6..9ddaee6b368d 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -375,7 +375,7 @@ static void vc4_hvs_update_dlist(struct drm_crtc *crtc)
spin_lock_irqsave(&dev->event_lock, flags);
- if (!vc4_state->feed_txp || vc4_state->txp_armed) { + if (!vc4_crtc->feeds_txp || vc4_state->txp_armed) { vc4_crtc->event = crtc->state->event; crtc->state->event = NULL; } @@ -395,10 +395,9 @@ void vc4_hvs_atomic_enable(struct drm_crtc *crtc, { struct drm_device *dev = crtc->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); - struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); - struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(new_crtc_state); struct drm_display_mode *mode = &crtc->state->adjusted_mode; - bool oneshot = vc4_state->feed_txp; + struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); + bool oneshot = vc4_crtc->feeds_txp;
vc4_hvs_update_dlist(crtc); vc4_hvs_init_channel(vc4, crtc, mode, oneshot); diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index f0b3e4cf5bce..028f19f7a5f8 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -233,6 +233,7 @@ static void vc4_hvs_pv_muxing_commit(struct vc4_dev *vc4, unsigned int i;
for_each_new_crtc_in_state(state, crtc, crtc_state, i) { + struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state); u32 dispctrl; u32 dsp3_mux; @@ -253,7 +254,7 @@ static void vc4_hvs_pv_muxing_commit(struct vc4_dev *vc4, * TXP IP, and we need to disable the FIFO2 -> pixelvalve1 * route. */ - if (vc4_state->feed_txp) + if (vc4_crtc->feeds_txp) dsp3_mux = VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX); else dsp3_mux = VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX); diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 2fc7f4b5fa09..26eda7542f74 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -391,7 +391,6 @@ static int vc4_txp_atomic_check(struct drm_crtc *crtc, { struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); - struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc_state); int ret;
ret = vc4_hvs_atomic_check(crtc, state); @@ -399,7 +398,6 @@ static int vc4_txp_atomic_check(struct drm_crtc *crtc, return ret;
crtc_state->no_vblank = true; - vc4_state->feed_txp = true;
return 0; } @@ -482,6 +480,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
vc4_crtc->pdev = pdev; vc4_crtc->data = &vc4_txp_crtc_data; + vc4_crtc->feeds_txp = true;
txp->pdev = pdev;
In some situation, we can end up being stuck on a non-blocking that went through properly.
The situation that seems to trigger it reliably is to first start a non-blocking commit, and then right after, and before we had any vblank interrupt), start a blocking commit.
This will lead to the first commit workqueue to be scheduled, setup the display, while the second commit is waiting for the first one to be completed.
The vblank interrupt will then be raised, vc4_crtc_handle_vblank() will run and will compare the active dlist in the HVS channel to the one associated with the crtc->state.
However, at that point, the second commit is waiting using drm_atomic_helper_wait_for_dependencies that occurs after drm_atomic_helper_swap_state has been called, so crtc->state points to the second commit state. vc4_crtc_handle_vblank() will compare the two dlist addresses and since they don't match will ignore the interrupt.
The vblank event will never be reported, and the first and second commit will wait for the first commit completion until they timeout.
The underlying reason is that it was never safe to do so. Indeed, accessing the ->state pointer access synchronization is based on ownership guarantees that can only occur within the functions and hooks defined as part of the KMS framework, and obviously the irq handler isn't one of them. The rework to move to generic helpers only uncovered the underlying issue.
However, since the code path between drm_atomic_helper_wait_for_dependencies() and drm_atomic_helper_wait_for_vblanks() is serialised and we can't get two commits in that path at the same time, we can work around this issue by setting a variable associated to struct drm_crtc to the dlist we expect, and then using it from the vc4_crtc_handle_vblank() function.
Since that state is shared with the modesetting path, we also need to introduce a spinlock to protect the code shared between the interrupt handler and the modesetting path, protecting only our new variable for now.
Link: https://lore.kernel.org/all/YWgteNaNeaS9uWDe@phenom.ffwll.local/ Fixes: 56d1fe0979dc ("drm/vc4: Make pageflip completion handling more robust.") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_crtc.c | 5 ++++- drivers/gpu/drm/vc4/vc4_drv.h | 14 ++++++++++++++ drivers/gpu/drm/vc4/vc4_hvs.c | 7 +++++-- 3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index b90187d2c819..98de8b265220 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -713,8 +713,9 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc) unsigned long flags;
spin_lock_irqsave(&dev->event_lock, flags); + spin_lock(&vc4_crtc->irq_lock); if (vc4_crtc->event && - (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)) || + (vc4_crtc->current_dlist == HVS_READ(SCALER_DISPLACTX(chan)) || vc4_crtc->feeds_txp)) { drm_crtc_send_vblank_event(crtc, vc4_crtc->event); vc4_crtc->event = NULL; @@ -728,6 +729,7 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc) */ vc4_hvs_unmask_underrun(dev, chan); } + spin_unlock(&vc4_crtc->irq_lock); spin_unlock_irqrestore(&dev->event_lock, flags); }
@@ -1127,6 +1129,7 @@ int vc4_crtc_init(struct drm_device *drm, struct vc4_crtc *vc4_crtc, return PTR_ERR(primary_plane); }
+ spin_lock_init(&vc4_crtc->irq_lock); drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL, crtc_funcs, NULL); drm_crtc_helper_add(crtc, crtc_helper_funcs); diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 3c69b89363cb..6d2480abcf08 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -500,6 +500,20 @@ struct vc4_crtc { * @feeds_txp: True if the CRTC feeds our writeback controller. */ bool feeds_txp; + + /** + * @irq_lock: Spinlock protecting the resources shared between + * the atomic code and our vblank handler. + */ + spinlock_t irq_lock; + + /** + * @current_dlist: Start offset of the display list currently + * set in the HVS for that CRTC. Protected by @irq_lock, and + * copied in vc4_hvs_update_dlist() for the CRTC interrupt + * handler to have access to that value. + */ + unsigned int current_dlist; };
static inline struct vc4_crtc * diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index 9ddaee6b368d..f8ed0f6a57e0 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -365,10 +365,9 @@ static void vc4_hvs_update_dlist(struct drm_crtc *crtc) struct vc4_dev *vc4 = to_vc4_dev(dev); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); + unsigned long flags;
if (crtc->state->event) { - unsigned long flags; - crtc->state->event->pipe = drm_crtc_index(crtc);
WARN_ON(drm_crtc_vblank_get(crtc) != 0); @@ -388,6 +387,10 @@ static void vc4_hvs_update_dlist(struct drm_crtc *crtc) HVS_WRITE(SCALER_DISPLISTX(vc4_state->assigned_channel), vc4_state->mm.start); } + + spin_lock_irqsave(&vc4_crtc->irq_lock, flags); + vc4_crtc->current_dlist = vc4_state->mm.start; + spin_unlock_irqrestore(&vc4_crtc->irq_lock, flags); }
void vc4_hvs_atomic_enable(struct drm_crtc *crtc,
Accessing the crtc->state pointer from outside the modesetting context is not allowed. We thus need to copy whatever we need from the KMS state to our structure in order to access it.
In VC4, a number of users of that pointers have crept in over the years, and the previous commits removed them all but the HVS channel a CRTC has been assigned.
Let's move this channel in struct vc4_crtc at atomic_begin() time, drop it from our private state structure, and remove our use of crtc->state from our vblank handler entirely.
Link: https://lore.kernel.org/all/YWgteNaNeaS9uWDe@phenom.ffwll.local/ Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_crtc.c | 4 ++-- drivers/gpu/drm/vc4/vc4_drv.h | 9 +++++++++ drivers/gpu/drm/vc4/vc4_hvs.c | 12 ++++++++++++ drivers/gpu/drm/vc4/vc4_txp.c | 1 + 4 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 98de8b265220..e3ed52d96f42 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -708,8 +708,7 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc) struct drm_crtc *crtc = &vc4_crtc->base; struct drm_device *dev = crtc->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); - struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); - u32 chan = vc4_state->assigned_channel; + u32 chan = vc4_crtc->current_hvs_channel; unsigned long flags;
spin_lock_irqsave(&dev->event_lock, flags); @@ -955,6 +954,7 @@ static const struct drm_crtc_funcs vc4_crtc_funcs = { static const struct drm_crtc_helper_funcs vc4_crtc_helper_funcs = { .mode_valid = vc4_crtc_mode_valid, .atomic_check = vc4_crtc_atomic_check, + .atomic_begin = vc4_hvs_atomic_begin, .atomic_flush = vc4_hvs_atomic_flush, .atomic_enable = vc4_crtc_atomic_enable, .atomic_disable = vc4_crtc_atomic_disable, diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 6d2480abcf08..4b550ebd9572 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -514,6 +514,14 @@ struct vc4_crtc { * handler to have access to that value. */ unsigned int current_dlist; + + /** + * @current_hvs_channel: HVS channel currently assigned to the + * CRTC. Protected by @irq_lock, and copied in + * vc4_hvs_atomic_begin() for the CRTC interrupt handler to have + * access to that value. + */ + unsigned int current_hvs_channel; };
static inline struct vc4_crtc * @@ -926,6 +934,7 @@ extern struct platform_driver vc4_hvs_driver; void vc4_hvs_stop_channel(struct drm_device *dev, unsigned int output); int vc4_hvs_get_fifo_from_output(struct drm_device *dev, unsigned int output); int vc4_hvs_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state); +void vc4_hvs_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state); void vc4_hvs_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state); void vc4_hvs_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state); void vc4_hvs_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state); diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index f8ed0f6a57e0..604933e20e6a 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -393,6 +393,18 @@ static void vc4_hvs_update_dlist(struct drm_crtc *crtc) spin_unlock_irqrestore(&vc4_crtc->irq_lock, flags); }
+void vc4_hvs_atomic_begin(struct drm_crtc *crtc, + struct drm_atomic_state *state) +{ + struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); + struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); + unsigned long flags; + + spin_lock_irqsave(&vc4_crtc->irq_lock, flags); + vc4_crtc->current_hvs_channel = vc4_state->assigned_channel; + spin_unlock_irqrestore(&vc4_crtc->irq_lock, flags); +} + void vc4_hvs_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state) { diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 26eda7542f74..9809ca3e2945 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -435,6 +435,7 @@ static void vc4_txp_atomic_disable(struct drm_crtc *crtc,
static const struct drm_crtc_helper_funcs vc4_txp_crtc_helper_funcs = { .atomic_check = vc4_txp_atomic_check, + .atomic_begin = vc4_hvs_atomic_begin, .atomic_flush = vc4_hvs_atomic_flush, .atomic_enable = vc4_txp_atomic_enable, .atomic_disable = vc4_txp_atomic_disable,
The vc4 HDMI driver has multiple path shared between the CEC, ALSA and KMS frameworks, plus two interrupt handlers (CEC and hotplug) that will read and modify a number of registers.
Even though not bug has been reported so far, it's definitely unsafe, so let's just add a spinlock to protect the register access of the HDMI controller.
Fixes: c8b75bca92cb ("drm/vc4: Add KMS support for Raspberry Pi.") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 202 ++++++++++++++++++++++++++-- drivers/gpu/drm/vc4/vc4_hdmi.h | 5 + drivers/gpu/drm/vc4/vc4_hdmi_phy.c | 37 +++++ drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 2 + 4 files changed, 236 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 8cc84395f4cb..484450831483 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -118,6 +118,10 @@ static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
static void vc4_hdmi_reset(struct vc4_hdmi *vc4_hdmi) { + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + HDMI_WRITE(HDMI_M_CTL, VC4_HD_M_SW_RST); udelay(1); HDMI_WRITE(HDMI_M_CTL, 0); @@ -129,24 +133,36 @@ static void vc4_hdmi_reset(struct vc4_hdmi *vc4_hdmi) VC4_HDMI_SW_RESET_FORMAT_DETECT);
HDMI_WRITE(HDMI_SW_RESET_CONTROL, 0); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi) { + unsigned long flags; + reset_control_reset(vc4_hdmi->reset);
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + HDMI_WRITE(HDMI_DVP_CTL, 0);
HDMI_WRITE(HDMI_CLOCK_STOP, HDMI_READ(HDMI_CLOCK_STOP) | VC4_DVP_HT_CLOCK_STOP_PIXEL); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
#ifdef CONFIG_DRM_VC4_HDMI_CEC static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) { + unsigned long cec_rate = clk_get_rate(vc4_hdmi->cec_clock); + unsigned long flags; u16 clk_cnt; u32 value;
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + value = HDMI_READ(HDMI_CEC_CNTRL_1); value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK;
@@ -154,9 +170,11 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) * Set the clock divider: the hsm_clock rate and this divider * setting will give a 40 kHz CEC clock. */ - clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ; + clk_cnt = cec_rate / CEC_CLOCK_FREQ; value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT; HDMI_WRITE(HDMI_CEC_CNTRL_1, value); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); } #else static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {} @@ -175,8 +193,16 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) connected = true; } else if (drm_probe_ddc(vc4_hdmi->ddc)) { connected = true; - } else if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) { - connected = true; + } else { + unsigned long flags; + u32 hotplug; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + hotplug = HDMI_READ(HDMI_HOTPLUG); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + if (hotplug & VC4_HDMI_HOTPLUG_CONNECTED) + connected = true; }
if (connected) { @@ -369,9 +395,12 @@ static int vc4_hdmi_stop_packet(struct drm_encoder *encoder, { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); u32 packet_id = type - 0x80; + unsigned long flags;
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_RAM_PACKET_CONFIG, HDMI_READ(HDMI_RAM_PACKET_CONFIG) & ~BIT(packet_id)); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
if (!poll) return 0; @@ -391,6 +420,7 @@ static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder, void __iomem *base = __vc4_hdmi_get_field_base(vc4_hdmi, ram_packet_start->reg); uint8_t buffer[VC4_HDMI_PACKET_STRIDE]; + unsigned long flags; ssize_t len, i; int ret;
@@ -408,6 +438,8 @@ static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder, return; }
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + for (i = 0; i < len; i += 7) { writel(buffer[i + 0] << 0 | buffer[i + 1] << 8 | @@ -425,6 +457,9 @@ static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder,
HDMI_WRITE(HDMI_RAM_PACKET_CONFIG, HDMI_READ(HDMI_RAM_PACKET_CONFIG) | BIT(packet_id)); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + ret = wait_for((HDMI_READ(HDMI_RAM_PACKET_STATUS) & BIT(packet_id)), 100); if (ret) @@ -544,6 +579,7 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) { struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + unsigned long flags;
if (!vc4_hdmi_supports_scrambling(encoder, mode)) return; @@ -554,8 +590,10 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, true); drm_scdc_set_scrambling(vc4_hdmi->ddc, true);
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_SCRAMBLER_CTL, HDMI_READ(HDMI_SCRAMBLER_CTL) | VC5_HDMI_SCRAMBLER_CTL_ENABLE); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
queue_delayed_work(system_wq, &vc4_hdmi->scrambling_work, msecs_to_jiffies(SCRAMBLING_POLLING_DELAY_MS)); @@ -565,6 +603,7 @@ static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); struct drm_crtc *crtc = encoder->crtc; + unsigned long flags;
/* * At boot, encoder->crtc will be NULL. Since we don't know the @@ -580,8 +619,10 @@ static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder) if (delayed_work_pending(&vc4_hdmi->scrambling_work)) cancel_delayed_work_sync(&vc4_hdmi->scrambling_work);
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_SCRAMBLER_CTL, HDMI_READ(HDMI_SCRAMBLER_CTL) & ~VC5_HDMI_SCRAMBLER_CTL_ENABLE); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
drm_scdc_set_scrambling(vc4_hdmi->ddc, false); drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, false); @@ -607,15 +648,23 @@ static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
HDMI_WRITE(HDMI_RAM_PACKET_CONFIG, 0);
HDMI_WRITE(HDMI_VID_CTL, HDMI_READ(HDMI_VID_CTL) | VC4_HD_VID_CTL_CLRRGB);
+ spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + mdelay(1);
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_VID_CTL, HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + vc4_hdmi_disable_scrambling(encoder); }
@@ -623,10 +672,13 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + unsigned long flags; int ret;
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_VID_CTL, HDMI_READ(HDMI_VID_CTL) | VC4_HD_VID_CTL_BLANKPIX); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
if (vc4_hdmi->variant->phy_disable) vc4_hdmi->variant->phy_disable(vc4_hdmi); @@ -645,8 +697,11 @@ static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder)
static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, bool enable) { + unsigned long flags; u32 csc_ctl;
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + csc_ctl = VC4_SET_FIELD(VC4_HD_CSC_CTL_ORDER_BGR, VC4_HD_CSC_CTL_ORDER);
@@ -676,14 +731,19 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, bool enable)
/* The RGB order applies even when CSC is disabled. */ HDMI_WRITE(HDMI_CSC_CTL, csc_ctl); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, bool enable) { + unsigned long flags; u32 csc_ctl;
csc_ctl = 0x07; /* RGB_CONVERT_MODE = custom matrix, || USE_RGB_TO_YCBCR */
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + if (enable) { /* CEA VICs other than #1 requre limited range RGB * output unless overridden by an AVI infoframe. @@ -715,6 +775,8 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, bool enable) }
HDMI_WRITE(HDMI_CSC_CTL, csc_ctl); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, @@ -738,6 +800,9 @@ static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, mode->crtc_vsync_end - interlaced, VC4_HDMI_VERTB_VBP)); + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
HDMI_WRITE(HDMI_HORZA, (vsync_pos ? VC4_HDMI_HORZA_VPOS : 0) | @@ -761,6 +826,8 @@ static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
HDMI_WRITE(HDMI_VERTB0, vertb_even); HDMI_WRITE(HDMI_VERTB1, vertb); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, @@ -784,10 +851,13 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, mode->crtc_vsync_end - interlaced, VC4_HDMI_VERTB_VBP)); + unsigned long flags; unsigned char gcp; bool gcp_en; u32 reg;
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + HDMI_WRITE(HDMI_VEC_INTERFACE_XBAR, 0x354021); HDMI_WRITE(HDMI_HORZA, (vsync_pos ? VC5_HDMI_HORZA_VPOS : 0) | @@ -846,13 +916,18 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, HDMI_WRITE(HDMI_GCP_CONFIG, reg);
HDMI_WRITE(HDMI_CLOCK_STOP, 0); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
static void vc4_hdmi_recenter_fifo(struct vc4_hdmi *vc4_hdmi) { + unsigned long flags; u32 drift; int ret;
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + drift = HDMI_READ(HDMI_FIFO_CTL); drift &= VC4_HDMI_FIFO_VALID_WRITE_MASK;
@@ -860,12 +935,20 @@ static void vc4_hdmi_recenter_fifo(struct vc4_hdmi *vc4_hdmi) drift & ~VC4_HDMI_FIFO_CTL_RECENTER); HDMI_WRITE(HDMI_FIFO_CTL, drift | VC4_HDMI_FIFO_CTL_RECENTER); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + usleep_range(1000, 1100); + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + HDMI_WRITE(HDMI_FIFO_CTL, drift & ~VC4_HDMI_FIFO_CTL_RECENTER); HDMI_WRITE(HDMI_FIFO_CTL, drift | VC4_HDMI_FIFO_CTL_RECENTER);
+ spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + ret = wait_for(HDMI_READ(HDMI_FIFO_CTL) & VC4_HDMI_FIFO_CTL_RECENTER_DONE, 1); WARN_ONCE(ret, "Timeout waiting for " @@ -899,6 +982,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); unsigned long pixel_rate = vc4_conn_state->pixel_rate; unsigned long bvb_rate, hsm_rate; + unsigned long flags; int ret;
/* @@ -967,11 +1051,15 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, if (vc4_hdmi->variant->phy_init) vc4_hdmi->variant->phy_init(vc4_hdmi, vc4_conn_state);
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + HDMI_WRITE(HDMI_SCHEDULER_CONTROL, HDMI_READ(HDMI_SCHEDULER_CONTROL) | VC4_HDMI_SCHEDULER_CONTROL_MANUAL_FORMAT | VC4_HDMI_SCHEDULER_CONTROL_IGNORE_VSYNC_PREDICTS);
+ spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + if (vc4_hdmi->variant->set_timings) vc4_hdmi->variant->set_timings(vc4_hdmi, conn_state, mode);
@@ -991,6 +1079,7 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + unsigned long flags;
if (vc4_encoder->hdmi_monitor && drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED) { @@ -1005,7 +1094,9 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, vc4_encoder->limited_rgb_range = false; }
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_FIFO_CTL, VC4_HDMI_FIFO_CTL_MASTER_SLAVE_N); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, @@ -1016,8 +1107,11 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC; bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC; + unsigned long flags; int ret;
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + HDMI_WRITE(HDMI_VID_CTL, VC4_HD_VID_CTL_ENABLE | VC4_HD_VID_CTL_CLRRGB | @@ -1034,6 +1128,8 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, HDMI_READ(HDMI_SCHEDULER_CONTROL) | VC4_HDMI_SCHEDULER_CONTROL_MODE_HDMI);
+ spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + ret = wait_for(HDMI_READ(HDMI_SCHEDULER_CONTROL) & VC4_HDMI_SCHEDULER_CONTROL_HDMI_ACTIVE, 1000); WARN_ONCE(ret, "Timeout waiting for " @@ -1046,6 +1142,8 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, HDMI_READ(HDMI_SCHEDULER_CONTROL) & ~VC4_HDMI_SCHEDULER_CONTROL_MODE_HDMI);
+ spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + ret = wait_for(!(HDMI_READ(HDMI_SCHEDULER_CONTROL) & VC4_HDMI_SCHEDULER_CONTROL_HDMI_ACTIVE), 1000); WARN_ONCE(ret, "Timeout waiting for " @@ -1053,6 +1151,8 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, }
if (vc4_encoder->hdmi_monitor) { + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + WARN_ON(!(HDMI_READ(HDMI_SCHEDULER_CONTROL) & VC4_HDMI_SCHEDULER_CONTROL_HDMI_ACTIVE)); HDMI_WRITE(HDMI_SCHEDULER_CONTROL, @@ -1062,6 +1162,8 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, HDMI_WRITE(HDMI_RAM_PACKET_CONFIG, VC4_HDMI_RAM_PACKET_ENABLE);
+ spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + vc4_hdmi_set_infoframes(encoder); }
@@ -1183,6 +1285,7 @@ static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *vc4_hdmi, unsigned int samplerate) { u32 hsm_clock = clk_get_rate(vc4_hdmi->audio_clock); + unsigned long flags; unsigned long n, m;
rational_best_approximation(hsm_clock, samplerate, @@ -1192,9 +1295,11 @@ static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *vc4_hdmi, VC4_HD_MAI_SMP_M_SHIFT) + 1, &n, &m);
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_MAI_SMP, VC4_SET_FIELD(n, VC4_HD_MAI_SMP_N) | VC4_SET_FIELD(m - 1, VC4_HD_MAI_SMP_M)); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
static void vc4_hdmi_set_n_cts(struct vc4_hdmi *vc4_hdmi, unsigned int samplerate) @@ -1205,6 +1310,8 @@ static void vc4_hdmi_set_n_cts(struct vc4_hdmi *vc4_hdmi, unsigned int samplerat u32 n, cts; u64 tmp;
+ lockdep_assert_held(&vc4_hdmi->hw_lock); + n = 128 * samplerate / 1000; tmp = (u64)(mode->clock * 1000) * n; do_div(tmp, 128 * samplerate); @@ -1234,6 +1341,7 @@ static int vc4_hdmi_audio_startup(struct device *dev, void *data) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; + unsigned long flags;
/* * If the HDMI encoder hasn't probed, or the encoder is @@ -1245,12 +1353,14 @@ static int vc4_hdmi_audio_startup(struct device *dev, void *data)
vc4_hdmi->audio.streaming = true;
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_MAI_CTL, VC4_HD_MAI_CTL_RESET | VC4_HD_MAI_CTL_FLUSH | VC4_HD_MAI_CTL_DLATE | VC4_HD_MAI_CTL_ERRORE | VC4_HD_MAI_CTL_ERRORF); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
if (vc4_hdmi->variant->phy_rng_enable) vc4_hdmi->variant->phy_rng_enable(vc4_hdmi); @@ -1262,6 +1372,7 @@ static void vc4_hdmi_audio_reset(struct vc4_hdmi *vc4_hdmi) { struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; struct device *dev = &vc4_hdmi->pdev->dev; + unsigned long flags; int ret;
vc4_hdmi->audio.streaming = false; @@ -1269,20 +1380,29 @@ static void vc4_hdmi_audio_reset(struct vc4_hdmi *vc4_hdmi) if (ret) dev_err(dev, "Failed to stop audio infoframe: %d\n", ret);
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + HDMI_WRITE(HDMI_MAI_CTL, VC4_HD_MAI_CTL_RESET); HDMI_WRITE(HDMI_MAI_CTL, VC4_HD_MAI_CTL_ERRORF); HDMI_WRITE(HDMI_MAI_CTL, VC4_HD_MAI_CTL_FLUSH); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
static void vc4_hdmi_audio_shutdown(struct device *dev, void *data) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
HDMI_WRITE(HDMI_MAI_CTL, VC4_HD_MAI_CTL_DLATE | VC4_HD_MAI_CTL_ERRORE | VC4_HD_MAI_CTL_ERRORF);
+ spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + if (vc4_hdmi->variant->phy_rng_disable) vc4_hdmi->variant->phy_rng_disable(vc4_hdmi);
@@ -1337,6 +1457,7 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; unsigned int sample_rate = params->sample_rate; unsigned int channels = params->channels; + unsigned long flags; u32 audio_packet_config, channel_mask; u32 channel_map; u32 mai_audio_format; @@ -1345,14 +1466,15 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, dev_dbg(dev, "%s: %u Hz, %d bit, %d channels\n", __func__, sample_rate, params->sample_width, channels);
+ vc4_hdmi_audio_set_mai_clock(vc4_hdmi, sample_rate); + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_MAI_CTL, VC4_SET_FIELD(channels, VC4_HD_MAI_CTL_CHNUM) | VC4_HD_MAI_CTL_WHOLSMP | VC4_HD_MAI_CTL_CHALIGN | VC4_HD_MAI_CTL_ENABLE);
- vc4_hdmi_audio_set_mai_clock(vc4_hdmi, sample_rate); - mai_sample_rate = sample_rate_to_mai_fmt(sample_rate); if (params->iec.status[0] & IEC958_AES0_NONAUDIO && params->channels == 8) @@ -1390,8 +1512,11 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, channel_map = vc4_hdmi->variant->channel_map(vc4_hdmi, channel_mask); HDMI_WRITE(HDMI_MAI_CHANNEL_MAP, channel_map); HDMI_WRITE(HDMI_AUDIO_PACKET_CONFIG, audio_packet_config); + vc4_hdmi_set_n_cts(vc4_hdmi, sample_rate);
+ spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + memcpy(&vc4_hdmi->audio.infoframe, ¶ms->cea, sizeof(params->cea)); vc4_hdmi_set_audio_infoframe(encoder);
@@ -1667,6 +1792,8 @@ static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1) struct cec_msg *msg = &vc4_hdmi->cec_rx_msg; unsigned int i;
+ lockdep_assert_held(&vc4_hdmi->hw_lock); + msg->len = 1 + ((cntrl1 & VC4_HDMI_CEC_REC_WRD_CNT_MASK) >> VC4_HDMI_CEC_REC_WRD_CNT_SHIFT);
@@ -1685,11 +1812,12 @@ static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1) } }
-static irqreturn_t vc4_cec_irq_handler_tx_bare(int irq, void *priv) +static irqreturn_t vc4_cec_irq_handler_tx_bare_locked(struct vc4_hdmi *vc4_hdmi) { - struct vc4_hdmi *vc4_hdmi = priv; u32 cntrl1;
+ lockdep_assert_held(&vc4_hdmi->hw_lock); + cntrl1 = HDMI_READ(HDMI_CEC_CNTRL_1); vc4_hdmi->cec_tx_ok = cntrl1 & VC4_HDMI_CEC_TX_STATUS_GOOD; cntrl1 &= ~VC4_HDMI_CEC_START_XMIT_BEGIN; @@ -1698,11 +1826,24 @@ static irqreturn_t vc4_cec_irq_handler_tx_bare(int irq, void *priv) return IRQ_WAKE_THREAD; }
-static irqreturn_t vc4_cec_irq_handler_rx_bare(int irq, void *priv) +static irqreturn_t vc4_cec_irq_handler_tx_bare(int irq, void *priv) { struct vc4_hdmi *vc4_hdmi = priv; + irqreturn_t ret; + + spin_lock(&vc4_hdmi->hw_lock); + ret = vc4_cec_irq_handler_tx_bare_locked(vc4_hdmi); + spin_unlock(&vc4_hdmi->hw_lock); + + return ret; +} + +static irqreturn_t vc4_cec_irq_handler_rx_bare_locked(struct vc4_hdmi *vc4_hdmi) +{ u32 cntrl1;
+ lockdep_assert_held(&vc4_hdmi->hw_lock); + vc4_hdmi->cec_rx_msg.len = 0; cntrl1 = HDMI_READ(HDMI_CEC_CNTRL_1); vc4_cec_read_msg(vc4_hdmi, cntrl1); @@ -1715,6 +1856,18 @@ static irqreturn_t vc4_cec_irq_handler_rx_bare(int irq, void *priv) return IRQ_WAKE_THREAD; }
+static irqreturn_t vc4_cec_irq_handler_rx_bare(int irq, void *priv) +{ + struct vc4_hdmi *vc4_hdmi = priv; + irqreturn_t ret; + + spin_lock(&vc4_hdmi->hw_lock); + ret = vc4_cec_irq_handler_rx_bare_locked(vc4_hdmi); + spin_unlock(&vc4_hdmi->hw_lock); + + return ret; +} + static irqreturn_t vc4_cec_irq_handler(int irq, void *priv) { struct vc4_hdmi *vc4_hdmi = priv; @@ -1725,14 +1878,17 @@ static irqreturn_t vc4_cec_irq_handler(int irq, void *priv) if (!(stat & VC4_HDMI_CPU_CEC)) return IRQ_NONE;
+ spin_lock(&vc4_hdmi->hw_lock); cntrl5 = HDMI_READ(HDMI_CEC_CNTRL_5); vc4_hdmi->cec_irq_was_rx = cntrl5 & VC4_HDMI_CEC_RX_CEC_INT; if (vc4_hdmi->cec_irq_was_rx) - ret = vc4_cec_irq_handler_rx_bare(irq, priv); + ret = vc4_cec_irq_handler_rx_bare_locked(vc4_hdmi); else - ret = vc4_cec_irq_handler_tx_bare(irq, priv); + ret = vc4_cec_irq_handler_tx_bare_locked(vc4_hdmi);
HDMI_WRITE(HDMI_CEC_CPU_CLEAR, VC4_HDMI_CPU_CEC); + spin_unlock(&vc4_hdmi->hw_lock); + return ret; }
@@ -1741,6 +1897,7 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap) struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); /* clock period in microseconds */ const u32 usecs = 1000000 / CEC_CLOCK_FREQ; + unsigned long flags; u32 val; int ret;
@@ -1748,6 +1905,8 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap) if (ret) return ret;
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + val = HDMI_READ(HDMI_CEC_CNTRL_5); val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET | VC4_HDMI_CEC_CNT_TO_4700_US_MASK | @@ -1778,12 +1937,17 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap) if (!vc4_hdmi->variant->external_irq_controller) HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
+ spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + return 0; }
static int vc4_hdmi_cec_disable(struct cec_adapter *adap) { struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
if (!vc4_hdmi->variant->external_irq_controller) HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC); @@ -1791,6 +1955,8 @@ static int vc4_hdmi_cec_disable(struct cec_adapter *adap) HDMI_WRITE(HDMI_CEC_CNTRL_5, HDMI_READ(HDMI_CEC_CNTRL_5) | VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
+ spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + pm_runtime_put(&vc4_hdmi->pdev->dev);
return 0; @@ -1807,10 +1973,14 @@ static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable) static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr) { struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); + unsigned long flags;
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_CEC_CNTRL_1, (HDMI_READ(HDMI_CEC_CNTRL_1) & ~VC4_HDMI_CEC_ADDR_MASK) | (log_addr & 0xf) << VC4_HDMI_CEC_ADDR_SHIFT); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + return 0; }
@@ -1819,6 +1989,7 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts, { struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); struct drm_device *dev = vc4_hdmi->connector.dev; + unsigned long flags; u32 val; unsigned int i;
@@ -1827,6 +1998,8 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts, return -ENOMEM; }
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + for (i = 0; i < msg->len; i += 4) HDMI_WRITE(HDMI_CEC_TX_DATA_1 + (i >> 2), (msg->msg[i]) | @@ -1842,6 +2015,9 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts, val |= VC4_HDMI_CEC_START_XMIT_BEGIN;
HDMI_WRITE(HDMI_CEC_CNTRL_1, val); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + return 0; }
@@ -1856,6 +2032,7 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) struct cec_connector_info conn_info; struct platform_device *pdev = vc4_hdmi->pdev; struct device *dev = &pdev->dev; + unsigned long flags; u32 value; int ret;
@@ -1875,10 +2052,12 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) cec_fill_conn_info_from_drm(&conn_info, &vc4_hdmi->connector); cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info);
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); value = HDMI_READ(HDMI_CEC_CNTRL_1); /* Set the logical address to Unregistered */ value |= VC4_HDMI_CEC_ADDR_MASK; HDMI_WRITE(HDMI_CEC_CNTRL_1, value); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
vc4_hdmi_cec_update_clk_div(vc4_hdmi);
@@ -1897,7 +2076,9 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) if (ret) goto err_remove_cec_rx_handler; } else { + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
ret = request_threaded_irq(platform_get_irq(pdev, 0), vc4_cec_irq_handler, @@ -2167,6 +2348,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL); if (!vc4_hdmi) return -ENOMEM; + spin_lock_init(&vc4_hdmi->hw_lock); INIT_DELAYED_WORK(&vc4_hdmi->scrambling_work, vc4_hdmi_scrambling_wq);
dev_set_drvdata(dev, vc4_hdmi); diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 33e9f665ab8e..006142fe8d4e 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -178,6 +178,11 @@ struct vc4_hdmi {
struct debugfs_regset32 hdmi_regset; struct debugfs_regset32 hd_regset; + + /** + * @hw_lock: Spinlock protecting device register access. + */ + spinlock_t hw_lock; };
static inline struct vc4_hdmi * diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_phy.c b/drivers/gpu/drm/vc4/vc4_hdmi_phy.c index 36535480f8e2..62148f0dc284 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi_phy.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi_phy.c @@ -130,31 +130,49 @@ void vc4_hdmi_phy_init(struct vc4_hdmi *vc4_hdmi, struct vc4_hdmi_connector_state *conn_state) { + unsigned long flags; + /* PHY should be in reset, like * vc4_hdmi_encoder_disable() does. */
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + HDMI_WRITE(HDMI_TX_PHY_RESET_CTL, 0xf << 16); HDMI_WRITE(HDMI_TX_PHY_RESET_CTL, 0); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
void vc4_hdmi_phy_disable(struct vc4_hdmi *vc4_hdmi) { + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_TX_PHY_RESET_CTL, 0xf << 16); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
void vc4_hdmi_phy_rng_enable(struct vc4_hdmi *vc4_hdmi) { + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_TX_PHY_CTL_0, HDMI_READ(HDMI_TX_PHY_CTL_0) & ~VC4_HDMI_TX_PHY_RNG_PWRDN); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
void vc4_hdmi_phy_rng_disable(struct vc4_hdmi *vc4_hdmi) { + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_TX_PHY_CTL_0, HDMI_READ(HDMI_TX_PHY_CTL_0) | VC4_HDMI_TX_PHY_RNG_PWRDN); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
static unsigned long long @@ -336,6 +354,8 @@ phy_get_channel_settings(enum vc4_hdmi_phy_channel chan,
static void vc5_hdmi_reset_phy(struct vc4_hdmi *vc4_hdmi) { + lockdep_assert_held(&vc4_hdmi->hw_lock); + HDMI_WRITE(HDMI_TX_PHY_RESET_CTL, 0x0f); HDMI_WRITE(HDMI_TX_PHY_POWERDOWN_CTL, BIT(10)); } @@ -348,10 +368,13 @@ void vc5_hdmi_phy_init(struct vc4_hdmi *vc4_hdmi, unsigned long long pixel_freq = conn_state->pixel_rate; unsigned long long vco_freq; unsigned char word_sel; + unsigned long flags; u8 vco_sel, vco_div;
vco_freq = phy_get_vco_freq(pixel_freq, &vco_sel, &vco_div);
+ spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + vc5_hdmi_reset_phy(vc4_hdmi);
HDMI_WRITE(HDMI_TX_PHY_POWERDOWN_CTL, @@ -501,23 +524,37 @@ void vc5_hdmi_phy_init(struct vc4_hdmi *vc4_hdmi, HDMI_READ(HDMI_TX_PHY_RESET_CTL) | VC4_HDMI_TX_PHY_RESET_CTL_PLL_RESETB | VC4_HDMI_TX_PHY_RESET_CTL_PLLDIV_RESETB); + + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
void vc5_hdmi_phy_disable(struct vc4_hdmi *vc4_hdmi) { + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); vc5_hdmi_reset_phy(vc4_hdmi); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
void vc5_hdmi_phy_rng_enable(struct vc4_hdmi *vc4_hdmi) { + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_TX_PHY_POWERDOWN_CTL, HDMI_READ(HDMI_TX_PHY_POWERDOWN_CTL) & ~VC4_HDMI_TX_PHY_POWERDOWN_CTL_RNDGEN_PWRDN); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); }
void vc5_hdmi_phy_rng_disable(struct vc4_hdmi *vc4_hdmi) { + unsigned long flags; + + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_TX_PHY_POWERDOWN_CTL, HDMI_READ(HDMI_TX_PHY_POWERDOWN_CTL) | VC4_HDMI_TX_PHY_POWERDOWN_CTL_RNDGEN_PWRDN); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); } diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h index 99dde6e06a37..fc971506bd4f 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h @@ -442,6 +442,8 @@ static inline void vc4_hdmi_write(struct vc4_hdmi *hdmi, const struct vc4_hdmi_variant *variant = hdmi->variant; void __iomem *base;
+ lockdep_assert_held(&hdmi->hw_lock); + WARN_ON(!pm_runtime_active(&hdmi->pdev->dev));
if (reg >= variant->num_registers) {
The vc4 HDMI controller registers into the KMS, CEC and ALSA frameworks.
However, no particular care is done to prevent the concurrent execution of different framework hooks from happening at the same time.
In order to protect against that scenario, let's introduce a mutex that relevant ALSA and KMS hooks will need to take to prevent concurrent execution.
CEC is left out at the moment though, since the .get_modes and .detect KMS hooks, when running cec_s_phys_addr_from_edid, can end up calling CEC's .adap_enable hook. This introduces some reentrancy that isn't easy to deal with properly.
The CEC hooks also don't share much state with the rest of the driver: the registers are entirely separate, we don't share any variable, the only thing that can conflict is the CEC clock divider setup that can be affected by a mode set.
However, after discussing it, it looks like CEC should be able to recover from this if it was to happen.
Fixes: bb7d78568814 ("drm/vc4: Add HDMI audio support") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 118 +++++++++++++++++++++++++++++++-- drivers/gpu/drm/vc4/vc4_hdmi.h | 14 ++++ 2 files changed, 126 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 484450831483..814f98414f2b 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -186,6 +186,8 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); bool connected = false;
+ mutex_lock(&vc4_hdmi->mutex); + WARN_ON(pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev));
if (vc4_hdmi->hpd_gpio && @@ -217,11 +219,13 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) }
pm_runtime_put(&vc4_hdmi->pdev->dev); + mutex_unlock(&vc4_hdmi->mutex); return connector_status_connected; }
cec_phys_addr_invalidate(vc4_hdmi->cec_adap); pm_runtime_put(&vc4_hdmi->pdev->dev); + mutex_unlock(&vc4_hdmi->mutex); return connector_status_disconnected; }
@@ -238,10 +242,14 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) int ret = 0; struct edid *edid;
+ mutex_lock(&vc4_hdmi->mutex); + edid = drm_get_edid(connector, vc4_hdmi->ddc); cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid); - if (!edid) - return -ENODEV; + if (!edid) { + ret = -ENODEV; + goto out; + }
vc4_encoder->hdmi_monitor = drm_detect_hdmi_monitor(edid);
@@ -261,6 +269,9 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) } }
+out: + mutex_unlock(&vc4_hdmi->mutex); + return ret; }
@@ -477,6 +488,8 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder) union hdmi_infoframe frame; int ret;
+ lockdep_assert_held(&vc4_hdmi->mutex); + ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi, connector, mode); if (ret < 0) { @@ -528,6 +541,8 @@ static void vc4_hdmi_set_hdr_infoframe(struct drm_encoder *encoder) struct drm_connector_state *conn_state = connector->state; union hdmi_infoframe frame;
+ lockdep_assert_held(&vc4_hdmi->mutex); + if (!vc4_hdmi->variant->supports_hdr) return;
@@ -544,6 +559,8 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
+ lockdep_assert_held(&vc4_hdmi->mutex); + vc4_hdmi_set_avi_infoframe(encoder); vc4_hdmi_set_spd_infoframe(encoder); /* @@ -563,6 +580,8 @@ static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder, struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); struct drm_display_info *display = &vc4_hdmi->connector.display_info;
+ lockdep_assert_held(&vc4_hdmi->mutex); + if (!vc4_encoder->hdmi_monitor) return false;
@@ -581,6 +600,8 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); unsigned long flags;
+ lockdep_assert_held(&vc4_hdmi->mutex); + if (!vc4_hdmi_supports_scrambling(encoder, mode)) return;
@@ -650,6 +671,8 @@ static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder, struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); unsigned long flags;
+ mutex_lock(&vc4_hdmi->mutex); + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
HDMI_WRITE(HDMI_RAM_PACKET_CONFIG, 0); @@ -666,6 +689,8 @@ static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder, spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
vc4_hdmi_disable_scrambling(encoder); + + mutex_unlock(&vc4_hdmi->mutex); }
static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder, @@ -675,6 +700,8 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder, unsigned long flags; int ret;
+ mutex_lock(&vc4_hdmi->mutex); + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_VID_CTL, HDMI_READ(HDMI_VID_CTL) | VC4_HD_VID_CTL_BLANKPIX); @@ -689,6 +716,8 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder, ret = pm_runtime_put(&vc4_hdmi->pdev->dev); if (ret < 0) DRM_ERROR("Failed to release power domain: %d\n", ret); + + mutex_unlock(&vc4_hdmi->mutex); }
static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder) @@ -985,6 +1014,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, unsigned long flags; int ret;
+ mutex_lock(&vc4_hdmi->mutex); + /* * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must * be faster than pixel clock, infinitesimally faster, tested in @@ -1005,13 +1036,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate); if (ret) { DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); - return; + goto out; }
ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev); if (ret < 0) { DRM_ERROR("Failed to retain power domain: %d\n", ret); - return; + goto out; }
ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate); @@ -1063,13 +1094,16 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, if (vc4_hdmi->variant->set_timings) vc4_hdmi->variant->set_timings(vc4_hdmi, conn_state, mode);
+ mutex_unlock(&vc4_hdmi->mutex); + return;
err_disable_pixel_clock: clk_disable_unprepare(vc4_hdmi->pixel_clock); err_put_runtime_pm: pm_runtime_put(&vc4_hdmi->pdev->dev); - +out: + mutex_unlock(&vc4_hdmi->mutex); return; }
@@ -1081,6 +1115,8 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); unsigned long flags;
+ mutex_lock(&vc4_hdmi->mutex); + if (vc4_encoder->hdmi_monitor && drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED) { if (vc4_hdmi->variant->csc_setup) @@ -1097,6 +1133,8 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_FIFO_CTL, VC4_HDMI_FIFO_CTL_MASTER_SLAVE_N); spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + mutex_unlock(&vc4_hdmi->mutex); }
static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, @@ -1110,6 +1148,8 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, unsigned long flags; int ret;
+ mutex_lock(&vc4_hdmi->mutex); + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
HDMI_WRITE(HDMI_VID_CTL, @@ -1169,6 +1209,8 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder,
vc4_hdmi_recenter_fifo(vc4_hdmi); vc4_hdmi_enable_scrambling(encoder); + + mutex_unlock(&vc4_hdmi->mutex); }
static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) @@ -1310,6 +1352,7 @@ static void vc4_hdmi_set_n_cts(struct vc4_hdmi *vc4_hdmi, unsigned int samplerat u32 n, cts; u64 tmp;
+ lockdep_assert_held(&vc4_hdmi->mutex); lockdep_assert_held(&vc4_hdmi->hw_lock);
n = 128 * samplerate / 1000; @@ -1343,13 +1386,17 @@ static int vc4_hdmi_audio_startup(struct device *dev, void *data) struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; unsigned long flags;
+ mutex_lock(&vc4_hdmi->mutex); + /* * If the HDMI encoder hasn't probed, or the encoder is * currently in DVI mode, treat the codec dai as missing. */ if (!encoder->crtc || !(HDMI_READ(HDMI_RAM_PACKET_CONFIG) & - VC4_HDMI_RAM_PACKET_ENABLE)) + VC4_HDMI_RAM_PACKET_ENABLE)) { + mutex_unlock(&vc4_hdmi->mutex); return -ENODEV; + }
vc4_hdmi->audio.streaming = true;
@@ -1365,6 +1412,8 @@ static int vc4_hdmi_audio_startup(struct device *dev, void *data) if (vc4_hdmi->variant->phy_rng_enable) vc4_hdmi->variant->phy_rng_enable(vc4_hdmi);
+ mutex_unlock(&vc4_hdmi->mutex); + return 0; }
@@ -1375,6 +1424,8 @@ static void vc4_hdmi_audio_reset(struct vc4_hdmi *vc4_hdmi) unsigned long flags; int ret;
+ lockdep_assert_held(&vc4_hdmi->mutex); + vc4_hdmi->audio.streaming = false; ret = vc4_hdmi_stop_packet(encoder, HDMI_INFOFRAME_TYPE_AUDIO, false); if (ret) @@ -1394,6 +1445,8 @@ static void vc4_hdmi_audio_shutdown(struct device *dev, void *data) struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); unsigned long flags;
+ mutex_lock(&vc4_hdmi->mutex); + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
HDMI_WRITE(HDMI_MAI_CTL, @@ -1408,6 +1461,8 @@ static void vc4_hdmi_audio_shutdown(struct device *dev, void *data)
vc4_hdmi->audio.streaming = false; vc4_hdmi_audio_reset(vc4_hdmi); + + mutex_unlock(&vc4_hdmi->mutex); }
static int sample_rate_to_mai_fmt(int samplerate) @@ -1466,6 +1521,8 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, dev_dbg(dev, "%s: %u Hz, %d bit, %d channels\n", __func__, sample_rate, params->sample_width, channels);
+ mutex_lock(&vc4_hdmi->mutex); + vc4_hdmi_audio_set_mai_clock(vc4_hdmi, sample_rate);
spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); @@ -1520,6 +1577,8 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, memcpy(&vc4_hdmi->audio.infoframe, ¶ms->cea, sizeof(params->cea)); vc4_hdmi_set_audio_infoframe(encoder);
+ mutex_unlock(&vc4_hdmi->mutex); + return 0; }
@@ -1570,7 +1629,9 @@ static int vc4_hdmi_audio_get_eld(struct device *dev, void *data, struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); struct drm_connector *connector = &vc4_hdmi->connector;
+ mutex_lock(&vc4_hdmi->mutex); memcpy(buf, connector->eld, min(sizeof(connector->eld), len)); + mutex_unlock(&vc4_hdmi->mutex);
return 0; } @@ -1901,6 +1962,17 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap) u32 val; int ret;
+ /* + * NOTE: This function should really take vc4_hdmi->mutex, but doing so + * results in a reentrancy since cec_s_phys_addr_from_edid() called in + * .detect or .get_modes might call .adap_enable, which leads to this + * function being called with that mutex held. + * + * Concurrency is not an issue for the moment since we don't share any + * state with KMS, so we can ignore the lock for now, but we need to + * keep it in mind if we were to change that assumption. + */ + ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev); if (ret) return ret; @@ -1947,6 +2019,17 @@ static int vc4_hdmi_cec_disable(struct cec_adapter *adap) struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); unsigned long flags;
+ /* + * NOTE: This function should really take vc4_hdmi->mutex, but doing so + * results in a reentrancy since cec_s_phys_addr_from_edid() called in + * .detect or .get_modes might call .adap_enable, which leads to this + * function being called with that mutex held. + * + * Concurrency is not an issue for the moment since we don't share any + * state with KMS, so we can ignore the lock for now, but we need to + * keep it in mind if we were to change that assumption. + */ + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
if (!vc4_hdmi->variant->external_irq_controller) @@ -1975,6 +2058,17 @@ static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr) struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); unsigned long flags;
+ /* + * NOTE: This function should really take vc4_hdmi->mutex, but doing so + * results in a reentrancy since cec_s_phys_addr_from_edid() called in + * .detect or .get_modes might call .adap_enable, which leads to this + * function being called with that mutex held. + * + * Concurrency is not an issue for the moment since we don't share any + * state with KMS, so we can ignore the lock for now, but we need to + * keep it in mind if we were to change that assumption. + */ + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_CEC_CNTRL_1, (HDMI_READ(HDMI_CEC_CNTRL_1) & ~VC4_HDMI_CEC_ADDR_MASK) | @@ -1993,6 +2087,17 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts, u32 val; unsigned int i;
+ /* + * NOTE: This function should really take vc4_hdmi->mutex, but doing so + * results in a reentrancy since cec_s_phys_addr_from_edid() called in + * .detect or .get_modes might call .adap_enable, which leads to this + * function being called with that mutex held. + * + * Concurrency is not an issue for the moment since we don't share any + * state with KMS, so we can ignore the lock for now, but we need to + * keep it in mind if we were to change that assumption. + */ + if (msg->len > 16) { drm_err(dev, "Attempting to transmit too much data (%d)\n", msg->len); return -ENOMEM; @@ -2348,6 +2453,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL); if (!vc4_hdmi) return -ENOMEM; + mutex_init(&vc4_hdmi->mutex); spin_lock_init(&vc4_hdmi->hw_lock); INIT_DELAYED_WORK(&vc4_hdmi->scrambling_work, vc4_hdmi_scrambling_wq);
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 006142fe8d4e..cf9bb21a8ef7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -183,6 +183,20 @@ struct vc4_hdmi { * @hw_lock: Spinlock protecting device register access. */ spinlock_t hw_lock; + + /** + * @mutex: Mutex protecting the driver access across multiple + * frameworks (KMS, ALSA). + * + * NOTE: While supported, CEC has been left out since + * cec_s_phys_addr_from_edid() might call .adap_enable and lead to a + * reentrancy issue between .get_modes (or .detect) and .adap_enable. + * Since we don't share any state between the CEC hooks and KMS', it's + * not a big deal. The only trouble might come from updating the CEC + * clock divider which might be affected by a modeset, but CEC should + * be resilient to that. + */ + struct mutex mutex; };
static inline struct vc4_hdmi *
Accessing the crtc->state pointer from outside the modesetting context is not allowed. We thus need to copy whatever we need from the KMS state to our structure in order to access it.
However, in the vc4 HDMI driver we do use that pointer in the ALSA code path, and potentially in the hotplug interrupt handler path.
These paths both need access to the CRTC adjusted mode in order for the proper dividers to be set for ALSA, and the scrambler state to be reinstated properly for hotplug.
Let's copy this mode into our private encoder structure and reference it from there when needed. Since that part is shared between KMS and other paths, we need to protect it using our mutex.
Link: https://lore.kernel.org/all/YWgteNaNeaS9uWDe@phenom.ffwll.local/ Fixes: bb7d78568814 ("drm/vc4: Add HDMI audio support") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 38 +++++++++++++++++++++++----------- drivers/gpu/drm/vc4/vc4_hdmi.h | 6 ++++++ 2 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 814f98414f2b..cb444571a3f7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -483,8 +483,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder) struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); struct drm_connector *connector = &vc4_hdmi->connector; struct drm_connector_state *cstate = connector->state; - struct drm_crtc *crtc = encoder->crtc; - const struct drm_display_mode *mode = &crtc->state->adjusted_mode; + const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; union hdmi_infoframe frame; int ret;
@@ -596,8 +595,8 @@ static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder,
static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) { - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; unsigned long flags;
lockdep_assert_held(&vc4_hdmi->mutex); @@ -623,18 +622,21 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; struct drm_crtc *crtc = encoder->crtc; unsigned long flags;
+ lockdep_assert_held(&vc4_hdmi->mutex); + /* * At boot, encoder->crtc will be NULL. Since we don't know the * state of the scrambler and in order to avoid any * inconsistency, let's disable it all the time. */ - if (crtc && !vc4_hdmi_supports_scrambling(encoder, &crtc->mode)) + if (crtc && !vc4_hdmi_supports_scrambling(encoder, mode)) return;
- if (crtc && !vc4_hdmi_mode_needs_scrambling(&crtc->mode)) + if (crtc && !vc4_hdmi_mode_needs_scrambling(mode)) return;
if (delayed_work_pending(&vc4_hdmi->scrambling_work)) @@ -1007,8 +1009,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, vc4_hdmi_encoder_get_connector_state(encoder, state); struct vc4_hdmi_connector_state *vc4_conn_state = conn_state_to_vc4_hdmi_conn_state(conn_state); - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; unsigned long pixel_rate = vc4_conn_state->pixel_rate; unsigned long bvb_rate, hsm_rate; unsigned long flags; @@ -1110,9 +1112,9 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, struct drm_atomic_state *state) { - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; - struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; + struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); unsigned long flags;
mutex_lock(&vc4_hdmi->mutex); @@ -1140,8 +1142,8 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, struct drm_atomic_state *state) { - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder); bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC; bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC; @@ -1217,6 +1219,19 @@ static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) { }
+static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + + mutex_lock(&vc4_hdmi->mutex); + memcpy(&vc4_hdmi->saved_adjusted_mode, + &crtc_state->adjusted_mode, + sizeof(vc4_hdmi->saved_adjusted_mode)); + mutex_unlock(&vc4_hdmi->mutex); +} + #define WIFI_2_4GHz_CH1_MIN_FREQ 2400000000ULL #define WIFI_2_4GHz_CH1_MAX_FREQ 2422000000ULL
@@ -1293,6 +1308,7 @@ vc4_hdmi_encoder_mode_valid(struct drm_encoder *encoder,
static const struct drm_encoder_helper_funcs vc4_hdmi_encoder_helper_funcs = { .atomic_check = vc4_hdmi_encoder_atomic_check, + .atomic_mode_set = vc4_hdmi_encoder_atomic_mode_set, .mode_valid = vc4_hdmi_encoder_mode_valid, .disable = vc4_hdmi_encoder_disable, .enable = vc4_hdmi_encoder_enable, @@ -1346,9 +1362,7 @@ static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *vc4_hdmi,
static void vc4_hdmi_set_n_cts(struct vc4_hdmi *vc4_hdmi, unsigned int samplerate) { - struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; - struct drm_crtc *crtc = encoder->crtc; - const struct drm_display_mode *mode = &crtc->state->adjusted_mode; + const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; u32 n, cts; u64 tmp;
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index cf9bb21a8ef7..a43cc5614d19 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -197,6 +197,12 @@ struct vc4_hdmi { * be resilient to that. */ struct mutex mutex; + + /** + * @saved_adjusted_mode: Copy of @drm_crtc_state.adjusted_mode + * for use by ALSA hooks and interrupt handlers. Protected by @mutex. + */ + struct drm_display_mode saved_adjusted_mode; };
static inline struct vc4_hdmi *
Even though we already check that the encoder->crtc pointer is there during in startup(), which is part of the open() path in ASoC, nothing guarantees that our encoder state won't change between the time when we open the device and the time we prepare it.
Move the sanity checks we do in startup() to a helper and call it from prepare().
Fixes: 91e99e113929 ("drm/vc4: hdmi: Register HDMI codec") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 35 +++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index cb444571a3f7..291fad018be3 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1394,20 +1394,36 @@ static inline struct vc4_hdmi *dai_to_hdmi(struct snd_soc_dai *dai) return snd_soc_card_get_drvdata(card); }
+static bool vc4_hdmi_audio_can_stream(struct vc4_hdmi *vc4_hdmi) +{ + struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; + + lockdep_assert_held(&vc4_hdmi->mutex); + + /* + * The encoder doesn't have a CRTC until the first modeset. + */ + if (!encoder->crtc) + return false; + + /* + * If the encoder is currently in DVI mode, treat the codec DAI + * as missing. + */ + if (!(HDMI_READ(HDMI_RAM_PACKET_CONFIG) & VC4_HDMI_RAM_PACKET_ENABLE)) + return false; + + return true; +} + static int vc4_hdmi_audio_startup(struct device *dev, void *data) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); - struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; unsigned long flags;
mutex_lock(&vc4_hdmi->mutex);
- /* - * If the HDMI encoder hasn't probed, or the encoder is - * currently in DVI mode, treat the codec dai as missing. - */ - if (!encoder->crtc || !(HDMI_READ(HDMI_RAM_PACKET_CONFIG) & - VC4_HDMI_RAM_PACKET_ENABLE)) { + if (!vc4_hdmi_audio_can_stream(vc4_hdmi)) { mutex_unlock(&vc4_hdmi->mutex); return -ENODEV; } @@ -1537,6 +1553,11 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data,
mutex_lock(&vc4_hdmi->mutex);
+ if (!vc4_hdmi_audio_can_stream(vc4_hdmi)) { + mutex_unlock(&vc4_hdmi->mutex); + return -EINVAL; + } + vc4_hdmi_audio_set_mai_clock(vc4_hdmi, sample_rate);
spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
We currently poke at encoder->crtc in the ALSA code path to determine whether the HDMI output is enabled or not, and thus whether we should allow the audio output.
However, that pointer is deprecated and shouldn't really be used by atomic drivers anymore. Since we have the infrastructure in place now, let's just create a flag that we toggle to report whether the controller is currently enabled and use that instead of encoder->crtc in ALSA.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 16 ++++++++++++---- drivers/gpu/drm/vc4/vc4_hdmi.h | 6 ++++++ 2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 291fad018be3..8fb8e7e57f9c 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -724,6 +724,11 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder,
static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder) { + struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + + mutex_lock(&vc4_hdmi->mutex); + vc4_hdmi->output_enabled = false; + mutex_unlock(&vc4_hdmi->mutex); }
static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, bool enable) @@ -1217,6 +1222,11 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder,
static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder) { + struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + + mutex_lock(&vc4_hdmi->mutex); + vc4_hdmi->output_enabled = true; + mutex_unlock(&vc4_hdmi->mutex); }
static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder, @@ -1396,14 +1406,12 @@ static inline struct vc4_hdmi *dai_to_hdmi(struct snd_soc_dai *dai)
static bool vc4_hdmi_audio_can_stream(struct vc4_hdmi *vc4_hdmi) { - struct drm_encoder *encoder = &vc4_hdmi->encoder.base.base; - lockdep_assert_held(&vc4_hdmi->mutex);
/* - * The encoder doesn't have a CRTC until the first modeset. + * If the controller is disabled, prevent any ALSA output. */ - if (!encoder->crtc) + if (!vc4_hdmi->output_enabled) return false;
/* diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index a43cc5614d19..5d3e97703e8d 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -203,6 +203,12 @@ struct vc4_hdmi { * for use by ALSA hooks and interrupt handlers. Protected by @mutex. */ struct drm_display_mode saved_adjusted_mode; + + /** + * @output_enabled: Is the HDMI controller currently active? + * Protected by @mutex. + */ + bool output_enabled; };
static inline struct vc4_hdmi *
We currently rely on two functions, vc4_hdmi_supports_scrambling() and vc4_hdmi_mode_needs_scrambling() to determine if we should enable and disable the scrambler for any given mode.
Since we might need to disable the controller at boot, we also always run vc4_hdmi_disable_scrambling() and thus call those functions without a mode yet, which in turns need to make some special casing in order for it to work.
Instead of duplicating the check for whether or not we need to take care of the scrambler in both vc4_hdmi_enable_scrambling() and vc4_hdmi_disable_scrambling(), we can do that check only when we enable it and store whether or not it's been enabled in our private structure.
We also need to initialize that flag at true to make sure we disable the scrambler at boot since we can't really know its state yet.
This allows to simplify a bit that part of the driver, and removes one user of our copy of the CRTC adjusted mode outside of KMS (since vc4_hdmi_disable_scrambling() might be called from the hotplug interrupt handler).
It also removes our last user of the legacy encoder->crtc pointer.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 22 ++++++++++++---------- drivers/gpu/drm/vc4/vc4_hdmi.h | 6 ++++++ 2 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 8fb8e7e57f9c..7b0cb08e6563 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -615,6 +615,8 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) VC5_HDMI_SCRAMBLER_CTL_ENABLE); spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
+ vc4_hdmi->scdc_enabled = true; + queue_delayed_work(system_wq, &vc4_hdmi->scrambling_work, msecs_to_jiffies(SCRAMBLING_POLLING_DELAY_MS)); } @@ -622,22 +624,14 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); - struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; - struct drm_crtc *crtc = encoder->crtc; unsigned long flags;
lockdep_assert_held(&vc4_hdmi->mutex);
- /* - * At boot, encoder->crtc will be NULL. Since we don't know the - * state of the scrambler and in order to avoid any - * inconsistency, let's disable it all the time. - */ - if (crtc && !vc4_hdmi_supports_scrambling(encoder, mode)) + if (!vc4_hdmi->scdc_enabled) return;
- if (crtc && !vc4_hdmi_mode_needs_scrambling(mode)) - return; + vc4_hdmi->scdc_enabled = false;
if (delayed_work_pending(&vc4_hdmi->scrambling_work)) cancel_delayed_work_sync(&vc4_hdmi->scrambling_work); @@ -2511,6 +2505,14 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi->pdev = pdev; vc4_hdmi->variant = variant;
+ /* + * Since we don't know the state of the controller and its + * display (if any), let's assume it's always enabled. + * vc4_hdmi_disable_scrambling() will thus run at boot, make + * sure it's disabled, and avoid any inconsistency. + */ + vc4_hdmi->scdc_enabled = true; + ret = variant->init_resources(vc4_hdmi); if (ret) return ret; diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 5d3e97703e8d..36c0b082a43b 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -209,6 +209,12 @@ struct vc4_hdmi { * Protected by @mutex. */ bool output_enabled; + + /** + * @scdc_enabled: Is the HDMI controller currently running with + * the scrambler on? Protected by @mutex. + */ + bool scdc_enabled; };
static inline struct vc4_hdmi *
On Mon, 25 Oct 2021 16:11:04 +0200, Maxime Ripard wrote:
This is a follow-up of the series here: https://lore.kernel.org/all/20210924135530.1036564-1-maxime@cerno.tech/
and the discussion that occured here: https://lore.kernel.org/all/YWgteNaNeaS9uWDe@phenom.ffwll.local/
The original series aimed at getting rid of the encoder->crtc pointer usage in the vc4 HDMI driver, some regression was noticed and the following discussion pointed out that we were doing a fair number of KMS state access outside of the mode set path, which is disallowed and unsafe.
[...]
Applied to drm/drm-misc (drm-misc-next).
Thanks! Maxime
dri-devel@lists.freedesktop.org
-
Maxime Ripard