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 <nmah...@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@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@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel
-- t _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel