[PATCH xserver] randr: Do not check the screen size bound for gpu screens

Nikhil Mahale nmahale at nvidia.com
Wed Aug 31 16:11:21 UTC 2016


> 
> OTOH this may indeed be a server bug, but your fix is not the right
> one, I wonder why we are doing a RRSetScreenSize for slave GPU-s at
> all. Since we already check that the Screen is big enough in
> ProcRRSetCrtcConfig?

We are looking for answer of this question, may be Dave or Ajax knows
about this.

Thanks,
Nikhil Mahale

On 08/31/2016 01:40 PM, Timo Aaltonen wrote:
> 
> Any update on this?
> 
> On 20.06.2016 20:27, Hans de Goede wrote:
>> Hi,
>>
>> On 20-06-16 18:32, Nikhil Mahale wrote:
>>> Sorry for late reply, Hans. See inline -
>>>
>>> On 06/13/2016 10:36 PM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 20-05-16 07:00, Nikhil Mahale wrote:
>>>>> For gpu screen, CrtcSet set/adjust the master screen size along
>>>>> mode in following callstack -
>>>>>
>>>>>   ProcRRSetCrtcConfig()
>>>>>     |
>>>>>     -> RRCrtcSet()
>>>>>         |
>>>>>         -> rrCheckPixmapBounding()
>>>>>             |
>>>>>             -> pScrPriv->rrScreenSetSize()
>>>>>
>>>>> Checking screen size bound for gpus screen cause some configurations
>>>>> to fails, e.g
>>>>>
>>>>>   $ xrandr --output eDP --mode 1920x1080 --pos 0x0 --output HDMI \
>>>>>   --mode 2560x1440 --pos 0x0
>>>>>
>>>>>   Here xrandr utility first sets screen size to 2560x1440 which
>>>>>   gets resized to 1920x1080 on RRSetCrtcConfig request for eDP,
>>>>>   and then RRSetCrtcConfig request for HDMI fails because
>>>>>   of failure of screen bound check.
>>>>
>>>> Hmm, but one has the same problem when not using clone mode,
>>>> then the master screen size must be made big enough to hold
>>>> both crtc-s, so how come this has never been a problem then ?
>>>>
>>>> I've a feeling that in that case we end up doing things in
>>>> finer grained steps. What if you do:
>>>>
>>>> $ xrandr --output eDP --mode 1920x1080 --pos 0x0
>>>> $ xrandr --output HDMI --mode 2560x1440 --pos 0x0
>>>>
>>>> ? Does that work ?  If that works, which I would expect it will,
>>>> then this should be fixed in the xrandr utility instead IMHO.
>>>>
>>>> Just circumventing the screen size check for slave outputs
>>>> seem to go against the xrandr-1.2 spec to me.
>>>>
>>>> Also I guess one can reproduce the same problem without using
>>>> slave-outputs, in which case your patch will not help.
>>>>
>>>
>>> I don't think we could reproduce this problem without using slave-
>>> output, because rrScreenSetSize() path which I explained in commit
>>> message is not there, isn't it?
>>>
>>> Let me try to explain differece between both sequence of commands,
>>> please correct me if required -
>>>
>>> $ xrandr --output eDP --mode 1920x1080 --pos 0x0 --output HDMI \
>>> --mode 2560x1440 --pos 0x0
>>>
>>> -----------------------------------------------------------------------
>>> | Cmd             | Screen size                  |  eDP      | HDMI   |
>>> -----------------------------------------------------------------------
>>> | RRSetScreenSize | 2560x1440                    |     -     |   -    |
>>> -----------------------------------------------------------------------
>>> | RRSetCrtcConfig | 1920x1080                    | 1920x1080 |   -    |
>>> |                 | changed from 2560x1440       |           |        |
>>> |                 | because of rrScreenSetSize() |           |        |
>>> |                 | path explained in commit     |           |        |
>>> |                 | msg. ....(A)                 |           |        |
>>> -----------------------------------------------------------------------
>>> | RRSetCrtcConfig | 2560x1440                    | 1920x1080 | failed |
>>> -----------------------------------------------------------------------
>>
>> Ah I see now, this will only happen when the eDP is a slave output,
>> I was thinking that the HDMI would be a slave output, but that does
>> not matter, this problem will happen independ of the HDMI being a normal
>> or a slave output, as long as the eDP is a slave output.
>>
>>> $ xrandr --output eDP --mode 1920x1080 --pos 0x0
>>> $ xrandr --output HDMI --mode 2560x1440 --pos 0x0
>>>
>>>
>>> ------------------------------------------------------------------------
>>> | Cmd             | Screen size                |  eDP      | HDMI      |
>>> ------------------------------------------------------------------------
>>> | RRSetScreenSize | 1920x1080                  |     -     |   -       |
>>> ------------------------------------------------------------------------
>>> | RRSetCrtcConfig | 1920x1080                  | 1920x1080 |   -       |
>>> ------------------------------------------------------------------------
>>> | RRSetScreenSize | 2560x1440                  | 1920x1080 |   -       |
>>> ------------------------------------------------------------------------
>>> | RRSetCrtcConfig | 2560x1440                  | 1920x1080 | 2560x1440 |
>>> ------------------------------------------------------------------------
>>>
>>> Case (A) is not there in second sequence of xrand commands, so you
>>> could not reproduce problem.
>>
>> Right, so you can get things to work without requiring a server change,
>> iow this may be considered a xrandr utility bug, it could reproduce the
>> sequence used in your second table, instead of skipping the second
>> RRSetScreenSize.
>>
>> OTOH this may indeed be a server bug, but your fix is not the right
>> one, I wonder why we are doing a RRSetScreenSize for slave GPU-s at
>> all. Since we already check that the Screen is big enough in
>> ProcRRSetCrtcConfig?
>>
>> I must admit that I'm not familiar enough with the code to answer
>> this myself, but the more I think about it the more your fix feels wrong,
>> because it will effectively lead to the following call sequence
>> equivalent:
>>
>>> | RRSetScreenSize | 2560x1440                  |     -     |   -       |
>>> ------------------------------------------------------------------------
>>> | rrSetScreenSize | 1920x1080                  |     -     |   -       |
>>> ------------------------------------------------------------------------
>>> | RRSetCrtcConfig | 1920x1080                  | 1920x1080 |   -       |
>>> ------------------------------------------------------------------------
>>> | rrSetScreenSize | 2560x1440                  | 1920x1080 |   -       |
>>> ------------------------------------------------------------------------
>>> | RRSetCrtcConfig | 2560x1440                  | 1920x1080 | 2560x1440 |
>>
>> So we get 2 extra rrSetScreenSize calls leading to extra bonus
>> flickering on
>> changing things.
>>
>> Where as instead we want:
>>
>>> | RRSetScreenSize | 2560x1440                  |     -     |   -       |
>>> ------------------------------------------------------------------------
>>> | RRSetCrtcConfig | 1920x1080                  | 1920x1080 |   -       |
>>> ------------------------------------------------------------------------
>>> | RRSetCrtcConfig | 2560x1440                  | 1920x1080 | 2560x1440 |
>>
>> As said I'm not familiar enough with all the subtleties of the randr
>> code to say that we can just skip calling rrCheckPixmapBounding() at all,
>> but we can change it to only ever enlarge the screen. That should be safe
>> to do, as I would expect the xrandr utility (*) to add an explicit
>> RRSetScreenSize at the end of configuration if the size needs to shrink.
>>
>> IOW what if we do this:
>>
>> --- a/randr/rrcrtc.c
>> +++ b/randr/rrcrtc.c
>> @@ -561,8 +561,8 @@ rrCheckPixmapBounding(ScreenPtr pScreen,
>>      new_width = newsize->x2 - newsize->x1;
>>      new_height = newsize->y2 - newsize->y1;
>>
>> -    if (new_width == screen_pixmap->drawable.width &&
>> -        new_height == screen_pixmap->drawable.height) {
>> +    if (new_width <= screen_pixmap->drawable.width &&
>> +        new_height <= screen_pixmap->drawable.height) {
>>      } else {
>>          pScrPriv->rrScreenSetSize(pScreen, new_width, new_height, 0, 0);
>>      }
>>
>> That would actually save us 2 pScrPriv->rrScreenSetSize() calls, so
>> that seems like a better fix to me.
>>
>> Dave or Ajax do you have any input on this ?
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>> *) And equivalent gui control-panel applets
>>
>>
>>>
>>> Thanks,
>>> Nikhil Mahale
>>>
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Nikhil Mahale <nmahale at nvidia.com>
>>>>> ---
>>>>>  randr/rrcrtc.c | 19 ++++++-------------
>>>>>  1 file changed, 6 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c
>>>>> index 566a3db..82db9a8 100644
>>>>> --- a/randr/rrcrtc.c
>>>>> +++ b/randr/rrcrtc.c
>>>>> @@ -1176,27 +1176,20 @@ ProcRRSetCrtcConfig(ClientPtr client)
>>>>>
>>>>>  #ifdef RANDR_12_INTERFACE
>>>>>          /*
>>>>> +         * For gpu screen, CrtcSet set/adjust the master screen size
>>>>> along
>>>>> +         * with mode.
>>>>> +         *
>>>>>           * Check screen size bounds if the DDX provides a 1.2
>>>>> interface
>>>>>           * for setting screen size. Else, assume the CrtcSet sets
>>>>>           * the size along with the mode. If the driver supports
>>>>> transforms,
>>>>>           * then it must allow crtcs to display a subset of the
>>>>> screen, so
>>>>>           * only do this check for drivers without transform support.
>>>>>           */
>>>>> -        if (pScrPriv->rrScreenSetSize && !crtc->transforms) {
>>>>> +        if (!pScreen->isGPU && pScrPriv->rrScreenSetSize &&
>>>>> !crtc->transforms) {
>>>>>              int source_width;
>>>>>              int source_height;
>>>>>              PictTransform transform;
>>>>>              struct pixman_f_transform f_transform, f_inverse;
>>>>> -            int width, height;
>>>>> -
>>>>> -            if (pScreen->isGPU) {
>>>>> -                width = pScreen->current_master->width;
>>>>> -                height = pScreen->current_master->height;
>>>>> -            }
>>>>> -            else {
>>>>> -                width = pScreen->width;
>>>>> -                height = pScreen->height;
>>>>> -            }
>>>>>
>>>>>              RRTransformCompute(stuff->x, stuff->y,
>>>>>                                 mode->mode.width, mode->mode.height,
>>>>> @@ -1206,13 +1199,13 @@ ProcRRSetCrtcConfig(ClientPtr client)
>>>>>
>>>>>              RRModeGetScanoutSize(mode, &transform, &source_width,
>>>>>                                   &source_height);
>>>>> -            if (stuff->x + source_width > width) {
>>>>> +            if (stuff->x + source_width > pScreen->width) {
>>>>>                  client->errorValue = stuff->x;
>>>>>                  free(outputs);
>>>>>                  return BadValue;
>>>>>              }
>>>>>
>>>>> -            if (stuff->y + source_height > height) {
>>>>> +            if (stuff->y + source_height > pScreen->height) {
>>>>>                  client->errorValue = stuff->y;
>>>>>                  free(outputs);
>>>>>                  return BadValue;
>>>>>
>>>> _______________________________________________
>>>> xorg-devel at lists.x.org: X.Org development
>>>> Archives: http://lists.x.org/archives/xorg-devel
>>>> Info: https://lists.x.org/mailman/listinfo/xorg-devel
>>>
>>> -----------------------------------------------------------------------------------
>>>
>>> This email message is for the sole use of the intended recipient(s)
>>> and may contain
>>> confidential information.  Any unauthorized review, use, disclosure or
>>> distribution
>>> is prohibited.  If you are not the intended recipient, please contact
>>> the sender by
>>> reply email and destroy all copies of the original message.
>>> -----------------------------------------------------------------------------------
>>>
>> _______________________________________________
>> xorg-devel at lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> 
> 


More information about the xorg-devel mailing list