[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