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

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

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.

> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/drm_client_modeset.c | 80 +++++++++++++++-------------
> >   1 file changed, 43 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> > index 888323137a6a..d413e119db3f 100644
> > --- a/drivers/gpu/drm/drm_client_modeset.c
> > +++ b/drivers/gpu/drm/drm_client_modeset.c
> > @@ -265,10 +265,15 @@ static void drm_client_connectors_enabled(struct drm_connector *connectors[],
> >   		enabled[i] = drm_connector_enabled(connectors[i], false);
> >   }
> >   
> > +static bool mode_valid(const struct drm_display_mode *mode)
> > +{
> > +	return mode->clock != 0;
> 
> A mode's clock isn't always known and not all drivers might set it 
> correctly. At least in simpledrm/ofdrm, we have to make up a clock value 
> for the firmware framebuffer. Otherwise some of our userspace would oops.
> 
> The test for clock != 0 makes sense, but it's maybe the wrong place to 
> do this. Would a test for hdisplay/vdisplay != 0 work instead?

That would work as well. drm_mode_validate_basic() rejects
everything with clock/hdisplay/vdisplay==0.

> 
> > +}
> > +
> >   static bool drm_client_target_cloned(struct drm_device *dev,
> >   				     struct drm_connector *connectors[],
> >   				     unsigned int connector_count,
> > -				     const struct drm_display_mode *modes[],
> > +				     struct drm_display_mode modes[],
> >   				     struct drm_client_offset offsets[],
> >   				     bool enabled[], int width, int height)
> >   {
> > @@ -296,15 +301,16 @@ static bool drm_client_target_cloned(struct drm_device *dev,
> >   	for (i = 0; i < connector_count; i++) {
> >   		if (!enabled[i])
> >   			continue;
> > -		modes[i] = drm_connector_pick_cmdline_mode(connectors[i]);
> > -		if (!modes[i]) {
> > +
> > +		drm_mode_copy(&modes[i], drm_connector_pick_cmdline_mode(connectors[i]));
> > +		if (!mode_valid(&modes[i])) {
> 
> You're copying and only then test for validity?

I thought drm_mode_copy() is a nop for NULL. Turns out I was wrong,
hence the v2.

For this specific case I suppose one could also write something like
 if (whatever_mode())
	drm_mode_copy(&modes[i], whatever_mode());
here, but having to repeat oneself is not so great.

We could of course avoid that with
 mode = whatever_mode();
 if (mode)
	drm_mode_copy(&modes[i], mode);
with the downside of needing yet another local
variable.

-- 
Ville Syrjälä
Intel