Re: frustrated by gitlab - where is the release history?

[Emil Velikov]

On Thu, 28 Feb 2019 at 08:18, Felix Miata  wrote:
>
> https://cgit.freedesktop.org/xorg/driver/xf86-video-intel/ has what I'm 
> looking for for the Intel
> DDX. I would like to find whatever corresponds to it for the server, which on 
> opensuse seems to be
> called xorg-x11-server. URL in its info is merely 
> http://xorg.freedesktop.org/. Name in Debian
> seems to be xserver-xorg-core. It seems like 
> https://gitlab.freedesktop.org/xorg/xserver should
> have it, but I don't see it. What am I missing?

The distribution names does not always follow the upstream repository name.
As in this case xorg-x11-server vs xserver-xorg-core vs xorg/xserver.

If your distribution does not list the correct URL, please file a bug report.
Then it's a matter of using $URL/tags - as Pekka pointed out.

HTH
Emil
___
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

Re: WebKit failing to find GLXFBConfig, confusion around fbconfigs + swrast

[Emil Velikov]

Hi Daniel,

On 27 August 2018 at 09:07, Daniel Drake  wrote:

> Questions:
>
> 1. What should webkit be doing in event of it not being to find a
> GLXFBConfig that corresponds to the X visual of it's window?
>
>
Attempt another config that user(webkit) knows how to work with?

> 2. Why is swrast coming into the picture? Is swrast being used for rendering?
>

You're using the modesetting DDX, so the 2D acceleration comes from
glamor/OpenGL.
When the driver name cannot be retrieved, glamor will use ... well no
driver - aka swrast.

Possible solutions/workarounds:
 - try the intel ddx - not everyone is using it, it sees development
yet no releases :-\
 - audit the codepaths if one cannot get the driver name otherwise

> 4. Why is there still a list of PCI IDs in the X server?
>
Nobody had the time/interest to fix that up. I did fold the 5 (IIRC)
different codepaths in Mesa down to 1.
ETIME and EWORK kind of got in the way of fixing up the X server.

I guess it could go up my priority list, if employer (Collabora) suggests it ;-)

HTH
Emil
___
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

Re: [PATCH xf86-video-sis] Minor adjustment to the compilation order

[Emil Velikov]

Hi Kevin,

Jfyi: the original seems to align with $LC_ALL=C ls | sort
Must admit that sorting tends to be a bit annoying, so personally I
try to stick with the command ;-)

HTH
Emil
___
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

Re: [PATCH xserver] glamor_egl: request GL2.1 when requesting Desktop GL context

[Emil Velikov]

On 21 August 2018 at 17:01, Icenowy Zheng  wrote:
> Some devices cannot support OpenGL 2.1, which is the minimum desktop GL
> version required by glamor. However, they may support OpenGL ES 2.0,
> which is the GLES version required by glamor. Usually in this situation
> the desktop GL version supported is 2.0 or 1.4.
>
> Currently, as no requirements are passed when creating desktop GL
> context, a OpenGL 1.4/2.0 context will be created, and glamor will
> arguing that the context is not suitable, although the GPU supports a
> suitable GLES context.
>
> Add version number 2.1 requirement when requesting non-core desktop GL
> context (core context has at least 3.1), so it will fall back to create
> GLES contexts when the version number requirement is not met.
>
I don't know glamor enough to say if OpenGL 2.1 or another version
should be required.
Small mildly related note below.

> Tested on a Intel 945GMS integrated GPU, which supports GL 1.4 and GLES
> 2.0. Before applying this, it will fail to launch X server when no
> configuration is present because of glamor initialization failure, after
> applying glamor will start with GLES.
>
> Signed-off-by: Icenowy Zheng 
> ---
>  glamor/glamor.h | 4 
>  glamor/glamor_egl.c | 4 
>  2 files changed, 8 insertions(+)
>
> diff --git a/glamor/glamor.h b/glamor/glamor.h
> index 09e9c895c..abee6a3e8 100644
> --- a/glamor/glamor.h
> +++ b/glamor/glamor.h
> @@ -75,6 +75,10 @@ typedef Bool (*GetDrawableModifiersFuncPtr) (DrawablePtr 
> draw,
>  #define GLAMOR_GL_CORE_VER_MAJOR 3
>  #define GLAMOR_GL_CORE_VER_MINOR 1
>
> +/* We need OpenGL 2.1 to work at least */
> +#define GLAMOR_GL_VER_MAJOR 2
> +#define GLAMOR_GL_VER_MINOR 1
> +
>  /* @glamor_init: Initialize glamor internal data structure.
>   *
>   * @screen: Current screen pointer.
> diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
> index b33d8ef15..8da8d2731 100644
> --- a/glamor/glamor_egl.c
> +++ b/glamor/glamor_egl.c
> @@ -946,6 +946,10 @@ glamor_egl_init(ScrnInfoPtr scrn, int fd)
>  EGL_NONE
>  };
>  static const EGLint config_attribs[] = {
> +EGL_CONTEXT_MAJOR_VERSION_KHR,
> +   GLAMOR_GL_VER_MAJOR,
> +   EGL_CONTEXT_MINOR_VERSION_KHR,
The EGL attributes are part of EGL_KHR_create_context, which we do not
check currently.
Since EGL_CONTEXT_MAJOR_VERSION_KHR is effectively an alias for
EGL_CONTEXT_CLIENT_VERSION one could use the latter and GL_VERSION of
the created context.

it's technically correct, although it feels like an overkill.
Worth checking what others think on the topic.

HTH
Emil
___
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

Re: gitlab group permissions update

[Emil Velikov]

Hi Adan,

On 20 August 2018 at 20:17, Adam Jackson  wrote:
> gitlab groups are recursive, which means if you are a member of the
> 'xorg' group, your permission level for every project in that group is
> at least as high as your permission at the top level. Most of the
> existing accounts were set as Maintainer, at which level you can do
> things like read and set the secret tokens used for web API
> integrations, which is maybe a little too permissive in general.
>
> I've bumped most people down to Developer, which is still enough to do
> things like close issues and merge MRs. The difference is essentially
> that Maintainers can modify the gitlab environment of a project, in
> addition to just the project's content like a Developer. If you need
> higher access for your subprojects, give a shout.
>
That solves some confusion, as the notification email came.
I'm 100% behind the reason, although a suggestion for the future:

I wonder about having this in gitlab/bugzilla issue tracking with
follow-up action/commit.
It serves as a nice example of transparent/open-source development
{admin really} model.

That said, I'm not 100% sure if permission changes are tracked - git
or otherwise.

HTH
Emil
___
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

Re: 4K@60 YCbCr420 missing mode in usermode

[Emil Velikov]

On 27 June 2018 at 09:40, Michel Dänzer  wrote:
> On 2018-06-26 07:11 PM, Emil Velikov wrote:
>> On 26 June 2018 at 17:23, Michel Dänzer  wrote:
>>> On 2018-06-26 05:43 PM, Emil Velikov wrote:
>>>> On 25 June 2018 at 22:45, Zuo, Jerry  wrote:
>>>>> Hello all:
>>>>>
>>>>>
>>>>>
>>>>> We are working on an issue affecting 4K@60 HDMI display not to light up, 
>>>>> but
>>>>> only showing up 4K@30 from:
>>>>> https://bugs.freedesktop.org/show_bug.cgi?id=106959 and others.
>>>>>
>>>>>
>>>>>
>>>>> Some displays (e.g., ASUS PA328) HDMI port shows YCbCr420 CEA extension
>>>>> block with 4K@60 supported. Such HDMI 4K@60 is not real HDMI 2.0, but 
>>>>> still
>>>>> following HDMI 1.4 spec. with maximum TMDS clock of 300MHz instead of
>>>>> 600MHz.
>>>>>
>>>>> To get such 4K@60 supported, it needs to limit the bandwidth by reducing 
>>>>> the
>>>>> color space to YCbCr420 only. We’ve already raised YCbCr420 only flag
>>>>> (attached patch) from kernel side to pass the mode validation, and expose 
>>>>> it
>>>>> to user space.
>>>>>
>>>>>
>>>>>
>>>>> We think that one of the issues that causes this problem is due to
>>>>> usermode pruning the 4K@60 mode from the modelist (attached Xorg.0.log). 
>>>>> It
>>>>> seems like when usermode receives all the modes, it doesn't take in 
>>>>> account
>>>>> the 4K@60 YCbCr4:2:0 specific mode. In order to pass validation of being
>>>>> added to usermode modelist, its pixel clk needs to be divided by 2 so that
>>>>> it won't exceed TMDS max physical pixel clk (300MHz). That might explain 
>>>>> the
>>>>> difference in modes between our usermode and modeset.
>>>>>
>>>>>
>>>>>
>>>>> Such YCbCr4:2:0 4K@60 special mode is marked in DRM by raising a flag
>>>>> (y420_vdb_modes) inside connector's display_info which can be seen in
>>>>> do_y420vdb_modes(). Usermode could rely on that flag to pick up such mode
>>>>> and halve the required pclk to prevent such mode getting pruned out.
>>>>>
>>>>>
>>>>>
>>>>> We were hoping for someone helps to look at it from usermode perspective.
>>>>> Thanks a lot.
>>>>>
>>>> Just some observations, while going through some coffee. Take them
>>>> with a pinch of salt.
>>>>
>>>> Currently the kernel edid parser (in drm core) handles the
>>>> EXT_VIDEO_DATA_BLOCK_420 extended block.
>>>> Additionally, the kernel allows such modes only as the (per connector)
>>>> ycbcr_420_allowed bool is set by the driver.
>>>>
>>>> Quick look shows that it's only enabled by i915 on gen10 && geminilake 
>>>> hardware.
>>>>
>>>> At the same time, X does it's own fairly partial edid parsing and
>>>> doesn't handle any(?) extended blocks.
>>>>
>>>> One solution is to update the X parser, although it seems like a
>>>> endless game of cat and mouse.
>>>> IMHO a much better approach is to not use edid codepaths for KMS
>>>> drivers (where AMDGPU is one).
>>>> On those, the supported modes is advertised by the kernel module via
>>>> drmModeGetConnector.
>>>
>>> We are getting the modes from the kernel; the issue is they are then
>>> pruned (presumably by xf86ProbeOutputModes => xf86ValidateModesClocks)
>>> due to violating the clock limits, as described by Jerry above.
>>>
>> Might have been too brief there. Here goes a more elaborate
>> suggestion, please point out any misunderstandings.
>>
>> If we look into the drivers we'll see a call to xf86InterpretEDID(),
>> followed by xf86OutputSetEDID().
>> The former doing a partial parsing of the edid, creating a xf86MonPtr
>> (timings information et al.) with the latter attaching it to the
>> output.
>>
>> Thus as we get into xf86ProbeOutputModes/xf86ValidateModesClocks the
>> Xserver probes the mode against given timing/bandwidth constrains,
>> discarding it where applicable.
>>
>> Considering that the DRM driver already does similar checks, X could
>> side-step the parsing and filtering

Re: 4K@60 YCbCr420 missing mode in usermode

[Emil Velikov]

On 26 June 2018 at 17:23, Michel Dänzer  wrote:
> On 2018-06-26 05:43 PM, Emil Velikov wrote:
>> Hi Jerry,
>>
>> On 25 June 2018 at 22:45, Zuo, Jerry  wrote:
>>> Hello all:
>>>
>>>
>>>
>>> We are working on an issue affecting 4K@60 HDMI display not to light up, but
>>> only showing up 4K@30 from:
>>> https://bugs.freedesktop.org/show_bug.cgi?id=106959 and others.
>>>
>>>
>>>
>>> Some displays (e.g., ASUS PA328) HDMI port shows YCbCr420 CEA extension
>>> block with 4K@60 supported. Such HDMI 4K@60 is not real HDMI 2.0, but still
>>> following HDMI 1.4 spec. with maximum TMDS clock of 300MHz instead of
>>> 600MHz.
>>>
>>> To get such 4K@60 supported, it needs to limit the bandwidth by reducing the
>>> color space to YCbCr420 only. We’ve already raised YCbCr420 only flag
>>> (attached patch) from kernel side to pass the mode validation, and expose it
>>> to user space.
>>>
>>>
>>>
>>> We think that one of the issues that causes this problem is due to
>>> usermode pruning the 4K@60 mode from the modelist (attached Xorg.0.log). It
>>> seems like when usermode receives all the modes, it doesn't take in account
>>> the 4K@60 YCbCr4:2:0 specific mode. In order to pass validation of being
>>> added to usermode modelist, its pixel clk needs to be divided by 2 so that
>>> it won't exceed TMDS max physical pixel clk (300MHz). That might explain the
>>> difference in modes between our usermode and modeset.
>>>
>>>
>>>
>>> Such YCbCr4:2:0 4K@60 special mode is marked in DRM by raising a flag
>>> (y420_vdb_modes) inside connector's display_info which can be seen in
>>> do_y420vdb_modes(). Usermode could rely on that flag to pick up such mode
>>> and halve the required pclk to prevent such mode getting pruned out.
>>>
>>>
>>>
>>> We were hoping for someone helps to look at it from usermode perspective.
>>> Thanks a lot.
>>>
>> Just some observations, while going through some coffee. Take them
>> with a pinch of salt.
>>
>> Currently the kernel edid parser (in drm core) handles the
>> EXT_VIDEO_DATA_BLOCK_420 extended block.
>> Additionally, the kernel allows such modes only as the (per connector)
>> ycbcr_420_allowed bool is set by the driver.
>>
>> Quick look shows that it's only enabled by i915 on gen10 && geminilake 
>> hardware.
>>
>> At the same time, X does it's own fairly partial edid parsing and
>> doesn't handle any(?) extended blocks.
>>
>> One solution is to update the X parser, although it seems like a
>> endless game of cat and mouse.
>> IMHO a much better approach is to not use edid codepaths for KMS
>> drivers (where AMDGPU is one).
>> On those, the supported modes is advertised by the kernel module via
>> drmModeGetConnector.
>
> We are getting the modes from the kernel; the issue is they are then
> pruned (presumably by xf86ProbeOutputModes => xf86ValidateModesClocks)
> due to violating the clock limits, as described by Jerry above.
>
Might have been too brief there. Here goes a more elaborate
suggestion, please point out any misunderstandings.

If we look into the drivers we'll see a call to xf86InterpretEDID(),
followed by xf86OutputSetEDID().
The former doing a partial parsing of the edid, creating a xf86MonPtr
(timings information et al.) with the latter attaching it to the
output.

Thus as we get into xf86ProbeOutputModes/xf86ValidateModesClocks the
Xserver probes the mode against given timing/bandwidth constrains,
discarding it where applicable.

Considering that the DRM driver already does similar checks, X could
side-step the parsing and filtering/validation all together.
Trusting the kernel should be reasonable, considering weston (and I
would imagine other wayland compositors) already do so.

Obviously, manually added modelines (via a config file) would still
need to be validated.

Thanks
Emil
___
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

Re: [PATCH] Improve the ButtonInfo description.

[Emil Velikov]

On 8 June 2018 at 01:59, Peter Hutterer  wrote:
> On Thu, Jun 07, 2018 at 03:32:08AM +0200, Roman Kapl wrote:
>> It failed to mention it is followed by a bit-mask and then the atoms.
>>
>> Signed-off-by: Roman Kapl 
>> ---
>>  include/X11/extensions/XI2proto.h | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> Patch content is correct, thanks. But what repository did you make this
> patch to?  This should go into the inputproto repo but it has the XI2proto.h
> in the root tree.
>
AFAICT this patch is against xorgproto, which I believe is the way forward?
The inputproto autogen.sh seems fairly clear about it.

-Emil
___
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

Re: 4K@60 YCbCr420 missing mode in usermode

[Emil Velikov]

Hi Jerry,

On 25 June 2018 at 22:45, Zuo, Jerry  wrote:
> Hello all:
>
>
>
> We are working on an issue affecting 4K@60 HDMI display not to light up, but
> only showing up 4K@30 from:
> https://bugs.freedesktop.org/show_bug.cgi?id=106959 and others.
>
>
>
> Some displays (e.g., ASUS PA328) HDMI port shows YCbCr420 CEA extension
> block with 4K@60 supported. Such HDMI 4K@60 is not real HDMI 2.0, but still
> following HDMI 1.4 spec. with maximum TMDS clock of 300MHz instead of
> 600MHz.
>
> To get such 4K@60 supported, it needs to limit the bandwidth by reducing the
> color space to YCbCr420 only. We’ve already raised YCbCr420 only flag
> (attached patch) from kernel side to pass the mode validation, and expose it
> to user space.
>
>
>
> We think that one of the issues that causes this problem is due to
> usermode pruning the 4K@60 mode from the modelist (attached Xorg.0.log). It
> seems like when usermode receives all the modes, it doesn't take in account
> the 4K@60 YCbCr4:2:0 specific mode. In order to pass validation of being
> added to usermode modelist, its pixel clk needs to be divided by 2 so that
> it won't exceed TMDS max physical pixel clk (300MHz). That might explain the
> difference in modes between our usermode and modeset.
>
>
>
> Such YCbCr4:2:0 4K@60 special mode is marked in DRM by raising a flag
> (y420_vdb_modes) inside connector's display_info which can be seen in
> do_y420vdb_modes(). Usermode could rely on that flag to pick up such mode
> and halve the required pclk to prevent such mode getting pruned out.
>
>
>
> We were hoping for someone helps to look at it from usermode perspective.
> Thanks a lot.
>
Just some observations, while going through some coffee. Take them
with a pinch of salt.

Currently the kernel edid parser (in drm core) handles the
EXT_VIDEO_DATA_BLOCK_420 extended block.
Additionally, the kernel allows such modes only as the (per connector)
ycbcr_420_allowed bool is set by the driver.

Quick look shows that it's only enabled by i915 on gen10 && geminilake hardware.

At the same time, X does it's own fairly partial edid parsing and
doesn't handle any(?) extended blocks.

One solution is to update the X parser, although it seems like a
endless game of cat and mouse.
IMHO a much better approach is to not use edid codepaths for KMS
drivers (where AMDGPU is one).
On those, the supported modes is advertised by the kernel module via
drmModeGetConnector.

Doubt I'll have time to tackle this, so this is a brain dump for
anyone interested in fixing this.

HTH
Emil
___
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

Re: [PATCH xf86-video-savage] Add check for max[HV]Value to ValidMode hook

[Emil Velikov]

On 25 June 2018 at 14:55, Stefan Dirsch  wrote:
> xorg-server 1.20 removed this check, so implement this in the driver
> itself.
>
Quick look shows that Adam, updated most drivers although a few are remaining.
Namely:
s3verge
tseng
vga
savage - addressed with this patch.

> Signed-off-by: Stefan Dirsch 
Reviewed-by: Emil Velikov 

I'll push this shortly. Although if you'd like to address the other
drivers, that'll be appreciated.

Thanks
Emil
___
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

Re: [PATCH xf86-video-r128] Include in order to declare uint32_t

[Emil Velikov]

On 25 June 2018 at 14:57, Stefan Dirsch  wrote:
> Apparently this is needed in src/atipcirename.h since xorg-server
> 1.20 in order to still build this driver.
>
> Signed-off-by: Stefan Dirsch 
Trivial - reviewed and pushed it to master.

Thanks
Emil
___
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

Re: [PATCH xserver] : bug 106963 : change the DPMS initialization to be conditional on not set from config

[Emil Velikov]

Hi John,

Please don't drop the ML when replying.

On 19 June 2018 at 20:32, John Lumby  wrote:
> I've tried to conform to those guidelines and resent -
> however I cannot use git send-email from this email account because I would
> have to send it from my dev system and  the mailing-list server rejects any
> email which says it is "From" hotmail but actually sourced from somewhere
> else.This is the best I can do.
>
It does sound a bit strange - I often send patches with the corporate
email in From: via my Gmail account.
Might be worth looking over at a later stage.

> Cheers, John
> 
> From: John Lumby 
> Sent: June 19, 2018 3:29 PM
> To: xorg-devel@lists.x.org
> Cc: Emil Velikov
> Subject: Re: [PATCH xserver] : bug 106963 : change the DPMS initialization
> to be conditional on not set from config
>
> Any DPMS timeout values set in ServerFlags section of the xorg.conf are
> being overwritten
>  by DPMS extension initialization.
> Therefore change the DPMS initialization of timeout values to be conditional
> on not set from config.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106963
>
Thanks for the update, patch is
Reviewed-by: Emil Velikov 

Adam, can we pick this up for 1.20 and earlier releases, if applicable?

Thanks
Emil
___
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

Re: [PATCH] release: fix mesa url detection after migration to gitlab

[Emil Velikov]

On 19 June 2018 at 16:37, Dylan Baker  wrote:
> Quoting Dylan Baker (2018-06-15 14:12:55)
>> Mesa's migration to gitlab has changed the URL and made things not work.
>> ---
>>  release.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/release.sh b/release.sh
>> index b2071b6..6b8e441 100755
>> --- a/release.sh
>> +++ b/release.sh
>> @@ -240,7 +240,7 @@ get_section() {
>> module_url=`echo $module_url | cut -d'/' -f3,4`
>>  else
>> # The look for mesa, xcb, etc...
>> -   module_url=`echo "$full_module_url" | $GREP -o -e "/mesa/.*" -e 
>> "/xcb/.*" -e "/xkeyboard-config" -e "/nouveau/xf86-video-nouveau" -e 
>> "/libevdev" -e "/wayland/.*" -e "/evemu"`
>> +   module_url=`echo "$full_module_url" | $GREP -o -e "mesa/.*" -e 
>> "/xcb/.*" -e "/xkeyboard-config" -e "/nouveau/xf86-video-nouveau" -e 
>> "/libevdev" -e "/wayland/.*" -e "/evemu"`
>> if [ $? -eq 0 ]; then
>>  module_url=`echo $module_url | cut -d'/' -f2,3`
>> else
>> --
>> 2.17.1
>>
>
> I should probably also mention I don't have (or want) Xorg push access :)
>
Fair enough. Pushed to master

remote: Updating patchwork state for
https://patchwork.freedesktop.org/project/Xorg/list/
remote: I: patch #229923 updated using rev
340cb7b046e2b43aeb62d9a278bc6acb657cbf7e.
remote: I: 1 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/util/modular
   f9c53a4..340cb7b  HEAD -> master


-Emil
___
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

Re: [PATCH xserver] : bug 106963 : change the DPMS initialization to be conditional on not set from config

[Emil Velikov]

Hi John,

On 19 June 2018 at 16:16, John Lumby  wrote:
>
> --- xorg/xserver/Xext/dpms.c.orig   2018-06-16 18:54:24.520660890 -0400
> +++ xorg/xserver/Xext/dpms.c2018-06-18 11:09:19.021529381 -0400
> @@ -45,9 +45,9 @@ Equipment Corporation.
>
>  CARD16 DPMSPowerLevel = 0;
>  Bool DPMSDisabledSwitch = FALSE;
> -CARD32 DPMSStandbyTime;
> -CARD32 DPMSSuspendTime;
> -CARD32 DPMSOffTime;
> +CARD32 DPMSStandbyTime = -1;
> +CARD32 DPMSSuspendTime = -1;
> +CARD32 DPMSOffTime = -1;
>  Bool DPMSEnabled;
>
>  Bool
> @@ -432,7 +432,15 @@ DPMSCloseDownExtension(ExtensionEntry *e
>  void
>  DPMSExtensionInit(void)
>  {
> -DPMSStandbyTime = DPMSSuspendTime = DPMSOffTime = ScreenSaverTime;
> +#define CONDITIONALLY_SET_DPMS_TIMEOUT(_timeout_value_) \
> +if (_timeout_value_ == -1) { /* not yet set from config */  \
> +_timeout_value_ = ScreenSaverTime;  \
> +}
> +
> +CONDITIONALLY_SET_DPMS_TIMEOUT(DPMSStandbyTime)
> +CONDITIONALLY_SET_DPMS_TIMEOUT(DPMSSuspendTime)
> +CONDITIONALLY_SET_DPMS_TIMEOUT(DPMSOffTime)
> +
>  DPMSPowerLevel = DPMSModeOn;
>  DPMSEnabled = DPMSSupported();

Functionality-wise the patch looks spot on. Can you please tweak the
formatting/commit message.
The specifics are listed here [1] although the gist is:
 - in the commit message explain why patch is needed - Otherwise any
user settings in xorg.conf will be discarded
 - use the full URL for bugzilla references - Fixes: https...
 - use git to manage/send patches - git send-email
--subject-prefix="PATCH xserver" --to=xorg-devel@lists.x.org ...

HTH
Emil

[1] https://www.x.org/wiki/Development/Documentation/SubmittingPatches/
___
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

Re: [PATCH xorgproto 1/2] Remove the use of no-op B16 & B32 bitfield macros in headers

[Emil Velikov]

On 18 June 2018 at 01:30, Keith Packard  wrote:
> Alan Coopersmith  writes:
>
>> These have always done nothing on all platforms except CRAY.
>> As https://bugs.freedesktop.org/show_bug.cgi?id=45202 points out
>> we don't even detect when they've been wrong for decades.
>>
>> Performed via:
>> find include -name '*.h' | grep -v md.h | xargs perl -i -p -e 's{\s+B\d+}{}g'
>> followed by manual whitespace fixups to preserve visual alignment.
>>
>> The #defines for B16 & B32 are left in place to preserve compatibility
>> in any code that used them outside the xorgproto repo.
>>

Good call about keeping those. Quick look shows a handful of users:

app/lbxproxy
app/xfs

driver/xf86-video-sis
driver/xf86-video-sisusb
driver/xf86-video-via
driver/xf86-video-vmware

lib/libICE
lib/libICE
lib/liblbxutil
lib/libSM
lib/libX11
lib/libxkbfile

mesa/drm
mesa/mesa - dri1, appledri, windowsdri copies

xserver - same as mesa

>> Signed-off-by: Alan Coopersmith 
>
> Acked-by: Keith Packard 
>

Reviewed-by: Emil Velikov 

HTH
Emil
___
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

Re: [PATCH] release: fix mesa url detection after migration to gitlab

[Emil Velikov]

On 15 June 2018 at 22:12, Dylan Baker  wrote:
> Mesa's migration to gitlab has changed the URL and made things not work.

Thanks for the fixup Dylan.

Reviewed-by: Emil Velikov 

-Emil
___
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

Re: [PATCH xserver 0/2] xwayland: couple of EGL backend API cleanup

[Emil Velikov]

On 11 June 2018 at 09:22, Olivier Fourdan  wrote:
> Hi,
>
> The recent issue with Present due to the wrong pixmap size being given
> to xwl_glamor_pixmap_get_wl_buffer() got me thinking.
>
> Considering that the code always use the drawable size now, that passing
> a wrong size is error prone because once the buffer is created for the
> given pixmap, subsequent calls to xwl_glamor_pixmap_get_wl_buffer() will
> always return that same buffer with GBM backend and considering EGLStream
> has no use for the given size either, there is really no point in passing
> the width/height in the API...
>
> The second patch was suggested by Emil, both GBM and EGLStream backends
> implement init_wl_registry() and has_wl_interfaces() so there is no
> point in marking those as optional.
>
> Those patches go on top and complement the previous series, i.e.:
>
>  https://patchwork.freedesktop.org/series/44302/
>  https://patchwork.freedesktop.org/series/44303/
>  https://patchwork.freedesktop.org/series/44423/
>  https://patchwork.freedesktop.org/series/44489/
>
What's the blocker of pushing these - short on commit access, other?
If the former, that should be addressed. You have done some serious
work on Xwayland.

That said, the series is
Reviewed-by: Emil Velikov 

-Emil
___
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

Re: [PATCH xserver 10/10] xwayland: EGL_IMG_context_priority required by EGLStream

[Emil Velikov]

On 5 June 2018 at 18:38, Olivier Fourdan  wrote:
> xwl_glamor_eglstream_init_egl() uses "EGL_IMG_context_priority"
> extension, make sure it's actually available before using it.
>
> Suggested-by: Emil Velikov 
> Signed-off-by: Olivier Fourdan 
> ---
>  hw/xwayland/xwayland-glamor-eglstream.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/xwayland/xwayland-glamor-eglstream.c 
> b/hw/xwayland/xwayland-glamor-eglstream.c
> index c226c0089..43f34eed1 100644
> --- a/hw/xwayland/xwayland-glamor-eglstream.c
> +++ b/hw/xwayland/xwayland-glamor-eglstream.c
> @@ -794,6 +794,12 @@ xwl_glamor_eglstream_init_egl(struct xwl_screen 
> *xwl_screen)
>  goto error;
>  }
>
> +if (!epoxy_has_egl_extension(xwl_screen->egl_display,
> + "EGL_IMG_context_priority")) {
> +ErrorF("EGL_IMG_context_priority not available\n");
> +goto error;
> +}
> +
Another approach is to adjust the array to omit the attributes.
Either way, the series is:

Reviewed-by: Emil Velikov 

Thanks for going through my nit-picky suggestions.
Emil
___
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

Re: [PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry

[Emil Velikov]

On 5 June 2018 at 18:28, Olivier Fourdan  wrote:
> Hi Emil,
>
> Many thanks for your detailed review!
>
> On Tue, Jun 5, 2018 at 12:37 PM, Emil Velikov  
> wrote:
>> Hi Olivier,
>>
>> There's a handful of mostly trivial suggestions below. The idea itself seems
>> reasonable IMHO. One gripe is that we're 'leaking' twice as much as before.
>>
>> Namely: even if the current backend cleans-up after itself (it some cases it
>> does not), the other backend 'leaks'. Not sure if/how much we should care in
>> that case.
>>
>> Was skimming if we cannot move more init_egl/init_screen tripping points
>> and I've noticed a few gotchas/bugs. None of which are requirement for the
>> series, although great to have.
>>
>>
>> xwl_glamor_gbm_init_egl
>>  - if !render_node error path is leaking
>
> humm, not sure, leaking what? we haven't allocated anything yet, have we?
> But anyhow, it's unralted to this refactoring I think.
>
You are right, this and the other bits listed just below are preexisting.
Thanks for tackling them.

The following are set during init_registry (wl_drm path) and
effectively leaked. Then again, there's plenty of small bits like that
throughout the code base.
I would _not_ bother with them bth.

::device_name -> strdup(device/node name)
::drm_fd -> open(^^)
::drm -> wl_registry_bind


>>> @@ -119,8 +134,8 @@ xwl_glamor_allow_commits(struct xwl_window *xwl_window)
>>>  {
>>>  struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
>>>
>>> -if (xwl_screen->egl_backend.allow_commits)
>>> -return xwl_screen->egl_backend.allow_commits(xwl_window);
>>> +if (xwl_screen->egl_backend && xwl_screen->egl_backend->allow_commits)
>> Why the NULL check? Unless I'm missing something that will never happen.
>
> We can have no “egl_backend” set at all if “-shm” was specified.
>
> Either we keep that test here or we need to check for
> "xwl_screen->glamor" in xwl_screen_post_damage(), I'll change for the
> later for clarity.
>
Ah, yes. Thanks.

-Emil
___
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

Re: [PATCH xserver 1/5] xwayland: Allow "-eglstream" option

[Emil Velikov]

On 4 June 2018 at 15:37, Olivier Fourdan  wrote:
> Hi
>
> On 4 June 2018 at 16:24, Emil Velikov  wrote:
>>
>> On 24 May 2018 at 15:10, Olivier Fourdan  wrote:
>> > The command line option "-eglstream" used to enable EGLi stream support
>> > for NVidia GPU was made available only when Xwayland was built with EGL
>> > stream support enabled.
>> >
>> > Wayland compositors who spawn Xwayland have no easy way to tell whether
>> > or not Xwayland was built with EGL stream support enabled, and adding
>> > "-eglstream" command line option to Xwayland when it wasn't built with
>> > EGL support would prevent Xwayland from starting (“Unrecognized option”
>> > error).
>> >
>> > Make sure we support the command line option "-eglstream" regardless of
>> > EGL stream support in Xwayland, obviously without EGL stream support
>> > this has no effect.
>> >
>> > Signed-off-by: Olivier Fourdan 
>> > ---
>> >  hw/xwayland/xwayland.c | 10 --
>> >  1 file changed, 4 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
>> > index 1d6b49979..b4049d2cc 100644
>> > --- a/hw/xwayland/xwayland.c
>> > +++ b/hw/xwayland/xwayland.c
>> > @@ -96,9 +96,7 @@ ddxUseMsg(void)
>> >  ErrorF("-rootless  run rootless, requires wm
>> > support\n");
>> >  ErrorF("-wm fd create X client for wm on given
>> > fd\n");
>> >  ErrorF("-listen fd add give fd as a listen socket\n");
>> > -#ifdef XWL_HAS_EGLSTREAM
>> >  ErrorF("-eglstream use eglstream backend for nvidia
>> > GPUs\n");
>> > -#endif
>> >  }
>> >
>> >  int
>> > @@ -117,11 +115,9 @@ ddxProcessArgument(int argc, char *argv[], int i)
>> >  else if (strcmp(argv[i], "-shm") == 0) {
>> >  return 1;
>> >  }
>> > -#ifdef XWL_HAS_EGLSTREAM
>> >  else if (strcmp(argv[i], "-eglstream") == 0) {
>> >  return 1;
>> >  }
>> > -#endif
>> >
>> >  return 0;
>> >  }
>> > @@ -988,11 +984,13 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char
>> > **argv)
>> >  else if (strcmp(argv[i], "-shm") == 0) {
>> >  xwl_screen->glamor = 0;
>> >  }
>> > -#ifdef XWL_HAS_EGLSTREAM
>> >  else if (strcmp(argv[i], "-eglstream") == 0) {
>> > +#ifdef XWL_HAS_EGLSTREAM
>> >  use_eglstreams = TRUE;
>> > -}
>> > +#else
>> > +ErrorF("xwayland glamor: eglstream backend support not
>> > enabled\n");
>> Something is really weird here:
>>
>> On one hand '-eglstream' is recognised and used (by potential user) on
>> the other "... support is not _enabled_" is printed.
>> Surely you meant "not built", right? After all explicitly passing the
>> enable (runtime) flag should be enough to enable it ;-)
>
>
> Yes, I literally mean "enabled at build time".
>
This wording is even better.

Thanks
Emil
___
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

Re: [PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry

[Emil Velikov]

Hi Olivier,

There's a handful of mostly trivial suggestions below. The idea itself seems
reasonable IMHO. One gripe is that we're 'leaking' twice as much as before.

Namely: even if the current backend cleans-up after itself (it some cases it
does not), the other backend 'leaks'. Not sure if/how much we should care in
that case.

Was skimming if we cannot move more init_egl/init_screen tripping points
and I've noticed a few gotchas/bugs. None of which are requirement for the
series, although great to have.


xwl_glamor_gbm_init_egl
 - if !render_node error path is leaking
 - surely we'd want to error out when GL_OES_EGL_image is missing
xwl_glamor_gbm_init_screen
 - no need to RegisterPrivateKey/AddCallback if a rendernode is opened

xwl_glamor_eglstream_init_egl
 - using EGL_IMG_context_priority w/o checking for it
xwl_glamor_eglstream_init_screen
 - the ->controller check is no longer needed

On 1 June 2018 at 15:31, Olivier Fourdan  wrote:

> +static Bool
> +xwl_glamor_gbm_has_egl_extension(void)
> +{
> +return epoxy_has_egl_extension(NULL, "EGL_MESA_platform_gbm");
I'd also check for EGL_KHR_platform_gbm - it's trivial enough ;-)


> @@ -71,9 +71,24 @@ xwl_glamor_init_wl_registry(struct xwl_screen *xwl_screen,
>  uint32_t id, const char *interface,
>  uint32_t version)
>  {
> -if (xwl_screen->egl_backend.init_wl_registry)
> -xwl_screen->egl_backend.init_wl_registry(xwl_screen, registry,
> - interface, id, version);
> +#ifdef GLAMOR_HAS_GBM
> +if (xwl_screen->gbm_backend.is_available &&
> +xwl_screen->gbm_backend.init_wl_registry &&
> +xwl_screen->gbm_backend.init_wl_registry(xwl_screen,
> + registry,
> + id,
> + interface,
> + version)); /* no-op */
> +#endif
> +#ifdef XWL_HAS_EGLSTREAM
> +else if (xwl_screen->eglstreams_backend.is_available &&
> + xwl_screen->eglstreams_backend.init_wl_registry &&
> + xwl_screen->eglstreams_backend.init_wl_registry(xwl_screen,
> + registry,
> + id,
> + interface,
> + version)); /* 
> no-op */
> +#endif
Both ifdef guards can go.


> @@ -119,8 +134,8 @@ xwl_glamor_allow_commits(struct xwl_window *xwl_window)
>  {
>  struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
>
> -if (xwl_screen->egl_backend.allow_commits)
> -return xwl_screen->egl_backend.allow_commits(xwl_window);
> +if (xwl_screen->egl_backend && xwl_screen->egl_backend->allow_commits)
Why the NULL check? Unless I'm missing something that will never happen.


> @@ -165,17 +180,35 @@ glamor_egl_fd_name_from_pixmap(ScreenPtr screen,
>  void
>  xwl_glamor_init_backends(struct xwl_screen *xwl_screen, Bool use_eglstreams)
>  {

General nit:
Drop the return type of xwl_glamor_init_$backend now that we check
::is_available.

Instead of relying on the coded probe order (alongside the
is_available = false workaround),
we could use -eglstream to control which backend is checked/probed first.

If that fails we fall-back to the other.

If that sounds reasonable, then the following can be reworked as follows:

> +#ifdef GLAMOR_HAS_GBM
> +/* Init GBM even if "-eglstream" is requested, as EGL streams may fail */
> +xwl_glamor_init_gbm(xwl_screen);
Here we add:

  if (!xwl_screen->gbm_backend.is_available && !use_eglstreams)
ErrorF(xwayland glamor: GBM (default) is not available);

> +#endif
>  #ifdef XWL_HAS_EGLSTREAM
> +xwl_glamor_init_eglstream(xwl_screen);

>  if (use_eglstreams) {
> -if (!xwl_glamor_init_eglstream(xwl_screen)) {
> -ErrorF("xwayland glamor: failed to setup eglstream backend\n");
> -use_eglstreams = FALSE;
> -}
> +/* Force GBM backend off */
> +xwl_screen->gbm_backend.is_available = FALSE;
This is no longer needed, and we can now fold the two conditionals -
just like the GBM codepath.

> +if (!xwl_screen->eglstreams_backend.is_available)
> +ErrorF("xwayland glamor: EGLstreams requested but not 
> available\n");
>  }
>  #endif
> -if (!use_eglstreams && !xwl_glamor_init_gbm(xwl_screen)) {
> -ErrorF("xwayland glamor: failed to setup GBM backend, falling back 
> to sw accel\n");
> -xwl_screen->glamor = 0;
> +}
> +
> +void
> +xwl_glamor_select_backend(struct xwl_screen *xwl_screen, Bool use_eglstreams)
> +{
> +if (xwl_screen->egl_backend == NULL && 
> xwl_screen->gbm_backend.is_available) {
> +if (xwl_glamor_has_wl_interfaces(xwl_screen, 
> _screen->gbm_backend))
> + 

Re: [PATCH xserver 3/5] xwayland: Add Wayland interfaces check

[Emil Velikov]

On 1 June 2018 at 15:31, Olivier Fourdan  wrote:

> +static Bool
> +xwl_glamor_gbm_has_wl_interfaces(struct xwl_screen *xwl_screen)
> +{
> +struct xwl_gbm_private *xwl_gbm = xwl_gbm_get(xwl_screen);
> +
> +if (xwl_gbm->drm == NULL) {
> +ErrorF("glamor: 'wl_drm' not supported\n");
> +return FALSE;
> +}
> +
Please add a comment about dmabuf. I know it's optional, but in the
long run we'd want it instead of wl_drm ;-)


> @@ -69,6 +69,11 @@ struct xwl_egl_backend {
>   const char *name, uint32_t id,
>   uint32_t version);
>
> +/* Check that the required Wayland interfaces are available. This
> + * callback is optional.
Since it's implemented by both backends, so I'd call it mandatory and
drop the NULL check in xwl_glamor_has_wl_interfaces.

-Emil
___
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

Re: [PATCH xserver 2/5] xwayland: move egl_backend to its own struct

[Emil Velikov]

On 1 June 2018 at 15:31, Olivier Fourdan  wrote:
> EGL backend availability requires both EGL extensions and Wayland
> interfaces to be present, so we will need to consider multiple backends
> during initialization.
>
> As a preliminary work, move the egl_backend to its own struct so that we
> can have more than one backend at any given time.
>
> Signed-off-by: Olivier Fourdan 
> ---
>  hw/xwayland/xwayland.h | 99 ++
>  1 file changed, 51 insertions(+), 48 deletions(-)
>
> diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
> index 0d4afdf8a..0d6a4c246 100644
> --- a/hw/xwayland/xwayland.h
> +++ b/hw/xwayland/xwayland.h
> @@ -57,6 +57,56 @@ struct xwl_format {
>
>  struct xwl_pixmap;
>  struct xwl_window;
> +struct xwl_screen;
> +
> +struct xwl_egl_backend {
> +/* Called once for each interface in the global registry. Backends
> + * should use this to bind to any wayland interfaces they need. This
> + * callback is optional.
> + */
Since both backends implement this, I'd drop the "optional" statement
alongside the NULL check in the code.
Might be better to keep as separate patch though.

-Emil
___
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

Re: [PATCH xserver 5/5] xwayland: small xdg_output cleanup

[Emil Velikov]

On 24 May 2018 at 15:11, Olivier Fourdan  wrote:
> Make xwl_output_get_xdg_output() private, it doesn't need to be
> available elsewhere.
>
s/small xdg_output cleanup/make xwl_output_get_xdg_output static/

With this and the nitpicks in 1/5 + 3/5 nitpicks the series is
Reviewed-by: Emil Velikov 

-Emil
___
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

Re: [PATCH xserver 3/5] xwayland: process Wayland events after adding screen

[Emil Velikov]

On 24 May 2018 at 15:11, Olivier Fourdan  wrote:
> When we're done adding a new screen, we need to process pending Wayland
> events again so that we don't end up processing xdg_output events when
> unexpected if glamor is disabled (either becauase "-shm" was passed or
> because "-eglstream" failed):
>
Please add some punctuation here. Something like below takes 1/3 the
time to parse.

When we're done adding a new screen, we need to process any pending
Wayland events again.
Hence we don't end up processing xdg_output events unexpectedly when
glamor is disabled. Be that because "-shm" was passed or "-eglstream"
has failed.

-Emil
___
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

Re: [PATCH xserver 1/5] xwayland: Allow "-eglstream" option

[Emil Velikov]

On 24 May 2018 at 15:10, Olivier Fourdan  wrote:
> The command line option "-eglstream" used to enable EGLi stream support
> for NVidia GPU was made available only when Xwayland was built with EGL
> stream support enabled.
>
> Wayland compositors who spawn Xwayland have no easy way to tell whether
> or not Xwayland was built with EGL stream support enabled, and adding
> "-eglstream" command line option to Xwayland when it wasn't built with
> EGL support would prevent Xwayland from starting (“Unrecognized option”
> error).
>
> Make sure we support the command line option "-eglstream" regardless of
> EGL stream support in Xwayland, obviously without EGL stream support
> this has no effect.
>
> Signed-off-by: Olivier Fourdan 
> ---
>  hw/xwayland/xwayland.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
> index 1d6b49979..b4049d2cc 100644
> --- a/hw/xwayland/xwayland.c
> +++ b/hw/xwayland/xwayland.c
> @@ -96,9 +96,7 @@ ddxUseMsg(void)
>  ErrorF("-rootless  run rootless, requires wm support\n");
>  ErrorF("-wm fd create X client for wm on given fd\n");
>  ErrorF("-listen fd add give fd as a listen socket\n");
> -#ifdef XWL_HAS_EGLSTREAM
>  ErrorF("-eglstream use eglstream backend for nvidia GPUs\n");
> -#endif
>  }
>
>  int
> @@ -117,11 +115,9 @@ ddxProcessArgument(int argc, char *argv[], int i)
>  else if (strcmp(argv[i], "-shm") == 0) {
>  return 1;
>  }
> -#ifdef XWL_HAS_EGLSTREAM
>  else if (strcmp(argv[i], "-eglstream") == 0) {
>  return 1;
>  }
> -#endif
>
>  return 0;
>  }
> @@ -988,11 +984,13 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char 
> **argv)
>  else if (strcmp(argv[i], "-shm") == 0) {
>  xwl_screen->glamor = 0;
>  }
> -#ifdef XWL_HAS_EGLSTREAM
>  else if (strcmp(argv[i], "-eglstream") == 0) {
> +#ifdef XWL_HAS_EGLSTREAM
>  use_eglstreams = TRUE;
> -}
> +#else
> +ErrorF("xwayland glamor: eglstream backend support not 
> enabled\n");
Something is really weird here:

On one hand '-eglstream' is recognised and used (by potential user) on
the other "... support is not _enabled_" is printed.
Surely you meant "not built", right? After all explicitly passing the
enable (runtime) flag should be enough to enable it ;-)

-Emil
___
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

Re: [PATCH xserver] glamor: Work around GEM usage

[Emil Velikov]

On 23 May 2018 at 22:23, Mark Kettenis  wrote:
>> From: Thomas Hellstrom 
>> Date: Wed, 23 May 2018 22:58:05 +0200
>>
>> On 05/23/2018 08:00 PM, Adam Jackson wrote:
>> > On Wed, 2018-05-23 at 11:14 +0200, Thomas Hellstrom wrote:
>> >> KMS drivers are not required to support GEM. In particular, vmwgfx
>> >> doesn't support flink and handles and names are identical.
>> >> Getting a bo name should really be part of a lower level API, if needed,
>> >> but in the mean time work around this by setting the name identical to
>> >> the handle if GEM isn't supported.
>> > I'd thought flink was old and busted anyway. Could this path just go
>> > away?
>> >
>> > - ajax
>>
>> I guess it's needed for dri2? If all drivers using glamor are OK with
>> dropping dri2, then that should be OK.
>
> OpenBSD doesn't implement dri3 (yet)
AFAICT other BSDs like FreeBSD, DragonFly and NetBSD are in the same boat.

That said I think we should incentivise them to implement it sooner
than later.

-Emil
___
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

Re: [PATCH xserver 2/2] glamor: Propagate glamor_fds_from_pixmap error in glamor_fd_from_pixmap

[Emil Velikov]

On 23 May 2018 at 10:43, Michel Dänzer <mic...@daenzer.net> wrote:
> From: Michel Dänzer <michel.daen...@amd.com>
>
> glamor_fds_from_pixmap returns 0 on error, but we were treating that as
> success, continuing with uninitialized stride and fd values.
>
> Also bail if the offset isn't 0, same as in dri3_fd_from_pixmap.
>
> Fixes: c8c276c9569b "glamor: Implement PixmapFromBuffers and
>  BuffersFromPixmap"
> Signed-off-by: Michel Dänzer <michel.daen...@amd.com>

I would suggest a one-liner as below. Smaller patch, plus is reads
easier on my end ;-)

-if (ret > 1) {
+if (ret != 1 || offsets[0] != 0) {

Regardless, the series is on point and is
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

-Emil
___
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

Re: [PATCH xserver 2/4] dri3: Robustly clamp to 1.0 if not all screens support 1.2

[Emil Velikov]

On 30 April 2018 at 08:06, Mario Kleiner  wrote:
> Checking for dri3_screen_info_rec.version >= 2 is insufficient,
> as some shipping drivers, e.g., intel-ddx, nouveau-ddx, set the
> version to DRI3_SCREEN_INFO_VERSION, ie. to whatever version the
> installed servers headers define.

Hey, at least ati/amdgpu does it properly ;-)

Although in practise there are enough cases/bugs where drivers set the
version they're build against.
Instead of the one they support/implement.

Just grep for VERSION - anythings that's not a preprocessor guard
should be fixed.
Guess we should fix those one of these days.

FWIW series looks spot-on, though I can see it's already merged.
Emil
___
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

Re: [Mesa-dev] [PATCH i-g-t] [RFC] CONTRIBUTING: commit rights docs

[Emil Velikov]

On 24 April 2018 at 20:14, Daniel Vetter <daniel.vet...@ffwll.ch> wrote:
> On Tue, Apr 24, 2018 at 7:30 PM, Emil Velikov <emil.l.veli...@gmail.com> 
> wrote:
>> On 13 April 2018 at 11:00, Daniel Vetter <daniel.vet...@ffwll.ch> wrote:
>>> This tries to align with the X.org communities's long-standing
>>> tradition of trying to be an inclusive community and handing out
>>> commit rights fairly freely.
>>>
>>> We also tend to not revoke commit rights for people no longer
>>> regularly active in a given project, as long as they're still part of
>>> the larger community.
>>>
>>> Finally make sure that commit rights, like anything happening on fd.o
>>> infrastructre, is subject to the fd.o's Code of Conduct.
>>>
>>> v2: Point at MAINTAINERS for contact info (Daniel S.)
>>>
>>> v3:
>>> - Make it clear that commit rights are voluntary and that committers
>>>   need to acknowledge positively when they're nominated by someone
>>>   else (Keith).
>>> - Encourage committers to drop their commit rights when they're no
>>>   longer active, and make it clear they'll get readded (Keith).
>>> - Add a line that maintainers and committers should actively nominate
>>>   new committers (me).
>>>
>>> v4: Typo (Petri).
>>>
>>> v5: Typo (Sean).
>>>
>>> v6: Wording clarifications and spelling (Jani).
>>>
>>> v7: Require an explicit commitment to the documented merge criteria
>>> and rules, instead of just the implied one through the Code of Conduct
>>> threat (Jani).
>>>
>>> Acked-by: Alex Deucher <alexander.deuc...@amd.com>
>>> Acked-by: Arkadiusz Hiler <arkadiusz.hi...@intel.com>
>>> Acked-by: Daniel Stone <dan...@fooishbar.org>
>>> Acked-by: Eric Anholt <e...@anholt.net>
>>> Acked-by: Gustavo Padovan <gust...@padovan.org>
>>> Acked-by: Petri Latvala <petri.latv...@intel.com>
>>> Cc: Alex Deucher <alexander.deuc...@amd.com>
>>> Cc: Arkadiusz Hiler <arkadiusz.hi...@intel.com>
>>> Cc: Ben Widawsky <b...@bwidawsk.net>
>>> Cc: Daniel Stone <dan...@fooishbar.org>
>>> Cc: Dave Airlie <airl...@gmail.com>
>>> Cc: Eric Anholt <e...@anholt.net>
>>> Cc: Gustavo Padovan <gust...@padovan.org>
>>> Cc: Jani Nikula <jani.nik...@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
>>> Cc: Keith Packard <kei...@keithp.com>
>>> Cc: Kenneth Graunke <kenn...@whitecape.org>
>>> Cc: Kristian H. Kristensen <hoegsb...@google.com>
>>> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
>>> Cc: Petri Latvala <petri.latv...@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
>>> Cc: Sean Paul <seanp...@chromium.org>
>>> Reviewed-by: Keith Packard <kei...@keithp.com>
>>> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
>>> ---
>>> If you wonder about the wide distribution list for an igt patch: I'd
>>> like to start a discussions about x.org community norms around commit
>>> rights at large, at least for all the shared repos. I plan to propose
>>> the same text for drm-misc and libdrm too, and hopefully others like
>>> mesa/xserver/wayland would follow.
>>>
>> I think the idea is pretty good, simply highlighting some bits.
>>
>> What you've outlined in this patch has been in practise for many years:
>>  a) undocumented, applicable to most xorg projects [1]
>>  b) documented, mesa
>
> Hm, I chatted with a few mesa devs about this, and I wasn't aware
> there's explicit documentation for mesa. Where is it? I'd very much
> want to align as much as we can.
>
See the "Developer git Access" section in [1]. FWIW I prefer the
wording used in this patch and the CoC reference is a big plus.

HTH
Emil

[1] https://www.mesa3d.org/repository.html
___
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

Re: [Mesa-dev] [PATCH i-g-t] [RFC] CONTRIBUTING: commit rights docs

[Emil Velikov]

On 13 April 2018 at 11:00, Daniel Vetter  wrote:
> This tries to align with the X.org communities's long-standing
> tradition of trying to be an inclusive community and handing out
> commit rights fairly freely.
>
> We also tend to not revoke commit rights for people no longer
> regularly active in a given project, as long as they're still part of
> the larger community.
>
> Finally make sure that commit rights, like anything happening on fd.o
> infrastructre, is subject to the fd.o's Code of Conduct.
>
> v2: Point at MAINTAINERS for contact info (Daniel S.)
>
> v3:
> - Make it clear that commit rights are voluntary and that committers
>   need to acknowledge positively when they're nominated by someone
>   else (Keith).
> - Encourage committers to drop their commit rights when they're no
>   longer active, and make it clear they'll get readded (Keith).
> - Add a line that maintainers and committers should actively nominate
>   new committers (me).
>
> v4: Typo (Petri).
>
> v5: Typo (Sean).
>
> v6: Wording clarifications and spelling (Jani).
>
> v7: Require an explicit commitment to the documented merge criteria
> and rules, instead of just the implied one through the Code of Conduct
> threat (Jani).
>
> Acked-by: Alex Deucher 
> Acked-by: Arkadiusz Hiler 
> Acked-by: Daniel Stone 
> Acked-by: Eric Anholt 
> Acked-by: Gustavo Padovan 
> Acked-by: Petri Latvala 
> Cc: Alex Deucher 
> Cc: Arkadiusz Hiler 
> Cc: Ben Widawsky 
> Cc: Daniel Stone 
> Cc: Dave Airlie 
> Cc: Eric Anholt 
> Cc: Gustavo Padovan 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Keith Packard 
> Cc: Kenneth Graunke 
> Cc: Kristian H. Kristensen 
> Cc: Maarten Lankhorst 
> Cc: Petri Latvala 
> Cc: Rodrigo Vivi 
> Cc: Sean Paul 
> Reviewed-by: Keith Packard 
> Signed-off-by: Daniel Vetter 
> ---
> If you wonder about the wide distribution list for an igt patch: I'd
> like to start a discussions about x.org community norms around commit
> rights at large, at least for all the shared repos. I plan to propose
> the same text for drm-misc and libdrm too, and hopefully others like
> mesa/xserver/wayland would follow.
>
I think the idea is pretty good, simply highlighting some bits.

What you've outlined in this patch has been in practise for many years:
 a) undocumented, applicable to most xorg projects [1]
 b) documented, mesa

IMHO having this explicitly documented and others
(wayland/weston/wayland-protocols and xserver repos) following suite
is a big plus.

HTH
Emil

[1] As in all of https://cgit.freedesktop.org/xorg but xserver
___
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

[PATCH xserver] configure.ac: make use of wayland-scanner.pc

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Replace the current (incorrect) assumption that wayland-scanner is
located in the wayland-client prefix. Make use of the wayland_scanner
variable in wayland-scanner.pc

It was introduced back in 2013 and we already require newer wayland bits

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
More or less the same patch has been part of weston and mesa for a few
years now.
---
 configure.ac | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 828d15e95..a851cc369 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2387,9 +2387,11 @@ if test "x$XWAYLAND" = xyes; then
AC_MSG_ERROR([Xwayland requires CLOCK_MONOTONIC support.])
fi
 
-   WAYLAND_PREFIX=`$PKG_CONFIG --variable=prefix wayland-client`
-   AC_PATH_PROG([WAYLAND_SCANNER], [wayland-scanner],,
-[${WAYLAND_PREFIX}/bin$PATH_SEPARATOR$PATH])
+   AC_PATH_PROG([WAYLAND_SCANNER], [wayland-scanner])
+   if test "x$WAYLAND_SCANNER" = x; then
+   PKG_CHECK_MODULES(WAYLAND_SCANNER, [wayland-scanner])
+   AC_SUBST(WAYLAND_SCANNER, `$PKG_CONFIG 
--variable=wayland_scanner wayland-scanner`)
+   fi
 
AC_SUBST(WAYLAND_PROTOCOLS_DATADIR, `$PKG_CONFIG --variable=pkgdatadir 
wayland-protocols`)
 fi
-- 
2.16.0

___
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

Re: [PATCH xserver 15/15] dri3: correctly handle failed get_supported_modifiers requests

[Emil Velikov]

On 6 April 2018 at 12:26, Daniel Stone <dan...@fooishbar.org> wrote:
> Hi,
>
> On 6 April 2018 at 12:18, Emil Velikov <emil.l.veli...@gmail.com> wrote:
>> On 4 April 2018 at 19:51, Adam Jackson <a...@nwnk.net> wrote:
>>> This, combined with 10/15, means you will now throw BadImplementation
>>> when we used to succeed and simply report no modifiers. I can't think
>>> of a good reason to make that an error, it just makes the client's life
>>> harder if it has to distinguish error from reply.
>>
>> I'm a bit split - on one hand any reasonable client should do error checking.
>> At least the Mesa one seems to do so.
>>
>> On the other, the client would handle failed and 0 modifiers identically.
>
> We generally reserve protocol errors for 'client screwed up' (most
> errors), or 'server has tied itself into a knot and does not know how
> to escape' (BadImplementation). The latter is usually accompanied by a
> comment saying '/* XXX: This is really hard so I didn't */'. In both
> cases, there's no way to do what the client wants so we send an error
> down to let it know that its expectations have been broken and it's
> difficult to recover.
>
> On the other hand, zero is a completely valid answer to 'how many
> modifiers do you support?', so that's what we should return. Error
> handling from asynchronous protocols is far more difficult than from
> just making a C function call, hence why we reserve it for cases which
> require active recovery, or are unrecoverable.
>
I see. Thank you very much for the comprehensive answer Dan.

-Emil
___
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

Re: [PATCH xserver 15/15] dri3: correctly handle failed get_supported_modifiers requests

[Emil Velikov]

On 4 April 2018 at 19:51, Adam Jackson <a...@nwnk.net> wrote:
> On Mon, 2018-04-02 at 16:41 +0100, Emil Velikov wrote:
>> From: Emil Velikov <emil.veli...@collabora.com>
>>
>> Currently depending on the code path hit, the helper will set some of
>> the output values and not others.
>>
>> It could also leak memory ;-)
>>
>> At the same time the caller was:
>>  - working around the broken behaviour - by initialising the variables
>>  - outright ignoring if the helper fails
>>
>> Fix all that by consistently setting the output variables and returning
>> a protocol error if we fail to get the data required.
>>
>> Bonus points: current code does not attribute if we cannot get the
>> modifiers info for certain format. A smaller format array is available,
>> yet the original length is stored.
>>
>> Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
>> Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
>> Cc: Daniel Stone <dani...@collabora.com>
>> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
>> ---
>> We could still return "success" to the user - I've opted for this
>> solution since we already bail on dixLookupWindow failure.
>
> This, combined with 10/15, means you will now throw BadImplementation
> when we used to succeed and simply report no modifiers. I can't think
> of a good reason to make that an error, it just makes the client's life
> harder if it has to distinguish error from reply.
>
I'm a bit split - on one hand any reasonable client should do error checking.
At least the Mesa one seems to do so.

On the other, the client would handle failed and 0 modifiers identically.

I'm fine either way. As soon as we get any other confirmation/comment
on this (or the rest of the series), I'll respin the lot.
There's a few conflicts with Dan's work.

Thanks
Emil
___
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

Re: Bug 105851 Xserver 1.20 RC2+ issues with Kwin + Present 1.2

[Emil Velikov]

On 5 April 2018 at 19:06, Mike Lothian  wrote:
> Nope still not working
>
> Intel DDX didn't launch, Modesetting did however compositing was disabled
> due to the previous failure. Upon reenabling the screen went blank, however
> things were still running, when I went to the top left corner I could see
> the window titles, then the outline of the kicker menu too
>
FWIW I'm perfectly fine with reverting my patch, although we should
really get to the bottom of it all.
It indicates something fundamentally broken elsewhere.

-Emil
___
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

Re: [PATCH xserver 0/8] GCC8 warning fixes

[Emil Velikov]

On 5 April 2018 at 18:13, Adam Jackson <a...@redhat.com> wrote:
> gcc 8's buffer size logic got a bit pickier, in mostly reasonable ways,
> but -Wmaybe-uninitialized got a lot more speculative. Regardless I'm
> tired of seeing warnings in my build logs.
>
There are some trivial suggestions but the most notable one is the
foobar snprintf statement in 2/8

With the last one tweaked (regardless of the remaining nitpicks)
patches 1-3 and 5-8 are
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

Note 4/8 needs some rework.

HTH
Emil
___
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

Re: [PATCH xserver 6/8] xkb: Silence some compiler warnings

[Emil Velikov]

On 5 April 2018 at 18:13, Adam Jackson  wrote:
> Of the form:
>
> ../xkb/XKBGAlloc.c: In function ‘SrvXkbAddGeomKeyAlias’:
> ../xkb/XKBGAlloc.c:591:13: warning: ‘strncpy’ specified bound 4 equals 
> destination size [-Wstringop-truncation]
>  strncpy(alias->real, realStr, XkbKeyNameLength);
>  ^~~
>
> This is intentional; the code that reads from these fields never reads
> more than 4 bytes anyway. Rephrase things in terms of memcpy so that's
> clear. Obviously this is awful but in XKB awful is par.
>
> Signed-off-by: Adam Jackson 
> ---
>  xkb/XKBGAlloc.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/xkb/XKBGAlloc.c b/xkb/XKBGAlloc.c
> index e9f55fa434..8958b0c523 100644
> --- a/xkb/XKBGAlloc.c
> +++ b/xkb/XKBGAlloc.c
> @@ -588,7 +588,8 @@ XkbAddGeomKeyAlias(XkbGeometryPtr geom, char *aliasStr, 
> char *realStr)
>   i++, alias++) {
>  if (strncmp(alias->alias, aliasStr, XkbKeyNameLength) == 0) {
>  memset(alias->real, 0, XkbKeyNameLength);
> -strncpy(alias->real, realStr, XkbKeyNameLength);
> +memcpy(alias->real, realStr,
> +   min(XkbKeyNameLength, strlen(realStr)));

min(XkbKeyNameLength-1 ...
To make it clear that the resulting string is NULL terminated (props
to the memset above).

>  return alias;
>  }
>  }
> @@ -598,8 +599,8 @@ XkbAddGeomKeyAlias(XkbGeometryPtr geom, char *aliasStr, 
> char *realStr)
>  }
>  alias = >key_aliases[geom->num_key_aliases];
>  memset(alias, 0, sizeof(XkbKeyAliasRec));
> -strncpy(alias->alias, aliasStr, XkbKeyNameLength);
> -strncpy(alias->real, realStr, XkbKeyNameLength);
> +memcpy(alias->alias, aliasStr, min(XkbKeyNameLength, strlen(aliasStr)));
> +memcpy(alias->real, realStr, min(XkbKeyNameLength, strlen(realStr)));
Ditto - min(XkbKeyNameLength-1 ...

>  geom->num_key_aliases++;
>  return alias;
>  }
> @@ -814,8 +815,8 @@ XkbAddGeomOverlayKey(XkbOverlayPtr overlay,
>  (_XkbAllocOverlayKeys(row, 1) != Success))
>  return NULL;
>  key = >keys[row->num_keys];
> -strncpy(key->under.name, under, XkbKeyNameLength);
> -strncpy(key->over.name, over, XkbKeyNameLength);
> +memcpy(key->under.name, under, min(XkbKeyNameLength, strlen(under)));
> +memcpy(key->over.name, over, min(XkbKeyNameLength, strlen(over)));

AFAICT _XkbAllocOverlayKeys does memset(...0...), yet that may not
obvious to the reader or compiler.
It's worth staying consistent with the above memset + min(...-1...)?

-Emil
___
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

Re: [PATCH xserver 5/8] dmx: Silence a string truncation warning.

[Emil Velikov]

On 5 April 2018 at 18:13, Adam Jackson  wrote:
> ../hw/dmx/config/dmxparse.c: In function ‘dmxConfigCreateOption’:
> ../hw/dmx/config/dmxparse.c:385:13: warning: ‘strncpy’ output truncated 
> before terminating nul copying as many bytes from a string as its length 
> [-Wstringop-truncation]
>  strncpy(option->string + offset, p->string, len);
>  ^~~~
> ../hw/dmx/config/dmxparse.c:383:23: note: length computed here
>  int len = strlen(p->string);
>^
>
> The thing it's warning about is intentional, the surrounding code does
> its own nul-termination. Make that obvious by using memcpy instead.
>
> Signed-off-by: Adam Jackson 
> ---
>  hw/dmx/config/dmxparse.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/dmx/config/dmxparse.c b/hw/dmx/config/dmxparse.c
> index cf510844d6..f66143a6a5 100644
> --- a/hw/dmx/config/dmxparse.c
> +++ b/hw/dmx/config/dmxparse.c
> @@ -382,7 +382,7 @@ dmxConfigCreateOption(DMXConfigTokenPtr pStart,
>  if (p->string) {
>  int len = strlen(p->string);
>
> -strncpy(option->string + offset, p->string, len);
> +memcpy(option->string + offset, p->string, len);
Speaking of surrounding code - worth using option->string[offset] like
the rest of the function?

-Emil
___
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

Re: [PATCH xserver 4/8] dmx: Clean up some argument parsing code

[Emil Velikov]

On 5 April 2018 at 18:13, Adam Jackson  wrote:
> This threw:
>
> ../hw/dmx/input/dmxarg.c: In function ‘dmxArgParse’:
> ../hw/dmx/input/dmxarg.c:128:5: warning: ‘strncpy’ specified bound depends on 
> the length of the source argument [-Wstringop-overflow=]
>  strncpy(tmp, string, len);
>  ^
> ../hw/dmx/input/dmxarg.c:126:11: note: length computed here
>  len = strlen(string) + 2;
>^~
>
> This code predates xstrtokenize, but that's no excuse.
>
> Signed-off-by: Adam Jackson 
> ---
>  hw/dmx/input/dmxarg.c | 23 +--
>  1 file changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/hw/dmx/input/dmxarg.c b/hw/dmx/input/dmxarg.c
> index 6c21ae959a..582ed3faa6 100644
> --- a/hw/dmx/input/dmxarg.c
> +++ b/hw/dmx/input/dmxarg.c
> @@ -114,30 +114,17 @@ dmxArgC(dmxArg a)
>  dmxArg
>  dmxArgParse(const char *string)
>  {
> -char *tmp;
> -char *start, *pt;
> +int i = 0;
>  dmxArg a = dmxArgCreate();
This allocates 2 pointers in a->argv

> -int done;
> -int len;
>
>  if (!string)
>  return a;
>
> -len = strlen(string) + 2;
> -tmp = malloc(len);
> -strncpy(tmp, string, len);
> +a->argv = (const char **)xstrtokenize(string, ",");
... and here we leak them.

I'd just open-code dmxArgCreate, above and remove that function
alongside the no longer used dmxArgAdd.

-Emil
___
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

Re: [PATCH xserver 3/8] dmx: Fix a read-from-uninitialized warning

[Emil Velikov]

On 5 April 2018 at 18:13, Adam Jackson  wrote:
> ../hw/dmx/dmxpixmap.c: In function ‘dmxBitmapToRegion’:
> ../include/regionstr.h:174:22: warning: ‘Box.x1’ may be used uninitialized in 
> this function [-Wmaybe-uninitialized]
>  (_pReg)->extents = *(_pBox);
>  ~^~
> ../hw/dmx/dmxpixmap.c:208:12: note: ‘Box.x1’ was declared here
>  BoxRec Box;
> ^~~
>
> Signed-off-by: Adam Jackson 
> ---
>  hw/dmx/dmxpixmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/dmx/dmxpixmap.c b/hw/dmx/dmxpixmap.c
> index 17aca9224b..7b317eaef1 100644
> --- a/hw/dmx/dmxpixmap.c
> +++ b/hw/dmx/dmxpixmap.c
> @@ -205,7 +205,7 @@ dmxBitmapToRegion(PixmapPtr pPixmap)
>  RegionPtr pReg, pTmpReg;
>  int x, y;
>  unsigned long previousPixel, currentPixel;
> -BoxRec Box;
> +BoxRec Box = { 0, };
As-is it will warn on some gcc/clang versions.
I'd throw in a few more zeroes - 0, 0, 0, 0.

At a later stage, one might as well fold this and the xnest copy into
a helper somewhere ;-)

-Emil
___
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

Re: [PATCH xserver 2/8] dmx: Fix some snprintf warnings.

[Emil Velikov]

On 6 April 2018 at 09:44, Peter Hutterer  wrote:
> On Thu, Apr 05, 2018 at 01:13:55PM -0400, Adam Jackson wrote:
>> snprintf doesn't terminate the string if it truncates, so things like
>> this are lurking crashers:
>
> it doesn't? which platforms is that on? Apparently windows, from a quick
> google but that's about it, right?
>
Indeed - quick search points to the following
https://stackoverflow.com/a/13067917

Dummy test with gcc 7.2.1 -Wall -Wextra -pedantic -std=c89/99/11 (yes,
c89 is missing the API) shows proper NULL terminated strings.

-Emil
___
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

Re: [PATCH xserver] xwayland: Silence a build warning if we can

[Emil Velikov]

On 6 April 2018 at 08:07, Pekka Paalanen <ppaala...@gmail.com> wrote:
> On Thu,  5 Apr 2018 13:35:11 -0400
> Adam Jackson <a...@redhat.com> wrote:
>
>> [735/786] Generating 
>> 'hw/xwayland/Xwayland@exe/relative-pointer-unstable-v1-protocol.c'.
>> Using "code" is deprecated - use private-code or public-code.
>> See the help page for details.
>>
>> Use private-code if wayland-scanner is new enough.
>>
>> Signed-off-by: Adam Jackson <a...@redhat.com>
>> ---
>>  configure.ac|  4 
>>  hw/xwayland/Makefile.am | 14 +++---
>>  hw/xwayland/meson.build |  9 -
>>  3 files changed, 19 insertions(+), 8 deletions(-)
>>
>
> Hi,
>
> I only read through the patch, and it looks quite ok to me, so:
> Reviewed-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
>
And another one from me
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

> But I still have a couple of questions below.
>
>> diff --git a/configure.ac b/configure.ac
>> index 290721891d..d8d88e2b15 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -2391,6 +2391,10 @@ if test "x$XWAYLAND" = xyes; then
>>   AC_PATH_PROG([WAYLAND_SCANNER], [wayland-scanner],,
>>[${WAYLAND_PREFIX}/bin$PATH_SEPARATOR$PATH])
>>
>> +PKG_CHECK_MODULES(WAYLAND_SCANNER, [wayland-scanner >= 1.14.91],
>> +  AC_SUBST(SCANNER_ARG, 'private-code'),
>> +  AC_SUBST(SCANNER_ARG, 'code'))
>
> Are you happy with the whitespace (tabs vs. spaces) here?
>
IIRC there's no hard rule.

> Thinking of cross-compiling, is this ok or should it be more paranoid
> about mismatching 'wayland-scanner' and 'wayland-scanner.pc'?
>
> Personally I would claim that version mismatch there would be user
> error, so this should be fine.
>
Since the Xserver detection does something very unusual* to begin
with, I wouldn't worry about it.

-Emil

The unusual bit:
 - fetches the wayland-client prefix
 - uses ^^ to search for wayland-scanner (via AC_PATH_PROG)

Instead one should really:
 - do plain wayland-scanner + AC_PATH_PROG
 - (as a fallback?) wayland-scanner + PKG_CHECK_MODULES +
--variable=wayland_scanner
___
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

Re: Bug 105851 Xserver 1.20 RC2+ issues with Kwin + Present 1.2

[Emil Velikov]

Hi Mike,

On 4 April 2018 at 09:12, Mike Lothian <m...@fireburn.co.uk> wrote:
> Hi
>
> Kwin doesn't seem to start with the following commit:
>
> commit abb9b58d1af9a0286162e52ef9db390d0c950fc1
> Author: Thierry Reding <tred...@nvidia.com>
> Date:   Fri Mar 16 14:24:21 2018 +0100
>
> present: Advertise protocol version 1.2
>
> Everything is implemented to support protocol version 1.2. Make it
> official.
>
> Reviewed-by: Daniel Stone <dani...@collabora.com>
> Signed-off-by: Thierry Reding <tred...@nvidia.com>
> Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
>
> Though I think the issue may lie in commit:
>
> commit 6a5d51e0823b43280e3646b7a0c919a3b76146ea
> Author: Emil Velikov <emil.veli...@collabora.com>
> Date:   Mon Mar 19 16:04:43 2018 +
>
> present: cap the version returned to the client
>
> As per the protocol, the server should not return version greater than
> the one supported by the client.
>
> Add a spec quote and tweak the numbers accordingly.
>
> I realise that Kwin will probably need to be modified to work with
> Present 1.2, but I don't think we should be seeing breakage unti that
> happens
>
AFAICT Kwin does not directly use the present extension, does it?

Can you ensure that you have mesa commit
696762eef57e83b4027acbdf0a6e74d1f75083b0.
It addresses some hunky stuff with the version handling.

-Emil
___
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

Re: [PATCH xserver 01/15] dri3: annotate the dri3_screen_info data as const

[Emil Velikov]

On 2 April 2018 at 20:34, Adam Jackson <a...@redhat.com> wrote:
> On Mon, 2018-04-02 at 16:41 +0100, Emil Velikov wrote:
>
>> Why do we have the explicit _rec and _ptr typecasts to begin with?
>
> Convention, mostly. The typedef for the struct is because 'struct' is a
> dumb word to need to say all the time. The typedef for the pointer is
> vaguely distasteful and I've been trying to stop.
>
s/dumb/annoying/

Using _t suffix over _rec seems more indicative IMHO.
Regardless, most of the series is a must have for 1.20 - if you want
to give it a skim that'll be appreciated.

Thanks
Emil
___
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

[PATCH xserver 2/2] docs: remove resource management references

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The code referenced was removed back in 2009.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 hw/xfree86/doc/ddxDesign.xml | 168 ---
 1 file changed, 168 deletions(-)

diff --git a/hw/xfree86/doc/ddxDesign.xml b/hw/xfree86/doc/ddxDesign.xml
index becb0c2c5..367844136 100644
--- a/hw/xfree86/doc/ddxDesign.xml
+++ b/hw/xfree86/doc/ddxDesign.xml
@@ -1107,16 +1107,6 @@ Here is what InitOutput() does:
   them.

 
-   
-  All additional resources that the screen needs must be registered
-  here.  This should be done with
-  xf86RegisterResources().  If some of the fixed
-  resources registered in the Probe phase are not needed or not
-  decoded by the hardware when in the OPERATING server state, their
-  status should be updated with
-  xf86SetOperatingState().
-   
-

   Modules may be loaded at any point in this function, and all
   modules that the driver will need must be loaded before the end
@@ -2599,20 +2589,6 @@ The
 These two helper functions make use of several core functions that are
 available at the driver level:
 
- 
- 
-void xf86ClaimFixedResources(resList list, int entityIndex);
- 
- 
-  This function registers the non-relocatable resources which cannot
-  be disabled and which therefore would cause the server to fail
-  immediately if they were found to conflict.  It also records
-  non-relocatable but sharable resources for processing after the
-  Probe() phase.
-   
-
- 
-
  
  
 void xf86AddEntityToScreen(ScrnInfoPtr pScrn, int entityIndex);
@@ -2692,52 +2668,6 @@ Several functions are provided to simplify resource 
registration:
  

 
-   
-The primary function for registration of resources is:
- 
- 
-resPtr xf86RegisterResources(int entityIndex, resList list,
- int access);
- 
- 
-  This function tries to register the resources in
-  list.  If list is NULL it 
tries
-  to determine the resources automatically.  This only works for
-  entities that provide a generic way to read out the resource ranges
-  they decode.  So far this is only the case for PCI devices.  By
-  default the PCI resources are registered as shared
-  (ResShared) if the driver wants to set a different
-  access type it can do so by specifying the access flags in the
-  third argument.  A value of 0 means to use the
-  default settings.  If for any reason the resource broker is not
-  able to register some of the requested resources the function will
-  return a pointer to a list of the failed ones.  In this case the
-  driver may be able to move the resource to different locations.
-  In case of PCI bus entities this is done by passing the list of
-  failed resources to xf86ReallocatePciResources().
-  When the registration succeeds, the return value is
-  NULL.
-   
-
- 
- 
-
-   
-   
-resPtr xf86ReallocatePciResources(int entityIndex, resPtr pRes);
-   
-   
-  This function takes a list of PCI resources that need to be
-  reallocated and returns NULL when all relocations 
are
-  successful.
-  xf86RegisterResources() should be called again to
-  register the relocated resources with the broker.
-  If the reallocation fails, a list of the resources that could not be
-  relocated is returned.
- 
-
-   
-
 
 Two functions are provided to obtain a resource range of a given type:
  
@@ -2795,93 +2725,6 @@ Two functions are provided to obtain a resource range of 
a given type:
  

 
-   
-The driver may replace the generic access control functions for an entity.
-This is done with the xf86SetAccessFuncs():
- 
- 
-void xf86SetAccessFuncs(EntityInfoPtr pEnt,
-xf86SetAccessFuncPtr funcs,
-xf86SetAccessFuncPtr oldFuncs);
- 
- with:
- 
-  typedef struct {
-  xf86AccessPtr mem;
-  xf86AccessPtr io;
-  xf86AccessPtr io_mem;
-  } xf86SetAccessFuncRec, *xf86SetAccessFuncPtr;
- 
- 
-  The driver can pass three functions: one for I/O access, one for
-  memory access and one for combined memory and I/O access.  If the
-  memory access and combined access functions are identical the
-  common level assumes that the memory access cannot be controlled
-  independently of I/O access, if the I/O access function and the
-  combined access functions are the same it is assumed that I/O can
-  not be controlled independently.  If memory 

[PATCH xserver 1/2] docs: purge some Isa references

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The respective Isa functions were dropped back in 2008

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 hw/xfree86/doc/ddxDesign.xml | 89 +---
 1 file changed, 2 insertions(+), 87 deletions(-)

diff --git a/hw/xfree86/doc/ddxDesign.xml b/hw/xfree86/doc/ddxDesign.xml
index a7f74e4d7..becb0c2c5 100644
--- a/hw/xfree86/doc/ddxDesign.xml
+++ b/hw/xfree86/doc/ddxDesign.xml
@@ -820,13 +820,11 @@ Here is what InitOutput() does:
   like probing for other details such as the amount of memory
   installed, etc.  It is recommended that the
   xf86MatchPciInstances() helper function be used
-  for identifying matching PCI devices, and similarly the
-  xf86MatchIsaInstances() for ISA (non-PCI) devices
+  for identifying matching PCI devices
   (see the RAC section).  These helpers also
   checks and claims the appropriate entity.  When not using the
   helper, that should be done with xf86CheckPciSlot()
-  and xf86ClaimPciSlot() for PCI devices and
-  xf86ClaimIsaSlot() for ISA devices (see the
+  and xf86ClaimPciSlot() for PCI devices (see the
   RAC section).

 
@@ -2491,53 +2489,6 @@ xorg.conf file to the devices:
   return value of -1 indicates an internal error.
   The returned foundEntities array should be freed
   by the driver with xfree() when it is no longer
-  needed in cases where the return value is greater than zero.
- 
-
-   
-
-   
-   
-int xf86MatchIsaInstances(const char *driverName,
-  SymTabPtr chipsets, IsaChipsets *ISAchipsets,
-  DriverPtr drvp, FindIsaDevProc FindIsaDevice,
-  GDevPtr *devList, int numDevs,
-  int **foundEntities);
-   
-   
-  This function finds matches between ISA cards that a driver supports
-  and config file device sections.  It is intended for use in the
-  ChipProbe() function of drivers for ISA cards.
-  devList and numDevs are
-  typically those found from calling 
xf86MatchDevice(),
-  and represent the active config file device sections relevant to
-  the driver.  ISAchipsets is a table that provides
-  a mapping between the driver's internal chipset tokens and the
-  resource classes.  FindIsaDevice is a
-  driver-provided function that probes the hardware and returns the
-  chipset token corresponding to what was detected, and
-  -1 if nothing was detected.
- 
-
- 
-  If the config file device section contains a chipset entry, then
-  it is checked against the chipsets list.  When
-  no chipset entry is present, the FindIsaDevice
-  function is called instead.
- 
-
- 
-  Entity index numbers for confirmed matches are returned as an
-  array via foundEntities.  The chipset token and
-  device section for each match are found in the
-  EntityInfoRec referenced by the indices.
- 
-
- 
-  The function return value is the number of confirmed matches.  A
-  return value of -1 indicates an internal error.
-  The returned foundEntities array should be freed
-  by the driver with xfree() when it is no longer
   needed in cases where the return value is greater than zero.
  
 
@@ -2581,18 +2532,6 @@ available at the driver level:
 

 
-   
-   
-Bool xf86ParseIsaBusString(const char *busID);
-   
-   
-  Compares a BusID string with the ISA bus ID string
-  ("ISA" or "ISA:").  If they match TRUE is returned,
-  and FALSE if they don't.
- 
-
-   
-


 Bool xf86CheckPciSlot(int bus, int device, int func);
@@ -2632,30 +2571,6 @@ available at the driver level:
 

 
-   
-   
-int xf86ClaimIsaSlot(DriverPtr drvp, int chipset,
- GDevPtr dev, Bool active);
-   
-   
-  This allocates an entity record entity and initialise the data
-  structures.  The return value is the index of the newly allocated
-  entity record.
- 
-
-   
-
-   
-   
-Bool xf86IsPrimaryIsa(void);
-   
-   
-  This function returns TRUE if the primary card is
-  an ISA (non-PCI) device, and FALSE otherwise.
- 
-
-   
-

 Two helper functions are provided to aid configuring entities:

-- 
2.16.0

___
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

[PATCH xserver 14/15] dri3: s/intersect/window/ to match the names used in caller

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 dri3/dri3_screen.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/dri3/dri3_screen.c b/dri3/dri3_screen.c
index f5ca07934..2b0e139fa 100644
--- a/dri3/dri3_screen.c
+++ b/dri3/dri3_screen.c
@@ -167,8 +167,8 @@ cache_formats_and_modifiers(ScreenPtr screen)
 int
 dri3_get_supported_modifiers(ScreenPtr screen, DrawablePtr drawable,
  CARD8 depth, CARD8 bpp,
- CARD32 *num_intersect_modifiers,
- CARD64 **intersect_modifiers,
+ CARD32 *num_window_modifiers,
+ CARD64 **window_modifiers,
  CARD32 *num_screen_modifiers,
  CARD64 **screen_modifiers)
 {
@@ -178,7 +178,7 @@ dri3_get_supported_modifiers(ScreenPtr screen, DrawablePtr 
drawable,
 int ret;
 CARD32  num_drawable_mods;
 CARD64 *drawable_mods;
-CARD64 *intersect_mods = NULL;
+CARD64 *window_mods = NULL;
 CARD64 *screen_mods = NULL;
 CARD32  format;
 dri3_dmabuf_format_ptr  screen_format = NULL;
@@ -204,7 +204,7 @@ dri3_get_supported_modifiers(ScreenPtr screen, DrawablePtr 
drawable,
 
 if (screen_format->num_modifiers == 0) {
 *num_screen_modifiers = 0;
-*num_intersect_modifiers = 0;
+*num_window_modifiers = 0;
 return Success;
 }
 
@@ -217,44 +217,44 @@ dri3_get_supported_modifiers(ScreenPtr screen, 
DrawablePtr drawable,
 }
 
 /* We're allocating slightly more memory than necessary but it reduces
- * the complexity of finding the intersection set.
+ * the complexity of finding the windowion set.
  */
 screen_mods = malloc(screen_format->num_modifiers * sizeof(CARD64));
 if (!screen_mods)
 return BadAlloc;
 if (num_drawable_mods > 0) {
-intersect_mods = malloc(screen_format->num_modifiers * sizeof(CARD64));
-if (!intersect_mods) {
+window_mods = malloc(screen_format->num_modifiers * sizeof(CARD64));
+if (!window_mods) {
 free(screen_mods);
 return BadAlloc;
 }
 }
 
 *num_screen_modifiers = 0;
-*num_intersect_modifiers = 0;
+*num_window_modifiers = 0;
 for (i = 0; i < screen_format->num_modifiers; i++) {
 CARD64 modifier = screen_format->modifiers[i];
-Bool intersect = FALSE;
+Bool window_mod = FALSE;
 
 for (j = 0; j < num_drawable_mods; j++) {
 if (drawable_mods[j] == modifier) {
-intersect = TRUE;
+window_mod = TRUE;
 break;
 }
 }
 
-if (intersect) {
-intersect_mods[*num_intersect_modifiers] = modifier;
-*num_intersect_modifiers += 1;
+if (window_mod) {
+window_mods[*num_window_modifiers] = modifier;
+*num_window_modifiers += 1;
 } else {
 screen_mods[*num_screen_modifiers] = modifier;
 *num_screen_modifiers += 1;
 }
 }
 
-assert(*num_intersect_modifiers + *num_screen_modifiers == 
screen_format->num_modifiers);
+assert(*num_window_modifiers + *num_screen_modifiers == 
screen_format->num_modifiers);
 
-*intersect_modifiers = intersect_mods;
+*window_modifiers = window_mods;
 *screen_modifiers = screen_mods;
 free(drawable_mods);
 
-- 
2.16.0

___
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

[PATCH xserver 15/15] dri3: correctly handle failed get_supported_modifiers requests

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Currently depending on the code path hit, the helper will set some of
the output values and not others.

It could also leak memory ;-)

At the same time the caller was:
 - working around the broken behaviour - by initialising the variables
 - outright ignoring if the helper fails

Fix all that by consistently setting the output variables and returning
a protocol error if we fail to get the data required.

Bonus points: current code does not attribute if we cannot get the
modifiers info for certain format. A smaller format array is available,
yet the original length is stored.

Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
We could still return "success" to the user - I've opted for this
solution since we already bail on dixLookupWindow failure.
---
 dri3/dri3_request.c | 18 ++
 dri3/dri3_screen.c  | 38 +-
 2 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/dri3/dri3_request.c b/dri3/dri3_request.c
index edcd0e782..9b7d81ea3 100644
--- a/dri3/dri3_request.c
+++ b/dri3/dri3_request.c
@@ -335,10 +335,10 @@ proc_dri3_get_supported_modifiers(ClientPtr client)
 };
 WindowPtr window;
 ScreenPtr pScreen;
-CARD64 *window_modifiers = NULL;
-CARD64 *screen_modifiers = NULL;
-CARD32 nwindowmodifiers = 0;
-CARD32 nscreenmodifiers = 0;
+CARD64 *window_modifiers;
+CARD64 *screen_modifiers;
+CARD32 nwindowmodifiers;
+CARD32 nscreenmodifiers;
 int status;
 int i;
 
@@ -349,10 +349,12 @@ proc_dri3_get_supported_modifiers(ClientPtr client)
 return status;
 pScreen = window->drawable.pScreen;
 
-dri3_get_supported_modifiers(pScreen, >drawable,
-stuff->depth, stuff->bpp,
- , _modifiers,
- , _modifiers);
+status = dri3_get_supported_modifiers(pScreen, >drawable,
+  stuff->depth, stuff->bpp,
+  , _modifiers,
+  , 
_modifiers);
+if (status != Success)
+return status;
 
 rep.numWindowModifiers = nwindowmodifiers;
 rep.numScreenModifiers = nscreenmodifiers;
diff --git a/dri3/dri3_screen.c b/dri3/dri3_screen.c
index 2b0e139fa..de97d55bc 100644
--- a/dri3/dri3_screen.c
+++ b/dri3/dri3_screen.c
@@ -178,7 +178,9 @@ dri3_get_supported_modifiers(ScreenPtr screen, DrawablePtr 
drawable,
 int ret;
 CARD32  num_drawable_mods;
 CARD64 *drawable_mods;
+CARD32  num_window_mods = 0;
 CARD64 *window_mods = NULL;
+CARD32  num_screen_mods = 0;
 CARD64 *screen_mods = NULL;
 CARD32  format;
 dri3_dmabuf_format_ptr  screen_format = NULL;
@@ -202,11 +204,8 @@ dri3_get_supported_modifiers(ScreenPtr screen, DrawablePtr 
drawable,
 if (screen_format == NULL)
 return BadMatch;
 
-if (screen_format->num_modifiers == 0) {
-*num_screen_modifiers = 0;
-*num_window_modifiers = 0;
-return Success;
-}
+if (screen_format->num_modifiers == 0)
+goto done;
 
 if (!info->get_drawable_modifiers ||
 !info->get_drawable_modifiers(drawable, format,
@@ -221,17 +220,13 @@ dri3_get_supported_modifiers(ScreenPtr screen, 
DrawablePtr drawable,
  */
 screen_mods = malloc(screen_format->num_modifiers * sizeof(CARD64));
 if (!screen_mods)
-return BadAlloc;
+goto bad_alloc;
 if (num_drawable_mods > 0) {
 window_mods = malloc(screen_format->num_modifiers * sizeof(CARD64));
-if (!window_mods) {
-free(screen_mods);
-return BadAlloc;
-}
+if (!window_mods)
+goto bad_alloc;
 }
 
-*num_screen_modifiers = 0;
-*num_window_modifiers = 0;
 for (i = 0; i < screen_format->num_modifiers; i++) {
 CARD64 modifier = screen_format->modifiers[i];
 Bool window_mod = FALSE;
@@ -245,18 +240,27 @@ dri3_get_supported_modifiers(ScreenPtr screen, 
DrawablePtr drawable,
 
 if (window_mod) {
 window_mods[*num_window_modifiers] = modifier;
-*num_window_modifiers += 1;
+num_window_mods++;
 } else {
 screen_mods[*num_screen_modifiers] = modifier;
-*num_screen_modifiers += 1;
+num_screen_mods++;
 }
 }
 
-assert(*num_window_modifiers + *num_screen_modifiers == 
screen_format->num_modifiers);
+assert(num_wind

[PATCH xserver 13/15] dri3: rework format/modifier caching

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Cut down the unnecessary malloc/memcpy/free by utilising the explicit
copy provided by the client.

But above all: do so, after ensuring we get valid data from the
implementation.

Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 dri3/dri3_screen.c | 50 +++---
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/dri3/dri3_screen.c b/dri3/dri3_screen.c
index ec9afe28a..f5ca07934 100644
--- a/dri3/dri3_screen.c
+++ b/dri3/dri3_screen.c
@@ -117,8 +117,10 @@ cache_formats_and_modifiers(ScreenPtr screen)
 {
 dri3_screen_priv_ptrds = dri3_screen_priv(screen);
 const dri3_screen_info_rec *info = ds->info;
-CARD32 *formats = NULL;
-CARD64 *modifiers = NULL;
+CARD32  num_formats;
+CARD32 *formats;
+CARD32  num_modifiers;
+CARD64 *modifiers;
 int i;
 
 if (ds->formats_cached)
@@ -127,34 +129,36 @@ cache_formats_and_modifiers(ScreenPtr screen)
 if (!info || !info->get_formats || !info->get_modifiers)
 return BadImplementation;
 
-(*info->get_formats) (screen, >num_formats, );
-ds->formats = calloc(ds->num_formats, sizeof(dri3_dmabuf_format_rec));
+if (!info->get_formats(screen, _formats, ))
+return BadAlloc;
+
+if (!num_formats) {
+ds->num_formats = 0;
+ds->formats_cached = TRUE;
+return Success;
+}
+
+ds->formats = calloc(num_formats, sizeof(dri3_dmabuf_format_rec));
 if (!ds->formats)
 return BadAlloc;
 
-for (i = 0; i < ds->num_formats; i++) {
+for (i = 0; i < num_formats; i++) {
 dri3_dmabuf_format_ptr iter = >formats[i];
 
+if (!info->get_modifiers(screen, formats[i],
+ _modifiers,
+ ))
+continue;
+
+if (!num_modifiers)
+continue;
+
 iter->format = formats[i];
-(*info->get_modifiers) (screen, formats[i],
->num_modifiers,
-);
-
-iter->modifiers = malloc(iter->num_modifiers * sizeof(CARD64));
-if (iter->modifiers == NULL)
-goto error;
-
-memcpy(iter->modifiers, modifiers,
-   iter->num_modifiers * sizeof(CARD64));
-goto done;
-
-error:
-iter->num_modifiers = 0;
-free(iter->modifiers);
-done:
-free(modifiers);
+iter->num_modifiers = num_modifiers;
+iter->modifiers = modifiers;
 }
-free(formats);
+
+ds->num_formats = i;
 ds->formats_cached = TRUE;
 
 return Success;
-- 
2.16.0

___
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

[PATCH xserver 07/15] xwayland: zero num_modifiers from the start

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The caller may ignore the return value (will be addressed with later
commit) so simply zero the count from the get-go. We're pretty much do
so, in all cases but one :-\

Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 hw/xwayland/xwayland-glamor.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 339420e05..59461be1f 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -779,13 +779,14 @@ glamor_get_modifiers(ScreenPtr screen, CARD32 format,
 struct xwl_format *xwl_format = NULL;
 int i;
 
+/* Explicitly zero the count as the caller may ignore the return value */
+*num_modifiers = 0;
+
 if (!xwl_screen->dmabuf_capable || !xwl_screen->dmabuf)
 return FALSE;
 
-if (xwl_screen->num_formats == 0) {
-   *num_modifiers = 0;
-   return TRUE;
-}
+if (xwl_screen->num_formats == 0)
+return TRUE;
 
 for (i = 0; i < xwl_screen->num_formats; i++) {
if (xwl_screen->formats[i].format == format) {
@@ -794,16 +795,12 @@ glamor_get_modifiers(ScreenPtr screen, CARD32 format,
}
 }
 
-if (!xwl_format) {
-   *num_modifiers = 0;
+if (!xwl_format)
 return FALSE;
-}
 
 *modifiers = calloc(xwl_format->num_modifiers, sizeof(uint64_t));
-if (*modifiers == NULL) {
-*num_modifiers = 0;
+if (*modifiers == NULL)
 return FALSE;
-}
 
 for (i = 0; i < xwl_format->num_modifiers; i++)
(*modifiers)[i] = xwl_format->modifiers[i];
-- 
2.16.0

___
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

[PATCH xserver 12/15] glamor: zero num_formats from the start

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The caller may ignore the return value (will be addressed with later
commit) so simply zero the count from the get-go. We're pretty much do
so, in all cases but one :-\

Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 glamor/glamor_egl.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index fab280ebf..3e1f45635 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -558,30 +558,26 @@ glamor_get_formats(ScreenPtr screen,
 struct glamor_egl_screen_private *glamor_egl;
 EGLint num;
 
+/* Explicitly zero the count as the caller may ignore the return value */
+*num_formats = 0;
+
 glamor_egl = glamor_egl_get_screen_private(xf86ScreenToScrn(screen));
 
 if (!glamor_egl->dmabuf_capable)
 return FALSE;
 
-if (!eglQueryDmaBufFormatsEXT(glamor_egl->display, 0, NULL, )) {
-*num_formats = 0;
+if (!eglQueryDmaBufFormatsEXT(glamor_egl->display, 0, NULL, ))
 return FALSE;
-}
 
-if (num == 0) {
-*num_formats = 0;
+if (num == 0)
 return TRUE;
-}
 
 *formats = calloc(num, sizeof(CARD32));
-if (*formats == NULL) {
-*num_formats = 0;
+if (*formats == NULL)
 return FALSE;
-}
 
 if (!eglQueryDmaBufFormatsEXT(glamor_egl->display, num,
   (EGLint *) *formats, )) {
-*num_formats = 0;
 free(*formats);
 return FALSE;
 }
-- 
2.16.0

___
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

[PATCH xserver 06/15] dri3: use designated initializers for {s, }proc_dri3_vector

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Seems like the underscore went missing with v1.2?
---
 dri3/dri3_request.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/dri3/dri3_request.c b/dri3/dri3_request.c
index 7ced6747c..edcd0e782 100644
--- a/dri3/dri3_request.c
+++ b/dri3/dri3_request.c
@@ -537,15 +537,15 @@ proc_dri3_buffers_from_pixmap(ClientPtr client)
 
 static const
 DispatchProc proc_dri3_vector[DRI3NumberRequests] = {
-proc_dri3_query_version,/* 0 */
-proc_dri3_open, /* 1 */
-proc_dri3_pixmap_from_buffer,   /* 2 */
-proc_dri3_buffer_from_pixmap,   /* 3 */
-proc_dri3_fence_from_fd,/* 4 */
-proc_dri3_fd_from_fence,/* 5 */
-proc_dri3_get_supported_modifiers,  /* 6 */
-proc_dri3_pixmap_from_buffers,  /* 7 */
-proc_dri3_buffers_from_pixmap,  /* 8 */
+[X_DRI3QueryVersion] = proc_dri3_query_version,
+[X_DRI3Open] = proc_dri3_open,
+[X_DRI3PixmapFromBuffer] = proc_dri3_pixmap_from_buffer,
+[X_DRI3BufferFromPixmap] = proc_dri3_buffer_from_pixmap,
+[X_DRI3FenceFromFD]  = proc_dri3_fence_from_fd,
+[X_DRI3FDFromFence]  = proc_dri3_fd_from_fence,
+[xDRI3GetSupportedModifiers] = proc_dri3_get_supported_modifiers,
+[xDRI3PixmapFromBuffers] = proc_dri3_pixmap_from_buffers,
+[xDRI3BuffersFromPixmap] = proc_dri3_buffers_from_pixmap,
 };
 
 int
@@ -681,15 +681,15 @@ sproc_dri3_buffers_from_pixmap(ClientPtr client)
 
 static const
 DispatchProc sproc_dri3_vector[DRI3NumberRequests] = {
-sproc_dri3_query_version,   /* 0 */
-sproc_dri3_open,/* 1 */
-sproc_dri3_pixmap_from_buffer,  /* 2 */
-sproc_dri3_buffer_from_pixmap,  /* 3 */
-sproc_dri3_fence_from_fd,   /* 4 */
-sproc_dri3_fd_from_fence,   /* 5 */
-sproc_dri3_get_supported_modifiers, /* 6 */
-sproc_dri3_pixmap_from_buffers, /* 7 */
-sproc_dri3_buffers_from_pixmap, /* 8 */
+[X_DRI3QueryVersion] = sproc_dri3_query_version,
+[X_DRI3Open] = sproc_dri3_open,
+[X_DRI3PixmapFromBuffer] = sproc_dri3_pixmap_from_buffer,
+[X_DRI3BufferFromPixmap] = sproc_dri3_buffer_from_pixmap,
+[X_DRI3FenceFromFD]  = sproc_dri3_fence_from_fd,
+[X_DRI3FDFromFence]  = sproc_dri3_fd_from_fence,
+[xDRI3GetSupportedModifiers] = sproc_dri3_get_supported_modifiers,
+[xDRI3PixmapFromBuffers] = sproc_dri3_pixmap_from_buffers,
+[xDRI3BuffersFromPixmap] = sproc_dri3_buffers_from_pixmap,
 };
 
 int _X_COLD
-- 
2.16.0

___
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

[PATCH xserver 04/15] dri3: simplify dri3_open() implementation

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 dri3/dri3_screen.c | 22 +-
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/dri3/dri3_screen.c b/dri3/dri3_screen.c
index f5e87bc9e..628f7616e 100644
--- a/dri3/dri3_screen.c
+++ b/dri3/dri3_screen.c
@@ -31,33 +31,21 @@
 #include 
 #include 
 
-static inline Bool has_open(const dri3_screen_info_rec *info) {
-if (info == NULL)
-return FALSE;
-
-return info->open != NULL ||
-(info->version >= 1 && info->open_client != NULL);
-}
-
 int
 dri3_open(ClientPtr client, ScreenPtr screen, RRProviderPtr provider, int *fd)
 {
 dri3_screen_priv_ptrds = dri3_screen_priv(screen);
 const dri3_screen_info_rec *info = ds->info;
-int rc;
 
-if (!has_open(info))
+if (info == NULL)
 return BadMatch;
 
 if (info->version >= 1 && info->open_client != NULL)
-rc = (*info->open_client) (client, screen, provider, fd);
-else
-rc = (*info->open) (screen, provider, fd);
-
-if (rc != Success)
-return rc;
+return (*info->open_client) (client, screen, provider, fd);
+if (info->open != NULL)
+return (*info->open) (screen, provider, fd);
 
-return Success;
+return BadMatch;
 }
 
 int
-- 
2.16.0

___
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

[PATCH xserver 05/15] dri3: annotate {s, }proc_dri3_vector as static const

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Read-only data, used only locally.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 dri3/dri3_request.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/dri3/dri3_request.c b/dri3/dri3_request.c
index fc258711b..7ced6747c 100644
--- a/dri3/dri3_request.c
+++ b/dri3/dri3_request.c
@@ -32,6 +32,9 @@
 #include 
 #include 
 
+/* Should be provided by the guts of X */
+typedef int (* DispatchProc) (ClientPtr client);
+
 static int
 proc_dri3_query_version(ClientPtr client)
 {
@@ -532,7 +535,8 @@ proc_dri3_buffers_from_pixmap(ClientPtr client)
 return Success;
 }
 
-int (*proc_dri3_vector[DRI3NumberRequests]) (ClientPtr) = {
+static const
+DispatchProc proc_dri3_vector[DRI3NumberRequests] = {
 proc_dri3_query_version,/* 0 */
 proc_dri3_open, /* 1 */
 proc_dri3_pixmap_from_buffer,   /* 2 */
@@ -675,7 +679,8 @@ sproc_dri3_buffers_from_pixmap(ClientPtr client)
 return (*proc_dri3_vector[stuff->dri3ReqType]) (client);
 }
 
-int (*sproc_dri3_vector[DRI3NumberRequests]) (ClientPtr) = {
+static const
+DispatchProc sproc_dri3_vector[DRI3NumberRequests] = {
 sproc_dri3_query_version,   /* 0 */
 sproc_dri3_open,/* 1 */
 sproc_dri3_pixmap_from_buffer,  /* 2 */
-- 
2.16.0

___
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

[PATCH xserver 10/15] dri3: return BadImplementation when missing ::get_{formats, modifiers}

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

If the implementations is missing the required functionality simply
return BadImplementation.

Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 dri3/dri3_screen.c | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/dri3/dri3_screen.c b/dri3/dri3_screen.c
index ee23336a5..ec9afe28a 100644
--- a/dri3/dri3_screen.c
+++ b/dri3/dri3_screen.c
@@ -124,16 +124,9 @@ cache_formats_and_modifiers(ScreenPtr screen)
 if (ds->formats_cached)
 return Success;
 
-if (!info)
+if (!info || !info->get_formats || !info->get_modifiers)
 return BadImplementation;
 
-if (!info->get_formats || !info->get_modifiers) {
-ds->formats = NULL;
-ds->num_formats = 0;
-ds->formats_cached = TRUE;
-return Success;
-}
-
 (*info->get_formats) (screen, >num_formats, );
 ds->formats = calloc(ds->num_formats, sizeof(dri3_dmabuf_format_rec));
 if (!ds->formats)
-- 
2.16.0

___
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

[PATCH xserver 03/15] dri3: annotate fds/strides/offsets arrays as const

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

It makes it perfectly clear that we should not be modifying them.
Should help highlight issues like the one fixed with previous commit.

Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Strictly speaking one should annotate everything but pixmap/screen as
const to be perfectly clear about in/out arguments. Yet again nobody
bothers annotating int arguments so meh for now ;-)
---
 dri3/dri3.h   | 6 +++---
 dri3/dri3_priv.h  | 6 --
 dri3/dri3_screen.c| 4 ++--
 glamor/glamor.h   | 6 +++---
 glamor/glamor_egl.c   | 4 ++--
 hw/xwayland/xwayland-glamor.c | 8 
 6 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/dri3/dri3.h b/dri3/dri3.h
index bd17522b0..fc76908e1 100644
--- a/dri3/dri3.h
+++ b/dri3/dri3.h
@@ -49,11 +49,11 @@ typedef PixmapPtr (*dri3_pixmap_from_fd_proc) (ScreenPtr 
screen,
 
 typedef PixmapPtr (*dri3_pixmap_from_fds_proc) (ScreenPtr screen,
 CARD8 num_fds,
-int *fds,
+const int *fds,
 CARD16 width,
 CARD16 height,
-CARD32 *strides,
-CARD32 *offsets,
+const CARD32 *strides,
+const CARD32 *offsets,
 CARD8 depth,
 CARD8 bpp,
 CARD64 modifier);
diff --git a/dri3/dri3_priv.h b/dri3/dri3_priv.h
index cf6632ddf..ea278049d 100644
--- a/dri3/dri3_priv.h
+++ b/dri3/dri3_priv.h
@@ -79,8 +79,10 @@ int
 dri3_open(ClientPtr client, ScreenPtr screen, RRProviderPtr provider, int *fd);
 
 int
-dri3_pixmap_from_fds(PixmapPtr *ppixmap, ScreenPtr screen, CARD8 num_fds, int 
*fds,
- CARD16 width, CARD16 height, CARD32 *strides, CARD32 
*offsets,
+dri3_pixmap_from_fds(PixmapPtr *ppixmap, ScreenPtr screen,
+ CARD8 num_fds, const int *fds,
+ CARD16 width, CARD16 height,
+ const CARD32 *strides, const CARD32 *offsets,
  CARD8 depth, CARD8 bpp, CARD64 modifier);
 
 int
diff --git a/dri3/dri3_screen.c b/dri3/dri3_screen.c
index 0b045c413..f5e87bc9e 100644
--- a/dri3/dri3_screen.c
+++ b/dri3/dri3_screen.c
@@ -62,9 +62,9 @@ dri3_open(ClientPtr client, ScreenPtr screen, RRProviderPtr 
provider, int *fd)
 
 int
 dri3_pixmap_from_fds(PixmapPtr *ppixmap, ScreenPtr screen,
- CARD8 num_fds, int *fds,
+ CARD8 num_fds, const int *fds,
  CARD16 width, CARD16 height,
- CARD32 *strides, CARD32 *offsets,
+ const CARD32 *strides, const CARD32 *offsets,
  CARD8 depth, CARD8 bpp, CARD64 modifier)
 {
 dri3_screen_priv_ptrds = dri3_screen_priv(screen);
diff --git a/glamor/glamor.h b/glamor/glamor.h
index 7b5676226..ef10e5cda 100644
--- a/glamor/glamor.h
+++ b/glamor/glamor.h
@@ -264,11 +264,11 @@ extern _X_EXPORT struct gbm_bo 
*glamor_gbm_bo_from_pixmap(ScreenPtr screen,
  * */
 extern _X_EXPORT PixmapPtr glamor_pixmap_from_fds(ScreenPtr screen,
   CARD8 num_fds,
-  int *fds,
+  const int *fds,
   CARD16 width,
   CARD16 height,
-  CARD32 *strides,
-  CARD32 *offsets,
+  const CARD32 *strides,
+  const CARD32 *offsets,
   CARD8 depth,
   CARD8 bpp,
   uint64_t modifier);
diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index 5992b9279..aeb21c162 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -476,9 +476,9 @@ gbm_format_for_depth(CARD8 depth)
 
 _X_EXPORT PixmapPtr
 glamor_pixmap_from_fds(ScreenPtr screen,
-   CARD8 num_fds, int *fds,
+   CARD8 num_fds, const int *fds,
CARD16 width, CARD16 height,
-   CARD32 *strides, CARD32 *offsets,
+  

[PATCH xserver 01/15] dri3: annotate the dri3_screen_info data as const

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

dri3_screen_info is the user provide dispatch. Something that we do
not and should not change.

When using the _ptr typecast + const the compiler barfs at us
(rightfully so), so use the _rec one.

Fixes: 56313829886 ("dri3: Add DRI3 extension")
Cc: Keith Packard <kei...@keithp.com>
Cc: Adam Jackson <a...@redhat.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Why do we have the explicit _rec and _ptr typecasts to begin with?

It reminds be of Pascal with it's infamous record ... from some 20
years ago.
---
 dri3/dri3.c   |  2 +-
 dri3/dri3.h   |  2 +-
 dri3/dri3_priv.h  |  2 +-
 dri3/dri3_screen.c| 12 ++--
 glamor/glamor_egl.c   |  2 +-
 hw/xwayland/xwayland-glamor.c |  2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/dri3/dri3.c b/dri3/dri3.c
index 8ac0f3ae2..ba32facd7 100644
--- a/dri3/dri3.c
+++ b/dri3/dri3.c
@@ -45,7 +45,7 @@ dri3_close_screen(ScreenPtr screen)
 }
 
 Bool
-dri3_screen_init(ScreenPtr screen, dri3_screen_info_ptr info)
+dri3_screen_init(ScreenPtr screen, const dri3_screen_info_rec *info)
 {
 dri3_screen_generation = serverGeneration;
 
diff --git a/dri3/dri3.h b/dri3/dri3.h
index 89ad13ad9..bd17522b0 100644
--- a/dri3/dri3.h
+++ b/dri3/dri3.h
@@ -104,7 +104,7 @@ typedef struct dri3_screen_info {
 } dri3_screen_info_rec, *dri3_screen_info_ptr;
 
 extern _X_EXPORT Bool
-dri3_screen_init(ScreenPtr screen, dri3_screen_info_ptr info);
+dri3_screen_init(ScreenPtr screen, const dri3_screen_info_rec *info);
 
 extern _X_EXPORT int
 dri3_send_open_reply(ClientPtr client, int fd);
diff --git a/dri3/dri3_priv.h b/dri3/dri3_priv.h
index 8447680ba..cf6632ddf 100644
--- a/dri3/dri3_priv.h
+++ b/dri3/dri3_priv.h
@@ -49,7 +49,7 @@ typedef struct dri3_screen_priv {
 CARD32  num_formats;
 dri3_dmabuf_format_ptr  formats;
 
-dri3_screen_info_ptrinfo;
+const dri3_screen_info_rec *info;
 } dri3_screen_priv_rec, *dri3_screen_priv_ptr;
 
 #define wrap(priv,real,mem,func) {\
diff --git a/dri3/dri3_screen.c b/dri3/dri3_screen.c
index df40f8281..0b045c413 100644
--- a/dri3/dri3_screen.c
+++ b/dri3/dri3_screen.c
@@ -31,7 +31,7 @@
 #include 
 #include 
 
-static inline Bool has_open(dri3_screen_info_ptr info) {
+static inline Bool has_open(const dri3_screen_info_rec *info) {
 if (info == NULL)
 return FALSE;
 
@@ -43,7 +43,7 @@ int
 dri3_open(ClientPtr client, ScreenPtr screen, RRProviderPtr provider, int *fd)
 {
 dri3_screen_priv_ptrds = dri3_screen_priv(screen);
-dri3_screen_info_ptrinfo = ds->info;
+const dri3_screen_info_rec *info = ds->info;
 int rc;
 
 if (!has_open(info))
@@ -68,7 +68,7 @@ dri3_pixmap_from_fds(PixmapPtr *ppixmap, ScreenPtr screen,
  CARD8 depth, CARD8 bpp, CARD64 modifier)
 {
 dri3_screen_priv_ptrds = dri3_screen_priv(screen);
-dri3_screen_info_ptrinfo = ds->info;
+const dri3_screen_info_rec *info = ds->info;
 PixmapPtr   pixmap;
 
 if (!info)
@@ -99,7 +99,7 @@ dri3_fds_from_pixmap(PixmapPtr pixmap, int *fds,
 {
 ScreenPtr   screen = pixmap->drawable.pScreen;
 dri3_screen_priv_ptrds = dri3_screen_priv(screen);
-dri3_screen_info_ptrinfo = ds->info;
+const dri3_screen_info_rec *info = ds->info;
 
 if (!info)
 return 0;
@@ -128,7 +128,7 @@ static int
 cache_formats_and_modifiers(ScreenPtr screen)
 {
 dri3_screen_priv_ptrds = dri3_screen_priv(screen);
-dri3_screen_info_ptrinfo = ds->info;
+const dri3_screen_info_rec *info = ds->info;
 CARD32 *formats = NULL;
 CARD64 *modifiers = NULL;
 int i;
@@ -188,7 +188,7 @@ dri3_get_supported_modifiers(ScreenPtr screen, DrawablePtr 
drawable,
  CARD64 **screen_modifiers)
 {
 dri3_screen_priv_ptrds = dri3_screen_priv(screen);
-dri3_screen_info_ptrinfo = ds->info;
+const dri3_screen_info_rec *info = ds->info;
 int i, j;
 int ret;
 CARD32  num_drawable_mods;
diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index 4a550932a..5992b9279 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -755,7 +755,7 @@ glamor_dri3_open_client(ClientPtr client,
 return Success;
 }
 
-static dri3_screen_info_rec glamor_dri3_info = {
+static const dri3_screen_info_rec glamor_dri3_info = {
 .version = 2,
 .open_client = glamor_dri3_open_client,
 .pixmap_from_fds = glamor_pixmap_from_fds,
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 7e9815626..7f64483bf 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/

[PATCH xserver 02/15] xwayland: don't close() fds we don't own

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The glamor_pixmap_from_fds error path erroneously closes the fds.
We don't own them, plus the caller closes them after the function in
called.

Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 hw/xwayland/xwayland-glamor.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 7f64483bf..6ba716263 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -704,8 +704,6 @@ glamor_pixmap_from_fds(ScreenPtr screen,
 return pixmap;
 
 error:
-for (i = 0; i < num_fds; i++)
-   close(fds[i]);
 return NULL;
 }
 
-- 
2.16.0

___
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

[PATCH xserver 09/15] dri3: check for ::get_drawable_modifiers failure

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Currently if the function fails, we'll fall into two false assumptions:
 - the the count is zero
 - that the storage pointer is safe for free()

I've just fixed the former (in glamore + xwayland) and have no
plans on adding yet another workaround for the latter.

Simply zero both variables. Regardless if the implementation is missing
the callback or it foobars with output variables (normally a bad idea).

Bonus points - this fixes a bug where we feed garbage to free() further
down ;-)

Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 dri3/dri3_screen.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/dri3/dri3_screen.c b/dri3/dri3_screen.c
index 628f7616e..ee23336a5 100644
--- a/dri3/dri3_screen.c
+++ b/dri3/dri3_screen.c
@@ -211,10 +211,13 @@ dri3_get_supported_modifiers(ScreenPtr screen, 
DrawablePtr drawable,
 return Success;
 }
 
-if (info->get_drawable_modifiers)
-(*info->get_drawable_modifiers) (drawable, format,
- _drawable_mods,
- _mods);
+if (!info->get_drawable_modifiers ||
+!info->get_drawable_modifiers(drawable, format,
+  _drawable_mods,
+  _mods)) {
+num_drawable_mods = 0;
+drawable_mods = NULL;
+}
 
 /* We're allocating slightly more memory than necessary but it reduces
  * the complexity of finding the intersection set.
-- 
2.16.0

___
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

[PATCH xserver 11/15] xwayland: zero num_formats from the start

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The caller may ignore the return value (will be addressed with later
commit) so simply zero the count from the get-go. We're pretty much do
so, in all cases but one :-\

Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 hw/xwayland/xwayland-glamor.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 59461be1f..1b9a6b030 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -750,19 +750,18 @@ glamor_get_formats(ScreenPtr screen,
 struct xwl_screen *xwl_screen = xwl_screen_get(screen);
 int i;
 
+/* Explicitly zero the count as the caller may ignore the return value */
+*num_formats = 0;
+
 if (!xwl_screen->dmabuf_capable || !xwl_screen->dmabuf)
 return FALSE;
 
-if (xwl_screen->num_formats == 0) {
-   *num_formats = 0;
+if (xwl_screen->num_formats == 0)
return TRUE;
-}
 
 *formats = calloc(xwl_screen->num_formats, sizeof(CARD32));
-if (*formats == NULL) {
-*num_formats = 0;
+if (*formats == NULL)
 return FALSE;
-}
 
 for (i = 0; i < xwl_screen->num_formats; i++)
(*formats)[i] = xwl_screen->formats[i].format;
-- 
2.16.0

___
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

[PATCH xserver 08/15] glamor: zero num_modifiers from the start

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The caller may ignore the return value (will be addressed with later
commit) so simply zero the count from the get-go. We're pretty much do
so, in all cases but one :-\

Fixes: cef12efc15c ("glamor: Implement GetSupportedModifiers")
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Cc: Daniel Stone <dani...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 glamor/glamor_egl.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index aeb21c162..fab280ebf 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -602,31 +602,27 @@ glamor_get_modifiers(ScreenPtr screen, CARD32 format,
 struct glamor_egl_screen_private *glamor_egl;
 EGLint num;
 
+/* Explicitly zero the count as the caller may ignore the return value */
+*num_modifiers = 0;
+
 glamor_egl = glamor_egl_get_screen_private(xf86ScreenToScrn(screen));
 
 if (!glamor_egl->dmabuf_capable)
 return FALSE;
 
 if (!eglQueryDmaBufModifiersEXT(glamor_egl->display, format, 0, NULL,
-NULL, )) {
-*num_modifiers = 0;
+NULL, ))
 return FALSE;
-}
 
-if (num == 0) {
-*num_modifiers = 0;
+if (num == 0)
 return TRUE;
-}
 
 *modifiers = calloc(num, sizeof(uint64_t));
-if (*modifiers == NULL) {
-*num_modifiers = 0;
+if (*modifiers == NULL)
 return FALSE;
-}
 
 if (!eglQueryDmaBufModifiersEXT(glamor_egl->display, format, num,
 (EGLuint64KHR *) *modifiers, NULL, )) {
-*num_modifiers = 0;
 free(*modifiers);
 return FALSE;
 }
-- 
2.16.0

___
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

Re: [PATCH xserver 2/2] glamor: Hide new DRI for behind Option "Debug" "dmabuf_capable"

[Emil Velikov]

On 28 March 2018 at 17:46, Adam Jackson  wrote:
> ... for xfree86, at least for now. Things appear to work for Xwayland
> but not yet for modesetting. Hopefully we can fix that before 1.20 but
> in the meantime this makes testing both paths easier than a rebuild.
>
> Signed-off-by: Adam Jackson 
> ---
>  glamor/glamor_egl.c  | 4 +++-
>  hw/xfree86/man/xorg.conf.man | 3 +++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
> index 2ea3efc58e..4a550932a0 100644
> --- a/glamor/glamor_egl.c
> +++ b/glamor/glamor_egl.c
> @@ -35,6 +35,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #define EGL_DISPLAY_NO_X_MESA
>
> @@ -960,7 +961,8 @@ glamor_egl_init(ScrnInfoPtr scrn, int fd)
>  "EGL_EXT_image_dma_buf_import") &&
>  epoxy_has_egl_extension(glamor_egl->display,
>  "EGL_EXT_image_dma_buf_import_modifiers"))
> -glamor_egl->dmabuf_capable = TRUE;
> +glamor_egl->dmabuf_capable = !!strstr(xf86Info.debug,
> +  "dmabuf_capable");
>  #endif
>
>  glamor_egl->saved_free_screen = scrn->FreeScreen;
> diff --git a/hw/xfree86/man/xorg.conf.man b/hw/xfree86/man/xorg.conf.man
> index 46ddd0ec9c..958926243c 100644
> --- a/hw/xfree86/man/xorg.conf.man
> +++ b/hw/xfree86/man/xorg.conf.man
> @@ -498,6 +498,9 @@ The options recognised by this section are:
>  .BI "Option \*qDebug\*q  \*q" string \*q
>  This comma-separated list provides a way to control various debugging 
> switches
>  from the config file.
> +At the moment the only defined value is
> +.B dmabuf_capable
> +which instructs glamor to enable some unstable buffer management code.

To reflect what the code does, this should be changed to something like:

Adding dmabuf_capable enables multiplane BO import in glamor. The
option has _no_ effect if the required EGL extensions are missing.

HTH
Emil
___
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

Re: [PATCH xserver 3/3] modesetting/drmmode: Use drmModeGetFB2

[Emil Velikov]

On 23 March 2018 at 13:50, Daniel Stone <dani...@collabora.com> wrote:
> Much like AddFB -> AddFB2, GetFB2 lets us get multiple buffers back as
> well as modifier information. This lets us use -background none with
> multiplanar formats, or modifiers which can't be inferred via a magic
> side channel.
>
AFAICT there's nothing special (or wrong) with the new API.

The flags field is a bit of an open question - should Xserver or
libdrm manage the value passed to the kernel?
Analogously - should we pass the flags back to the user (drmModeFB2::flags)?

Current design seems perfectly fine IMHO, although others might disagree.

> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 65 
> ++--
>  1 file changed, 50 insertions(+), 15 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
> b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 39ed16f98..82ea49386 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -1080,9 +1080,13 @@ create_pixmap_for_fbcon(drmmode_ptr drmmode, 
> ScrnInfoPtr pScrn, int fbcon_id)
>  {
>  PixmapPtr pixmap = drmmode->fbcon_pixmap;
>  drmModeFBPtr fbcon;
> +drmModeFB2Ptr fbcon2;
>  ScreenPtr pScreen = xf86ScrnToScreen(pScrn);
>  uint32_t handles[4] = { 0, };
>  CARD32 strides[4] = { 0, }, offsets[4] = { 0, };
> +uint64_t modifier;
> +int width, height;
> +int depth = 0, bpp = 0;
>  int fds[4] = { -1, -1, -1, -1 };
>  int num_fds;
>  int i;
> @@ -1090,31 +1094,63 @@ create_pixmap_for_fbcon(drmmode_ptr drmmode, 
> ScrnInfoPtr pScrn, int fbcon_id)
>  if (pixmap)
>  return pixmap;
>
> -fbcon = drmModeGetFB(drmmode->fd, fbcon_id);
> -if (fbcon == NULL)
> -return NULL;
> +fbcon2 = drmModeGetFB2(drmmode->fd, fbcon_id);
> +if (fbcon2) {
Declare and initialize fbcon2 in one go - C99 feature that everyone
has (even the people who use MSVC to build Xserver & friends).
Then wrap the whole fbcon2 hunk in a #ifdef HAVE_GETFB2 + add a
configure/meson check.

Alternatively add a weak drmModeGetFB2 function which returns NULL and
forget all the above ;-)


> +width = fbcon2->width;
> +height = fbcon2->height;
> +memcpy(handles, fbcon2->handles, sizeof(handles));
> +memcpy(strides, fbcon2->pitches, sizeof(strides));
> +memcpy(offsets, fbcon2->offsets, sizeof(offsets));
> +modifier = fbcon2->modifier;
> +switch (fbcon2->pixel_format) {
> +case DRM_FORMAT_RGB565:
> +depth = 16;
Missing bpp?

Idea: Instead of introducing another format <> {depth, bpp} mapping,
might as well add some helpers?

> +break;
> +case DRM_FORMAT_XRGB:
> +depth = 24;
> +bpp = 32;
> +break;
> +case DRM_FORMAT_XRGB2101010:
> +depth = 30;
> +bpp = 32;
> +    default:
> +break;
Error instead of silently continuing?
Having a fallback to GetFB1 is possible, but IMHO not something we want.

Regardless of the helper suggestion, but with the rest addressed
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

-Emil
___
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

Re: [PATCH xserver 2/3] modesetting/drmmode: Use glamor_pixmap_from_fds

[Emil Velikov]

On 23 March 2018 at 13:50, Daniel Stone <dani...@collabora.com> wrote:
> glamor_pixmap_from_fds saves us the trouble of creating the pixmap and
> dealing with its header ourselves; instead we pass the whole thing off
> to Glamor.
>
> This requires us to provide FDs rather than handles for the import, but
> Glamor was already doing that internally, so no functional change.
>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 52 
> +++-
>  1 file changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
> b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 4b22abd0f..39ed16f98 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -30,6 +30,7 @@
>  #endif
>
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1058,6 +1059,21 @@ drmmode_crtc_dpms(xf86CrtcPtr crtc, int mode)
>  drmmode_crtc->dpms_mode = mode;
>  }
>
> +static int
> +handles_to_fds(drmmode_ptr drmmode, uint32_t *handles, int *fds)
Nit: Although uncommon in the xserver codebase, I can use uint32_t
handles[4] ;-)

> +{
> +int err;
> +int i;
> +
> +for (i = 0; i < 4 && handles[i]; i++) {
> +err = drmPrimeHandleToFD(drmmode->fd, handles[i], O_CLOEXEC, 
> [i]);
> +    if (err != 0)
> +return 0;
Close the existing fds ones on error?

With the fd leak plugged (regardless of the nit)
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

Aside: there's plenty of duplication in the area + some partial depth
<> format mappings.

-Emil
___
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

Re: [PATCH xserver 1/3] modesetting/drmmode: Remove unused flink call

[Emil Velikov]

On 23 March 2018 at 13:50, Daniel Stone <dani...@collabora.com> wrote:
> We don't use flink in the GetFB import path anymore, as we do an
> FD-based import instead.
>
Indeed - the flink is a left-over from the original radeon code that
this was based on.
The radeon specifics were removed and codepath was enabled with commit
caabc4e8554 ("modesetting: add support for background none.")

Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

-Emil
___
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

Re: [PATCH xserver] modesetting: Fix up some XXX from removing GLAMOR_HAS_DRM_*

[Emil Velikov]

Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

Thanks!
Emil
___
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

Re: [PATCH xserver 1/5] configure: remove libdrm version check

[Emil Velikov]

On 21 March 2018 at 14:12, Adam Jackson <a...@nwnk.net> wrote:
> On Tue, 2018-03-20 at 16:41 +0000, Emil Velikov wrote:
>
>> Humble ping?
>>
>> Patch 2 might need a respin based on the feedback from Daniel/Louis.
>> Although the rest of the series should be pretty trivial.
>
> Said feedback appears not to be in my mailbox? The XXX bits should be
> cleaned up but this is better than before, so merged:
>
Right, meant to say "on the feedback expected from ...". AKA there is
none presently.

Thank you Adam!
Emil
___
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

Re: [PATCH xserver] present: cap the version returned to the client

[Emil Velikov]

On 21 March 2018 at 08:02, Julien Cristau <jcris...@debian.org> wrote:
> On Mon, Mar 19, 2018 at 16:04:43 +0000, Emil Velikov wrote:
>
>> From: Emil Velikov <emil.veli...@collabora.com>
>>
>> As per the protocol, the server should not return version greater than
>> the one supported by the client.
>>
>> Add a spec quote and tweak the numbers accordingly.
>>
>> Fixes: 5c5c1b77982 ("present: Add Present extension")
>> Cc: Thierry Reding <tred...@nvidia.com>
>> Cc: Daniel Stone <dani...@collabora.com>
>> Cc: Keith Packard <kei...@keithp.com>
>> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
>> ---
>> Analogous to the DRI3 patch here
>> https://patchwork.freedesktop.org/patch/210343/
>> ---
>>  present/present_request.c | 14 +-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/present/present_request.c b/present/present_request.c
>> index c6afe5fa7..f52efa52b 100644
>> --- a/present/present_request.c
>> +++ b/present/present_request.c
>> @@ -41,7 +41,19 @@ proc_present_query_version(ClientPtr client)
>>  };
>>
>>  REQUEST_SIZE_MATCH(xPresentQueryVersionReq);
>> -(void) stuff;
>> +/* From presentproto:
>> + *
>> + * The client sends the highest supported version to the server
>> + * and the server sends the highest version it supports, but no
>> + * higher than the requested version.
>> + */
>> +
>> +if (rep.majorVersion > stuff->majorVersion ||
>> +rep.minorVersion > stuff->minorVersion) {
>> +rep.majorVersion = stuff->majorVersion;
>> +rep.minorVersion = stuff->minorVersion;
>> +}
>
> Doesn't this break when e.g. client supports 2.2 and server supports
> 1.4, where we'll return 2.2 instead of 1.4?

Present has never bumped major, so things should be fine.
in fact only two extensions have done so - DRI2 and XFIXES - more
details on those below.

That said, I'll tweak the checks (this + dri3) to handle more corner
cases. The original issue was that server was returning greater
version than the one requested.

-Emil

DRI3 never saw a v1 release, while the history behind XFIXES is very
messy - here's a brief.

804a9fda12f70e66feac5e45bc8293a7e436689b
1.0 -> 2.0: introduced new events/requests, broke one existing event

f92db7128c857b3925846a9c8519e9554a1c67e2
no version change: _lots_ of breakage

2e9a7b2004d943eaf1be1778c94790528c573cb1
2.0 -> 4.0: introduced new events/requests, changed version meaning
major: incompatible -> new requests
minor: only BC changes -> only minor adjustments or BC changes

9760b4bdd1f9fdd6a33b9f876c4a835ed969aa84
4.0 -> 5.0: introduced new events/requests
___
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

Re: [PATCH xserver 1/5] configure: remove libdrm version check

[Emil Velikov]

On 7 March 2018 at 18:45, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> From: Emil Velikov <emil.veli...@collabora.com>
>
> We already require said version.
>
> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
> ---
>  configure.ac | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index f82c0a66a..14fe34995 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1999,8 +1999,7 @@ if test "x$XORG" = xyes; then
> fi
>
> if test "x$DRM" = xyes; then
> -   dnl 2.4.46 is required for cursor hotspot support.
> -   PKG_CHECK_EXISTS(libdrm >= 2.4.46, 
> XORG_DRIVER_MODESETTING=yes, XORG_DRIVER_MODESETTING=no)
> +   XORG_DRIVER_MODESETTING=yes
Humble ping?

Patch 2 might need a respin based on the feedback from Daniel/Louis.
Although the rest of the series should be pretty trivial.

Thanks
Emil
___
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

Re: [PATCH xserver] dri3: cap the version returned to the client

[Emil Velikov]

On 19 March 2018 at 19:59, Adam Jackson <a...@nwnk.net> wrote:
> On Mon, 2018-03-19 at 12:04 +0000, Emil Velikov wrote:
>> On 13 March 2018 at 18:38, Emil Velikov <emil.l.veli...@gmail.com> wrote:
>> > From: Emil Velikov <emil.veli...@collabora.com>
>> >
>> > As per the protocol, the server should not return version greater than
>> > the one supported by the client.
>> >
>> > Add a spec quote and tweak the numbers accordingly.
>> >
>> > Fixes: 563138298868 ("dri3: Add DRI3 extension")
>> > Cc: Daniel Stone <dani...@collabora.com>
>> > Cc: Keith Packard <kei...@keithp.com>
>> > Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
>
> Merged:
>
> remote: I: patch #210343 updated using rev 
> ae5c0dd199a5fbfbdf7a2d6b8c1b28c410289106.
> remote: I: 1 patch(es) updated to state Accepted.
> To ssh://git.freedesktop.org/git/xorg/xserver
>6a5d51e082..ae5c0dd199  master -> master
>
> Though I think both this and the corresponding present patch are broken
> if the client sends a version number of 0.42...
>
True. We could bail out if the client sends a too old (garbage) version - WDYT?

In general handling of server_major != client_major could also be
improved, yet major won't be bumped in the foreseeable future.

Thanks
-Emil
___
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

Re: [PATCH] present: Advertise protocol version 1.2

[Emil Velikov]

On 16 March 2018 at 13:24, Thierry Reding <thierry.red...@gmail.com> wrote:
> From: Thierry Reding <tred...@nvidia.com>
>
> Everything is implemented to support protocol version 1.2. Make it
> official.
>
> Reviewed-by: Daniel Stone <dani...@collabora.com>
> Signed-off-by: Thierry Reding <tred...@nvidia.com>
Yes, please.
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

-Emil
___
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

[PATCH xserver] present: cap the version returned to the client

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

As per the protocol, the server should not return version greater than
the one supported by the client.

Add a spec quote and tweak the numbers accordingly.

Fixes: 5c5c1b77982 ("present: Add Present extension")
Cc: Thierry Reding <tred...@nvidia.com>
Cc: Daniel Stone <dani...@collabora.com>
Cc: Keith Packard <kei...@keithp.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Analogous to the DRI3 patch here
https://patchwork.freedesktop.org/patch/210343/
---
 present/present_request.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/present/present_request.c b/present/present_request.c
index c6afe5fa7..f52efa52b 100644
--- a/present/present_request.c
+++ b/present/present_request.c
@@ -41,7 +41,19 @@ proc_present_query_version(ClientPtr client)
 };
 
 REQUEST_SIZE_MATCH(xPresentQueryVersionReq);
-(void) stuff;
+/* From presentproto:
+ *
+ * The client sends the highest supported version to the server
+ * and the server sends the highest version it supports, but no
+ * higher than the requested version.
+ */
+
+if (rep.majorVersion > stuff->majorVersion ||
+rep.minorVersion > stuff->minorVersion) {
+rep.majorVersion = stuff->majorVersion;
+rep.minorVersion = stuff->minorVersion;
+}
+
 if (client->swapped) {
 swaps();
 swapl();
-- 
2.16.0

___
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

Re: [PATCH xserver 2/3] modesetting: Remove #ifdefs XF86_PDEV_SERVER_FD

[Emil Velikov]

On 19 March 2018 at 15:43, Alan Coopersmith <alan.coopersm...@oracle.com> wrote:
> On 03/19/18 06:33 AM, Emil Velikov wrote:
>> On 15 March 2018 at 18:33, Adam Jackson <a...@redhat.com> wrote:
>>> On Wed, 2018-03-14 at 21:48 +0100, Thomas Klausner wrote:
>>>> On Wed, Mar 14, 2018 at 01:33:28PM -0700, Alan Coopersmith wrote:
>>>>> On 03/14/18 01:01 PM, Thomas Klausner wrote:
>>>>>> I see a build failure in xorg-server-1.19.99.901 on NetBSD.
>>>>>
>>>>> Looks like the same failure I saw on Solaris and sent in a patch
>>>>> for to revert the removal of that #ifdef.
>>>>>
>>>>> https://patchwork.freedesktop.org/patch/207937/
>>>>
>>>> This patch works for me, thank you.
>>>
>>> Sorry for letting this one fall through the cracks. Merged, thanks:
>>>
>>> remote: I: patch #207937 updated using rev 
>>> 7fc89251ef5e7363dfbf6d831ed448bbcd8519b8.
>>> remote: I: 1 patch(es) updated to state Accepted.
>>> To ssh://git.freedesktop.org/git/xorg/xserver
>>>edf08bd654..7fc89251ef  master -> master
>>>
>> Another solution is to unconditionally include xf86platformBus.h in
>> the modesetting driver.
>
> Nope, as I mentioned in the thread around the patch I sent, that's not
> enough as just including the header without restoring the #ifdefs
> results in:
>
> driver.c: In function ‘probe_hw’:
> driver.c:246:14: error: implicit declaration of function
> ‘xf86_platform_device_odev_attributes’; did you mean
> ‘xf86platformAddGPUDevices’? [-Werror=implicit-function-declaration]
>  fd = xf86_platform_device_odev_attributes(platform_dev)->fd;
>   ^~~~
>   xf86platformAddGPUDevices
> driver.c:246:14: warning: nested extern declaration of
> ‘xf86_platform_device_odev_attributes’ [-Wnested-externs]
> driver.c:246:64: error: invalid type argument of ‘->’ (have ‘int’)
>  fd = xf86_platform_device_odev_attributes(platform_dev)->fd;
> ^~
>
> It looks like you'd need to replace the #ifdef XF86_PDEV_SERVER_FD with
> #ifdef XSERVER_PLATFORM_BUS in probe_hw() as well.
>
Indeed the current varying API (and ABI) based on the configure
selected, is rather 'lovely'.
Guess, I could pull-up my sleeves and untangle some of that if we have
reviewers ;-)

Pardon for the noise.

-Emil
___
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

Re: [PATCH xserver 2/3] modesetting: Remove #ifdefs XF86_PDEV_SERVER_FD

[Emil Velikov]

On 15 March 2018 at 18:33, Adam Jackson  wrote:
> On Wed, 2018-03-14 at 21:48 +0100, Thomas Klausner wrote:
>> On Wed, Mar 14, 2018 at 01:33:28PM -0700, Alan Coopersmith wrote:
>> > On 03/14/18 01:01 PM, Thomas Klausner wrote:
>> > > I see a build failure in xorg-server-1.19.99.901 on NetBSD.
>> >
>> > Looks like the same failure I saw on Solaris and sent in a patch
>> > for to revert the removal of that #ifdef.
>> >
>> > https://patchwork.freedesktop.org/patch/207937/
>>
>> This patch works for me, thank you.
>
> Sorry for letting this one fall through the cracks. Merged, thanks:
>
> remote: I: patch #207937 updated using rev 
> 7fc89251ef5e7363dfbf6d831ed448bbcd8519b8.
> remote: I: 1 patch(es) updated to state Accepted.
> To ssh://git.freedesktop.org/git/xorg/xserver
>edf08bd654..7fc89251ef  master -> master
>
Another solution is to unconditionally include xf86platformBus.h in
the modesetting driver.
Just like every other #include xf86platformBus.h instance in the
xserver codebase.

HTH
Emil
___
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

Re: [PATCH xserver] dri3: cap the version returned to the client

[Emil Velikov]

On 13 March 2018 at 18:38, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> From: Emil Velikov <emil.veli...@collabora.com>
>
> As per the protocol, the server should not return version greater than
> the one supported by the client.
>
> Add a spec quote and tweak the numbers accordingly.
>
> Fixes: 563138298868 ("dri3: Add DRI3 extension")
> Cc: Daniel Stone <dani...@collabora.com>
> Cc: Keith Packard <kei...@keithp.com>
> Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
> ---
> Note: we might want this in the stable releases?
>
> Related: The DRI2 implementation has identical bug.
> Although fixing that (unlike DRI3) might cause some ill written apps to
> rightfully fall on their face.
>
> Should we bother - yay, nay are appreciated.
> ---
>  dri3/dri3_request.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/dri3/dri3_request.c b/dri3/dri3_request.c
> index 7f3f0d08c..fc258711b 100644
> --- a/dri3/dri3_request.c
> +++ b/dri3/dri3_request.c
> @@ -45,7 +45,19 @@ proc_dri3_query_version(ClientPtr client)
>  };
>
>  REQUEST_SIZE_MATCH(xDRI3QueryVersionReq);
> -(void) stuff;
> +/* From DRI3 proto:
> + *
> + * The client sends the highest supported version to the server
> + * and the server sends the highest version it supports, but no
> + * higher than the requested version.
> + */
> +
> +if (rep.majorVersion > stuff->majorVersion ||
> +rep.minorVersion > stuff->minorVersion) {
> +rep.majorVersion = stuff->majorVersion;
> +rep.minorVersion = stuff->minorVersion;
> +}
> +
>  if (client->swapped) {
>  swaps();
>  swapl();
> --
Humble ping anyone?

-Emil
___
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

Re: [PATCH xserver] build: Bump Mesa requirement to 17.1

[Emil Velikov]

On 14 March 2018 at 18:43, Adam Jackson  wrote:
> gbm_bo_get_modifier is new in 17.1, which is 10 months old and two
> stable branches ago.
>
> Signed-off-by: Adam Jackson 
> ---
>  configure.ac| 2 +-
>  glx/meson.build | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index f82c0a66a..18cfda69e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1113,7 +1113,7 @@ case "$DRI2,$HAVE_DRI2PROTO" in
> yes,yes | auto,yes)
> AC_DEFINE(DRI2, 1, [Build DRI2 extension])
> DRI2=yes
> -   LIBGL="gl >= 9.2.0"
> +   LIBGL="gl >= 17.1"
If the concern is gbm_bo_get_modifier then the LIBGBM version should
be bumped, right?
Then one can drop all the GBM_BO_WITH_MODIFIERS hunks, analogous to my
earlier series [1]

-Emil

[1] https://patchwork.freedesktop.org/series/39553/
___
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

Re: [PATCH xserver] exa: promise not to touch the data when swapping pointers

[Emil Velikov]

On 13 March 2018 at 10:55, Eric Engestrom <eric.engest...@imgtec.com> wrote:
> exa/exa.c:525:10: warning: initialization discards ‘const’ qualifier from 
> pointer target type [-Wdiscarded-qualifiers]
>  swap(pExaGC, pGC, funcs);
>   ^
>
> Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com>
> ---
>  exa/exa_priv.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/exa/exa_priv.h b/exa/exa_priv.h
> index ca4db720fbe871b50b7e..912e214789adba95c7fa 100644
> --- a/exa/exa_priv.h
> +++ b/exa/exa_priv.h
> @@ -244,7 +244,7 @@ extern DevPrivateKeyRec exaScreenPrivateKeyRec;
>  }
>  #else
>  #define swap(priv, real, mem) {\
> -void *tmp = priv->Saved##mem; \
> +const void *tmp = priv->Saved##mem; \

Hmm what compiler are you using - any clang/gcc should hit the HAVE_TYPEOF case.
Regardless, the patch is spot on:

Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

-Emil
___
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

Re: [PATCH xserver v2 1/4] os: move xf86PrivsElevated here

[Emil Velikov]

On 13 March 2018 at 21:46, Ben Crocker <bcroc...@redhat.com> wrote:

> --- a/include/os.h
> +++ b/include/os.h
> @@ -368,6 +368,9 @@ System(const char *cmdline);
>  #define Fclose(a) fclose(a)
>  #endif
>
> +extern _X_EXPORT Bool
> +PrivsElevated(void);
> +
Any particular reason why this is exported?
Is it simply mimicking the surrounding code, or there's a genuine reason for it?

Not an issue either way, the series is
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

Somewhat unrelated:
Seems like commit 49f77fff1495c0a2050fb18f9b1fc627839bbfc2 exported
1000+ symbols as part of (?) folding the extension modules in core.
Yet it never followed to hide the internal functions as things were done.

-Emil
___
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

Re: [PATCH xserver v2 3/4] xfree86: replace all uses of xf86PrivsElevated with PrivsElevated

[Emil Velikov]

On 13 March 2018 at 21:46, Ben Crocker  wrote:

> -extern _X_EXPORT Bool
> -xf86PrivsElevated(void);
>
FWIW, I cannot spot any external users of the symbol, so removing it
should be fine.

-Emil
___
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

[PATCH xserver] dri3: cap the version returned to the client

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

As per the protocol, the server should not return version greater than
the one supported by the client.

Add a spec quote and tweak the numbers accordingly.

Fixes: 563138298868 ("dri3: Add DRI3 extension")
Cc: Daniel Stone <dani...@collabora.com>
Cc: Keith Packard <kei...@keithp.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Note: we might want this in the stable releases?

Related: The DRI2 implementation has identical bug.
Although fixing that (unlike DRI3) might cause some ill written apps to
rightfully fall on their face.

Should we bother - yay, nay are appreciated.
---
 dri3/dri3_request.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/dri3/dri3_request.c b/dri3/dri3_request.c
index 7f3f0d08c..fc258711b 100644
--- a/dri3/dri3_request.c
+++ b/dri3/dri3_request.c
@@ -45,7 +45,19 @@ proc_dri3_query_version(ClientPtr client)
 };
 
 REQUEST_SIZE_MATCH(xDRI3QueryVersionReq);
-(void) stuff;
+/* From DRI3 proto:
+ *
+ * The client sends the highest supported version to the server
+ * and the server sends the highest version it supports, but no
+ * higher than the requested version.
+ */
+
+if (rep.majorVersion > stuff->majorVersion ||
+rep.minorVersion > stuff->minorVersion) {
+rep.majorVersion = stuff->majorVersion;
+rep.minorVersion = stuff->minorVersion;
+}
+
 if (client->swapped) {
 swaps();
 swapl();
-- 
2.16.0

___
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

Re: [PATCH xserver] meson: Require libdrm for dri1/2/3 when configured 'auto' as well as 'true'

[Emil Velikov]

On 8 March 2018 at 12:34, Jon Turney <jon.tur...@dronecode.org.uk> wrote:
> If dri1/2/3 are configured for auto-detection, libdrm is required, as well
> as the corresponding proto.  (Practically we will always have the
> corresponding protos now, as they are part of xorgproto).
>
> Signed-off-by: Jon Turney <jon.tur...@dronecode.org.uk>

Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

-Emil
___
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

Re: [PATCH:libX11] If XGetImage fails to create image, don't dereference it to bounds check

[Emil Velikov]

On 7 March 2018 at 20:10, Alan Coopersmith <alan.coopersm...@oracle.com> wrote:
> On 03/ 7/18 05:36 AM, Emil Velikov wrote:
>> Hi Alan,
>>
>> On 6 March 2018 at 21:47, Alan Coopersmith <alan.coopersm...@oracle.com> 
>> wrote:
>>> Reported by gcc 7.3:
>>>
>>> GetImage.c:110:25: warning: potential null pointer dereference 
>>> [-Wnull-dereference]
>>>   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
>>> ~^~~~
>>>
>>> Introduced by 8ea762f94f4c942d898fdeb590a1630c83235c17 in Xlib 1.6.4
>>>
>>> Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>
>>> ---
>>>  src/GetImage.c | 16 +---
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/GetImage.c b/src/GetImage.c
>>> index ff32d589..44a576a1 100644
>>> --- a/src/GetImage.c
>>> +++ b/src/GetImage.c
>>> @@ -105,14 +105,16 @@ XImage *XGetImage (
>>> planes = 1;
>>> }
>>>
>>> -   if (!image)
>>> +   if (!image) {
>>> Xfree(data);
>>> -   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
>>> -   INT_MAX / image->height <= image->bytes_per_line ||
>>> -   INT_MAX / planes <= image->height * image->bytes_per_line ||
>>> -   nbytes < planes * image->height * image->bytes_per_line) {
>>> -   XDestroyImage(image);
>>> -   image = NULL;
>>> +   } else {
>>> +if (planes < 1 || image->height < 1 || image->bytes_per_line < 
>>> 1 ||
>>> +INT_MAX / image->height <= image->bytes_per_line ||
>>> +INT_MAX / planes <= image->height * image->bytes_per_line 
>>> ||
>>> +nbytes < planes * image->height * image->bytes_per_line) {
>>> +XDestroyImage(image);
>>> +image = NULL;
>>> +}
>>
>> Instead of reshuffling, one could easily add the missing unlock/sync
>> in the error path.
>> Or even a goto unlock?
>
> Sorry - not sure I'm following - are you suggesting something like this?
>
> --- a/src/GetImage.c
> +++ b/src/GetImage.c
> @@ -104,17 +104,20 @@ XImage *XGetImage (
> _XGetScanlinePad(dpy, (int) rep.depth), 0);
> planes = 1;
> }
>
> -   if (!image)
> +   if (!image) {
> Xfree(data);
> +   goto unlock;
> +   }
> if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
> INT_MAX / image->height <= image->bytes_per_line ||
> INT_MAX / planes <= image->height * image->bytes_per_line ||
> nbytes < planes * image->height * image->bytes_per_line) {
> XDestroyImage(image);
> image = NULL;
> }
> +  unlock:
> UnlockDisplay(dpy);
> SyncHandle();
> return (image);
>  }
>
Yes, precisely.

>
> That should be effectively equivalent, just less change in indentation for
> the lines in between.
>
You're correct - it's functionally identical although it seems cleaner.

Regardless of which version you opt for
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

-Emil
___
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

[PATCH xserver 2/5] Remove always true GLAMOR_HAS_DRM_* guards

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

With earlier commit the required version was bumped to 2.4.89, thus the
guards always evaluate to true.

Fixes: e4e3447603b ("Add RandR leases with modesetting driver support
[v6]")
Cc: Keith Packard <kei...@keithp.com>
Cc: Daniel Stone <dani...@collabora.com>
Cc: Louis-Francis Ratté-Boulianne <l...@collabora.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Daniel, Louis,
Seems like the existing code has subtle bugs ... right?

Namely we have compile-time checks for atomic/modifiers where a runtime
ones must be used. See the XXX fragments in particular.

Note: apart from the usual "old build server, new runtime machine" this
suffers in the opposite direction "new build server + kernel not
supporting X at runtime"
---
 configure.ac | 11 ---
 glamor/glamor_egl.c  |  4 ---
 hw/xfree86/drivers/modesetting/driver.c  |  2 --
 hw/xfree86/drivers/modesetting/drmmode_display.c | 42 ++--
 hw/xfree86/drivers/modesetting/pageflip.c|  2 --
 hw/xfree86/drivers/modesetting/present.c |  5 ++-
 include/dix-config.h.in  |  9 -
 include/meson.build  |  6 
 8 files changed, 12 insertions(+), 69 deletions(-)

diff --git a/configure.ac b/configure.ac
index 14fe34995..e9a393856 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2107,17 +2107,6 @@ if test "x$GLAMOR" = xyes; then
AC_MSG_ERROR([Glamor for Xorg requires $LIBGBM])
fi
fi
-
-   PKG_CHECK_EXISTS(libdrm >= 2.4.62,
-[AC_DEFINE(GLAMOR_HAS_DRM_ATOMIC, 1, [libdrm supports 
atomic API])],
-[])
-   PKG_CHECK_EXISTS(libdrm >= 2.4.74,
-[AC_DEFINE(GLAMOR_HAS_DRM_NAME_FROM_FD_2, 1, [Have 
GLAMOR_HAS_DRM_NAME_FROM_FD_2])],
-[])
-
-   PKG_CHECK_EXISTS(libdrm >= 2.4.83,
-[AC_DEFINE(GLAMOR_HAS_DRM_MODIFIERS, 1, [Have 
GLAMOR_HAS_DRM_MODIFIERS])],
-[])
 fi
 AM_CONDITIONAL([GLAMOR_EGL], [test "x$GBM" = xyes])
 
diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
index 8389d5f29..4eb66cb9f 100644
--- a/glamor/glamor_egl.c
+++ b/glamor/glamor_egl.c
@@ -799,11 +799,7 @@ glamor_egl_screen_init(ScreenPtr screen, struct 
glamor_context *glamor_ctx)
 /* To do DRI3 device FD generation, we need to open a new fd
  * to the same device we were handed in originally.
  */
-#ifdef GLAMOR_HAS_DRM_NAME_FROM_FD_2
 glamor_egl->device_path = drmGetDeviceNameFromFd2(glamor_egl->fd);
-#else
-glamor_egl->device_path = drmGetDeviceNameFromFd(glamor_egl->fd);
-#endif
 
 if (!dri3_screen_init(screen, _dri3_info)) {
 xf86DrvMsg(scrn->scrnIndex, X_ERROR,
diff --git a/hw/xfree86/drivers/modesetting/driver.c 
b/hw/xfree86/drivers/modesetting/driver.c
index f20284bb0..2702a2eb2 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -1018,11 +1018,9 @@ PreInit(ScrnInfoPtr pScrn, int flags)
 }
 #endif
 
-#ifdef GLAMOR_HAS_DRM_ATOMIC
 ret = drmSetClientCap(ms->fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
 ret |= drmSetClientCap(ms->fd, DRM_CLIENT_CAP_ATOMIC, 1);
 ms->atomic_modeset = (ret == 0);
-#endif
 
 if (drmmode_pre_init(pScrn, >drmmode, pScrn->bitsPerPixel / 8) == 
FALSE) {
 xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "KMS setup failed\n");
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 1027e637a..e0a982ac6 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -58,8 +58,6 @@ static PixmapPtr drmmode_create_pixmap_header(ScreenPtr 
pScreen, int width, int
   int depth, int bitsPerPixel, int 
devKind,
   void *pPixData);
 
-#ifdef GLAMOR_HAS_DRM_MODIFIERS
-
 static inline uint32_t *
 formats_ptr(struct drm_format_modifier_blob *blob)
 {
@@ -72,8 +70,6 @@ modifiers_ptr(struct drm_format_modifier_blob *blob)
 return (struct drm_format_modifier *)(((char *)blob) + 
blob->modifiers_offset);
 }
 
-#endif
-
 Bool
 drmmode_is_format_supported(ScrnInfoPtr scrn, uint32_t format, uint64_t 
modifier)
 {
@@ -392,7 +388,6 @@ drmmode_prop_info_free(drmmode_prop_info_ptr info, int 
num_props)
 free(info[i].enum_values);
 }
 
-#ifdef GLAMOR_HAS_DRM_ATOMIC
 static int
 plane_add_prop(drmModeAtomicReq *req, drmmode_crtc_private_ptr drmmode_crtc,
enum drmmode_plane_property prop, uint64_t val)
@@ -480,7 +475,6 @@ drm_mode_destroy(xf86CrtcPtr crtc, drmmode_mode_ptr mode)
 xorg_list_de

[PATCH xserver 3/5] modesetting: remove always true defined(DRM_CAP_PRIME) guards

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

The macro was available in libdrm for ages. Furthermore having a guard
like this is a very bad idea.

Building on an old server will result in a missing run-time functionality.
Since it's UABI one can use a local fallback, old kernels will return
-EINVAL and the fallback path will kick in.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 hw/xfree86/drivers/modesetting/driver.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/driver.c 
b/hw/xfree86/drivers/modesetting/driver.c
index 2702a2eb2..8c5a2635f 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -227,7 +227,7 @@ check_outputs(int fd, int *count)
 *count = res->count_connectors;
 
 ret = res->count_connectors > 0;
-#if defined(DRM_CAP_PRIME) && defined(GLAMOR_HAS_GBM_LINEAR)
+#if defined(GLAMOR_HAS_GBM_LINEAR)
 if (ret == FALSE) {
 uint64_t value = 0;
 if (drmGetCap(fd, DRM_CAP_PRIME, ) == 0 &&
@@ -1003,7 +1003,6 @@ PreInit(ScrnInfoPtr pScrn, int flags)
 xf86ReturnOptValBool(ms->drmmode.Options, OPTION_PAGEFLIP, TRUE);
 
 pScrn->capabilities = 0;
-#ifdef DRM_CAP_PRIME
 ret = drmGetCap(ms->fd, DRM_CAP_PRIME, );
 if (ret == 0) {
 if (connector_count && (value & DRM_PRIME_CAP_IMPORT)) {
@@ -1016,7 +1015,6 @@ PreInit(ScrnInfoPtr pScrn, int flags)
 pScrn->capabilities |= RR_Capability_SourceOutput | 
RR_Capability_SourceOffload;
 #endif
 }
-#endif
 
 ret = drmSetClientCap(ms->fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
 ret |= drmSetClientCap(ms->fd, DRM_CLIENT_CAP_ATOMIC, 1);
-- 
2.16.0

___
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

[PATCH xserver 5/5] modesetting: remove fallback DRM_CAP_* defines

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

All the macros are available in the libdrm that we depend on.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 hw/xfree86/drivers/modesetting/driver.c  | 8 
 hw/xfree86/drivers/modesetting/drmmode_display.h | 7 ---
 2 files changed, 15 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/driver.c 
b/hw/xfree86/drivers/modesetting/driver.c
index 8c5a2635f..2f9e3ac5c 100644
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -794,14 +794,6 @@ msShouldDoubleShadow(ScrnInfoPtr pScrn, modesettingPtr ms)
 return ret;
 }
 
-#ifndef DRM_CAP_CURSOR_WIDTH
-#define DRM_CAP_CURSOR_WIDTH 0x8
-#endif
-
-#ifndef DRM_CAP_CURSOR_HEIGHT
-#define DRM_CAP_CURSOR_HEIGHT 0x9
-#endif
-
 static Bool
 ms_get_drm_master_fd(ScrnInfoPtr pScrn)
 {
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h 
b/hw/xfree86/drivers/modesetting/drmmode_display.h
index ee59711cb..d2dc7c225 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.h
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.h
@@ -285,11 +285,4 @@ void drmmode_copy_fb(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode);
 int drmmode_crtc_set_fb(xf86CrtcPtr crtc, DisplayModePtr mode, uint32_t fb_id,
 int x, int y, uint32_t flags, void *data);
 
-#ifndef DRM_CAP_DUMB_PREFERRED_DEPTH
-#define DRM_CAP_DUMB_PREFERRED_DEPTH 3
-#endif
-#ifndef DRM_CAP_DUMB_PREFER_SHADOW
-#define DRM_CAP_DUMB_PREFER_SHADOW 4
-#endif
-
 #endif
-- 
2.16.0

___
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

[PATCH xserver 4/5] modesetting: remove always true DRM_IOCTL_CRTC_QUEUE_SEQUENCE guard

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

We already require libdrm 2.4.89 which provides the definition plus
guarding kernel UABI like that is generally a bad idea.

See previous commit for details why :-)

Cc: Keith Packard <kei...@keithp.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 hw/xfree86/drivers/modesetting/vblank.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/vblank.c 
b/hw/xfree86/drivers/modesetting/vblank.c
index 1d331ccdb..ae3018b4b 100644
--- a/hw/xfree86/drivers/modesetting/vblank.c
+++ b/hw/xfree86/drivers/modesetting/vblank.c
@@ -182,7 +182,6 @@ ms_get_kernel_ust_msc(xf86CrtcPtr crtc,
 drmVBlank vbl;
 int ret;
 
-#ifdef DRM_IOCTL_CRTC_QUEUE_SEQUENCE
 if (ms->has_queue_sequence || !ms->tried_queue_sequence) {
 uint64_t ns;
 ms->tried_queue_sequence = TRUE;
@@ -196,7 +195,6 @@ ms_get_kernel_ust_msc(xf86CrtcPtr crtc,
 return ret == 0;
 }
 }
-#endif
 /* Get current count */
 vbl.request.type = DRM_VBLANK_RELATIVE | drmmode_crtc->vblank_pipe;
 vbl.request.sequence = 0;
@@ -226,7 +224,6 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags,
 
 for (;;) {
 /* Queue an event at the specified sequence */
-#ifdef DRM_IOCTL_CRTC_QUEUE_SEQUENCE
 if (ms->has_queue_sequence || !ms->tried_queue_sequence) {
 uint32_t drm_flags = 0;
 uint64_t kernel;
@@ -255,7 +252,6 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags,
 goto check;
 }
 }
-#endif
 vbl.request.type = DRM_VBLANK_EVENT | drmmode_crtc->vblank_pipe;
 if (flags & MS_QUEUE_RELATIVE)
 vbl.request.type |= DRM_VBLANK_RELATIVE;
@@ -273,9 +269,7 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags,
 *msc_queued = ms_kernel_msc_to_crtc_msc(crtc, 
vbl.reply.sequence);
 return TRUE;
 }
-#ifdef DRM_IOCTL_CRTC_QUEUE_SEQUENCE
 check:
-#endif
 if (errno != EBUSY) {
 ms_drm_abort_seq(scrn, seq);
 return FALSE;
-- 
2.16.0

___
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

[PATCH xserver 1/5] configure: remove libdrm version check

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

We already require said version.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 configure.ac | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index f82c0a66a..14fe34995 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1999,8 +1999,7 @@ if test "x$XORG" = xyes; then
fi
 
if test "x$DRM" = xyes; then
-   dnl 2.4.46 is required for cursor hotspot support.
-   PKG_CHECK_EXISTS(libdrm >= 2.4.46, XORG_DRIVER_MODESETTING=yes, 
XORG_DRIVER_MODESETTING=no)
+   XORG_DRIVER_MODESETTING=yes
fi
 
AC_SUBST([XORG_LIBS])
-- 
2.16.0

___
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

[PATCH xserver 1/5] configure: remove libdrm version check

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

We already require said version.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 configure.ac | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index f82c0a66a..14fe34995 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1999,8 +1999,7 @@ if test "x$XORG" = xyes; then
fi
 
if test "x$DRM" = xyes; then
-   dnl 2.4.46 is required for cursor hotspot support.
-   PKG_CHECK_EXISTS(libdrm >= 2.4.46, XORG_DRIVER_MODESETTING=yes, 
XORG_DRIVER_MODESETTING=no)
+   XORG_DRIVER_MODESETTING=yes
fi
 
AC_SUBST([XORG_LIBS])
-- 
2.16.0

___
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

Re: [PATCH:libX11] If XGetImage fails to create image, don't dereference it to bounds check

[Emil Velikov]

Hi Alan,

On 6 March 2018 at 21:47, Alan Coopersmith  wrote:
> Reported by gcc 7.3:
>
> GetImage.c:110:25: warning: potential null pointer dereference 
> [-Wnull-dereference]
>   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
> ~^~~~
>
> Introduced by 8ea762f94f4c942d898fdeb590a1630c83235c17 in Xlib 1.6.4
>
> Signed-off-by: Alan Coopersmith 
> ---
>  src/GetImage.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/src/GetImage.c b/src/GetImage.c
> index ff32d589..44a576a1 100644
> --- a/src/GetImage.c
> +++ b/src/GetImage.c
> @@ -105,14 +105,16 @@ XImage *XGetImage (
> planes = 1;
> }
>
> -   if (!image)
> +   if (!image) {
> Xfree(data);
> -   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
> -   INT_MAX / image->height <= image->bytes_per_line ||
> -   INT_MAX / planes <= image->height * image->bytes_per_line ||
> -   nbytes < planes * image->height * image->bytes_per_line) {
> -   XDestroyImage(image);
> -   image = NULL;
> +   } else {
> +if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 
> ||
> +INT_MAX / image->height <= image->bytes_per_line ||
> +INT_MAX / planes <= image->height * image->bytes_per_line ||
> +nbytes < planes * image->height * image->bytes_per_line) {
> +XDestroyImage(image);
> +image = NULL;
> +}

Instead of reshuffling, one could easily add the missing unlock/sync
in the error path.
Or even a goto unlock?

-Emil
___
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

Re: [PATCH xserver] Require libdrm 2.4.89 or newer

[Emil Velikov]

On 3 March 2018 at 01:05, Keith Packard  wrote:
> Both autotools and meson build systems had complicated logic around
> what version of libdrm to require for various options. Remove that and
> just check for a new enough version to support all of the options
> which need libdrm.
>
> Signed-off-by: Keith Packard 
> ---
>  configure.ac |  7 +--
>  meson.build  | 12 ++--
>  2 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 439d42390..0ba7550a1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1281,12 +1281,7 @@ AM_CONDITIONAL(DRI3, test "x$DRI3" = xyes)
>  if test "x$DRI" = xyes || test "x$DRI2" = xyes || test "x$DRI3" = xyes || 
> test "x$CONFIG_UDEV_KMS" = xyes; then
> if test "x$DRM" = xyes; then
> AC_DEFINE(WITH_LIBDRM, 1, [Building with libdrm support])
> -   if test "x$DRI2" = xyes; then
> -   dnl 2.4.65 is required for drmGetDevice
> -   PKG_CHECK_MODULES([LIBDRM], libdrm >= 2.4.65)
> -   else
> -   PKG_CHECK_MODULES([LIBDRM], $LIBDRM)
> -   fi
> +   PKG_CHECK_MODULES([LIBDRM], $LIBDRM)
Seems like you missed the version here. LIBDRM is some very ancient
version, just enough for kdriver/xwayland.

-Emil
___
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

Re: [PATCH util/modular] xorg.modules: Replace individual proto modules with xorgproto

[Emil Velikov]

On 1 March 2018 at 13:35, Jon Turney <jon.tur...@dronecode.org.uk> wrote:
> On 28/02/2018 15:17, Emil Velikov wrote:
>>
>> On 28 February 2018 at 13:04, Jon Turney <jon.tur...@dronecode.org.uk>
>> wrote:
>>>
>>> Signed-off-by: Jon Turney <jon.tur...@dronecode.org.uk>
>>> ---
>>>   xorg.modules | 845
>>> ++-
>>>   1 file changed, 143 insertions(+), 702 deletions(-)
>>>
>>> diff --git a/xorg.modules b/xorg.modules
>>> index b34d14d..93bc8cf 100644
>>> --- a/xorg.modules
>>> +++ b/xorg.modules
>>> @@ -65,35 +65,7 @@
>>> 
>>> 
>>>   
>>
>>
>>> -  
>>
>> Should stay?
>
>
> Oops.
>
>>
>>> +  
>>>   
>>> 
>>>
>>> @@ -366,257 +338,13 @@
>>
>>
>>
>>> +  
>>
>> I'd say keep this as autotools and toggle as 2/2...
>>
>>> +>> +checkoutdir="xorg/proto/xorgproto"/>
>>>   
>>> -  
>>> -
>>> -
>>> 
>>> -
>>
>> .. hence this hunk will stay as-is and drop util-macros with 2/2?
>
>
> I don't really see using a meson as a change as this module didn't exists
> before, but I take your point that it reduces the diff.
>
>>
>> It's fairly fiddly to have the above subtleties within such a massive
>> patch.
>>
>> With the above, tweaks:
>> Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
>
>
> Updated patch attached.
>
The comments are addressed. I trust you haven't changed anything else ;-)
Fwiw my r-b still stands.

-Emil
___
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

Re: [PATCH util/modular] xorg.modules: Replace individual proto modules with xorgproto

[Emil Velikov]

On 28 February 2018 at 13:04, Jon Turney <jon.tur...@dronecode.org.uk> wrote:
> Signed-off-by: Jon Turney <jon.tur...@dronecode.org.uk>
> ---
>  xorg.modules | 845 
> ++-
>  1 file changed, 143 insertions(+), 702 deletions(-)
>
> diff --git a/xorg.modules b/xorg.modules
> index b34d14d..93bc8cf 100644
> --- a/xorg.modules
> +++ b/xorg.modules
> @@ -65,35 +65,7 @@
>
>
>  

> -  
Should stay?

> +  
>  
>
>
> @@ -366,257 +338,13 @@


> +  
I'd say keep this as autotools and toggle as 2/2...

> + +checkoutdir="xorg/proto/xorgproto"/>
>  
> -  
> -
> -
>
> -
.. hence this hunk will stay as-is and drop util-macros with 2/2?

It's fairly fiddly to have the above subtleties within such a massive patch.

With the above, tweaks:
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

-Emil
___
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

[PATCH xserver] protocol.txt: add GLX req. 35 - SetClientInfo2ARB

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Noticed while skimming for the typo'd version ;-)

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 dix/protocol.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/dix/protocol.txt b/dix/protocol.txt
index 244556a08..a1bf93652 100644
--- a/dix/protocol.txt
+++ b/dix/protocol.txt
@@ -149,6 +149,7 @@ R031 GLX:CreateWindow
 R032 GLX:DeleteWindow
 R033 GLX:SetClientInfoARB
 R034 GLX:CreateContextAttribsARB
+R035 GLX:SetClientInfo2ARB
 R101 GLX:NewList
 R102 GLX:EndList
 R103 GLX:DeleteLists
-- 
2.16.0

___
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

Re: [PATCH util/modular v2] release.sh: Add support for mesa-demos

[Emil Velikov]

On 23 February 2018 at 14:13, Andreas Boll  wrote:
> v2: Rebase on Mesa cleanup.
> Move demos into its own elif statement.
>
> Signed-off-by: Andreas Boll 
> ---
> Rebased on https://patchwork.freedesktop.org/patch/206466/
>
>  release.sh | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
Thanks for the re-spin. Reviewed and pushed both patches to master.

-Emil
___
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

[PATCH util-modular] release.sh: remove workaround for early Mesa versions

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 release.sh | 14 --
 1 file changed, 14 deletions(-)

diff --git a/release.sh b/release.sh
index ff89d2e..99bd0c3 100755
--- a/release.sh
+++ b/release.sh
@@ -584,24 +584,10 @@ process_module() {
 list_cc=$list_dri_devel
 elif [ x"$section" = xmesa ]; then
 host_current=$host_mesa
-mesa_version=`echo $pkg_version | sed 's:-rc.*::'`
 section_path=archive
 srv_path="/srv/$host_current/www/$section_path"
 list_to=$list_mesa_announce
 list_cc=$list_mesa_devel
-
-# Prior to 17.0.x Mesa uses separate folder for each release
-if test `echo $mesa_version | cut -d'.' -f1` -lt 17; then
-section_path=$section_path/$mesa_version
-srv_path="/srv/$host_current/www/$section_path"
-echo "Info: creating mesa directory on web server:"
-ssh $USER_NAME$hostname mkdir -p $srv_path  &>/dev/null
-if [ $? -ne 0 ]; then
-echo "Error: cannot create the path \"$srv_path\" on the web 
server."
-cd $top_src
-return 1
-fi
-fi
 fi
 
 # Module xkeyboard-config goes in a subdir of the xorg "data" section
-- 
2.16.0

___
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

Re: [PATCH util/modular] release.sh: Add support for mesa-demos

[Emil Velikov]

On 23 February 2018 at 09:40, Andreas Boll  wrote:
> Signed-off-by: Andreas Boll 
> ---
>  release.sh | 34 ++
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/release.sh b/release.sh
> index ff89d2e..2045197 100755
> --- a/release.sh
> +++ b/release.sh
> @@ -264,8 +264,10 @@ get_section() {
> if [ $? -ne 0 ]; then
> echo "Error: unable to extract section from $module_url second 
> field."
> return 1
> -   elif [ x"$section" != xdrm ] && [ x"$section" != xmesa ]; then
> -   echo "Error: section $section is not supported, only libdrm and 
> mesa are."
> +   elif [ x"$section" != xdrm ] &&
> +[ x"$section" != xmesa ] &&
> +[ x"$section" != xdemos ]; then
> +   echo "Error: section $section is not supported, only libdrm, mesa 
> and demos are."
> return 1
> fi
>  fi
> @@ -582,7 +584,8 @@ process_module() {
>  section_path=libdrm
>  srv_path="/srv/$host_current/www/$section_path"
>  list_cc=$list_dri_devel
> -elif [ x"$section" = xmesa ]; then
> +elif [ x"$section" = xmesa ] ||
> + [ x"$section" = xdemos ]; then
>  host_current=$host_mesa
>  mesa_version=`echo $pkg_version | sed 's:-rc.*::'`
>  section_path=archive
> @@ -590,16 +593,23 @@ process_module() {
>  list_to=$list_mesa_announce
>  list_cc=$list_mesa_devel
>
> -# Prior to 17.0.x Mesa uses separate folder for each release
> -if test `echo $mesa_version | cut -d'.' -f1` -lt 17; then
> -section_path=$section_path/$mesa_version
> +if [ x"$section" = xdemos ]; then
> +section_path=$section_path/$section
>  srv_path="/srv/$host_current/www/$section_path"
Let's keep demos as a separate if statement. As-is it gets a bit fiddly to read.

> -echo "Info: creating mesa directory on web server:"
> -ssh $USER_NAME$hostname mkdir -p $srv_path  &>/dev/null
> -if [ $? -ne 0 ]; then
> -echo "Error: cannot create the path \"$srv_path\" on the web 
> server."
> -cd $top_src
> -return 1
> +fi
> +
> +# Prior to 17.0.x Mesa uses separate folder for each release
> +if [ x"$section" = xmesa ]; then
> +if test `echo $mesa_version | cut -d'.' -f1` -lt 17; then
> +section_path=$section_path/$mesa_version
> +srv_path="/srv/$host_current/www/$section_path"
> +echo "Info: creating mesa directory on web server:"
> +ssh $USER_NAME$hostname mkdir -p $srv_path  &>/dev/null
> +if [ $? -ne 0 ]; then
> +echo "Error: cannot create the path \"$srv_path\" on the 
> web server."
> +cd $top_src
> +return 1
> +fi
Pretty sure this section can go now. I'll send a patch in a second.

-Emil
___
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

Re: [PATCH xserver 6/6] mi: Mention extension loading in verbose logs

[Emil Velikov]

On 21 February 2018 at 16:42, Adam Jackson <a...@nwnk.net> wrote:
> On Mon, 2018-02-19 at 15:18 +0000, Emil Velikov wrote:
>> From: Emil Velikov <emil.veli...@collabora.com>
>>
>> Listing the extensions is useful, despite being annoying for normal
>> usecases. Print it only when extra (lvl 3) vebose is requested.
>
> I appreciate lowering the log verbosity, but this repeats both of the
> problems I mentioned when I took this printf out the first time: "a)
> we're printing a line for every extension, whether it's enabled or
> not, and b) we're not actually initializing the extension at this
> point." (8468e2443)
>
> Put it just before the call to ->initFunc in InitExtensions to fix both
> issues.
>
Ack. Patch updated [1] with a copy in your inbox, in case it helps.


> Merged 1-5, thanks:
>
Thanks
Emil

[1] https://patchwork.freedesktop.org/patch/205981/
___
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

[PATCH xserver] mi: Mention extension loading in verbose logs

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Listing the extensions is useful, despite being annoying for normal
usecases. Print it only when extra (lvl 3) vebose is requested.

v2: Move the logging to InitExtensions(), as requested by Adam.

Cc: Adam Jackson <a...@nwnk.net>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 mi/miinitext.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/mi/miinitext.c b/mi/miinitext.c
index e55073bf3..5596e212f 100644
--- a/mi/miinitext.c
+++ b/mi/miinitext.c
@@ -104,6 +104,7 @@ SOFTWARE.
 #include "nonsdk_extinit.h"
 #endif
 #include "micmap.h"
+#include "os.h"
 #include "globals.h"
 
 /* List of built-in (statically linked) extensions */
@@ -260,6 +261,9 @@ InitExtensions(int argc, char *argv[])
 ext = [i];
 if (ext->initFunc != NULL &&
 (ext->disablePtr == NULL || !*ext->disablePtr)) {
+LogMessageVerb(X_INFO, 3, "Initializing extension %s\n",
+   ext->name);
+
 (ext->initFunc) ();
 }
 }
-- 
2.16.0

___
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

[PATCH xserver 6/6] mi: Mention extension loading in verbose logs

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Listing the extensions is useful, despite being annoying for normal
usecases. Print it only when extra (lvl 3) vebose is requested.

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 mi/miinitext.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/mi/miinitext.c b/mi/miinitext.c
index e55073bf3..abe3fce8c 100644
--- a/mi/miinitext.c
+++ b/mi/miinitext.c
@@ -104,6 +104,7 @@ SOFTWARE.
 #include "nonsdk_extinit.h"
 #endif
 #include "micmap.h"
+#include "os.h"
 #include "globals.h"
 
 /* List of built-in (statically linked) extensions */
@@ -302,6 +303,9 @@ LoadExtensionList(const ExtensionModule ext[], int size, 
Bool builtin)
 return;
 
 for (i = 0; i < size; i++, newext++) {
+LogMessageVerb(X_INFO, 3, "Loading%s extension %s\n", builtin ?
+   " built-in" : "", ext[i].name);
+
 newext->name = ext[i].name;
 newext->initFunc = ext[i].initFunc;
 newext->disablePtr = ext[i].disablePtr;
-- 
2.16.0

___
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

[PATCH xserver 1/6] glx: keep glvnd_vendor a private [static] variable

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
 glx/glxext.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/glx/glxext.c b/glx/glxext.c
index a51c13ff1..37416a4e4 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -497,11 +497,11 @@ xorgGlxServerPreInit(const ExtensionEntry *extEntry)
 return glxGeneration == serverGeneration;
 }
 
-static GlxServerVendor *glvnd_vendor = NULL;
-
 static GlxServerVendor *
 xorgGlxInitGLVNDVendor(void)
 {
+static GlxServerVendor *glvnd_vendor = NULL;
+
 if (glvnd_vendor == NULL) {
 GlxServerImports *imports = NULL;
 imports = glxServer.allocateServerImports();
@@ -521,6 +521,7 @@ xorgGlxInitGLVNDVendor(void)
 static void
 xorgGlxServerInit(CallbackListPtr *pcbl, void *param, void *ext)
 {
+GlxServerVendor *glvnd_vendor;
 const ExtensionEntry *extEntry = ext;
 int i;
 
@@ -528,7 +529,8 @@ xorgGlxServerInit(CallbackListPtr *pcbl, void *param, void 
*ext)
 return;
 }
 
-if (!xorgGlxInitGLVNDVendor()) {
+glvnd_vendor = xorgGlxInitGLVNDVendor();
+if (!glvnd_vendor) {
 return;
 }
 
-- 
2.16.0

___
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