| Message ID | 1448563935-3157-1-git-send-email-thellstrom@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote: > If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is > implemented, the cursor hotspot is set to (0,0) which is incompatible > with vmwgfx where the hotspot should instead remain unchanged. > > So if the driver implements both cursor_set2 and cursor_set, prefer calling > the latter from the drm_mode_cursor ioctl. > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> That looks like papering over a bug in the client - it simply shouldn't mix these two if it expects the hotspot to stick around. There was also recently a big discussion about hotspot behaviour, and the conclusion was that qxl got it all wrong. Since that was specifically added for qxl I'm not sure how well this was ever tested ... The other problem seems to be that X is racy with updating cursors (since it does a setPos and setCursor separately, despite that this ioctl can do it in one go) and so if you move the hotspot you get a jumpy cursor. Radeon tried to paper over that but fundamentally you need to fix X and use atomic (or at least universal plane cursor support) to fix this. Given all that I'm leaning towards "the future is atomic", let's please not add hacks to legacy code. Aside: Atomic doesn't have the hotspot-x/y properties yet, but really the only reason was that we don't yet have an atomic driver needing them. Adding those props will be a tiny patch of a few lines. Cheers, Daniel > --- > drivers/gpu/drm/drm_crtc.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 24c5434..93f80a5 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -2874,7 +2874,8 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, > > static int drm_mode_cursor_common(struct drm_device *dev, > struct drm_mode_cursor2 *req, > - struct drm_file *file_priv) > + struct drm_file *file_priv, > + bool from_2) > { > struct drm_crtc *crtc; > int ret = 0; > @@ -2907,7 +2908,8 @@ static int drm_mode_cursor_common(struct drm_device *dev, > goto out; > } > /* Turns off the cursor if handle is 0 */ > - if (crtc->funcs->cursor_set2) > + if (crtc->funcs->cursor_set2 && > + (from_2 || !crtc->funcs->cursor_set)) > ret = crtc->funcs->cursor_set2(crtc, file_priv, req->handle, > req->width, req->height, req->hot_x, req->hot_y); > else > @@ -2953,7 +2955,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev, > memcpy(&new_req, req, sizeof(struct drm_mode_cursor)); > new_req.hot_x = new_req.hot_y = 0; > > - return drm_mode_cursor_common(dev, &new_req, file_priv); > + return drm_mode_cursor_common(dev, &new_req, file_priv, false); > } > > /** > @@ -2976,7 +2978,7 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev, > { > struct drm_mode_cursor2 *req = data; > > - return drm_mode_cursor_common(dev, req, file_priv); > + return drm_mode_cursor_common(dev, req, file_priv, true); > } > > /** > -- > 2.4.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 11/27/2015 11:11 AM, Daniel Vetter wrote: > On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote: >> If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is >> implemented, the cursor hotspot is set to (0,0) which is incompatible >> with vmwgfx where the hotspot should instead remain unchanged. >> >> So if the driver implements both cursor_set2 and cursor_set, prefer calling >> the latter from the drm_mode_cursor ioctl. >> >> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > That looks like papering over a bug in the client - it simply shouldn't > mix these two if it expects the hotspot to stick around. There was also > recently a big discussion about hotspot behaviour, and the conclusion was > that qxl got it all wrong. Since that was specifically added for qxl I'm > not sure how well this was ever tested ... No, this is not the case, This is for old Xorg userspace that first sets the hotspot using our ancient driver-private ioctl and then calls drm_mode_cursor() to update the cursor. Now if we were to implement cursor_set2, which is apparently needed to get gnome-shell/Wayland cursors in the right place, without this fix, it would break old Xorg, so we don't have much choice in this case from what I can tell. The root problem here is that the drm_mode_cursor() behaviour in the presence of cursor_set2 didn't take the existing vmware hotspot semantics into account. > The other problem seems to be that X is racy with updating cursors (since > it does a setPos and setCursor separately, despite that this ioctl can do > it in one go) and so if you move the hotspot you get a jumpy cursor. > Radeon tried to paper over that but fundamentally you need to fix X and > use atomic (or at least universal plane cursor support) to fix this. > > Given all that I'm leaning towards "the future is atomic", let's please > not add hacks to legacy code. Aside: Atomic doesn't have the hotspot-x/y > properties yet, but really the only reason was that we don't yet have an > atomic driver needing them. Adding those props will be a tiny patch of a > few lines. Sinclair has started working on atomic support for vmwgfx, but we really need to fix this also in the legacy code, ugly or not. Thanks, Thomas > > Cheers, Daniel >> --- >> drivers/gpu/drm/drm_crtc.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index 24c5434..93f80a5 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -2874,7 +2874,8 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, >> >> static int drm_mode_cursor_common(struct drm_device *dev, >> struct drm_mode_cursor2 *req, >> - struct drm_file *file_priv) >> + struct drm_file *file_priv, >> + bool from_2) >> { >> struct drm_crtc *crtc; >> int ret = 0; >> @@ -2907,7 +2908,8 @@ static int drm_mode_cursor_common(struct drm_device *dev, >> goto out; >> } >> /* Turns off the cursor if handle is 0 */ >> - if (crtc->funcs->cursor_set2) >> + if (crtc->funcs->cursor_set2 && >> + (from_2 || !crtc->funcs->cursor_set)) >> ret = crtc->funcs->cursor_set2(crtc, file_priv, req->handle, >> req->width, req->height, req->hot_x, req->hot_y); >> else >> @@ -2953,7 +2955,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev, >> memcpy(&new_req, req, sizeof(struct drm_mode_cursor)); >> new_req.hot_x = new_req.hot_y = 0; >> >> - return drm_mode_cursor_common(dev, &new_req, file_priv); >> + return drm_mode_cursor_common(dev, &new_req, file_priv, false); >> } >> >> /** >> @@ -2976,7 +2978,7 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev, >> { >> struct drm_mode_cursor2 *req = data; >> >> - return drm_mode_cursor_common(dev, req, file_priv); >> + return drm_mode_cursor_common(dev, req, file_priv, true); >> } >> >> /** >> -- >> 2.4.3 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=BQIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=LvaoWKneZBMqoqvonmNbtrQ9sY1dL5unCG_D2_Xb--Y&s=IpenYkOeOFSMkCBvMp1_TEam7-KdFX4zbfCC943GC3M&e=
On Fri, Nov 27, 2015 at 12:42:15PM +0100, Thomas Hellstrom wrote: > On 11/27/2015 11:11 AM, Daniel Vetter wrote: > > On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote: > >> If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is > >> implemented, the cursor hotspot is set to (0,0) which is incompatible > >> with vmwgfx where the hotspot should instead remain unchanged. > >> > >> So if the driver implements both cursor_set2 and cursor_set, prefer calling > >> the latter from the drm_mode_cursor ioctl. > >> > >> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > > That looks like papering over a bug in the client - it simply shouldn't > > mix these two if it expects the hotspot to stick around. There was also > > recently a big discussion about hotspot behaviour, and the conclusion was > > that qxl got it all wrong. Since that was specifically added for qxl I'm > > not sure how well this was ever tested ... > > No, this is not the case, This is for old Xorg userspace that first sets > the hotspot using our ancient > driver-private ioctl and then calls drm_mode_cursor() to update the cursor. > > Now if we were to implement cursor_set2, which is apparently needed to > get gnome-shell/Wayland cursors in the right place, without this fix, it > would break old Xorg, so we don't have much choice in this case from > what I can tell. > > The root problem here is that the drm_mode_cursor() behaviour in the > presence of cursor_set2 didn't take the existing vmware hotspot > semantics into account. > > > The other problem seems to be that X is racy with updating cursors (since > > it does a setPos and setCursor separately, despite that this ioctl can do > > it in one go) and so if you move the hotspot you get a jumpy cursor. > > Radeon tried to paper over that but fundamentally you need to fix X and > > use atomic (or at least universal plane cursor support) to fix this. > > > > Given all that I'm leaning towards "the future is atomic", let's please > > not add hacks to legacy code. Aside: Atomic doesn't have the hotspot-x/y > > properties yet, but really the only reason was that we don't yet have an > > atomic driver needing them. Adding those props will be a tiny patch of a > > few lines. > > Sinclair has started working on atomic support for vmwgfx, but we really > need to fix this also in > the legacy code, ugly or not. I was thinking maybe it would be cleaner to not reset the hotspot to 0,0 in drm_mode_cursor_ioctl() and instead keep what was previously configured, and only do the reset to 0,0 in lastclose (or some other suitable place)? But I suppose that could break something else that assume that drm_mode_cursor_ioctl() will do the reset. > > Thanks, > Thomas > > > > > > Cheers, Daniel > >> --- > >> drivers/gpu/drm/drm_crtc.c | 10 ++++++---- > >> 1 file changed, 6 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > >> index 24c5434..93f80a5 100644 > >> --- a/drivers/gpu/drm/drm_crtc.c > >> +++ b/drivers/gpu/drm/drm_crtc.c > >> @@ -2874,7 +2874,8 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, > >> > >> static int drm_mode_cursor_common(struct drm_device *dev, > >> struct drm_mode_cursor2 *req, > >> - struct drm_file *file_priv) > >> + struct drm_file *file_priv, > >> + bool from_2) > >> { > >> struct drm_crtc *crtc; > >> int ret = 0; > >> @@ -2907,7 +2908,8 @@ static int drm_mode_cursor_common(struct drm_device *dev, > >> goto out; > >> } > >> /* Turns off the cursor if handle is 0 */ > >> - if (crtc->funcs->cursor_set2) > >> + if (crtc->funcs->cursor_set2 && > >> + (from_2 || !crtc->funcs->cursor_set)) > >> ret = crtc->funcs->cursor_set2(crtc, file_priv, req->handle, > >> req->width, req->height, req->hot_x, req->hot_y); > >> else > >> @@ -2953,7 +2955,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev, > >> memcpy(&new_req, req, sizeof(struct drm_mode_cursor)); > >> new_req.hot_x = new_req.hot_y = 0; > >> > >> - return drm_mode_cursor_common(dev, &new_req, file_priv); > >> + return drm_mode_cursor_common(dev, &new_req, file_priv, false); > >> } > >> > >> /** > >> @@ -2976,7 +2978,7 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev, > >> { > >> struct drm_mode_cursor2 *req = data; > >> > >> - return drm_mode_cursor_common(dev, req, file_priv); > >> + return drm_mode_cursor_common(dev, req, file_priv, true); > >> } > >> > >> /** > >> -- > >> 2.4.3 > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=BQIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=LvaoWKneZBMqoqvonmNbtrQ9sY1dL5unCG_D2_Xb--Y&s=IpenYkOeOFSMkCBvMp1_TEam7-KdFX4zbfCC943GC3M&e= > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 11/27/2015 01:02 PM, Ville Syrjälä wrote: > On Fri, Nov 27, 2015 at 12:42:15PM +0100, Thomas Hellstrom wrote: >> On 11/27/2015 11:11 AM, Daniel Vetter wrote: >>> On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote: >>>> If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is >>>> implemented, the cursor hotspot is set to (0,0) which is incompatible >>>> with vmwgfx where the hotspot should instead remain unchanged. >>>> >>>> So if the driver implements both cursor_set2 and cursor_set, prefer calling >>>> the latter from the drm_mode_cursor ioctl. >>>> >>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> >>> That looks like papering over a bug in the client - it simply shouldn't >>> mix these two if it expects the hotspot to stick around. There was also >>> recently a big discussion about hotspot behaviour, and the conclusion was >>> that qxl got it all wrong. Since that was specifically added for qxl I'm >>> not sure how well this was ever tested ... >> No, this is not the case, This is for old Xorg userspace that first sets >> the hotspot using our ancient >> driver-private ioctl and then calls drm_mode_cursor() to update the cursor. >> >> Now if we were to implement cursor_set2, which is apparently needed to >> get gnome-shell/Wayland cursors in the right place, without this fix, it >> would break old Xorg, so we don't have much choice in this case from >> what I can tell. >> >> The root problem here is that the drm_mode_cursor() behaviour in the >> presence of cursor_set2 didn't take the existing vmware hotspot >> semantics into account. >> >>> The other problem seems to be that X is racy with updating cursors (since >>> it does a setPos and setCursor separately, despite that this ioctl can do >>> it in one go) and so if you move the hotspot you get a jumpy cursor. >>> Radeon tried to paper over that but fundamentally you need to fix X and >>> use atomic (or at least universal plane cursor support) to fix this. >>> >>> Given all that I'm leaning towards "the future is atomic", let's please >>> not add hacks to legacy code. Aside: Atomic doesn't have the hotspot-x/y >>> properties yet, but really the only reason was that we don't yet have an >>> atomic driver needing them. Adding those props will be a tiny patch of a >>> few lines. >> Sinclair has started working on atomic support for vmwgfx, but we really >> need to fix this also in >> the legacy code, ugly or not. > I was thinking maybe it would be cleaner to not reset the hotspot to > 0,0 in drm_mode_cursor_ioctl() and instead keep what was previously > configured, and only do the reset to 0,0 in lastclose (or some other > suitable place)? But I suppose that could break something else that > assume that drm_mode_cursor_ioctl() will do the reset. If something assumes that drm_mode_cursor_ioctl() will reset the hotspot, then we're already in trouble because then we have different semantics of the same ioctl assumed by user-space. However, from what I can tell, getting the cursor hotspot position from within core DRM would require new lock protected hotspot coordinate members in the crtc structure... /Thomas > >> Thanks, >> Thomas >> >> >>> Cheers, Daniel >>>> --- >>>> drivers/gpu/drm/drm_crtc.c | 10 ++++++---- >>>> 1 file changed, 6 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >>>> index 24c5434..93f80a5 100644 >>>> --- a/drivers/gpu/drm/drm_crtc.c >>>> +++ b/drivers/gpu/drm/drm_crtc.c >>>> @@ -2874,7 +2874,8 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, >>>> >>>> static int drm_mode_cursor_common(struct drm_device *dev, >>>> struct drm_mode_cursor2 *req, >>>> - struct drm_file *file_priv) >>>> + struct drm_file *file_priv, >>>> + bool from_2) >>>> { >>>> struct drm_crtc *crtc; >>>> int ret = 0; >>>> @@ -2907,7 +2908,8 @@ static int drm_mode_cursor_common(struct drm_device *dev, >>>> goto out; >>>> } >>>> /* Turns off the cursor if handle is 0 */ >>>> - if (crtc->funcs->cursor_set2) >>>> + if (crtc->funcs->cursor_set2 && >>>> + (from_2 || !crtc->funcs->cursor_set)) >>>> ret = crtc->funcs->cursor_set2(crtc, file_priv, req->handle, >>>> req->width, req->height, req->hot_x, req->hot_y); >>>> else >>>> @@ -2953,7 +2955,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev, >>>> memcpy(&new_req, req, sizeof(struct drm_mode_cursor)); >>>> new_req.hot_x = new_req.hot_y = 0; >>>> >>>> - return drm_mode_cursor_common(dev, &new_req, file_priv); >>>> + return drm_mode_cursor_common(dev, &new_req, file_priv, false); >>>> } >>>> >>>> /** >>>> @@ -2976,7 +2978,7 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev, >>>> { >>>> struct drm_mode_cursor2 *req = data; >>>> >>>> - return drm_mode_cursor_common(dev, req, file_priv); >>>> + return drm_mode_cursor_common(dev, req, file_priv, true); >>>> } >>>> >>>> /** >>>> -- >>>> 2.4.3 >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=BQIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=LvaoWKneZBMqoqvonmNbtrQ9sY1dL5unCG_D2_Xb--Y&s=IpenYkOeOFSMkCBvMp1_TEam7-KdFX4zbfCC943GC3M&e= >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=BQIDAw&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=lwkgECy6-cIOL48-pV4s5urKAJj4oXa5XJTz2d7w2lU&s=3KpOcVke0GPiMcmHBTMvxHe0SpUBMxF_D3A76OhRXtE&e=
On Fri, Nov 27, 2015 at 01:24:11PM +0100, Thomas Hellstrom wrote: > On 11/27/2015 01:02 PM, Ville Syrjälä wrote: > > On Fri, Nov 27, 2015 at 12:42:15PM +0100, Thomas Hellstrom wrote: > >> On 11/27/2015 11:11 AM, Daniel Vetter wrote: > >>> On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote: > >>>> If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is > >>>> implemented, the cursor hotspot is set to (0,0) which is incompatible > >>>> with vmwgfx where the hotspot should instead remain unchanged. > >>>> > >>>> So if the driver implements both cursor_set2 and cursor_set, prefer calling > >>>> the latter from the drm_mode_cursor ioctl. > >>>> > >>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > >>> That looks like papering over a bug in the client - it simply shouldn't > >>> mix these two if it expects the hotspot to stick around. There was also > >>> recently a big discussion about hotspot behaviour, and the conclusion was > >>> that qxl got it all wrong. Since that was specifically added for qxl I'm > >>> not sure how well this was ever tested ... > >> No, this is not the case, This is for old Xorg userspace that first sets > >> the hotspot using our ancient > >> driver-private ioctl and then calls drm_mode_cursor() to update the cursor. > >> > >> Now if we were to implement cursor_set2, which is apparently needed to > >> get gnome-shell/Wayland cursors in the right place, without this fix, it > >> would break old Xorg, so we don't have much choice in this case from > >> what I can tell. > >> > >> The root problem here is that the drm_mode_cursor() behaviour in the > >> presence of cursor_set2 didn't take the existing vmware hotspot > >> semantics into account. Ugh. I think the simplest solution is to not mix up the two hotspots, i.e. separately keep track of both the legacy vmwgfx hotspot and the drm core hotspot. Then in the actual cursor set code add them. A bit of ugly in the vmwgfx code (but not much), instead of leaking that driver private legacy ioctl semantics into drm core. Would that work? > >>> The other problem seems to be that X is racy with updating cursors (since > >>> it does a setPos and setCursor separately, despite that this ioctl can do > >>> it in one go) and so if you move the hotspot you get a jumpy cursor. > >>> Radeon tried to paper over that but fundamentally you need to fix X and > >>> use atomic (or at least universal plane cursor support) to fix this. > >>> > >>> Given all that I'm leaning towards "the future is atomic", let's please > >>> not add hacks to legacy code. Aside: Atomic doesn't have the hotspot-x/y > >>> properties yet, but really the only reason was that we don't yet have an > >>> atomic driver needing them. Adding those props will be a tiny patch of a > >>> few lines. > >> Sinclair has started working on atomic support for vmwgfx, but we really > >> need to fix this also in > >> the legacy code, ugly or not. > > I was thinking maybe it would be cleaner to not reset the hotspot to > > 0,0 in drm_mode_cursor_ioctl() and instead keep what was previously > > configured, and only do the reset to 0,0 in lastclose (or some other > > suitable place)? But I suppose that could break something else that > > assume that drm_mode_cursor_ioctl() will do the reset. > > If something assumes that drm_mode_cursor_ioctl() will reset the > hotspot, then we're already in trouble because then we have different > semantics of the same ioctl assumed by user-space. > > However, from what I can tell, getting the cursor hotspot position from > within core DRM would require new lock protected hotspot coordinate > members in the crtc structure... Yeah, and for atomic we definitely want those in drm_crtc_state, but for legacy we'd need it in drm_crtc. I'd prefer if we don't need to add it to both places ;-) Cheers, Daniel
On 11/29/2015 10:18 AM, Daniel Vetter wrote: > On Fri, Nov 27, 2015 at 01:24:11PM +0100, Thomas Hellstrom wrote: >> On 11/27/2015 01:02 PM, Ville Syrjälä wrote: >>> On Fri, Nov 27, 2015 at 12:42:15PM +0100, Thomas Hellstrom wrote: >>>> On 11/27/2015 11:11 AM, Daniel Vetter wrote: >>>>> On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote: >>>>>> If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is >>>>>> implemented, the cursor hotspot is set to (0,0) which is incompatible >>>>>> with vmwgfx where the hotspot should instead remain unchanged. >>>>>> >>>>>> So if the driver implements both cursor_set2 and cursor_set, prefer calling >>>>>> the latter from the drm_mode_cursor ioctl. >>>>>> >>>>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> >>>>> That looks like papering over a bug in the client - it simply shouldn't >>>>> mix these two if it expects the hotspot to stick around. There was also >>>>> recently a big discussion about hotspot behaviour, and the conclusion was >>>>> that qxl got it all wrong. Since that was specifically added for qxl I'm >>>>> not sure how well this was ever tested ... >>>> No, this is not the case, This is for old Xorg userspace that first sets >>>> the hotspot using our ancient >>>> driver-private ioctl and then calls drm_mode_cursor() to update the cursor. >>>> >>>> Now if we were to implement cursor_set2, which is apparently needed to >>>> get gnome-shell/Wayland cursors in the right place, without this fix, it >>>> would break old Xorg, so we don't have much choice in this case from >>>> what I can tell. >>>> >>>> The root problem here is that the drm_mode_cursor() behaviour in the >>>> presence of cursor_set2 didn't take the existing vmware hotspot >>>> semantics into account. > Ugh. I think the simplest solution is to not mix up the two hotspots, i.e. > separately keep track of both the legacy vmwgfx hotspot and the drm core > hotspot. Then in the actual cursor set code add them. A bit of ugly in the > vmwgfx code (but not much), instead of leaking that driver private legacy > ioctl semantics into drm core. Would that work? > > Hmm. Yes it would probably work. Good idea. I guess we just need to make sure that both hotspots are reset to (0,0) at master drop. /Thomas
On Mon, Nov 30, 2015 at 02:35:53PM +0100, Thomas Hellstrom wrote: > On 11/29/2015 10:18 AM, Daniel Vetter wrote: > > On Fri, Nov 27, 2015 at 01:24:11PM +0100, Thomas Hellstrom wrote: > >> On 11/27/2015 01:02 PM, Ville Syrjälä wrote: > >>> On Fri, Nov 27, 2015 at 12:42:15PM +0100, Thomas Hellstrom wrote: > >>>> On 11/27/2015 11:11 AM, Daniel Vetter wrote: > >>>>> On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote: > >>>>>> If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is > >>>>>> implemented, the cursor hotspot is set to (0,0) which is incompatible > >>>>>> with vmwgfx where the hotspot should instead remain unchanged. > >>>>>> > >>>>>> So if the driver implements both cursor_set2 and cursor_set, prefer calling > >>>>>> the latter from the drm_mode_cursor ioctl. > >>>>>> > >>>>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > >>>>> That looks like papering over a bug in the client - it simply shouldn't > >>>>> mix these two if it expects the hotspot to stick around. There was also > >>>>> recently a big discussion about hotspot behaviour, and the conclusion was > >>>>> that qxl got it all wrong. Since that was specifically added for qxl I'm > >>>>> not sure how well this was ever tested ... > >>>> No, this is not the case, This is for old Xorg userspace that first sets > >>>> the hotspot using our ancient > >>>> driver-private ioctl and then calls drm_mode_cursor() to update the cursor. > >>>> > >>>> Now if we were to implement cursor_set2, which is apparently needed to > >>>> get gnome-shell/Wayland cursors in the right place, without this fix, it > >>>> would break old Xorg, so we don't have much choice in this case from > >>>> what I can tell. > >>>> > >>>> The root problem here is that the drm_mode_cursor() behaviour in the > >>>> presence of cursor_set2 didn't take the existing vmware hotspot > >>>> semantics into account. > > Ugh. I think the simplest solution is to not mix up the two hotspots, i.e. > > separately keep track of both the legacy vmwgfx hotspot and the drm core > > hotspot. Then in the actual cursor set code add them. A bit of ugly in the > > vmwgfx code (but not much), instead of leaking that driver private legacy > > ioctl semantics into drm core. Would that work? > > > > > Hmm. > > Yes it would probably work. Good idea. > I guess we just need to make sure that both hotspots are reset to (0,0) > at master drop. Resetting kms state is an entire different can of worms topic. On one hand we don't want it, since if you apply changes from the default (at early boot or maybe with kernel cmdline options) we want userspace to take over all those settings. Otoh if your compositor dies and leaves a mess behind we should reset to /something/, but due to the above it's undefined what the reset values should be. At least in general. The other problem is trying to figure out when exactly you should restore - we already have plenty fun trying to decide when to restore fbcon, with a pile of hacks. With atomic at least userspace can take a full snapshot of all kms state, even if it doesn't understand all the properties. Only requirement is that all tunables are exported as properties. Then it can always restore that snapshot. Given that I'm leaning towars "this should be solved in userspace by some priveldged daemon like logind". Means old userspace is out of luck if X dies (at least for all the properties that don't get naturally overwritten), but that's been the case since forever. Trying to do that in the kernel is imo something that's not really possible - the kernel knows too little about overall system state and configuration. -Daniel
On 11/30/2015 03:55 PM, Daniel Vetter wrote: > On Mon, Nov 30, 2015 at 02:35:53PM +0100, Thomas Hellstrom wrote: >> On 11/29/2015 10:18 AM, Daniel Vetter wrote: >>> On Fri, Nov 27, 2015 at 01:24:11PM +0100, Thomas Hellstrom wrote: >>>> On 11/27/2015 01:02 PM, Ville Syrjälä wrote: >>>>> On Fri, Nov 27, 2015 at 12:42:15PM +0100, Thomas Hellstrom wrote: >>>>>> On 11/27/2015 11:11 AM, Daniel Vetter wrote: >>>>>>> On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote: >>>>>>>> If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is >>>>>>>> implemented, the cursor hotspot is set to (0,0) which is incompatible >>>>>>>> with vmwgfx where the hotspot should instead remain unchanged. >>>>>>>> >>>>>>>> So if the driver implements both cursor_set2 and cursor_set, prefer calling >>>>>>>> the latter from the drm_mode_cursor ioctl. >>>>>>>> >>>>>>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> >>>>>>> That looks like papering over a bug in the client - it simply shouldn't >>>>>>> mix these two if it expects the hotspot to stick around. There was also >>>>>>> recently a big discussion about hotspot behaviour, and the conclusion was >>>>>>> that qxl got it all wrong. Since that was specifically added for qxl I'm >>>>>>> not sure how well this was ever tested ... >>>>>> No, this is not the case, This is for old Xorg userspace that first sets >>>>>> the hotspot using our ancient >>>>>> driver-private ioctl and then calls drm_mode_cursor() to update the cursor. >>>>>> >>>>>> Now if we were to implement cursor_set2, which is apparently needed to >>>>>> get gnome-shell/Wayland cursors in the right place, without this fix, it >>>>>> would break old Xorg, so we don't have much choice in this case from >>>>>> what I can tell. >>>>>> >>>>>> The root problem here is that the drm_mode_cursor() behaviour in the >>>>>> presence of cursor_set2 didn't take the existing vmware hotspot >>>>>> semantics into account. >>> Ugh. I think the simplest solution is to not mix up the two hotspots, i.e. >>> separately keep track of both the legacy vmwgfx hotspot and the drm core >>> hotspot. Then in the actual cursor set code add them. A bit of ugly in the >>> vmwgfx code (but not much), instead of leaking that driver private legacy >>> ioctl semantics into drm core. Would that work? >>> >>> >> Hmm. >> >> Yes it would probably work. Good idea. >> I guess we just need to make sure that both hotspots are reset to (0,0) >> at master drop. > Resetting kms state is an entire different can of worms topic. On one hand > we don't want it, since if you apply changes from the default (at early > boot or maybe with kernel cmdline options) we want userspace to take over > all those settings. Otoh if your compositor dies and leaves a mess behind > we should reset to /something/, but due to the above it's undefined what > the reset values should be. At least in general. The other problem is > trying to figure out when exactly you should restore - we already have > plenty fun trying to decide when to restore fbcon, with a pile of hacks. > > With atomic at least userspace can take a full snapshot of all kms state, > even if it doesn't understand all the properties. Only requirement is that > all tunables are exported as properties. Then it can always restore that > snapshot. Given that I'm leaning towars "this should be solved in > userspace by some priveldged daemon like logind". Means old userspace is > out of luck if X dies (at least for all the properties that don't get > naturally overwritten), but that's been the case since forever. > > Trying to do that in the kernel is imo something that's not really > possible - the kernel knows too little about overall system state and > configuration. You're probably right, but in this particular case a master using the legacy vmware hotspot would be severely confused by another master using the drm core hotspot, and vice versa even if both thought they did everything right and reset "their" hotspot at master_set(). Another option would of course be to use some heuristic to try to determine what hotspot is the "correct" one to use, instead of adding them. /Thomas > -Daniel
On Mon, Nov 30, 2015 at 04:52:22PM +0100, Thomas Hellstrom wrote: > On 11/30/2015 03:55 PM, Daniel Vetter wrote: > > On Mon, Nov 30, 2015 at 02:35:53PM +0100, Thomas Hellstrom wrote: > >> On 11/29/2015 10:18 AM, Daniel Vetter wrote: > >>> On Fri, Nov 27, 2015 at 01:24:11PM +0100, Thomas Hellstrom wrote: > >>>> On 11/27/2015 01:02 PM, Ville Syrjälä wrote: > >>>>> On Fri, Nov 27, 2015 at 12:42:15PM +0100, Thomas Hellstrom wrote: > >>>>>> On 11/27/2015 11:11 AM, Daniel Vetter wrote: > >>>>>>> On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote: > >>>>>>>> If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is > >>>>>>>> implemented, the cursor hotspot is set to (0,0) which is incompatible > >>>>>>>> with vmwgfx where the hotspot should instead remain unchanged. > >>>>>>>> > >>>>>>>> So if the driver implements both cursor_set2 and cursor_set, prefer calling > >>>>>>>> the latter from the drm_mode_cursor ioctl. > >>>>>>>> > >>>>>>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > >>>>>>> That looks like papering over a bug in the client - it simply shouldn't > >>>>>>> mix these two if it expects the hotspot to stick around. There was also > >>>>>>> recently a big discussion about hotspot behaviour, and the conclusion was > >>>>>>> that qxl got it all wrong. Since that was specifically added for qxl I'm > >>>>>>> not sure how well this was ever tested ... > >>>>>> No, this is not the case, This is for old Xorg userspace that first sets > >>>>>> the hotspot using our ancient > >>>>>> driver-private ioctl and then calls drm_mode_cursor() to update the cursor. > >>>>>> > >>>>>> Now if we were to implement cursor_set2, which is apparently needed to > >>>>>> get gnome-shell/Wayland cursors in the right place, without this fix, it > >>>>>> would break old Xorg, so we don't have much choice in this case from > >>>>>> what I can tell. > >>>>>> > >>>>>> The root problem here is that the drm_mode_cursor() behaviour in the > >>>>>> presence of cursor_set2 didn't take the existing vmware hotspot > >>>>>> semantics into account. > >>> Ugh. I think the simplest solution is to not mix up the two hotspots, i.e. > >>> separately keep track of both the legacy vmwgfx hotspot and the drm core > >>> hotspot. Then in the actual cursor set code add them. A bit of ugly in the > >>> vmwgfx code (but not much), instead of leaking that driver private legacy > >>> ioctl semantics into drm core. Would that work? > >>> > >>> > >> Hmm. > >> > >> Yes it would probably work. Good idea. > >> I guess we just need to make sure that both hotspots are reset to (0,0) > >> at master drop. > > Resetting kms state is an entire different can of worms topic. On one hand > > we don't want it, since if you apply changes from the default (at early > > boot or maybe with kernel cmdline options) we want userspace to take over > > all those settings. Otoh if your compositor dies and leaves a mess behind > > we should reset to /something/, but due to the above it's undefined what > > the reset values should be. At least in general. The other problem is > > trying to figure out when exactly you should restore - we already have > > plenty fun trying to decide when to restore fbcon, with a pile of hacks. > > > > With atomic at least userspace can take a full snapshot of all kms state, > > even if it doesn't understand all the properties. Only requirement is that > > all tunables are exported as properties. Then it can always restore that > > snapshot. Given that I'm leaning towars "this should be solved in > > userspace by some priveldged daemon like logind". Means old userspace is > > out of luck if X dies (at least for all the properties that don't get > > naturally overwritten), but that's been the case since forever. > > > > Trying to do that in the kernel is imo something that's not really > > possible - the kernel knows too little about overall system state and > > configuration. > > You're probably right, but in this particular case a master using the > legacy vmware hotspot would be severely confused by another master using > the drm core hotspot, and vice versa even if both thought they did > everything right and reset "their" hotspot at master_set(). > > Another option would of course be to use some heuristic to try to > determine what hotspot is the "correct" one to use, instead of adding them. Yeah, a one-off for vmwgfx shouldn't cause problems, since we already have the master hooks for vmwgfx anyway. Just wanted to add that doing this in generic code is imo fraught with peril and long-term probably not something where we want to go to. I think it's perfectly fine if you add a bit of code to vmwgfx to reset things and make the interplay with vmw-legacy and drm hotspot work. Cheers, Daniel
Reviewed-by: Sinclair Yeh <syeh@vmware.com> On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote: > If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is > implemented, the cursor hotspot is set to (0,0) which is incompatible > with vmwgfx where the hotspot should instead remain unchanged. > > So if the driver implements both cursor_set2 and cursor_set, prefer calling > the latter from the drm_mode_cursor ioctl. > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> > --- > drivers/gpu/drm/drm_crtc.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 24c5434..93f80a5 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -2874,7 +2874,8 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, > > static int drm_mode_cursor_common(struct drm_device *dev, > struct drm_mode_cursor2 *req, > - struct drm_file *file_priv) > + struct drm_file *file_priv, > + bool from_2) > { > struct drm_crtc *crtc; > int ret = 0; > @@ -2907,7 +2908,8 @@ static int drm_mode_cursor_common(struct drm_device *dev, > goto out; > } > /* Turns off the cursor if handle is 0 */ > - if (crtc->funcs->cursor_set2) > + if (crtc->funcs->cursor_set2 && > + (from_2 || !crtc->funcs->cursor_set)) > ret = crtc->funcs->cursor_set2(crtc, file_priv, req->handle, > req->width, req->height, req->hot_x, req->hot_y); > else > @@ -2953,7 +2955,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev, > memcpy(&new_req, req, sizeof(struct drm_mode_cursor)); > new_req.hot_x = new_req.hot_y = 0; > > - return drm_mode_cursor_common(dev, &new_req, file_priv); > + return drm_mode_cursor_common(dev, &new_req, file_priv, false); > } > > /** > @@ -2976,7 +2978,7 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev, > { > struct drm_mode_cursor2 *req = data; > > - return drm_mode_cursor_common(dev, req, file_priv); > + return drm_mode_cursor_common(dev, req, file_priv, true); > } > > /** > -- > 2.4.3 >
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 24c5434..93f80a5 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2874,7 +2874,8 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, static int drm_mode_cursor_common(struct drm_device *dev, struct drm_mode_cursor2 *req, - struct drm_file *file_priv) + struct drm_file *file_priv, + bool from_2) { struct drm_crtc *crtc; int ret = 0; @@ -2907,7 +2908,8 @@ static int drm_mode_cursor_common(struct drm_device *dev, goto out; } /* Turns off the cursor if handle is 0 */ - if (crtc->funcs->cursor_set2) + if (crtc->funcs->cursor_set2 && + (from_2 || !crtc->funcs->cursor_set)) ret = crtc->funcs->cursor_set2(crtc, file_priv, req->handle, req->width, req->height, req->hot_x, req->hot_y); else @@ -2953,7 +2955,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev, memcpy(&new_req, req, sizeof(struct drm_mode_cursor)); new_req.hot_x = new_req.hot_y = 0; - return drm_mode_cursor_common(dev, &new_req, file_priv); + return drm_mode_cursor_common(dev, &new_req, file_priv, false); } /** @@ -2976,7 +2978,7 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev, { struct drm_mode_cursor2 *req = data; - return drm_mode_cursor_common(dev, req, file_priv); + return drm_mode_cursor_common(dev, req, file_priv, true); } /**
If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is implemented, the cursor hotspot is set to (0,0) which is incompatible with vmwgfx where the hotspot should instead remain unchanged. So if the driver implements both cursor_set2 and cursor_set, prefer calling the latter from the drm_mode_cursor ioctl. Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com> --- drivers/gpu/drm/drm_crtc.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)