Re: [Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Andy Shevchenko]

On Wed, May 5, 2021 at 1:30 PM Hans de Goede  wrote:
> On 5/5/21 12:02 PM, Andy Shevchenko wrote:

...

> But we do really need to document the behavior better here
> in the kdoc for fwnode_get_next_child_node() and
> device_get_next_child_node().

Totally agree!

> of_get_next_child has this bit, which applies to those too:
>
>  *  Returns a node pointer with refcount incremented, use of_node_put() on
>  *  it when done. Returns NULL when prev is the last child. Decrements the
>  *  refcount of prev.
>
> I'll prepare a patch to add this to the kdoc for fwnode_get_next_child_node()
> and device_get_next_child_node() once I'm done with readying v3 of this 
> series.

Thanks!

-- 
With Best Regards,
Andy Shevchenko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Hans de Goede]

Hi,

On 5/5/21 12:02 PM, Andy Shevchenko wrote:
> On Wed, May 5, 2021 at 12:28 PM Hans de Goede  wrote:
>> On 5/5/21 11:17 AM, Andy Shevchenko wrote:
>>> On Wed, May 5, 2021 at 12:07 PM Hans de Goede  wrote:
 On 5/4/21 9:52 AM, Andy Shevchenko wrote:
> On Monday, May 3, 2021, Hans de Goede  > wrote:
>>>
>>> ...
>>>
> +   fwnode = device_get_next_child_node(kdev, fwnode);
>>>
> Who is dropping reference counting on fwnode ?

 We are dealing with ACPI fwnode-s here and those are not ref-counted, they
 are embedded inside a struct acpi_device and their lifetime is tied to
 that struct. They should probably still be ref-counted (with the count
 never dropping to 0) so that the generic fwnode functions behave the same
 anywhere but atm the ACPI nodes are not refcounted, see: 
 acpi_get_next_subnode()
 in drivers/acpi/property.c which is the get_next_child_node() 
 implementation
 for ACPI fwnode-s.
>>>
>>> Yes, ACPI currently is exceptional, but fwnode API is not.
>>> If you may guarantee that this case won't ever be outside of ACPI
>>
>> Yes I can guarantee that currently this code (which is for the i915
>> driver only) only deals with ACPI fwnodes.
>>
>>> and
>>> even though if ACPI won't ever gain a reference counting for fwnodes,
>>> we can leave it as is.
>>
>> Would it not be better to add fake ref-counting to the ACPI fwnode
>> next_child_node() op though. I believe just getting a reference
>> on the return value there should work fine; and then all fwnode
>> implementations would be consistent ?
> 
> But it's already there by absent put/get callbacks.

Ah, I completely missed that the put/get-s are actually done
through function pointers in fwnode_operations. I assumed that there
was a kref embedded inside the fwnode_handle struct and that they
operated directly on that.

So this whole discussion is entirely based on that misunderstanding,
my bad, sorry.

So yes you are right, things are already consistent thanks to the
absent put/get callbacks.

But we do really need to document the behavior better here
in the kdoc for fwnode_get_next_child_node() and
device_get_next_child_node().

of_get_next_child has this bit, which applies to those too:

 *  Returns a node pointer with refcount incremented, use of_node_put() on
 *  it when done. Returns NULL when prev is the last child. Decrements the
 *  refcount of prev.

I'll prepare a patch to add this to the kdoc for fwnode_get_next_child_node()
and device_get_next_child_node() once I'm done with readying v3 of this series.

Regards,

Hans

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Sakari Ailus]

Hi Andy, Hans,

On Wed, May 05, 2021 at 12:17:14PM +0300, Andy Shevchenko wrote:
> On Wed, May 5, 2021 at 12:07 PM Hans de Goede  wrote:
> > On 5/4/21 9:52 AM, Andy Shevchenko wrote:
> > > On Monday, May 3, 2021, Hans de Goede  > > > wrote:
> 
> ...
> 
> > > +   fwnode = device_get_next_child_node(kdev, fwnode);
> 
> > > Who is dropping reference counting on fwnode ?
> >
> > We are dealing with ACPI fwnode-s here and those are not ref-counted, they
> > are embedded inside a struct acpi_device and their lifetime is tied to
> > that struct. They should probably still be ref-counted (with the count
> > never dropping to 0) so that the generic fwnode functions behave the same
> > anywhere but atm the ACPI nodes are not refcounted, see: 
> > acpi_get_next_subnode()
> > in drivers/acpi/property.c which is the get_next_child_node() implementation
> > for ACPI fwnode-s.
> 
> Yes, ACPI currently is exceptional, but fwnode API is not.
> If you may guarantee that this case won't ever be outside of ACPI and
> even though if ACPI won't ever gain a reference counting for fwnodes,
> we can leave it as is.
> 
> > > I’m in the middle of a pile of fixes for fwnode refcounting when
> > > for_each_child or get_next_child is used. So, please double check you
> > > drop a reference.
> >
> > The kdoc comments on device_get_next_child_node() / 
> > fwnode_get_next_child_node()
> > do not mention anything about these functions returning a reference.
> 
> It's possible. I dunno if it had to be done earlier. Sakari?

The fwnode API has worked with references for a long time, looks like from
the very beginning of it, as in patch
8a0662d9ed2968e1186208336a8e1fab3fdfea63 .

If you're expecting an fwnode family of function returning another node,
then that function has to have taken a reference to that node before
returning it. Otherwise there's no guarantee that node is still there when
it is returned.

It may be this is not documented for every function doing so. That should
probably be added.

-- 
Kind regards,

Sakari Ailus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Andy Shevchenko]

On Wed, May 5, 2021 at 12:28 PM Hans de Goede  wrote:
> On 5/5/21 11:17 AM, Andy Shevchenko wrote:
> > On Wed, May 5, 2021 at 12:07 PM Hans de Goede  wrote:
> >> On 5/4/21 9:52 AM, Andy Shevchenko wrote:
> >>> On Monday, May 3, 2021, Hans de Goede  >>> > wrote:
> >
> > ...
> >
> >>> +   fwnode = device_get_next_child_node(kdev, fwnode);
> >
> >>> Who is dropping reference counting on fwnode ?
> >>
> >> We are dealing with ACPI fwnode-s here and those are not ref-counted, they
> >> are embedded inside a struct acpi_device and their lifetime is tied to
> >> that struct. They should probably still be ref-counted (with the count
> >> never dropping to 0) so that the generic fwnode functions behave the same
> >> anywhere but atm the ACPI nodes are not refcounted, see: 
> >> acpi_get_next_subnode()
> >> in drivers/acpi/property.c which is the get_next_child_node() 
> >> implementation
> >> for ACPI fwnode-s.
> >
> > Yes, ACPI currently is exceptional, but fwnode API is not.
> > If you may guarantee that this case won't ever be outside of ACPI
>
> Yes I can guarantee that currently this code (which is for the i915
> driver only) only deals with ACPI fwnodes.
>
> > and
> > even though if ACPI won't ever gain a reference counting for fwnodes,
> > we can leave it as is.
>
> Would it not be better to add fake ref-counting to the ACPI fwnode
> next_child_node() op though. I believe just getting a reference
> on the return value there should work fine; and then all fwnode
> implementations would be consistent ?

But it's already there by absent put/get callbacks. On fwnode level it
is like you described. So, talking for a good pattern we have to call
the fwnode_handle_put() independently and always for for_each_child
and get_next_child usages.

> (note I did not check that the of and swnode code do return
> a reference but I would assume so).

Yes, it's only ACPI that survives w/o reference counting.

> >>> I’m in the middle of a pile of fixes for fwnode refcounting when 
> >>> for_each_child or get_next_child is used. So, please double check you 
> >>> drop a reference.
> >>
> >> The kdoc comments on device_get_next_child_node() / 
> >> fwnode_get_next_child_node()
> >> do not mention anything about these functions returning a reference.
> >
> > It's possible. I dunno if it had to be done earlier. Sakari?
> >
> >> So I think we need to first make up our mind here how we want this all to
> >> work and then fix the actual implementation and docs before fixing callers.
> >
> > We have already issues, so I prefer not to wait for a documentation
> > update, because for old kernels it will still be an issue.
>
> I wonder if we really have issues though, in practice fwnodes are
> generated from an devicetree or ACPI tables (or by platform codes
> adding swnodes) and then these pretty much stick around for ever.

Overlays. Not for ever.

> IOW the initial refcount of 1 is never dropped at least for of-nodes
> and ACPI nodes.

>  I know there are some exceptions like device-tree
> overlays which I guess may also be dynamically removed again, but those
> exceptions are not widely used.

ACPI overlays are quite used (at least by two people I know and a few
more that asked questions about them here and there), but luckily it
doesn't require refcounting (yet?).

> And if we forget to drop a reference in the worst case we have a small
> non-re-occuring (so not growing) memleak.

And is it good to provoke all kinds of tools (kmemleak, *SANs, etc)? I
do not think so. If we are writing good code it should be good enough.

> Where as if we start adding
> put() calls everywhere we may end up freeing things which are still
> in use; or dropping refcounts below 0 triggering WARNs in various
> places (IIRC).

Which is good. Then we will discover real issues.

> So it seems the cure is potentially worse then the disease in this
> case.

I tend to disagree with you. How in this case we can go below 0 in
case we know that we took a counter? If somewhere else the code will
do that, it is a problem that has to be fixed on case-by-case basis.

> So if you want to work on this, then IMHO it would be best to first make
> sure that all the fwnode implementations behave in the same way wrt
> ref-counting, before adding the missing put() calls in various
> places.
>
> And once the behavior is consistent

It's consistent now independently of the beneath layer from fwnode API p.o.v.

> then we can also document this
> properly making it easier for other people to do the right thing
> when using these functions.



-- 
With Best Regards,
Andy Shevchenko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Hans de Goede]

Hi,

On 5/5/21 11:17 AM, Andy Shevchenko wrote:
> On Wed, May 5, 2021 at 12:07 PM Hans de Goede  wrote:
>> On 5/4/21 9:52 AM, Andy Shevchenko wrote:
>>> On Monday, May 3, 2021, Hans de Goede >> > wrote:
> 
> ...
> 
>>> +   fwnode = device_get_next_child_node(kdev, fwnode);
> 
>>> Who is dropping reference counting on fwnode ?
>>
>> We are dealing with ACPI fwnode-s here and those are not ref-counted, they
>> are embedded inside a struct acpi_device and their lifetime is tied to
>> that struct. They should probably still be ref-counted (with the count
>> never dropping to 0) so that the generic fwnode functions behave the same
>> anywhere but atm the ACPI nodes are not refcounted, see: 
>> acpi_get_next_subnode()
>> in drivers/acpi/property.c which is the get_next_child_node() implementation
>> for ACPI fwnode-s.
> 
> Yes, ACPI currently is exceptional, but fwnode API is not.
> If you may guarantee that this case won't ever be outside of ACPI

Yes I can guarantee that currently this code (which is for the i915
driver only) only deals with ACPI fwnodes.

> and
> even though if ACPI won't ever gain a reference counting for fwnodes,
> we can leave it as is.

Would it not be better to add fake ref-counting to the ACPI fwnode
next_child_node() op though. I believe just getting a reference
on the return value there should work fine; and then all fwnode
implementations would be consistent ?

(note I did not check that the of and swnode code do return
a reference but I would assume so).

>>> I’m in the middle of a pile of fixes for fwnode refcounting when 
>>> for_each_child or get_next_child is used. So, please double check you drop 
>>> a reference.
>>
>> The kdoc comments on device_get_next_child_node() / 
>> fwnode_get_next_child_node()
>> do not mention anything about these functions returning a reference.
> 
> It's possible. I dunno if it had to be done earlier. Sakari?
> 
>> So I think we need to first make up our mind here how we want this all to
>> work and then fix the actual implementation and docs before fixing callers.
> 
> We have already issues, so I prefer not to wait for a documentation
> update, because for old kernels it will still be an issue.

I wonder if we really have issues though, in practice fwnodes are
generated from an devicetree or ACPI tables (or by platform codes
adding swnodes) and then these pretty much stick around for ever.

IOW the initial refcount of 1 is never dropped at least for of-nodes
and ACPI nodes.  I know there are some exceptions like device-tree
overlays which I guess may also be dynamically removed again, but those
exceptions are not widely used.

And if we forget to drop a reference in the worst case we have a small
non-re-occuring (so not growing) memleak. Where as if we start adding
put() calls everywhere we may end up freeing things which are still
in use; or dropping refcounts below 0 triggering WARNs in various
places (IIRC).

So it seems the cure is potentially worse then the disease in this
case.

So if you want to work on this, then IMHO it would be best to first make
sure that all the fwnode implementations behave in the same way wrt
ref-counting, before adding the missing put() calls in various
places.

And once the behavior is consistent then we can also document this
properly making it easier for other people to do the right thing
when using these functions.

Regards,

Hans

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Andy Shevchenko]

On Wed, May 5, 2021 at 12:07 PM Hans de Goede  wrote:
> On 5/4/21 9:52 AM, Andy Shevchenko wrote:
> > On Monday, May 3, 2021, Hans de Goede  > > wrote:

...

> > +   fwnode = device_get_next_child_node(kdev, fwnode);

> > Who is dropping reference counting on fwnode ?
>
> We are dealing with ACPI fwnode-s here and those are not ref-counted, they
> are embedded inside a struct acpi_device and their lifetime is tied to
> that struct. They should probably still be ref-counted (with the count
> never dropping to 0) so that the generic fwnode functions behave the same
> anywhere but atm the ACPI nodes are not refcounted, see: 
> acpi_get_next_subnode()
> in drivers/acpi/property.c which is the get_next_child_node() implementation
> for ACPI fwnode-s.

Yes, ACPI currently is exceptional, but fwnode API is not.
If you may guarantee that this case won't ever be outside of ACPI and
even though if ACPI won't ever gain a reference counting for fwnodes,
we can leave it as is.

> > I’m in the middle of a pile of fixes for fwnode refcounting when 
> > for_each_child or get_next_child is used. So, please double check you drop 
> > a reference.
>
> The kdoc comments on device_get_next_child_node() / 
> fwnode_get_next_child_node()
> do not mention anything about these functions returning a reference.

It's possible. I dunno if it had to be done earlier. Sakari?

> So I think we need to first make up our mind here how we want this all to
> work and then fix the actual implementation and docs before fixing callers.

We have already issues, so I prefer not to wait for a documentation
update, because for old kernels it will still be an issue.

In any case most of my fixes are against LED subsystem (drivers) and
they are valid due to use in the OF environment.

-- 
With Best Regards,
Andy Shevchenko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Hans de Goede]

Hi,

On 5/4/21 9:52 AM, Andy Shevchenko wrote:
> 
> 
> On Monday, May 3, 2021, Hans de Goede  > wrote:
> 
> From: Heikki Krogerus  >
> 
> On Intel platforms we know that the ACPI connector device
> node order will follow the order the driver (i915) decides.
> The decision is made using the custom Intel ACPI OpRegion
> (intel_opregion.c), though the driver does not actually know
> that the values it sends to ACPI there are used for
> associating a device node for the connectors, and assigning
> address for them.
> 
> In reality that custom Intel ACPI OpRegion actually violates
> ACPI specification (we supply dynamic information to objects
> that are defined static, for example _ADR), however, it
> makes assigning correct connector node for a connector entry
> straightforward (it's one-on-one mapping).
> 
> Signed-off-by: Heikki Krogerus  >
> [hdego...@redhat.com : Move 
> intel_acpi_assign_connector_fwnodes() to
>  intel_acpi.c]
> Signed-off-by: Hans de Goede  >
> ---
>  drivers/gpu/drm/i915/display/intel_acpi.c    | 40 
>  drivers/gpu/drm/i915/display/intel_acpi.h    |  3 ++
>  drivers/gpu/drm/i915/display/intel_display.c |  1 +
>  3 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c 
> b/drivers/gpu/drm/i915/display/intel_acpi.c
> index 833d0c1be4f1..9f266dfda7dd 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> @@ -263,3 +263,43 @@ void intel_acpi_device_id_update(struct 
> drm_i915_private *dev_priv)
>         }
>         drm_connector_list_iter_end(&conn_iter);
>  }
> +
> +/* NOTE: The connector order must be final before this is called. */
> +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915)
> +{
> +       struct drm_connector_list_iter conn_iter;
> +       struct drm_device *drm_dev = &i915->drm;
> +       struct device *kdev = &drm_dev->pdev->dev;
> +       struct fwnode_handle *fwnode = NULL;
> +       struct drm_connector *connector;
> +       struct acpi_device *adev;
> +
> +       drm_connector_list_iter_begin(drm_dev, &conn_iter);
> +       drm_for_each_connector_iter(connector, &conn_iter) {
> +               /* Always getting the next, even when the last was not 
> used. */
> +               fwnode = device_get_next_child_node(kdev, fwnode);
> +               if (!fwnode)
> +                       break;
> 
> 
> 
> Who is dropping reference counting on fwnode ?

We are dealing with ACPI fwnode-s here and those are not ref-counted, they
are embedded inside a struct acpi_device and their lifetime is tied to
that struct. They should probably still be ref-counted (with the count
never dropping to 0) so that the generic fwnode functions behave the same
anywhere but atm the ACPI nodes are not refcounted, see: acpi_get_next_subnode()
in drivers/acpi/property.c which is the get_next_child_node() implementation
for ACPI fwnode-s.

> I’m in the middle of a pile of fixes for fwnode refcounting when 
> for_each_child or get_next_child is used. So, please double check you drop a 
> reference.

The kdoc comments on device_get_next_child_node() / fwnode_get_next_child_node()
do not mention anything about these functions returning a reference.

So I think we need to first make up our mind here how we want this all to
work and then fix the actual implementation and docs before fixing callers.

Regards,

Hans




>  
> 
> +
> +               switch (connector->connector_type) {
> +               case DRM_MODE_CONNECTOR_LVDS:
> +               case DRM_MODE_CONNECTOR_eDP:
> +               case DRM_MODE_CONNECTOR_DSI:
> +                       /*
> +                        * Integrated displays have a specific address 
> 0x1f on
> +                        * most Intel platforms, but not on all of them.
> +                        */
> +                       adev = 
> acpi_find_child_device(ACPI_COMPANION(kdev),
> +                                                     0x1f, 0);
> +                       if (adev) {
> +                               connector->fwnode = 
> acpi_fwnode_handle(adev);
> +                               break;
> +                       }
> +                       fallthrough;
> +               default:
> +                       connector->fwnode = fwnode;
> +                       break;
> +               }
> +       }
> +       drm_connector_list_iter_end(&conn_iter);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h 
> b/drivers/gpu/drm/i915/display/intel_acpi.h
> index e8b068661d22..d24356

Re: [Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Heikki Krogerus]

Hi Andy,

> > +/* NOTE: The connector order must be final before this is called. */
> > +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915)
> > +{
> > +   struct drm_connector_list_iter conn_iter;
> > +   struct drm_device *drm_dev = &i915->drm;
> > +   struct device *kdev = &drm_dev->pdev->dev;
> > +   struct fwnode_handle *fwnode = NULL;
> > +   struct drm_connector *connector;
> > +   struct acpi_device *adev;
> > +
> > +   drm_connector_list_iter_begin(drm_dev, &conn_iter);
> > +   drm_for_each_connector_iter(connector, &conn_iter) {
> > +   /* Always getting the next, even when the last was not
> > used. */
> > +   fwnode = device_get_next_child_node(kdev, fwnode);
> > +   if (!fwnode)
> > +   break;
> 
> Who is dropping reference counting on fwnode ?
> 
> I’m in the middle of a pile of fixes for fwnode refcounting when
> for_each_child or get_next_child is used. So, please double check you drop
> a reference.

Sorry Andy. This patch is from time before the software nodes
implementation of the get_next_child callback handled the ref counting
properly.

Br,

-- 
heikki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Andy Shevchenko]

On Monday, May 3, 2021, Hans de Goede  wrote:

> From: Heikki Krogerus 
>
> On Intel platforms we know that the ACPI connector device
> node order will follow the order the driver (i915) decides.
> The decision is made using the custom Intel ACPI OpRegion
> (intel_opregion.c), though the driver does not actually know
> that the values it sends to ACPI there are used for
> associating a device node for the connectors, and assigning
> address for them.
>
> In reality that custom Intel ACPI OpRegion actually violates
> ACPI specification (we supply dynamic information to objects
> that are defined static, for example _ADR), however, it
> makes assigning correct connector node for a connector entry
> straightforward (it's one-on-one mapping).
>
> Signed-off-by: Heikki Krogerus 
> [hdego...@redhat.com: Move intel_acpi_assign_connector_fwnodes() to
>  intel_acpi.c]
> Signed-off-by: Hans de Goede 
> ---
>  drivers/gpu/drm/i915/display/intel_acpi.c| 40 
>  drivers/gpu/drm/i915/display/intel_acpi.h|  3 ++
>  drivers/gpu/drm/i915/display/intel_display.c |  1 +
>  3 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c
> b/drivers/gpu/drm/i915/display/intel_acpi.c
> index 833d0c1be4f1..9f266dfda7dd 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> @@ -263,3 +263,43 @@ void intel_acpi_device_id_update(struct
> drm_i915_private *dev_priv)
> }
> drm_connector_list_iter_end(&conn_iter);
>  }
> +
> +/* NOTE: The connector order must be final before this is called. */
> +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915)
> +{
> +   struct drm_connector_list_iter conn_iter;
> +   struct drm_device *drm_dev = &i915->drm;
> +   struct device *kdev = &drm_dev->pdev->dev;
> +   struct fwnode_handle *fwnode = NULL;
> +   struct drm_connector *connector;
> +   struct acpi_device *adev;
> +
> +   drm_connector_list_iter_begin(drm_dev, &conn_iter);
> +   drm_for_each_connector_iter(connector, &conn_iter) {
> +   /* Always getting the next, even when the last was not
> used. */
> +   fwnode = device_get_next_child_node(kdev, fwnode);
> +   if (!fwnode)
> +   break;



Who is dropping reference counting on fwnode ?

I’m in the middle of a pile of fixes for fwnode refcounting when
for_each_child or get_next_child is used. So, please double check you drop
a reference.


> +
> +   switch (connector->connector_type) {
> +   case DRM_MODE_CONNECTOR_LVDS:
> +   case DRM_MODE_CONNECTOR_eDP:
> +   case DRM_MODE_CONNECTOR_DSI:
> +   /*
> +* Integrated displays have a specific address
> 0x1f on
> +* most Intel platforms, but not on all of them.
> +*/
> +   adev = acpi_find_child_device(ACPI_
> COMPANION(kdev),
> + 0x1f, 0);
> +   if (adev) {
> +   connector->fwnode =
> acpi_fwnode_handle(adev);
> +   break;
> +   }
> +   fallthrough;
> +   default:
> +   connector->fwnode = fwnode;
> +   break;
> +   }
> +   }
> +   drm_connector_list_iter_end(&conn_iter);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h
> b/drivers/gpu/drm/i915/display/intel_acpi.h
> index e8b068661d22..d2435691f4b5 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.h
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.h
> @@ -12,11 +12,14 @@ struct drm_i915_private;
>  void intel_register_dsm_handler(void);
>  void intel_unregister_dsm_handler(void);
>  void intel_acpi_device_id_update(struct drm_i915_private *i915);
> +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915);
>  #else
>  static inline void intel_register_dsm_handler(void) { return; }
>  static inline void intel_unregister_dsm_handler(void) { return; }
>  static inline
>  void intel_acpi_device_id_update(struct drm_i915_private *i915) {
> return; }
> +static inline
> +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915)
> { return; }
>  #endif /* CONFIG_ACPI */
>
>  #endif /* __INTEL_ACPI_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 828ef4c5625f..87cad549632c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14970,6 +14970,7 @@ int intel_modeset_init_nogem(struct
> drm_i915_private *i915)
>
> drm_modeset_lock_all(dev);
> intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
> +   intel_acpi_assign_connector_fwnodes(i915);
> drm_modeset_unlock

[Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Hans de Goede]

From: Heikki Krogerus 

On Intel platforms we know that the ACPI connector device
node order will follow the order the driver (i915) decides.
The decision is made using the custom Intel ACPI OpRegion
(intel_opregion.c), though the driver does not actually know
that the values it sends to ACPI there are used for
associating a device node for the connectors, and assigning
address for them.

In reality that custom Intel ACPI OpRegion actually violates
ACPI specification (we supply dynamic information to objects
that are defined static, for example _ADR), however, it
makes assigning correct connector node for a connector entry
straightforward (it's one-on-one mapping).

Signed-off-by: Heikki Krogerus 
[hdego...@redhat.com: Move intel_acpi_assign_connector_fwnodes() to
 intel_acpi.c]
Signed-off-by: Hans de Goede 
---
 drivers/gpu/drm/i915/display/intel_acpi.c| 40 
 drivers/gpu/drm/i915/display/intel_acpi.h|  3 ++
 drivers/gpu/drm/i915/display/intel_display.c |  1 +
 3 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c 
b/drivers/gpu/drm/i915/display/intel_acpi.c
index 833d0c1be4f1..9f266dfda7dd 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.c
+++ b/drivers/gpu/drm/i915/display/intel_acpi.c
@@ -263,3 +263,43 @@ void intel_acpi_device_id_update(struct drm_i915_private 
*dev_priv)
}
drm_connector_list_iter_end(&conn_iter);
 }
+
+/* NOTE: The connector order must be final before this is called. */
+void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915)
+{
+   struct drm_connector_list_iter conn_iter;
+   struct drm_device *drm_dev = &i915->drm;
+   struct device *kdev = &drm_dev->pdev->dev;
+   struct fwnode_handle *fwnode = NULL;
+   struct drm_connector *connector;
+   struct acpi_device *adev;
+
+   drm_connector_list_iter_begin(drm_dev, &conn_iter);
+   drm_for_each_connector_iter(connector, &conn_iter) {
+   /* Always getting the next, even when the last was not used. */
+   fwnode = device_get_next_child_node(kdev, fwnode);
+   if (!fwnode)
+   break;
+
+   switch (connector->connector_type) {
+   case DRM_MODE_CONNECTOR_LVDS:
+   case DRM_MODE_CONNECTOR_eDP:
+   case DRM_MODE_CONNECTOR_DSI:
+   /*
+* Integrated displays have a specific address 0x1f on
+* most Intel platforms, but not on all of them.
+*/
+   adev = acpi_find_child_device(ACPI_COMPANION(kdev),
+ 0x1f, 0);
+   if (adev) {
+   connector->fwnode = acpi_fwnode_handle(adev);
+   break;
+   }
+   fallthrough;
+   default:
+   connector->fwnode = fwnode;
+   break;
+   }
+   }
+   drm_connector_list_iter_end(&conn_iter);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h 
b/drivers/gpu/drm/i915/display/intel_acpi.h
index e8b068661d22..d2435691f4b5 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.h
+++ b/drivers/gpu/drm/i915/display/intel_acpi.h
@@ -12,11 +12,14 @@ struct drm_i915_private;
 void intel_register_dsm_handler(void);
 void intel_unregister_dsm_handler(void);
 void intel_acpi_device_id_update(struct drm_i915_private *i915);
+void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915);
 #else
 static inline void intel_register_dsm_handler(void) { return; }
 static inline void intel_unregister_dsm_handler(void) { return; }
 static inline
 void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; }
+static inline
+void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) { 
return; }
 #endif /* CONFIG_ACPI */
 
 #endif /* __INTEL_ACPI_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 828ef4c5625f..87cad549632c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14970,6 +14970,7 @@ int intel_modeset_init_nogem(struct drm_i915_private 
*i915)
 
drm_modeset_lock_all(dev);
intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
+   intel_acpi_assign_connector_fwnodes(i915);
drm_modeset_unlock_all(dev);
 
for_each_intel_crtc(dev, crtc) {
-- 
2.31.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries

[Hans de Goede]

From: Heikki Krogerus 

On Intel platforms we know that the ACPI connector device
node order will follow the order the driver (i915) decides.
The decision is made using the custom Intel ACPI OpRegion
(intel_opregion.c), though the driver does not actually know
that the values it sends to ACPI there are used for
associating a device node for the connectors, and assigning
address for them.

In reality that custom Intel ACPI OpRegion actually violates
ACPI specification (we supply dynamic information to objects
that are defined static, for example _ADR), however, it
makes assigning correct connector node for a connector entry
straightforward (it's one-on-one mapping).

Signed-off-by: Heikki Krogerus 
[hdego...@redhat.com: Move intel_acpi_assign_connector_fwnodes() to
 intel_acpi.c]
Signed-off-by: Hans de Goede 
---
 drivers/gpu/drm/i915/display/intel_acpi.c| 40 
 drivers/gpu/drm/i915/display/intel_acpi.h|  3 ++
 drivers/gpu/drm/i915/display/intel_display.c |  1 +
 3 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c 
b/drivers/gpu/drm/i915/display/intel_acpi.c
index 833d0c1be4f1..9f266dfda7dd 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.c
+++ b/drivers/gpu/drm/i915/display/intel_acpi.c
@@ -263,3 +263,43 @@ void intel_acpi_device_id_update(struct drm_i915_private 
*dev_priv)
}
drm_connector_list_iter_end(&conn_iter);
 }
+
+/* NOTE: The connector order must be final before this is called. */
+void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915)
+{
+   struct drm_connector_list_iter conn_iter;
+   struct drm_device *drm_dev = &i915->drm;
+   struct device *kdev = &drm_dev->pdev->dev;
+   struct fwnode_handle *fwnode = NULL;
+   struct drm_connector *connector;
+   struct acpi_device *adev;
+
+   drm_connector_list_iter_begin(drm_dev, &conn_iter);
+   drm_for_each_connector_iter(connector, &conn_iter) {
+   /* Always getting the next, even when the last was not used. */
+   fwnode = device_get_next_child_node(kdev, fwnode);
+   if (!fwnode)
+   break;
+
+   switch (connector->connector_type) {
+   case DRM_MODE_CONNECTOR_LVDS:
+   case DRM_MODE_CONNECTOR_eDP:
+   case DRM_MODE_CONNECTOR_DSI:
+   /*
+* Integrated displays have a specific address 0x1f on
+* most Intel platforms, but not on all of them.
+*/
+   adev = acpi_find_child_device(ACPI_COMPANION(kdev),
+ 0x1f, 0);
+   if (adev) {
+   connector->fwnode = acpi_fwnode_handle(adev);
+   break;
+   }
+   fallthrough;
+   default:
+   connector->fwnode = fwnode;
+   break;
+   }
+   }
+   drm_connector_list_iter_end(&conn_iter);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h 
b/drivers/gpu/drm/i915/display/intel_acpi.h
index e8b068661d22..d2435691f4b5 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.h
+++ b/drivers/gpu/drm/i915/display/intel_acpi.h
@@ -12,11 +12,14 @@ struct drm_i915_private;
 void intel_register_dsm_handler(void);
 void intel_unregister_dsm_handler(void);
 void intel_acpi_device_id_update(struct drm_i915_private *i915);
+void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915);
 #else
 static inline void intel_register_dsm_handler(void) { return; }
 static inline void intel_unregister_dsm_handler(void) { return; }
 static inline
 void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; }
+static inline
+void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) { 
return; }
 #endif /* CONFIG_ACPI */
 
 #endif /* __INTEL_ACPI_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 828ef4c5625f..87cad549632c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14970,6 +14970,7 @@ int intel_modeset_init_nogem(struct drm_i915_private 
*i915)
 
drm_modeset_lock_all(dev);
intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
+   intel_acpi_assign_connector_fwnodes(i915);
drm_modeset_unlock_all(dev);
 
for_each_intel_crtc(dev, crtc) {
-- 
2.31.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx