On Fri, Apr 05, 2024 at 06:32:46AM +0300, Dmitry Baryshkov wrote:
> On Thu, Apr 04, 2024 at 11:33:28PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > crtc->mode is legacy junk and shouldn't really be used with
> > atomic drivers.
> >
> > Most (all?) atomic drivers do end up still calling
> > drm_atomic_helper_update_legacy_modeset_state() at some
> > point, so crtc->mode does still get populated, and this
> > does work for now. But eventually would be nice to eliminate
> > all the legacy stuff from atomic drivers.
> >
> > Switching to crtc->state->mode would require some bigger
> > changes however, as we currently drop the crtc->mutex
> > before we're done using the mode. So leave the junk in
> > for now and just add a FIXME to remind us that this
> > needs fixing.
>
>
> What about using allocated duplicate modes to fill modes[] array? This
> requires additional allocations, but it will solve most if not all modes
> lifetime issues.
I think there are two obvious solutions:
1. drm_mode_duplicate() as you suggest
upside: existing 'modes[i] != NULL' checks work as is
downside: introduces more error paths due to potential kmalloc() fails
2. Make modes[] and array of structs rather than pointers
up/downsides: the opposite of option 1
So neither is a trivial search and replace job.
>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_client_modeset.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> > index 2b7d0be04911..8ef03608b424 100644
> > --- a/drivers/gpu/drm/drm_client_modeset.c
> > +++ b/drivers/gpu/drm/drm_client_modeset.c
> > @@ -699,6 +699,10 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> > *
> > * This is crtc->mode and not crtc->state->mode for the
> > * fastboot check to work correctly.
> > + *
> > + * FIXME using legacy crtc->mode with atomic drivers
> > + * is dodgy. Switch to crtc->state->mode, after taking
> > + * care of the resulting locking/lifetime issues.
> > */
> > DRM_DEBUG_KMS("looking for current mode on connector %s\n",
> > connector->name);
> > --
> > 2.43.2
> >
>
> --
> With best wishes
> Dmitry
--
Ville Syrjälä
Intel
