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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart