[PATCH] randr: add provider object and provider property support (v4)

[Dave Airlie]

On Mon, Jul 2, 2012 at 7:52 PM, Keith Packard <keithp at keithp.com> wrote:
> Dave Airlie <airlied at gmail.com> writes:
>
>
>> +struct _rrProvider {
>> +    RRProvider id;
>> +    ScreenPtr pScreen; /* gpu screen more than likely */
>> +    int current_role;
>
> uint32_t current_role

indeed,

>
>> diff --git a/randr/rrprovider.c b/randr/rrprovider.c
>> new file mode 100644
>> index 0000000..549db5d
>> --- /dev/null
>> +++ b/randr/rrprovider.c
>> @@ -0,0 +1,338 @@
>> +/*
>> + * Copyright © 2006 Keith Packard
>
> Please fix this.

cool,

>
>
>> +#define ADD_PROVIDER(iter) do {                                 \
>> +    pScrPriv = rrGetScrPriv((iter));                            \
>> +    if (pScrPriv->provider) {                                   \
>> +        providers[count_providers] = pScrPriv->provider->id;    \
>> +        if (client->swapped)                                    \
>> +            swapl(&providers[count_providers]);                 \
>> +        count_providers++;                                      \
>> +    }                                                           \
>> +    } while(0)
>
> 'iter' seems like a poor name choice - it's really 'pScreen'. And, I
> assume you'll be re-using this macro in a future patch, because,
> otherwise, ick.

yes there are plans for the ugly macro, probably need to be _pScreen
since the callee will want pScreen,
>
>> +    if (rep.allowed_roles & RR_Role_Slave_Output) {
>> +        rep.nCrtcs = pScrPriv->numCrtcs;
>> +        rep.nOutputs = pScrPriv->numOutputs;
> ...
>> +    if (rep.allowed_roles & RR_Role_Slave_Output) {
>> +        for (i = 0; i < pScrPriv->numCrtcs; i++) {
>
> Do masters not get crtcs and outputs in this list?

Oh good question, yes I suppose they should alright, will fix that.

>
>
>> +    for (i = 0 ; i < stuff->numProviders; i++)
>> +    {
>> +        VERIFY_RR_PROVIDER(providers[i], provider, DixReadAccess);
>> +
>> +        pScreen = provider->pScreen;
>> +        pScrPriv = rrGetScrPriv(pScreen);
>> +
>> +        pScrPriv->rrProviderSetRole(pScreen, provider, roles[i]);
>
> I think this needs to be an atomic function so that the driver can swap
> roles all at once. Otherwise, I don't know how you'd switch the LVDS
> connection in your laptop from one GPU to the other, without going
> through an intermediate black state.

Well I probably need to think about it, since you need to call into
two drivers, one driver can't run the show here,
But since we don't support GPU switch yet with this infrastructure I'd
rather consider it at that time. Yes its an API/ABI change
but really I'm not afraid of those anymore.


>
> I assume this is essentially cut&paste from the other randr property
> code, so I'm not going to waste time looking at it...

totally 100%.

Dave.