[PATCH] modesetting: and entity setup to generic probe path.

[Dave Airlie]

On 17 November 2015 at 21:57, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 17 November 2015 at 00:23, Dave Airlie <airlied at gmail.com> wrote:
>> From: Dave Airlie <airlied at redhat.com>
>>
>> Totally untested, cross fingers hope it works.
>>
>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>> ---
>>  hw/xfree86/drivers/modesetting/driver.c | 25 +++++++++++++++++++++----
>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
>> index 2ca65fb..49a2925 100644
>> --- a/hw/xfree86/drivers/modesetting/driver.c
>> +++ b/hw/xfree86/drivers/modesetting/driver.c
>> @@ -454,13 +454,12 @@ Probe(DriverPtr drv, int flags)
>>      }
>>
>>      for (i = 0; i < numDevSections; i++) {
>> -
>> +        int entity_num;
>>          dev = xf86FindOptionValue(devSections[i]->options, "kmsdev");
>>          if (probe_hw(dev, NULL)) {
>> -            int entity;
>>
>> -            entity = xf86ClaimFbSlot(drv, 0, devSections[i], TRUE);
>> -            scrn = xf86ConfigFbEntity(scrn, 0, entity, NULL, NULL, NULL, NULL);
>> +            entity_num = xf86ClaimFbSlot(drv, 0, devSections[i], TRUE);
>> +            scrn = xf86ConfigFbEntity(scrn, 0, entity_num, NULL, NULL, NULL, NULL);
> Changing entity to entity_num does not bring anything here. Split in
> off into separate patch ?

It just makes the code look the same in all the places that use it,
and I have to move it up a few lines. I don't think it makes sense to
bother doing this separately.

>
>>          }
>>
>>          if (scrn) {
>> @@ -470,6 +469,24 @@ Probe(DriverPtr drv, int flags)
>>
>>              xf86DrvMsg(scrn->scrnIndex, X_INFO,
>>                         "using %s\n", dev ? dev : "default device");
>> +            {
>> +                DevUnion *pPriv;
>> +                EntityInfoPtr pEnt;
>> +
>> +                xf86SetEntitySharable(entity_num);
>> +
>> +                if (ms_entity_index == -1)
>> +                    ms_entity_index = xf86AllocateEntityPrivateIndex();
>> +
>> +                pEnt = xf86GetEntityInfo(entity_num);
> We don't need this. We only use pEnt->index which should be identical
> with entity_num. Upon closer look the ati (and amdgpu?) ddx could do
> the same ? Alternatively we're leaking pEnt.
>
>> +                pPriv = xf86GetEntityPrivate(pEnt->index,
>> +                                             ms_entity_index);
>> +
>> +                xf86SetEntityInstanceForScreen(scrn, pEnt->index, xf86GetNumEntityInstances(pEnt->index) - 1);
>> +
>> +                if (!pPriv->ptr)
>> +                    pPriv->ptr = xnfcalloc(sizeof(modesettingEntRec), 1);
>> +            }
> Why the the extra brackets and strange line wrapping ?
>
> Upon closer look the platform_probe code could use the same cleanups -
> might even consolidate the two ?

I've consolidated them all in a new patch, though I think you are correct
on the need to lookup pEnt, I've dropped that.

Dave.