[PATCH xserver] randr: Do not check the screen size bound for gpu screens
Timo Aaltonen
tjaalton at ubuntu.com
Wed Aug 31 08:10:52 UTC 2016
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
--
t
More information about the xorg-devel
mailing list