[PATCHv2,19/22] drm/bridge: tc358767: copy the mode data, instead of storing the pointer

Message ID 20190326103146.24795-20-tomi.valkeinen@ti.com
State Superseded
Headers show
Series
  • drm/bridge: tc358767: DP support
Related show

Commit Message

Tomi Valkeinen March 26, 2019, 10:31 a.m.
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(-)

Comments

Andrzej Hajda April 15, 2019, 10:09 a.m. | #1
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)
Tomi Valkeinen April 15, 2019, 11:19 a.m. | #2
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
Andrzej Hajda April 15, 2019, 12:12 p.m. | #3
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
>
Laurent Pinchart April 20, 2019, 10:20 p.m. | #4
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

Patch

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)