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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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
