Re: [PATCH v2 1/3] OvmfPkg: VMWare SVGA2 display device register definitions

[Laszlo Ersek <lersek@redhat.com>]

On 04/04/17 01:17, Jordan Justen wrote:
> On 2017-04-02 15:44:55, Phil Dennis-Jordan wrote:
>> From: Phil Dennis-Jordan <phil@philjordan.eu>
>>
>> This adds a header file defining symbolic constants for the VMWare SVGA2
>> virtual display device in preparation for supporting it in QemuVideoDXE.
>>
>> It is an extract of the file lib/vmware/svga_reg.h from commit
>> 329dd537456f93a806841ec8a8213aed11395def of VMWare's vmware-svga
>> repository at git://git.code.sf.net/p/vmware-svga/git (See also
>> http://vmware-svga.sourceforge.net/ )
>>
>> Only the bare essentials necessary for initialisation, modesetting and
>> framebuffer access have been kept from the original file.
>>
>> The original file was released by VMWare under the MIT license, this
>> has been retained.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> ---
>>
>> Notes:
>>     v2:
>>     - New, custom header file instead of importing VMWare's verbatim. [Laszlo]
>>
>>  OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h | 102 ++++++++++++++++++++
>>  1 file changed, 102 insertions(+)
>>
>> diff --git a/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h b/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h
> 
> I think EDK II's file naming convention would prefer VmwareSvga2.h, or
> possibly VmWareSvga2.h. (But, the former looks better to me.)
> 
> Since it covers svga and svga2, should we just drop the '2' from the
> filename?

Yeah, I suggested sort of the same things, after you -- I reviewed the
patches without reading your (earlier) feedback. I suggested VMW_ and
Vmw as prefixes, but I'm equally fine with the more verbose VMWARE_ and
Vmware.

I also agree with dropping "2" from file names / identifiers / macros
etc. (Or, at least, we should be consistent about the use of "2".)


> 
>> new file mode 100644
>> index 000000000000..9db553155957
>> --- /dev/null
>> +++ b/OvmfPkg/Include/IndustryStandard/VMWareSVGA2.h
>> @@ -0,0 +1,102 @@
>> +/** @file
>> +
>> +  Macro and enum definitions of a subset of port numbers, register identifiers
>> +  and values required for driving the VMWare SVGA2 virtual display adapter,
>> +  also implemented by Qemu.
>> +
>> +  This file's contents was extracted from file lib/vmware/svga_reg.h in commit
>> +  329dd537456f93a806841ec8a8213aed11395def of VMWare's vmware-svga repository:
>> +  git://git.code.sf.net/p/vmware-svga/git
>> +
>> +
>> +  Copyright 1998-2009 VMware, Inc.  All rights reserved.
>> +  Portions Copyright 2017 Phil Dennis-Jordan <phil@philjordan.eu>
>> +
>> +  Permission is hereby granted, free of charge, to any person
>> +  obtaining a copy of this software and associated documentation
>> +  files (the "Software"), to deal in the Software without
>> +  restriction, including without limitation the rights to use, copy,
>> +  modify, merge, publish, distribute, sublicense, and/or sell copies
>> +  of the Software, and to permit persons to whom the Software is
>> +  furnished to do so, subject to the following conditions:
>> +
>> +  The above copyright notice and this permission notice shall be
>> +  included in all copies or substantial portions of the Software.
>> +
>> +  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> +  EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> +  MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> +  NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
>> +  BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>> +  ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
>> +  CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>> +  SOFTWARE.
>> +
>> +**/
>> +
>> +#ifndef _VMWARE_SVGA2_H_
>> +#define _VMWARE_SVGA2_H_
>> +
>> +//
>> +// IDs for recognising the device
>> +//
>> +#define PCI_VENDOR_ID_VMWARE            0x15AD
>> +#define PCI_DEVICE_ID_VMWARE_SVGA2      0x0405
>> +
>> +//
>> +// I/O port BAR offsets for register selection and read/write.
>> +//
>> +// The register index is written to the 32-bit index port, followed by a 32-bit
>> +// read or write on the value port to read or set that register's contents.
>> +//
>> +#define SVGA_INDEX_PORT         0x0
>> +#define SVGA_VALUE_PORT         0x1
> 
> Thanks for taking Laszlo's advice about pulling out the minimal
> definitions and EDK II-ifying it. (I think he may be right about the
> Xen contribution.)
> 
> One request I have is, how about prefixing all the SVGA items as
> VMWARE_SVGA? SVGA seems to generic of a term here.

+1 (as commented elsewhere)

Thanks!
Laszlo

> 
> Thanks,
> 
> -Jordan
> 
>> +
>> +//
>> +// Some of the device's register indices for basic framebuffer functionality.
>> +//
>> +enum {
>> +  SVGA_REG_ID = 0,
>> +  SVGA_REG_ENABLE = 1,
>> +  SVGA_REG_WIDTH = 2,
>> +  SVGA_REG_HEIGHT = 3,
>> +  SVGA_REG_MAX_WIDTH = 4,
>> +  SVGA_REG_MAX_HEIGHT = 5,
>> +
>> +  SVGA_REG_BITS_PER_PIXEL = 7,
>> +
>> +  SVGA_REG_RED_MASK = 9,
>> +  SVGA_REG_GREEN_MASK = 10,
>> +  SVGA_REG_BLUE_MASK = 11,
>> +  SVGA_REG_BYTES_PER_LINE = 12,
>> +
>> +  SVGA_REG_FB_OFFSET = 14,
>> +
>> +  SVGA_REG_FB_SIZE = 16,
>> +  SVGA_REG_CAPABILITIES = 17,
>> +
>> +  SVGA_REG_HOST_BITS_PER_PIXEL = 28,
>> +};
>> +
>> +//
>> +// Values used with SVGA_REG_ID for sanity-checking the device and getting
>> +// its version.
>> +//
>> +#define SVGA_MAGIC         0x900000UL
>> +#define SVGA_MAKE_ID(ver)  (SVGA_MAGIC << 8 | (ver))
>> +
>> +#define SVGA_VERSION_2     2
>> +#define SVGA_ID_2          SVGA_MAKE_ID(SVGA_VERSION_2)
>> +
>> +#define SVGA_VERSION_1     1
>> +#define SVGA_ID_1          SVGA_MAKE_ID(SVGA_VERSION_1)
>> +
>> +#define SVGA_VERSION_0     0
>> +#define SVGA_ID_0          SVGA_MAKE_ID(SVGA_VERSION_0)
>> +
>> +//
>> +// One of the capability bits advertised by SVGA_REG_CAPABILITIES.
>> +//
>> +#define SVGA_CAP_8BIT_EMULATION     0x00000100
>> +
>> +#endif
>> -- 
>> 2.3.2 (Apple Git-55)
>>