Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

2020-07-21 Thread Thomas Zimmermann
Hi

Am 20.07.20 um 21:18 schrieb Lyude Paul:
> On Mon, 2020-07-20 at 09:04 +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 17.07.20 um 00:43 schrieb Lyude Paul:
>>> On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
 This patch adds support for G200 desktop cards. We can reuse the whole
 memory and modesetting code. A few PCI and DAC register values have to
 be updated accordingly.

 The most significant change is in the PLL setup. The get the clock
 limits
 and reference clocks, parses the device's BIOS. With no BIOS found, safe
 defaults are being used.

 Signed-off-by: Thomas Zimmermann 
 Co-developed-by: Egbert Eich 
 Signed-off-by: Egbert Eich 
 Co-developed-by: Takashi Iwai 
 Signed-off-by: Takashi Iwai 
 ---
  MAINTAINERS|   2 +-
  drivers/gpu/drm/mgag200/Kconfig|  12 +--
  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 -
  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 
  5 files changed, 220 insertions(+), 9 deletions(-)

 diff --git a/MAINTAINERS b/MAINTAINERS
 index 415954a98934..4c6f96e2b79b 100644
 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -5406,7 +5406,7 @@ S:   Orphan / Obsolete
  F:drivers/gpu/drm/mga/
  F:include/uapi/drm/mga_drm.h
  
 -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
 +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
  M:Dave Airlie 
  S:Odd Fixes
  F:drivers/gpu/drm/mgag200/
 diff --git a/drivers/gpu/drm/mgag200/Kconfig
 b/drivers/gpu/drm/mgag200/Kconfig
 index 93be766715c9..eec59658a938 100644
 --- a/drivers/gpu/drm/mgag200/Kconfig
 +++ b/drivers/gpu/drm/mgag200/Kconfig
 @@ -1,13 +1,11 @@
  # SPDX-License-Identifier: GPL-2.0-only
  config DRM_MGAG200
 -  tristate "Kernel modesetting driver for MGA G200 server engines"
 +  tristate "Matrox G200"
depends on DRM && PCI && MMU
select DRM_GEM_SHMEM_HELPER
select DRM_KMS_HELPER
help
 -   This is a KMS driver for the MGA G200 server chips, it
 -   does not support the original MGA G200 or any of the desktop
 -   chips. It requires 0.3.0 of the modesetting userspace driver,
 -   and a version of mga driver that will fail on KMS enabled
 -   devices.
 -
 +   This is a KMS driver for Matrox G200 chips. It supports the original
 +   MGA G200 desktop chips and the server variants. It requires 0.3.0
 +   of the modesetting userspace driver, and a version of mga driver
 +   that will fail on KMS enabled devices.
 diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
 b/drivers/gpu/drm/mgag200/mgag200_drv.c
 index f7652e16365c..419817d6e2cd 100644
 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
 +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
 @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
u8 crtcext3;
  
switch (mdev->type) {
 +  case G200_PCI:
 +  case G200_AGP:
 +  if (mgag200_has_sgram(mdev))
 +  option = 0x4049cd21;
 +  else
 +  option = 0x40499121;
 +  option2 = 0x8000;
 +  break;
case G200_SE_A:
case G200_SE_B:
if (mgag200_has_sgram(mdev))
 @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device
 *mdev)
return 0;
  }
  
 +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
 +  unsigned char __iomem *bios,
 +  size_t size)
 +{
 +  static const unsigned int expected_length[6] = {
 +  0, 64, 64, 64, 128, 128
 +  };
 +
 +  struct drm_device *dev = >base;
 +  unsigned char __iomem *pins;
>>>
>>> huh, never realized you could write directly to unsigned char __iomem
>>> pointers!
>>
>> I took the patch as-is, but this probably wouldn't work on all
>> architectures.
> 
> Something occurred to me just now - do we actually care? I don't think I've
> ever seen mgag200 on anything other then x86_64 systems

That's no big deal. As Sam mentioned, it fixes some warnings and does
not cause overhead. Besides, we have a bug in fbdev that is caused by
direct I/O within framebuffer memory. So I'll better do it right here.

Wrt architecture, the MGAs have a PowerPC mode where they interpret
certain I/O as big endian IIRC. They where standard on some old Macs.
(?) I guess it's not really relevant any longer.

Best regards
Thomas

> 
>>
 +  unsigned int pins_len, version;
 +  int offset;
 +  int tmp;
 +
 +  if (size < MGA_BIOS_OFFSET + 1)
 +  return;
 +
 +  if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
 +  bios[48] != 'R' || bios[49] != 'O' 

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

2020-07-20 Thread Sam Ravnborg
On Mon, Jul 20, 2020 at 03:18:56PM -0400, Lyude Paul wrote:
> On Mon, 2020-07-20 at 09:04 +0200, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 17.07.20 um 00:43 schrieb Lyude Paul:
> > > On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
> > > > This patch adds support for G200 desktop cards. We can reuse the whole
> > > > memory and modesetting code. A few PCI and DAC register values have to
> > > > be updated accordingly.
> > > > 
> > > > The most significant change is in the PLL setup. The get the clock
> > > > limits
> > > > and reference clocks, parses the device's BIOS. With no BIOS found, safe
> > > > defaults are being used.
> > > > 
> > > > Signed-off-by: Thomas Zimmermann 
> > > > Co-developed-by: Egbert Eich 
> > > > Signed-off-by: Egbert Eich 
> > > > Co-developed-by: Takashi Iwai 
> > > > Signed-off-by: Takashi Iwai 
> > > > ---
> > > >  MAINTAINERS|   2 +-
> > > >  drivers/gpu/drm/mgag200/Kconfig|  12 +--
> > > >  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 -
> > > >  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
> > > >  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 
> > > >  5 files changed, 220 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 415954a98934..4c6f96e2b79b 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -5406,7 +5406,7 @@ S:Orphan / Obsolete
> > > >  F: drivers/gpu/drm/mga/
> > > >  F: include/uapi/drm/mga_drm.h
> > > >  
> > > > -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
> > > > +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
> > > >  M: Dave Airlie 
> > > >  S: Odd Fixes
> > > >  F: drivers/gpu/drm/mgag200/
> > > > diff --git a/drivers/gpu/drm/mgag200/Kconfig
> > > > b/drivers/gpu/drm/mgag200/Kconfig
> > > > index 93be766715c9..eec59658a938 100644
> > > > --- a/drivers/gpu/drm/mgag200/Kconfig
> > > > +++ b/drivers/gpu/drm/mgag200/Kconfig
> > > > @@ -1,13 +1,11 @@
> > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > >  config DRM_MGAG200
> > > > -   tristate "Kernel modesetting driver for MGA G200 server engines"
> > > > +   tristate "Matrox G200"
> > > > depends on DRM && PCI && MMU
> > > > select DRM_GEM_SHMEM_HELPER
> > > > select DRM_KMS_HELPER
> > > > help
> > > > -This is a KMS driver for the MGA G200 server chips, it
> > > > -does not support the original MGA G200 or any of the desktop
> > > > -chips. It requires 0.3.0 of the modesetting userspace driver,
> > > > -and a version of mga driver that will fail on KMS enabled
> > > > -devices.
> > > > -
> > > > +This is a KMS driver for Matrox G200 chips. It supports the 
> > > > original
> > > > +MGA G200 desktop chips and the server variants. It requires 
> > > > 0.3.0
> > > > +of the modesetting userspace driver, and a version of mga 
> > > > driver
> > > > +that will fail on KMS enabled devices.
> > > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > > b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > > index f7652e16365c..419817d6e2cd 100644
> > > > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > > @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
> > > > u8 crtcext3;
> > > >  
> > > > switch (mdev->type) {
> > > > +   case G200_PCI:
> > > > +   case G200_AGP:
> > > > +   if (mgag200_has_sgram(mdev))
> > > > +   option = 0x4049cd21;
> > > > +   else
> > > > +   option = 0x40499121;
> > > > +   option2 = 0x8000;
> > > > +   break;
> > > > case G200_SE_A:
> > > > case G200_SE_B:
> > > > if (mgag200_has_sgram(mdev))
> > > > @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device
> > > > *mdev)
> > > > return 0;
> > > >  }
> > > >  
> > > > +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
> > > > +   unsigned char __iomem *bios,
> > > > +   size_t size)
> > > > +{
> > > > +   static const unsigned int expected_length[6] = {
> > > > +   0, 64, 64, 64, 128, 128
> > > > +   };
> > > > +
> > > > +   struct drm_device *dev = >base;
> > > > +   unsigned char __iomem *pins;
> > > 
> > > huh, never realized you could write directly to unsigned char __iomem
> > > pointers!
> > 
> > I took the patch as-is, but this probably wouldn't work on all
> > architectures.
> 
> Something occurred to me just now - do we actually care? I don't think I've
> ever seen mgag200 on anything other then x86_64 systems

For starters to silence the sparse warnings.
Also other parts of the driver uses the io{read,write} functions
so to be consistent this code shoudl also do it.
And to avoid having bad 

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

2020-07-20 Thread Lyude Paul
On Mon, 2020-07-20 at 09:04 +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 17.07.20 um 00:43 schrieb Lyude Paul:
> > On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
> > > This patch adds support for G200 desktop cards. We can reuse the whole
> > > memory and modesetting code. A few PCI and DAC register values have to
> > > be updated accordingly.
> > > 
> > > The most significant change is in the PLL setup. The get the clock
> > > limits
> > > and reference clocks, parses the device's BIOS. With no BIOS found, safe
> > > defaults are being used.
> > > 
> > > Signed-off-by: Thomas Zimmermann 
> > > Co-developed-by: Egbert Eich 
> > > Signed-off-by: Egbert Eich 
> > > Co-developed-by: Takashi Iwai 
> > > Signed-off-by: Takashi Iwai 
> > > ---
> > >  MAINTAINERS|   2 +-
> > >  drivers/gpu/drm/mgag200/Kconfig|  12 +--
> > >  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 -
> > >  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
> > >  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 
> > >  5 files changed, 220 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 415954a98934..4c6f96e2b79b 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -5406,7 +5406,7 @@ S:  Orphan / Obsolete
> > >  F:   drivers/gpu/drm/mga/
> > >  F:   include/uapi/drm/mga_drm.h
> > >  
> > > -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
> > > +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
> > >  M:   Dave Airlie 
> > >  S:   Odd Fixes
> > >  F:   drivers/gpu/drm/mgag200/
> > > diff --git a/drivers/gpu/drm/mgag200/Kconfig
> > > b/drivers/gpu/drm/mgag200/Kconfig
> > > index 93be766715c9..eec59658a938 100644
> > > --- a/drivers/gpu/drm/mgag200/Kconfig
> > > +++ b/drivers/gpu/drm/mgag200/Kconfig
> > > @@ -1,13 +1,11 @@
> > >  # SPDX-License-Identifier: GPL-2.0-only
> > >  config DRM_MGAG200
> > > - tristate "Kernel modesetting driver for MGA G200 server engines"
> > > + tristate "Matrox G200"
> > >   depends on DRM && PCI && MMU
> > >   select DRM_GEM_SHMEM_HELPER
> > >   select DRM_KMS_HELPER
> > >   help
> > > -  This is a KMS driver for the MGA G200 server chips, it
> > > -  does not support the original MGA G200 or any of the desktop
> > > -  chips. It requires 0.3.0 of the modesetting userspace driver,
> > > -  and a version of mga driver that will fail on KMS enabled
> > > -  devices.
> > > -
> > > +  This is a KMS driver for Matrox G200 chips. It supports the original
> > > +  MGA G200 desktop chips and the server variants. It requires 0.3.0
> > > +  of the modesetting userspace driver, and a version of mga driver
> > > +  that will fail on KMS enabled devices.
> > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > index f7652e16365c..419817d6e2cd 100644
> > > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > > @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
> > >   u8 crtcext3;
> > >  
> > >   switch (mdev->type) {
> > > + case G200_PCI:
> > > + case G200_AGP:
> > > + if (mgag200_has_sgram(mdev))
> > > + option = 0x4049cd21;
> > > + else
> > > + option = 0x40499121;
> > > + option2 = 0x8000;
> > > + break;
> > >   case G200_SE_A:
> > >   case G200_SE_B:
> > >   if (mgag200_has_sgram(mdev))
> > > @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device
> > > *mdev)
> > >   return 0;
> > >  }
> > >  
> > > +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
> > > + unsigned char __iomem *bios,
> > > + size_t size)
> > > +{
> > > + static const unsigned int expected_length[6] = {
> > > + 0, 64, 64, 64, 128, 128
> > > + };
> > > +
> > > + struct drm_device *dev = >base;
> > > + unsigned char __iomem *pins;
> > 
> > huh, never realized you could write directly to unsigned char __iomem
> > pointers!
> 
> I took the patch as-is, but this probably wouldn't work on all
> architectures.

Something occurred to me just now - do we actually care? I don't think I've
ever seen mgag200 on anything other then x86_64 systems

> 
> > > + unsigned int pins_len, version;
> > > + int offset;
> > > + int tmp;
> > > +
> > > + if (size < MGA_BIOS_OFFSET + 1)
> > > + return;
> > > +
> > > + if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
> > > + bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
> > > + return;
> > > +
> > > + offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
> > > +
> > > + if (offset + 5 > size)
> > > + return;
> > > +
> > > + pins = bios + offset;
> > > + if (pins[0] == 0x2e && pins[1] == 0x41) {
> > > + version = pins[5];
> > > + pins_len = pins[2];
> > > + } else {
> > > + version = 1;
> > > + pins_len = 

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

2020-07-20 Thread Thomas Zimmermann
Hi

Am 17.07.20 um 00:43 schrieb Lyude Paul:
> On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
>> This patch adds support for G200 desktop cards. We can reuse the whole
>> memory and modesetting code. A few PCI and DAC register values have to
>> be updated accordingly.
>>
>> The most significant change is in the PLL setup. The get the clock limits
>> and reference clocks, parses the device's BIOS. With no BIOS found, safe
>> defaults are being used.
>>
>> Signed-off-by: Thomas Zimmermann 
>> Co-developed-by: Egbert Eich 
>> Signed-off-by: Egbert Eich 
>> Co-developed-by: Takashi Iwai 
>> Signed-off-by: Takashi Iwai 
>> ---
>>  MAINTAINERS|   2 +-
>>  drivers/gpu/drm/mgag200/Kconfig|  12 +--
>>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 -
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
>>  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 
>>  5 files changed, 220 insertions(+), 9 deletions(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 415954a98934..4c6f96e2b79b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5406,7 +5406,7 @@ S: Orphan / Obsolete
>>  F:  drivers/gpu/drm/mga/
>>  F:  include/uapi/drm/mga_drm.h
>>  
>> -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
>> +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
>>  M:  Dave Airlie 
>>  S:  Odd Fixes
>>  F:  drivers/gpu/drm/mgag200/
>> diff --git a/drivers/gpu/drm/mgag200/Kconfig 
>> b/drivers/gpu/drm/mgag200/Kconfig
>> index 93be766715c9..eec59658a938 100644
>> --- a/drivers/gpu/drm/mgag200/Kconfig
>> +++ b/drivers/gpu/drm/mgag200/Kconfig
>> @@ -1,13 +1,11 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  config DRM_MGAG200
>> -tristate "Kernel modesetting driver for MGA G200 server engines"
>> +tristate "Matrox G200"
>>  depends on DRM && PCI && MMU
>>  select DRM_GEM_SHMEM_HELPER
>>  select DRM_KMS_HELPER
>>  help
>> - This is a KMS driver for the MGA G200 server chips, it
>> - does not support the original MGA G200 or any of the desktop
>> - chips. It requires 0.3.0 of the modesetting userspace driver,
>> - and a version of mga driver that will fail on KMS enabled
>> - devices.
>> -
>> + This is a KMS driver for Matrox G200 chips. It supports the original
>> + MGA G200 desktop chips and the server variants. It requires 0.3.0
>> + of the modesetting userspace driver, and a version of mga driver
>> + that will fail on KMS enabled devices.
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index f7652e16365c..419817d6e2cd 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
>>  u8 crtcext3;
>>  
>>  switch (mdev->type) {
>> +case G200_PCI:
>> +case G200_AGP:
>> +if (mgag200_has_sgram(mdev))
>> +option = 0x4049cd21;
>> +else
>> +option = 0x40499121;
>> +option2 = 0x8000;
>> +break;
>>  case G200_SE_A:
>>  case G200_SE_B:
>>  if (mgag200_has_sgram(mdev))
>> @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device *mdev)
>>  return 0;
>>  }
>>  
>> +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
>> +unsigned char __iomem *bios,
>> +size_t size)
>> +{
>> +static const unsigned int expected_length[6] = {
>> +0, 64, 64, 64, 128, 128
>> +};
>> +
>> +struct drm_device *dev = >base;
>> +unsigned char __iomem *pins;
> 
> huh, never realized you could write directly to unsigned char __iomem 
> pointers!

I took the patch as-is, but this probably wouldn't work on all
architectures.

> 
>> +unsigned int pins_len, version;
>> +int offset;
>> +int tmp;
>> +
>> +if (size < MGA_BIOS_OFFSET + 1)
>> +return;
>> +
>> +if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
>> +bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
>> +return;
>> +
>> +offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
>> +
>> +if (offset + 5 > size)
>> +return;
>> +
>> +pins = bios + offset;
>> +if (pins[0] == 0x2e && pins[1] == 0x41) {
>> +version = pins[5];
>> +pins_len = pins[2];
>> +} else {
>> +version = 1;
>> +pins_len = pins[0] + (pins[1] << 8);
>> +}
>> +
>> +if (version < 1 || version > 5) {
>> +drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);
> 
> Did you maybe mean pins or PINS here? or is PInS some weird abbreviation 
> matrox
> uses?

It's the name of a data structure


https://gitlab.freedesktop.org/xorg/driver/xf86-video-mga/-/blob/master/mga_PInS.txt

I have no idea what it stands for.

> 
>> +

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

2020-07-16 Thread Sam Ravnborg
Hi Lyude.

On Thu, Jul 16, 2020 at 06:43:40PM -0400, Lyude Paul wrote:
> On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
> > This patch adds support for G200 desktop cards. We can reuse the whole
> > memory and modesetting code. A few PCI and DAC register values have to
> > be updated accordingly.
> > 
> > The most significant change is in the PLL setup. The get the clock limits
> > and reference clocks, parses the device's BIOS. With no BIOS found, safe
> > defaults are being used.
> > 
> > Signed-off-by: Thomas Zimmermann 
> > Co-developed-by: Egbert Eich 
> > Signed-off-by: Egbert Eich 
> > Co-developed-by: Takashi Iwai 
> > Signed-off-by: Takashi Iwai 
> > ---
> >  MAINTAINERS|   2 +-
> >  drivers/gpu/drm/mgag200/Kconfig|  12 +--
> >  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 -
> >  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
> >  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 
> >  5 files changed, 220 insertions(+), 9 deletions(-)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 415954a98934..4c6f96e2b79b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5406,7 +5406,7 @@ S:Orphan / Obsolete
> >  F: drivers/gpu/drm/mga/
> >  F: include/uapi/drm/mga_drm.h
> >  
> > -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
> > +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
> >  M: Dave Airlie 
> >  S: Odd Fixes
> >  F: drivers/gpu/drm/mgag200/
> > diff --git a/drivers/gpu/drm/mgag200/Kconfig 
> > b/drivers/gpu/drm/mgag200/Kconfig
> > index 93be766715c9..eec59658a938 100644
> > --- a/drivers/gpu/drm/mgag200/Kconfig
> > +++ b/drivers/gpu/drm/mgag200/Kconfig
> > @@ -1,13 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  config DRM_MGAG200
> > -   tristate "Kernel modesetting driver for MGA G200 server engines"
> > +   tristate "Matrox G200"
> > depends on DRM && PCI && MMU
> > select DRM_GEM_SHMEM_HELPER
> > select DRM_KMS_HELPER
> > help
> > -This is a KMS driver for the MGA G200 server chips, it
> > -does not support the original MGA G200 or any of the desktop
> > -chips. It requires 0.3.0 of the modesetting userspace driver,
> > -and a version of mga driver that will fail on KMS enabled
> > -devices.
> > -
> > +This is a KMS driver for Matrox G200 chips. It supports the original
> > +MGA G200 desktop chips and the server variants. It requires 0.3.0
> > +of the modesetting userspace driver, and a version of mga driver
> > +that will fail on KMS enabled devices.
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > index f7652e16365c..419817d6e2cd 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
> > u8 crtcext3;
> >  
> > switch (mdev->type) {
> > +   case G200_PCI:
> > +   case G200_AGP:
> > +   if (mgag200_has_sgram(mdev))
> > +   option = 0x4049cd21;
> > +   else
> > +   option = 0x40499121;
> > +   option2 = 0x8000;
> > +   break;
> > case G200_SE_A:
> > case G200_SE_B:
> > if (mgag200_has_sgram(mdev))
> > @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device *mdev)
> > return 0;
> >  }
> >  
> > +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
> > +   unsigned char __iomem *bios,
> > +   size_t size)
> > +{
> > +   static const unsigned int expected_length[6] = {
> > +   0, 64, 64, 64, 128, 128
> > +   };
> > +
> > +   struct drm_device *dev = >base;
> > +   unsigned char __iomem *pins;
> 
> huh, never realized you could write directly to unsigned char __iomem 
> pointers!
You cannot :-)

It works for some architectures but not for all.
On sparc64, for instance, this will fail.
The right thing is to use the accessors in io.h

sparse will help you finding such illegal uses of __iomem *.
The good thing is that the pointers are annotated __iomem here.

Thomas:
Please run sparse and fix the warnings using the right accessors
for this patch.

Sam

> 
> > +   unsigned int pins_len, version;
> > +   int offset;
> > +   int tmp;
> > +
> > +   if (size < MGA_BIOS_OFFSET + 1)
> > +   return;
> > +
> > +   if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
> > +   bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
> > +   return;
> > +
> > +   offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
> > +
> > +   if (offset + 5 > size)
> > +   return;
> > +
> > +   pins = bios + offset;
> > +   if (pins[0] == 0x2e && pins[1] == 0x41) {
> > +   version = pins[5];
> > +   pins_len = pins[2];
> > +   } else {
> > +   version = 1;
> > +   pins_len = pins[0] + (pins[1] << 8);
> > +   }
> > +

Re: [PATCH 8/8] drm/mgag200: Add support for G200 desktop cards

2020-07-16 Thread Lyude Paul
On Wed, 2020-07-15 at 16:59 +0200, Thomas Zimmermann wrote:
> This patch adds support for G200 desktop cards. We can reuse the whole
> memory and modesetting code. A few PCI and DAC register values have to
> be updated accordingly.
> 
> The most significant change is in the PLL setup. The get the clock limits
> and reference clocks, parses the device's BIOS. With no BIOS found, safe
> defaults are being used.
> 
> Signed-off-by: Thomas Zimmermann 
> Co-developed-by: Egbert Eich 
> Signed-off-by: Egbert Eich 
> Co-developed-by: Takashi Iwai 
> Signed-off-by: Takashi Iwai 
> ---
>  MAINTAINERS|   2 +-
>  drivers/gpu/drm/mgag200/Kconfig|  12 +--
>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 125 -
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  10 ++
>  drivers/gpu/drm/mgag200/mgag200_mode.c |  80 
>  5 files changed, 220 insertions(+), 9 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 415954a98934..4c6f96e2b79b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5406,7 +5406,7 @@ S:  Orphan / Obsolete
>  F:   drivers/gpu/drm/mga/
>  F:   include/uapi/drm/mga_drm.h
>  
> -DRM DRIVER FOR MGA G200 SERVER GRAPHICS CHIPS
> +DRM DRIVER FOR MGA G200 GRAPHICS CHIPS
>  M:   Dave Airlie 
>  S:   Odd Fixes
>  F:   drivers/gpu/drm/mgag200/
> diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
> index 93be766715c9..eec59658a938 100644
> --- a/drivers/gpu/drm/mgag200/Kconfig
> +++ b/drivers/gpu/drm/mgag200/Kconfig
> @@ -1,13 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config DRM_MGAG200
> - tristate "Kernel modesetting driver for MGA G200 server engines"
> + tristate "Matrox G200"
>   depends on DRM && PCI && MMU
>   select DRM_GEM_SHMEM_HELPER
>   select DRM_KMS_HELPER
>   help
> -  This is a KMS driver for the MGA G200 server chips, it
> -  does not support the original MGA G200 or any of the desktop
> -  chips. It requires 0.3.0 of the modesetting userspace driver,
> -  and a version of mga driver that will fail on KMS enabled
> -  devices.
> -
> +  This is a KMS driver for Matrox G200 chips. It supports the original
> +  MGA G200 desktop chips and the server variants. It requires 0.3.0
> +  of the modesetting userspace driver, and a version of mga driver
> +  that will fail on KMS enabled devices.
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
> b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index f7652e16365c..419817d6e2cd 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -64,6 +64,14 @@ static int mgag200_regs_init(struct mga_device *mdev)
>   u8 crtcext3;
>  
>   switch (mdev->type) {
> + case G200_PCI:
> + case G200_AGP:
> + if (mgag200_has_sgram(mdev))
> + option = 0x4049cd21;
> + else
> + option = 0x40499121;
> + option2 = 0x8000;
> + break;
>   case G200_SE_A:
>   case G200_SE_B:
>   if (mgag200_has_sgram(mdev))
> @@ -115,6 +123,117 @@ static int mgag200_regs_init(struct mga_device *mdev)
>   return 0;
>  }
>  
> +static void mgag200_g200_interpret_bios(struct mga_device *mdev,
> + unsigned char __iomem *bios,
> + size_t size)
> +{
> + static const unsigned int expected_length[6] = {
> + 0, 64, 64, 64, 128, 128
> + };
> +
> + struct drm_device *dev = >base;
> + unsigned char __iomem *pins;

huh, never realized you could write directly to unsigned char __iomem pointers!

> + unsigned int pins_len, version;
> + int offset;
> + int tmp;
> +
> + if (size < MGA_BIOS_OFFSET + 1)
> + return;
> +
> + if (bios[45] != 'M' || bios[46] != 'A' || bios[47] != 'T' ||
> + bios[48] != 'R' || bios[49] != 'O' || bios[50] != 'X')
> + return;
> +
> + offset = (bios[MGA_BIOS_OFFSET + 1] << 8) | bios[MGA_BIOS_OFFSET];
> +
> + if (offset + 5 > size)
> + return;
> +
> + pins = bios + offset;
> + if (pins[0] == 0x2e && pins[1] == 0x41) {
> + version = pins[5];
> + pins_len = pins[2];
> + } else {
> + version = 1;
> + pins_len = pins[0] + (pins[1] << 8);
> + }
> +
> + if (version < 1 || version > 5) {
> + drm_warn(dev, "Unknown BIOS PInS version: %d\n", version);

Did you maybe mean pins or PINS here? or is PInS some weird abbreviation matrox
uses?

> + return;
> + }
> + if (pins_len != expected_length[version]) {
> + drm_warn(dev, "Unexpected BIOS PInS size: %d expeced: %d\n",
> +  pins_len, expected_length[version]);
> + return;
> + }
> +
> + if (offset + pins_len > size)
> + return;
> +
> + drm_dbg_kms(dev, "MATROX BIOS PInS