| Message ID | 20190326103146.24795-20-tomi.valkeinen@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/bridge: tc358767: DP support | expand |
On 26.03.2019 11:31, Tomi Valkeinen wrote: > In tc_bridge_mode_set callback, we store the pointer to the given > drm_display_mode, and use the mode later. Storing a pointer in such a > way looks very suspicious to me, and I have observed odd issues where > the timings were apparently (at least mostly) zero. > > Do a copy of the drm_display_mode instead to ensure we don't refer to > freed/modified data. Why not tc->bridge.encoder->crtc->state->adjusted_mode or tc->bridge.encoder->crtc->state->mode ? They should exists if the mode is set. Regards Andrzej > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/tc358767.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 114d535c296b..8732d5b05453 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -205,7 +205,7 @@ struct tc_data { > /* display edid */ > struct edid *edid; > /* current mode */ > - const struct drm_display_mode *mode; > + struct drm_display_mode mode; > > u32 rev; > u8 assr; > @@ -1021,12 +1021,12 @@ static int tc_stream_enable(struct tc_data *tc) > /* PXL PLL setup */ > if (tc_test_pattern) { > ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk), > - 1000 * tc->mode->clock); > + 1000 * tc->mode.clock); > if (ret) > goto err; > } > > - ret = tc_set_video_mode(tc, tc->mode); > + ret = tc_set_video_mode(tc, &tc->mode); > if (ret) > goto err; > > @@ -1166,7 +1166,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge, > { > struct tc_data *tc = bridge_to_tc(bridge); > > - tc->mode = mode; > + tc->mode = *mode; > } > > static int tc_connector_get_modes(struct drm_connector *connector)
On 15/04/2019 13:09, Andrzej Hajda wrote: > On 26.03.2019 11:31, Tomi Valkeinen wrote: >> In tc_bridge_mode_set callback, we store the pointer to the given >> drm_display_mode, and use the mode later. Storing a pointer in such a >> way looks very suspicious to me, and I have observed odd issues where >> the timings were apparently (at least mostly) zero. >> >> Do a copy of the drm_display_mode instead to ensure we don't refer to >> freed/modified data. > > > Why not tc->bridge.encoder->crtc->state->adjusted_mode or > > tc->bridge.encoder->crtc->state->mode ? > > > They should exists if the mode is set. Well, one reason was that my main concern was to get the DP output working (as it was quite broken wrt. the link setup), and I wanted to minimize all the other changes. This change felt the simplest way to fix this issue and get forward, without possibly causing new problems. Second, I'm a bit confused about DRM mode setting, and didn't want to possibly move the driver into the wrong direction. It's not clear to me whether the "mode" referred here is about the input or the output mode. And, in both cases, if there's a bridge between the CRTC and the TC358767, we would definitely be looking at the wrong mode if we peek at the CRTC's. Tomi
On 15.04.2019 13:19, Tomi Valkeinen wrote: > On 15/04/2019 13:09, Andrzej Hajda wrote: >> On 26.03.2019 11:31, Tomi Valkeinen wrote: >>> In tc_bridge_mode_set callback, we store the pointer to the given >>> drm_display_mode, and use the mode later. Storing a pointer in such a >>> way looks very suspicious to me, and I have observed odd issues where >>> the timings were apparently (at least mostly) zero. >>> >>> Do a copy of the drm_display_mode instead to ensure we don't refer to >>> freed/modified data. >> >> Why not tc->bridge.encoder->crtc->state->adjusted_mode or >> >> tc->bridge.encoder->crtc->state->mode ? >> >> >> They should exists if the mode is set. > Well, one reason was that my main concern was to get the DP output > working (as it was quite broken wrt. the link setup), and I wanted to > minimize all the other changes. This change felt the simplest way to fix > this issue and get forward, without possibly causing new problems. > > Second, I'm a bit confused about DRM mode setting, and didn't want to > possibly move the driver into the wrong direction. It's not clear to me > whether the "mode" referred here is about the input or the output mode. > And, in both cases, if there's a bridge between the CRTC and the > TC358767, we would definitely be looking at the wrong mode if we peek at > the CRTC's. DRM does not support multiple intermediate modes, there is .mode as requested by userspace and .adjusted_mode with twisted semantic, with recent improvements[1]. [1]: https://www.kernel.org/doc/html/latest/gpu/drm-kms.html Regards Andrzej > > Tomi >
Hi Andrzej, On Mon, Apr 15, 2019 at 02:12:46PM +0200, Andrzej Hajda wrote: > On 15.04.2019 13:19, Tomi Valkeinen wrote: > > On 15/04/2019 13:09, Andrzej Hajda wrote: > >> On 26.03.2019 11:31, Tomi Valkeinen wrote: > >>> In tc_bridge_mode_set callback, we store the pointer to the given > >>> drm_display_mode, and use the mode later. Storing a pointer in such a > >>> way looks very suspicious to me, and I have observed odd issues where > >>> the timings were apparently (at least mostly) zero. > >>> > >>> Do a copy of the drm_display_mode instead to ensure we don't refer to > >>> freed/modified data. > >> > >> Why not tc->bridge.encoder->crtc->state->adjusted_mode or > >> > >> tc->bridge.encoder->crtc->state->mode ? > >> > >> They should exists if the mode is set. > > > > Well, one reason was that my main concern was to get the DP output > > working (as it was quite broken wrt. the link setup), and I wanted to > > minimize all the other changes. This change felt the simplest way to fix > > this issue and get forward, without possibly causing new problems. > > > > Second, I'm a bit confused about DRM mode setting, and didn't want to > > possibly move the driver into the wrong direction. It's not clear to me > > whether the "mode" referred here is about the input or the output mode. > > And, in both cases, if there's a bridge between the CRTC and the > > TC358767, we would definitely be looking at the wrong mode if we peek at > > the CRTC's. > > DRM does not support multiple intermediate modes, *yet* :-) I don't know when we will need intermediate modes and bridge states, but we'll get there. I think this patch is safe, and I agree with Tomi that it minimizes the impact on the driver. > there is .mode as requested by userspace and .adjusted_mode with > twisted semantic, with recent improvements[1]. > > [1]: https://www.kernel.org/doc/html/latest/gpu/drm-kms.html
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 114d535c296b..8732d5b05453 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -205,7 +205,7 @@ struct tc_data { /* display edid */ struct edid *edid; /* current mode */ - const struct drm_display_mode *mode; + struct drm_display_mode mode; u32 rev; u8 assr; @@ -1021,12 +1021,12 @@ static int tc_stream_enable(struct tc_data *tc) /* PXL PLL setup */ if (tc_test_pattern) { ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk), - 1000 * tc->mode->clock); + 1000 * tc->mode.clock); if (ret) goto err; } - ret = tc_set_video_mode(tc, tc->mode); + ret = tc_set_video_mode(tc, &tc->mode); if (ret) goto err; @@ -1166,7 +1166,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge, { struct tc_data *tc = bridge_to_tc(bridge); - tc->mode = mode; + tc->mode = *mode; } static int tc_connector_get_modes(struct drm_connector *connector)
In tc_bridge_mode_set callback, we store the pointer to the given drm_display_mode, and use the mode later. Storing a pointer in such a way looks very suspicious to me, and I have observed odd issues where the timings were apparently (at least mostly) zero. Do a copy of the drm_display_mode instead to ensure we don't refer to freed/modified data. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/bridge/tc358767.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)