Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Mon, Dec 02, 2024 at 10:07:23PM +0200, Imre Deak wrote: > On Mon, Dec 02, 2024 at 05:35:34PM +0100, Simona Vetter wrote: > > On Tue, Nov 26, 2024 at 06:18:56PM +0200, Imre Deak wrote: > > > Atm when the connector is added to the drm_mode_config::connector_list, > > > the connector may not be fully initialized yet. This is not a problem > > > for user space, which will see the connector only after it's registered > > > later, it could be a problem for in-kernel users looking up connectors > > > via the above list. > > > > > > To resolve the above issue, add a way to separately initialize the DRM > > > core specific parts of the connector and add it to the above list. This > > > will move adding the connector to the list after the properties on the > > > connector have been added, this is ok since these steps don't have a > > > dependency. > > > > > > v2: (Jani) > > > - Let initing DDC as well via drm_connector_init_core(). > > > - Rename __drm_connector_init to drm_connector_init_core_and_add(). > > > > > > Cc: Jani Nikula > > > Reviewed-by: Rodrigo Vivi (v1) > > > Signed-off-by: Imre Deak > > > > So looking at the thread, I guess it'd be good to document some consensus > > stance on kunit tests, and whether or not we're at the point where for new > > things or extensions of functions that already have kunit coverage we need > > them. But I think that's orthogonal. > > > > On the patch itself, I don't think it's the right fix. And by extension, I > > don't think the i915 fix is correct either, because we have a bigger mess > > here: > > > > - GETCONNECTOR does indeed not show a connector that's not yet registers. > > > > - But GETRESOURCES happily lists them. Which means we have a very silly > > race here, which isn't good. > > Right, didn't notice that, it needs to be fixed as well. > > > - I think the correct solution is to move the list_add to the registration > > step, which would also move connectors in-line with other dynamically > > added kms objects like blobs or fbs (although those have a single-step > > registration+init function, so it's less obvious what's going on). > > > > - The same thing applies on the unregister side of things, once a > > connector is unregistered I don't think it should stick around in any > > list. But I'm not entirely sure, would need to check with Lyude to make > > sure this doesn't break anything in mst helpers. > > > > Now implementing this is going to be a bit a mess: > > > > - For static connectors drivers _really_ want the connectors to be on the > > lists, otherwise a lot of the driver setup code just wont work. And > > we've worked towards removing all the explicit drm_connector_register > > calls, readding feels a bit silly. > > > > - I think short-term we could just use the connector type to decide this, > > if it's MST it's a dynamic connector, and the list_add would need to be > > done late in drm_connector_register. > > > > - Once the need pops up for other connectors to be dynamic (like for > > dynamic drm_bridge hotplug) we can add a drm_connector_dynamic_init or > > similar. I don't think splitting up _init further like you do here in > > two functions is correct, because the only place where it's correct to > > add a dynamic/hotplugged connector to the lists is in > > drm_connector_register, not anywhere else. > > Afaiu the above means adding drm_connector_dynamic_init() which would > only init the connector w/o adding it to the connector list (i.e. what > drm_connector_init_core() does) and adding this connector to the list > from drm_connector_register(), hence not needing the drm_connector_add() > interface. drm_connector_dynamic_init() is the more explicit approach, my suggestion was to just hard-code this behavior for dp mst connectors. Which is a bit a hack. Either is fine with me. > I agree this would be better, in the following way: the deferred > registration via drm_connector_register_all() should continue to work - > if drm_connector_register() is called by the driver before the drm > device is registered. So a dynamically inited connector should be added > to the list - if not yet added - early in the function before returning > if the drm device is not yet registered. > > Are you ok with the above for now, also fixing GETRESOURCES by checking > there if the connector is already registered? I don't think you need to check for registered connectors. For dynamic ones this should be impossible, and userspace cannot call any ioctl before drm_dev_register registers all the non-dynamic connectors. > Moving the addition of the connector to the list later could be done > once the deferred registration happens in a different way (i.e. not via > the current connector list). I don't think you need that special case, the current code should work, even for dynamic connectors? Cheers, Sima > > > - It would be really nice if we could add a check to > > drm_connector_register to make sure it's
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Tue, 03 Dec 2024, Maxime Ripard wrote: > On Mon, Dec 02, 2024 at 05:44:27PM +0200, Jani Nikula wrote: >> >> It's super tempting for people to just get their jobs done. If doing >> >> the right thing adds yet another hurdle, we may see more stuff being >> >> added in drivers instead of drm core. >> > >> > I really enjoy hidden threats. >> >> None were implied. That's your interpretation of what I honestly think >> is a plausible outcome. > > I obviously misinterpreted what you were saying then. Sorry for the > whole tone of that mail. Don't worry about it. Likewise, my mail wasn't a stellar example of communication either. Sorry about that. Let's move on. >> I try to push people towards contributing to drm core instead of >> drivers, and it's not always easy as it is. It's just a guess, but >> I'll bet the majority of drm contributors have never run kunit tests >> themselves. > > Right, but I don't think it's worth worrying over either. If one stops > contributing because they are afraid of running one documented command > that takes a few seconds, they would have done so at any other obstacle. > We have much bigger barriers of entry, at several levels. > > All of them are here for a good reason, and because we have collectively > judged that the trade-off between adding a barrier and increasing the > quality of the framework was worth it. > > I believe tests are worth it too. > > But anyway, it's really not what I had in mind. Would you mind drafting some ground rules for what you think the requirements for kunit tests should be? What's the bare minimum, what's the goal? BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Mon, Dec 02, 2024 at 05:44:27PM +0200, Jani Nikula wrote: > >> It's super tempting for people to just get their jobs done. If doing > >> the right thing adds yet another hurdle, we may see more stuff being > >> added in drivers instead of drm core. > > > > I really enjoy hidden threats. > > None were implied. That's your interpretation of what I honestly think > is a plausible outcome. I obviously misinterpreted what you were saying then. Sorry for the whole tone of that mail. > I try to push people towards contributing to drm core instead of > drivers, and it's not always easy as it is. It's just a guess, but > I'll bet the majority of drm contributors have never run kunit tests > themselves. Right, but I don't think it's worth worrying over either. If one stops contributing because they are afraid of running one documented command that takes a few seconds, they would have done so at any other obstacle. We have much bigger barriers of entry, at several levels. All of them are here for a good reason, and because we have collectively judged that the trade-off between adding a barrier and increasing the quality of the framework was worth it. I believe tests are worth it too. But anyway, it's really not what I had in mind. Maxime signature.asc Description: PGP signature
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Tue, Nov 26, 2024 at 06:18:56PM +0200, Imre Deak wrote:
> Atm when the connector is added to the drm_mode_config::connector_list,
> the connector may not be fully initialized yet. This is not a problem
> for user space, which will see the connector only after it's registered
> later, it could be a problem for in-kernel users looking up connectors
> via the above list.
>
> To resolve the above issue, add a way to separately initialize the DRM
> core specific parts of the connector and add it to the above list. This
> will move adding the connector to the list after the properties on the
> connector have been added, this is ok since these steps don't have a
> dependency.
>
> v2: (Jani)
> - Let initing DDC as well via drm_connector_init_core().
> - Rename __drm_connector_init to drm_connector_init_core_and_add().
>
> Cc: Jani Nikula
> Reviewed-by: Rodrigo Vivi (v1)
> Signed-off-by: Imre Deak
So looking at the thread, I guess it'd be good to document some consensus
stance on kunit tests, and whether or not we're at the point where for new
things or extensions of functions that already have kunit coverage we need
them. But I think that's orthogonal.
On the patch itself, I don't think it's the right fix. And by extension, I
don't think the i915 fix is correct either, because we have a bigger mess
here:
- GETCONNECTOR does indeed not show a connector that's not yet registers.
- But GETRESOURCES happily lists them. Which means we have a very silly
race here, which isn't good.
- I think the correct solution is to move the list_add to the registration
step, which would also move connectors in-line with other dynamically
added kms objects like blobs or fbs (although those have a single-step
registration+init function, so it's less obvious what's going on).
- The same thing applies on the unregister side of things, once a
connector is unregistered I don't think it should stick around in any
list. But I'm not entirely sure, would need to check with Lyude to make
sure this doesn't break anything in mst helpers.
Now implementing this is going to be a bit a mess:
- For static connectors drivers _really_ want the connectors to be on the
lists, otherwise a lot of the driver setup code just wont work. And
we've worked towards removing all the explicit drm_connector_register
calls, readding feels a bit silly.
- I think short-term we could just use the connector type to decide this,
if it's MST it's a dynamic connector, and the list_add would need to be
done late in drm_connector_register.
- Once the need pops up for other connectors to be dynamic (like for
dynamic drm_bridge hotplug) we can add a drm_connector_dynamic_init or
similar. I don't think splitting up _init further like you do here in
two functions is correct, because the only place where it's correct to
add a dynamic/hotplugged connector to the lists is in
drm_connector_register, not anywhere else.
- It would be really nice if we could add a check to
drm_connector_register to make sure it's not called for any connector
which is already on the connector list, since that's a driver bug. Of
course this would mean we'd need to split that from the internal version
we call from drm_dev_register.
Unfortunately that's not yet doable because the following todo isn't
completed yet:
https://dri.freedesktop.org/docs/drm/gpu/todo.html#connector-register-unregister-fixes
I guess just add another bullet there?
Does this sound like a plan or am I completely wrong here?
Cheers, Sima
> ---
> drivers/gpu/drm/drm_connector.c | 111 ++--
> include/drm/drm_connector.h | 6 ++
> 2 files changed, 97 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index fc35f47e2849e..fd7acae8656b2 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -218,11 +218,11 @@ void drm_connector_free_work_fn(struct work_struct
> *work)
> }
> }
>
> -static int __drm_connector_init(struct drm_device *dev,
> - struct drm_connector *connector,
> - const struct drm_connector_funcs *funcs,
> - int connector_type,
> - struct i2c_adapter *ddc)
> +static int __drm_connector_init_core(struct drm_device *dev,
> + struct drm_connector *connector,
> + const struct drm_connector_funcs *funcs,
> + int connector_type,
> + struct i2c_adapter *ddc)
> {
> struct drm_mode_config *config = &dev->mode_config;
> int ret;
> @@ -273,6 +273,7 @@ static int __drm_connector_init(struct drm_device *dev,
> /* provide ddc symlink in sysfs */
> connector->ddc = ddc;
>
> + INIT_LIST_HEAD(&connector->head);
> INIT_LIST_HEAD(&connector->global_co
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Mon, Dec 02, 2024 at 04:07:56PM +0100, Maxime Ripard wrote:
> On Mon, Dec 02, 2024 at 03:24:43PM +0200, Imre Deak wrote:
> > On Mon, Dec 02, 2024 at 02:07:36PM +0200, Jani Nikula wrote:
> > > On Mon, 02 Dec 2024, Maxime Ripard wrote:
> > > > It's not about whether we have a problem or not: you introduce new
> > > > framework functions, you need to have kunit tests to check their
> > > > behaviour.
> > >
> > > I don't fundamentally disagree with that goal, but it does seem like a
> > > pretty drastic policy change. I don't recall a discussion where we made
> > > that decision, nor can I find any documentation stating this. Or what
> > > exactly the requirement is; it's totally unclear to me.
> > >
> > > Had I been involved, I would've pointed out that while adding tests is
> > > good, it inevitably increases the friction of adding new stuff to drm
> > > core. It's super tempting for people to just get their jobs done. If
> > > doing the right thing adds yet another hurdle, we may see more stuff
> > > being added in drivers instead of drm core.
> > >
> > > (Case in point, we already hacked around the problem being solved here
> > > with commit d58f65df2dcb ("drm/i915/dp_mst: Fix connector initialization
> > > in intel_dp_add_mst_connector()"). We could've just dropped the ball
> > > right there.)
> >
> > Fwiw, in this case adding tests for drm_connector_init_core() and
> > drm_connector_add() looks simple enough.
> >
> > IIUC it's the 3 testcases in drmm_connector_init_tests[] performed for
> > drm_connector_init_core() and additional 3 test cases checking that (1)
> > drm_connector_init_core() doesn't add the connector to the connector
> > list, (2) drm_connector_add() adds it and (3) drm_connector_add() fails
> > (by not adding the connector to the list and emitting a dmesg WARN) if
> > drm_connector_init_core() was not called for the connector previously.
> > For the last test I actually need to add the corresponding assert/early
> > return to drm_connector_add().
> >
> > If Maxim could confirm the above, I could resend the patchset adding
> > these tests.
>
> Yep, sounds great, thanks!
Ok. The subtest (3) above checking if drm_connector_add() fails as
expected if drm_connector_init_core() was not called before would also
generate a dmesg warn, via a
if (drm_WARN_ON(dev, !connector->funcs))
return;
early return I'm adding to drm_connector_add() in the new version of the
patchset. This fails the kunit test, as always when an error or warn is
printed to the log. I couldn't find a good way to suppress this warn
(don't want to modify the function being tested) to make the testcase
pass. I think this test case could be omitted, since it's tested by all
users implicitly anyway via the above assert. Is this acceptable?
> Maxime
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Mon, Dec 02, 2024 at 05:35:34PM +0100, Simona Vetter wrote: > On Tue, Nov 26, 2024 at 06:18:56PM +0200, Imre Deak wrote: > > Atm when the connector is added to the drm_mode_config::connector_list, > > the connector may not be fully initialized yet. This is not a problem > > for user space, which will see the connector only after it's registered > > later, it could be a problem for in-kernel users looking up connectors > > via the above list. > > > > To resolve the above issue, add a way to separately initialize the DRM > > core specific parts of the connector and add it to the above list. This > > will move adding the connector to the list after the properties on the > > connector have been added, this is ok since these steps don't have a > > dependency. > > > > v2: (Jani) > > - Let initing DDC as well via drm_connector_init_core(). > > - Rename __drm_connector_init to drm_connector_init_core_and_add(). > > > > Cc: Jani Nikula > > Reviewed-by: Rodrigo Vivi (v1) > > Signed-off-by: Imre Deak > > So looking at the thread, I guess it'd be good to document some consensus > stance on kunit tests, and whether or not we're at the point where for new > things or extensions of functions that already have kunit coverage we need > them. But I think that's orthogonal. > > On the patch itself, I don't think it's the right fix. And by extension, I > don't think the i915 fix is correct either, because we have a bigger mess > here: > > - GETCONNECTOR does indeed not show a connector that's not yet registers. > > - But GETRESOURCES happily lists them. Which means we have a very silly > race here, which isn't good. Right, didn't notice that, it needs to be fixed as well. > - I think the correct solution is to move the list_add to the registration > step, which would also move connectors in-line with other dynamically > added kms objects like blobs or fbs (although those have a single-step > registration+init function, so it's less obvious what's going on). > > - The same thing applies on the unregister side of things, once a > connector is unregistered I don't think it should stick around in any > list. But I'm not entirely sure, would need to check with Lyude to make > sure this doesn't break anything in mst helpers. > > Now implementing this is going to be a bit a mess: > > - For static connectors drivers _really_ want the connectors to be on the > lists, otherwise a lot of the driver setup code just wont work. And > we've worked towards removing all the explicit drm_connector_register > calls, readding feels a bit silly. > > - I think short-term we could just use the connector type to decide this, > if it's MST it's a dynamic connector, and the list_add would need to be > done late in drm_connector_register. > > - Once the need pops up for other connectors to be dynamic (like for > dynamic drm_bridge hotplug) we can add a drm_connector_dynamic_init or > similar. I don't think splitting up _init further like you do here in > two functions is correct, because the only place where it's correct to > add a dynamic/hotplugged connector to the lists is in > drm_connector_register, not anywhere else. Afaiu the above means adding drm_connector_dynamic_init() which would only init the connector w/o adding it to the connector list (i.e. what drm_connector_init_core() does) and adding this connector to the list from drm_connector_register(), hence not needing the drm_connector_add() interface. I agree this would be better, in the following way: the deferred registration via drm_connector_register_all() should continue to work - if drm_connector_register() is called by the driver before the drm device is registered. So a dynamically inited connector should be added to the list - if not yet added - early in the function before returning if the drm device is not yet registered. Are you ok with the above for now, also fixing GETRESOURCES by checking there if the connector is already registered? Moving the addition of the connector to the list later could be done once the deferred registration happens in a different way (i.e. not via the current connector list). > - It would be really nice if we could add a check to > drm_connector_register to make sure it's not called for any connector > which is already on the connector list, since that's a driver bug. Of > course this would mean we'd need to split that from the internal version > we call from drm_dev_register. > > Unfortunately that's not yet doable because the following todo isn't > completed yet: > > > https://dri.freedesktop.org/docs/drm/gpu/todo.html#connector-register-unregister-fixes > > I guess just add another bullet there? > > Does this sound like a plan or am I completely wrong here? > > Cheers, Sima > > > > --- > > drivers/gpu/drm/drm_connector.c | 111 ++-- > > include/drm/drm_connector.h | 6 ++ > > 2 files changed, 97 insertions(+), 20 deletions(-) > > >
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Mon, Dec 02, 2024 at 02:07:36PM +0200, Jani Nikula wrote:
> On Mon, 02 Dec 2024, Maxime Ripard wrote:
> > It's not about whether we have a problem or not: you introduce new
> > framework functions, you need to have kunit tests to check their
> > behaviour.
>
> I don't fundamentally disagree with that goal,
You don't really have to agree. You asked for my review, you have it.
> but it does seem like a pretty drastic policy change. I don't recall a
> discussion where we made that decision, nor can I find any
> documentation stating this. Or what exactly the requirement is; it's
> totally unclear to me.
There isn't, because there's no such policy, even though it's definitely
something I'd like. This situation is different though:
drm_connector_init is already a function that is being tested. It seems
natural to not dilute testing when adding new variant, disregarding what
the policy of the rest of the framework is.
> Had I been involved, I would've pointed out that while adding tests is
> good, it inevitably increases the friction of adding new stuff to drm
> core.
You also know what increases the friction of adding new stuff? Adding
new stuff. Or writing documentation. Or writing commit log. Or sending
emails / making pull requests. Or asking for cross-reviews. Or having an
open-source user-space requirement. It seems pretty arbitrary to draw
the line right where testing starts.
> It's super tempting for people to just get their jobs done. If doing
> the right thing adds yet another hurdle, we may see more stuff being
> added in drivers instead of drm core.
I really enjoy hidden threats. And it's not like i915 is a great example
there.
> (Case in point, we already hacked around the problem being solved here
> with commit d58f65df2dcb ("drm/i915/dp_mst: Fix connector initialization
> in intel_dp_add_mst_connector()"). We could've just dropped the ball
> right there.)
Case in point indeed.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Mon, Dec 02, 2024 at 03:24:43PM +0200, Imre Deak wrote:
> On Mon, Dec 02, 2024 at 02:07:36PM +0200, Jani Nikula wrote:
> > On Mon, 02 Dec 2024, Maxime Ripard wrote:
> > > It's not about whether we have a problem or not: you introduce new
> > > framework functions, you need to have kunit tests to check their
> > > behaviour.
> >
> > I don't fundamentally disagree with that goal, but it does seem like a
> > pretty drastic policy change. I don't recall a discussion where we made
> > that decision, nor can I find any documentation stating this. Or what
> > exactly the requirement is; it's totally unclear to me.
> >
> > Had I been involved, I would've pointed out that while adding tests is
> > good, it inevitably increases the friction of adding new stuff to drm
> > core. It's super tempting for people to just get their jobs done. If
> > doing the right thing adds yet another hurdle, we may see more stuff
> > being added in drivers instead of drm core.
> >
> > (Case in point, we already hacked around the problem being solved here
> > with commit d58f65df2dcb ("drm/i915/dp_mst: Fix connector initialization
> > in intel_dp_add_mst_connector()"). We could've just dropped the ball
> > right there.)
>
> Fwiw, in this case adding tests for drm_connector_init_core() and
> drm_connector_add() looks simple enough.
>
> IIUC it's the 3 testcases in drmm_connector_init_tests[] performed for
> drm_connector_init_core() and additional 3 test cases checking that (1)
> drm_connector_init_core() doesn't add the connector to the connector
> list, (2) drm_connector_add() adds it and (3) drm_connector_add() fails
> (by not adding the connector to the list and emitting a dmesg WARN) if
> drm_connector_init_core() was not called for the connector previously.
> For the last test I actually need to add the corresponding assert/early
> return to drm_connector_add().
>
> If Maxim could confirm the above, I could resend the patchset adding
> these tests.
Yep, sounds great, thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Mon, 02 Dec 2024, Maxime Ripard wrote: > On Mon, Dec 02, 2024 at 02:07:36PM +0200, Jani Nikula wrote: >> On Mon, 02 Dec 2024, Maxime Ripard wrote: >> > It's not about whether we have a problem or not: you introduce new >> > framework functions, you need to have kunit tests to check their >> > behaviour. >> >> I don't fundamentally disagree with that goal, > > You don't really have to agree. You asked for my review, you have it. > >> but it does seem like a pretty drastic policy change. I don't recall a >> discussion where we made that decision, nor can I find any >> documentation stating this. Or what exactly the requirement is; it's >> totally unclear to me. > > There isn't, because there's no such policy, even though it's definitely > something I'd like. This situation is different though: > drm_connector_init is already a function that is being tested. It seems > natural to not dilute testing when adding new variant, disregarding what > the policy of the rest of the framework is. "You do X, you need do have Y" coming from a maintainer sure sounded like hard rules. I was surprised. >> It's super tempting for people to just get their jobs done. If doing >> the right thing adds yet another hurdle, we may see more stuff being >> added in drivers instead of drm core. > > I really enjoy hidden threats. None were implied. That's your interpretation of what I honestly think is a plausible outcome. I try to push people towards contributing to drm core instead of drivers, and it's not always easy as it is. It's just a guess, but I'll bet the majority of drm contributors have never run kunit tests themselves. > And it's not like i915 is a great example there. Sincerely, is this the level of discussion we really want to have? BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Mon, Dec 02, 2024 at 02:07:36PM +0200, Jani Nikula wrote:
> On Mon, 02 Dec 2024, Maxime Ripard wrote:
> > It's not about whether we have a problem or not: you introduce new
> > framework functions, you need to have kunit tests to check their
> > behaviour.
>
> I don't fundamentally disagree with that goal, but it does seem like a
> pretty drastic policy change. I don't recall a discussion where we made
> that decision, nor can I find any documentation stating this. Or what
> exactly the requirement is; it's totally unclear to me.
>
> Had I been involved, I would've pointed out that while adding tests is
> good, it inevitably increases the friction of adding new stuff to drm
> core. It's super tempting for people to just get their jobs done. If
> doing the right thing adds yet another hurdle, we may see more stuff
> being added in drivers instead of drm core.
>
> (Case in point, we already hacked around the problem being solved here
> with commit d58f65df2dcb ("drm/i915/dp_mst: Fix connector initialization
> in intel_dp_add_mst_connector()"). We could've just dropped the ball
> right there.)
Fwiw, in this case adding tests for drm_connector_init_core() and
drm_connector_add() looks simple enough.
IIUC it's the 3 testcases in drmm_connector_init_tests[] performed for
drm_connector_init_core() and additional 3 test cases checking that (1)
drm_connector_init_core() doesn't add the connector to the connector
list, (2) drm_connector_add() adds it and (3) drm_connector_add() fails
(by not adding the connector to the list and emitting a dmesg WARN) if
drm_connector_init_core() was not called for the connector previously.
For the last test I actually need to add the corresponding assert/early
return to drm_connector_add().
If Maxim could confirm the above, I could resend the patchset adding
these tests.
--Imre
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Mon, 02 Dec 2024, Maxime Ripard wrote:
> It's not about whether we have a problem or not: you introduce new
> framework functions, you need to have kunit tests to check their
> behaviour.
I don't fundamentally disagree with that goal, but it does seem like a
pretty drastic policy change. I don't recall a discussion where we made
that decision, nor can I find any documentation stating this. Or what
exactly the requirement is; it's totally unclear to me.
Had I been involved, I would've pointed out that while adding tests is
good, it inevitably increases the friction of adding new stuff to drm
core. It's super tempting for people to just get their jobs done. If
doing the right thing adds yet another hurdle, we may see more stuff
being added in drivers instead of drm core.
(Case in point, we already hacked around the problem being solved here
with commit d58f65df2dcb ("drm/i915/dp_mst: Fix connector initialization
in intel_dp_add_mst_connector()"). We could've just dropped the ball
right there.)
BR,
Jani.
--
Jani Nikula, Intel
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Fri, Nov 29, 2024 at 06:12:01PM +0200, Imre Deak wrote: > On Fri, Nov 29, 2024 at 03:46:28PM +0100, Maxime Ripard wrote: > > On Fri, Nov 29, 2024 at 04:26:01PM +0200, Imre Deak wrote: > > > Adding more people from DRM core domain. Any comment, objection to this > > > change? > > > > > > On Tue, Nov 26, 2024 at 06:18:56PM +0200, Imre Deak wrote: > > > > Atm when the connector is added to the drm_mode_config::connector_list, > > > > the connector may not be fully initialized yet. This is not a problem > > > > for user space, which will see the connector only after it's registered > > > > later, it could be a problem for in-kernel users looking up connectors > > > > via the above list. > > > > It could be, or it actually is a problem? If so, in what situation? > > An actual problem is that after an MST connector is created and added to > the connector list, the connector could be probed without the connector > being fully initialized during a hotplug event handling along with all > the other connectors on the list. The connector's helper functions could > be still unset leading to a NULL deref while trying to call the > connector's detect/detect_ctx callbacks, or if these callbacks are set > already, the detect handler could see a connector which is not yet > initialized fully. Ack. Like I said to Jani, this needs to be in the commit log then. > > > > To resolve the above issue, add a way to separately initialize the DRM > > > > core specific parts of the connector and add it to the above list. This > > > > will move adding the connector to the list after the properties on the > > > > connector have been added, this is ok since these steps don't have a > > > > dependency. > > > > > > > > v2: (Jani) > > > > - Let initing DDC as well via drm_connector_init_core(). > > > > - Rename __drm_connector_init to drm_connector_init_core_and_add(). > > > > > > > > Cc: Jani Nikula > > > > Reviewed-by: Rodrigo Vivi (v1) > > > > Signed-off-by: Imre Deak > > > > If we do have an actual problem to solve, we'll need kunit tests too. > > I don't have a good idea for this. The problem is not in a parituclar > function, rather in the order a connector is initialized and added to > the connector list. The above is an actual problem though, so I don't > think fixing that should be blocked by this. It's not about whether we have a problem or not: you introduce new framework functions, you need to have kunit tests to check their behaviour. Maxime signature.asc Description: PGP signature
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Fri, Nov 29, 2024 at 05:58:56PM +0200, Jani Nikula wrote: > On Fri, 29 Nov 2024, Maxime Ripard wrote: > > On Fri, Nov 29, 2024 at 04:26:01PM +0200, Imre Deak wrote: > >> Adding more people from DRM core domain. Any comment, objection to this > >> change? > >> > >> On Tue, Nov 26, 2024 at 06:18:56PM +0200, Imre Deak wrote: > >> > Atm when the connector is added to the drm_mode_config::connector_list, > >> > the connector may not be fully initialized yet. This is not a problem > >> > for user space, which will see the connector only after it's registered > >> > later, it could be a problem for in-kernel users looking up connectors > >> > via the above list. > > > > It could be, or it actually is a problem? If so, in what situation? > > It's an actual problem. > > While in most cases connectors are created at probe, for DP MST they're > created at runtime via the ->add_connector hook. We want to call > drm_connector_init() on them soon in that hook, so we can pass the > connector around and expect it to have connector->dev etc. initialized > while we continue its initialization. > > But there's a race. As soon as we call drm_connector_init(), the > connector gets added to dev->mode_config.connector_list, and any drm > code may discover it. For example connector polling. And we might not be > done with the initialization yet. Ack. This should be in the commit log then. > >> > To resolve the above issue, add a way to separately initialize the DRM > >> > core specific parts of the connector and add it to the above list. This > >> > will move adding the connector to the list after the properties on the > >> > connector have been added, this is ok since these steps don't have a > >> > dependency. > >> > > >> > v2: (Jani) > >> > - Let initing DDC as well via drm_connector_init_core(). > >> > - Rename __drm_connector_init to drm_connector_init_core_and_add(). > >> > > >> > Cc: Jani Nikula > >> > Reviewed-by: Rodrigo Vivi (v1) > >> > Signed-off-by: Imre Deak > > > > If we do have an actual problem to solve, we'll need kunit tests too. > > That's not an unreasonable request, per se, but what exactly should they > test? That the new init core didn't add stuff to the list, and the new > add connector did? Everything we test with drm_connector_init* already, plus the fact that we now might have callers that call add without init, so we need to test that too. Maxime signature.asc Description: PGP signature
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Fri, Nov 29, 2024 at 03:46:28PM +0100, Maxime Ripard wrote: > On Fri, Nov 29, 2024 at 04:26:01PM +0200, Imre Deak wrote: > > Adding more people from DRM core domain. Any comment, objection to this > > change? > > > > On Tue, Nov 26, 2024 at 06:18:56PM +0200, Imre Deak wrote: > > > Atm when the connector is added to the drm_mode_config::connector_list, > > > the connector may not be fully initialized yet. This is not a problem > > > for user space, which will see the connector only after it's registered > > > later, it could be a problem for in-kernel users looking up connectors > > > via the above list. > > It could be, or it actually is a problem? If so, in what situation? An actual problem is that after an MST connector is created and added to the connector list, the connector could be probed without the connector being fully initialized during a hotplug event handling along with all the other connectors on the list. The connector's helper functions could be still unset leading to a NULL deref while trying to call the connector's detect/detect_ctx callbacks, or if these callbacks are set already, the detect handler could see a connector which is not yet initialized fully. > > > To resolve the above issue, add a way to separately initialize the DRM > > > core specific parts of the connector and add it to the above list. This > > > will move adding the connector to the list after the properties on the > > > connector have been added, this is ok since these steps don't have a > > > dependency. > > > > > > v2: (Jani) > > > - Let initing DDC as well via drm_connector_init_core(). > > > - Rename __drm_connector_init to drm_connector_init_core_and_add(). > > > > > > Cc: Jani Nikula > > > Reviewed-by: Rodrigo Vivi (v1) > > > Signed-off-by: Imre Deak > > If we do have an actual problem to solve, we'll need kunit tests too. I don't have a good idea for this. The problem is not in a parituclar function, rather in the order a connector is initialized and added to the connector list. The above is an actual problem though, so I don't think fixing that should be blocked by this. > > Maxime
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Fri, 29 Nov 2024, Maxime Ripard wrote: > On Fri, Nov 29, 2024 at 04:26:01PM +0200, Imre Deak wrote: >> Adding more people from DRM core domain. Any comment, objection to this >> change? >> >> On Tue, Nov 26, 2024 at 06:18:56PM +0200, Imre Deak wrote: >> > Atm when the connector is added to the drm_mode_config::connector_list, >> > the connector may not be fully initialized yet. This is not a problem >> > for user space, which will see the connector only after it's registered >> > later, it could be a problem for in-kernel users looking up connectors >> > via the above list. > > It could be, or it actually is a problem? If so, in what situation? It's an actual problem. While in most cases connectors are created at probe, for DP MST they're created at runtime via the ->add_connector hook. We want to call drm_connector_init() on them soon in that hook, so we can pass the connector around and expect it to have connector->dev etc. initialized while we continue its initialization. But there's a race. As soon as we call drm_connector_init(), the connector gets added to dev->mode_config.connector_list, and any drm code may discover it. For example connector polling. And we might not be done with the initialization yet. >> > To resolve the above issue, add a way to separately initialize the DRM >> > core specific parts of the connector and add it to the above list. This >> > will move adding the connector to the list after the properties on the >> > connector have been added, this is ok since these steps don't have a >> > dependency. >> > >> > v2: (Jani) >> > - Let initing DDC as well via drm_connector_init_core(). >> > - Rename __drm_connector_init to drm_connector_init_core_and_add(). >> > >> > Cc: Jani Nikula >> > Reviewed-by: Rodrigo Vivi (v1) >> > Signed-off-by: Imre Deak > > If we do have an actual problem to solve, we'll need kunit tests too. That's not an unreasonable request, per se, but what exactly should they test? That the new init core didn't add stuff to the list, and the new add connector did? BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
On Fri, Nov 29, 2024 at 04:26:01PM +0200, Imre Deak wrote: > Adding more people from DRM core domain. Any comment, objection to this > change? > > On Tue, Nov 26, 2024 at 06:18:56PM +0200, Imre Deak wrote: > > Atm when the connector is added to the drm_mode_config::connector_list, > > the connector may not be fully initialized yet. This is not a problem > > for user space, which will see the connector only after it's registered > > later, it could be a problem for in-kernel users looking up connectors > > via the above list. It could be, or it actually is a problem? If so, in what situation? > > To resolve the above issue, add a way to separately initialize the DRM > > core specific parts of the connector and add it to the above list. This > > will move adding the connector to the list after the properties on the > > connector have been added, this is ok since these steps don't have a > > dependency. > > > > v2: (Jani) > > - Let initing DDC as well via drm_connector_init_core(). > > - Rename __drm_connector_init to drm_connector_init_core_and_add(). > > > > Cc: Jani Nikula > > Reviewed-by: Rodrigo Vivi (v1) > > Signed-off-by: Imre Deak If we do have an actual problem to solve, we'll need kunit tests too. Maxime signature.asc Description: PGP signature
Re: [PATCH v2 1/4] drm/dp: Add a way to init/add a connector in separate steps
Adding more people from DRM core domain. Any comment, objection to this
change?
On Tue, Nov 26, 2024 at 06:18:56PM +0200, Imre Deak wrote:
> Atm when the connector is added to the drm_mode_config::connector_list,
> the connector may not be fully initialized yet. This is not a problem
> for user space, which will see the connector only after it's registered
> later, it could be a problem for in-kernel users looking up connectors
> via the above list.
>
> To resolve the above issue, add a way to separately initialize the DRM
> core specific parts of the connector and add it to the above list. This
> will move adding the connector to the list after the properties on the
> connector have been added, this is ok since these steps don't have a
> dependency.
>
> v2: (Jani)
> - Let initing DDC as well via drm_connector_init_core().
> - Rename __drm_connector_init to drm_connector_init_core_and_add().
>
> Cc: Jani Nikula
> Reviewed-by: Rodrigo Vivi (v1)
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/drm_connector.c | 111 ++--
> include/drm/drm_connector.h | 6 ++
> 2 files changed, 97 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index fc35f47e2849e..fd7acae8656b2 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -218,11 +218,11 @@ void drm_connector_free_work_fn(struct work_struct
> *work)
> }
> }
>
> -static int __drm_connector_init(struct drm_device *dev,
> - struct drm_connector *connector,
> - const struct drm_connector_funcs *funcs,
> - int connector_type,
> - struct i2c_adapter *ddc)
> +static int __drm_connector_init_core(struct drm_device *dev,
> + struct drm_connector *connector,
> + const struct drm_connector_funcs *funcs,
> + int connector_type,
> + struct i2c_adapter *ddc)
> {
> struct drm_mode_config *config = &dev->mode_config;
> int ret;
> @@ -273,6 +273,7 @@ static int __drm_connector_init(struct drm_device *dev,
> /* provide ddc symlink in sysfs */
> connector->ddc = ddc;
>
> + INIT_LIST_HEAD(&connector->head);
> INIT_LIST_HEAD(&connector->global_connector_list_entry);
> INIT_LIST_HEAD(&connector->probed_modes);
> INIT_LIST_HEAD(&connector->modes);
> @@ -288,14 +289,6 @@ static int __drm_connector_init(struct drm_device *dev,
>
> drm_connector_get_cmdline_mode(connector);
>
> - /* We should add connectors at the end to avoid upsetting the connector
> - * index too much.
> - */
> - spin_lock_irq(&config->connector_list_lock);
> - list_add_tail(&connector->head, &config->connector_list);
> - config->num_connector++;
> - spin_unlock_irq(&config->connector_list_lock);
> -
> if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL &&
> connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
> drm_connector_attach_edid_property(connector);
> @@ -332,6 +325,86 @@ static int __drm_connector_init(struct drm_device *dev,
> return ret;
> }
>
> +/**
> + * drm_connector_init_core - Initialize the core state of a preallocated
> connector
> + * @dev: DRM device
> + * @connector: the connector to init
> + * @funcs: callbacks for this connector
> + * @connector_type: user visible type of the connector
> + * @ddc: pointer to the associated ddc adapter
> + *
> + * Initialises the core state of preallocated connector. This is
> + * equivalent to drm_connector_init(), without adding the connector to
> + * drm_mode_config::connector_list. This call must be followed by calling
> + * drm_connector_add() during initialization to expose the connector to
> + * in-kernel users via the above list.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_connector_init_core(struct drm_device *dev,
> + struct drm_connector *connector,
> + const struct drm_connector_funcs *funcs,
> + int connector_type,
> + struct i2c_adapter *ddc)
> +{
> + if (drm_WARN_ON(dev, !(funcs && funcs->destroy)))
> + return -EINVAL;
> +
> + return __drm_connector_init_core(dev, connector, funcs, connector_type,
> ddc);
> +}
> +EXPORT_SYMBOL(drm_connector_init_core);
> +
> +/**
> + * drm_connector_add - Add the connector
> + * @connector: the connector to add
> + *
> + * Add the connector to the drm_mode_config::connector_list, exposing the
> + * connector to in-kernel users. This call must be preceded by a call to
> + * drm_connector_init_core().
> + */
> +void drm_connector_add(struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_mode_config
