[PATCH 11/36] xfree86: add framework for provider support in ddx.

[Dave Airlie]

On Mon, Jul 2, 2012 at 8:35 PM, Dave Airlie <airlied at gmail.com> wrote:
> On Mon, Jul 2, 2012 at 8:07 PM, Keith Packard <keithp at keithp.com> wrote:
>> Dave Airlie <airlied at gmail.com> writes:
>>
>>> +xf86ProviderPtr
>>> +xf86ProviderCreate(ScrnInfoPtr scrn,
>>> +                   const xf86ProviderFuncsRec *funcs, const char *name)
>>> +{
>>> +    xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
>>> +    xf86ProviderPtr provider;
>>> +    int len;
>>> +
>>> +    if (xf86_config->provider)
>>> +        return xf86_config->provider;
>>
>> A 'create' function shouldn't return an existing object; that's more
>> like a 'get' function. Do you expect callers to not know if the object
>> exists yet?
>
> I suppose it shouldn't happen, I was just being careful, we should
> only have one provider object per device, maybe I should just make
> that an assert.
>>>       * Requests that the driver resize the screen.
>>> @@ -681,6 +731,7 @@ typedef struct _xf86CrtcConfig {
>>>      /* callback when crtc configuration changes */
>>>      xf86_crtc_notify_proc_ptr xf86_crtc_notify;
>>>
>>> +    xf86ProviderPtr provider;
>>>  } xf86CrtcConfigRec, *xf86CrtcConfigPtr;
>>
>> Could the elements of 'provider' just become members of the crtc config rec?
>
> will check that tomorrow, probably could do alright, but I just liked
> keeping things in an object.
>>
>>> @@ -1552,6 +1552,19 @@ xf86RandR12CreateObjects12(ScreenPtr pScreen)
>>>              output->funcs->create_resources(output);
>>>          RRPostPendingProperties(output->randr_output);
>>>      }
>>> +
>>> +    {
>>> +        xf86ProviderPtr provider = config->provider;
>>> +        provider->randr_provider = RRProviderCreate(pScreen, provider->name,
>>> +                                                    strlen(provider->name), provider);
>>> +
>>> +        if (provider->scrn->is_gpu) {
>>> +            RRProviderSetRolesAbilities(provider->randr_provider, provider->scrn->roles,
>>> +                                        provider->scrn->abilities);
>>> +            RRProviderSetCurrentRole(provider->randr_provider, provider->scrn->current_role);
>>> +        }
>>> +    }
>>> +
>>
>> Ok, I'm lost here -- this assumes that config->provider exists, and yet
>> xf86ProviderCreate isn't being called with some default values in case
>> the driver doesn't do that yet.
>
> Yeah I should check here in case drivers don't create a provider object alright.

I could move the provider stuff into the toplevel struct alright,
though that would mean
a one provider per GPU model which is what I think is correct.

I suppose I need to review the randr protocol bits first before I decide.

Dave.