Re: [PATCH 4/8] drm/client: Make copies of modes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 08, 2024 at 10:33:44PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 07, 2024 at 09:36:13AM +0200, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 03.10.24 um 13:33 schrieb Ville Syrjala:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > drm_client_firmware_config() is currently picking up the current
> > > mode of the crtc via the legacy crtc->mode, which is not supposed
> > > to be used by atomic drivers at all. We can't simply switch over
> > > to the proper crtc->state->mode because we drop the crtc->mutex
> > > (which protects crtc->state) before the mode gets used.
> > >
> > > The most straightforward solution to extend the lifetime of
> > > modes[] seem to be to make full copies of the modes instead
> > > of just storing pointers. We do have to replace the NULL checks
> > > with something else though. Checking that mode->clock!=0
> > > should be sufficient.
> > >
> > > And with this we can undo also commit 3eadd887dbac
> > > ("drm/client:Fully protect modes[] with dev->mode_config.mutex")
> > > as the lifetime of modes[] no longer has anything to do with
> > > that lock.
> > 
> > I think it would be a lot better to first build that mode list while 
> > holding the mutex, and afterwards copy the resulting modes before 
> > releasing the lock. The code below is convoluted with drm_mode_copy().
> 
> My first thought was to make copies but still keep track
> of pointers. That idea was a complete disaster because you
> now had to carefully free the modes on the list.
> 
> I then considred some kind of double list approach, but that
> too seemed more complicated/confusing than the (IMO fairly
> straightforward) apporach I ended up with. I'd prefer to reduce
> the nummber of arrays this thing uses rather than increase them.

Had another look at the double array approach, and still
tought the result would be quite disgusting.

So I think the only other viable option is to keep the single
array of pointers, and stick copies onto it. But that introduces
more ways to leak memory and/or access already freed memory.
I don't really like the extra complexity that this requires.
It'd perhaps be more palatable if the whole thing would be
redesigned to be more AoS instead of SoA...

-- 
Ville Syrjälä
Intel