[PATCH] platform: support non-pci platform devices

[Hans de Goede]

Hi,

On 06/14/2014 09:58 PM, Rob Clark wrote:
> This makes things not completely fail if DDX implements platformProbe()
> but the device is not actually a PCI device.  Also, the platform device
> name does not always match the DDX name, so deal with that.  I'm sure
> there are more cases that find_non_pci_driver() needs to handle for
> that, but this is a start.
> 
> Signed-off-by: Rob Clark <robdclark at gmail.com>

Looks good, one small remark below.

> ---
> NOTE: I still need to figure out a sane way to workaround the existing
> bug with non-pci platform devices.  Currently, if DDX implements the
> platformProbe() hook, then in xf86platformProbeDev() it will get claimed
> in the autoAddGPU loop, resulting that the old-school Probe() fallback
> (if you have .conf file) fails too because the device is already claimed.
> We kind of need a way that the DDX can detect that the xserver does not
> have this fix, so that it can work around the bug by failing
> platformProbe().
> 
>  hw/xfree86/common/xf86platformBus.c | 65 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 61 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/xfree86/common/xf86platformBus.c b/hw/xfree86/common/xf86platformBus.c
> index dd118a2..eca0f8c 100644
> --- a/hw/xfree86/common/xf86platformBus.c
> +++ b/hw/xfree86/common/xf86platformBus.c
> @@ -199,6 +199,41 @@ xf86_check_platform_slot(const struct xf86_platform_device *pd)
>      return TRUE;
>  }
>  
> +static int
> +find_non_pci_driver(const char *busid, char *returnList[], int returnListMax)
> +{
> +    /* Add more entries here if we ever return more than 4 drivers for
> +       any device */
> +    const char *driverList[5] = { NULL, NULL, NULL, NULL, NULL };
> +    int i = 0;
> +    char *p, *s;
> +
> +    s = xstrdup(busid);
> +    p = strtok(s, ":");
> +
> +    if (strcmp(p, "platform"))
> +        goto out;
> +
> +    /* extract device name: */
> +    p = strtok(NULL, ":");
> +
> +    /* check for special cases where DDX driver name does not match busid: */
> +    if (!strcmp(p, "mdp")) {
> +        driverList[i++] = "freedreno";
> +    }
> +
> +    /* add name derived from busid last: */
> +    driverList[i++] = p;
> +
> +    for (i = 0; (i < returnListMax) && (driverList[i] != NULL); i++) {
> +        returnList[i] = xnfstrdup(driverList[i]);
> +    }
> +
> +out:
> +    free(s);
> +    return i;                   /* Number of entries added */
> +}
> +
>  /**
>   *  @return The numbers of found devices that match with the current system
>   *  drivers.
> @@ -230,6 +265,9 @@ xf86PlatformMatchDriver(char *matches[], int nmatches)
>  
>              if ((info != NULL) && (j < nmatches)) {
>                  j += xf86VideoPtrToDriverList(info, &(matches[j]), nmatches - j);
> +            } else if (j < nmatches) {
> +                char *busid = xf86_get_platform_attrib(i, ODEV_ATTRIB_BUSID);
> +                j += find_non_pci_driver(busid, &(matches[j]), nmatches - j);
>              }
>          }
>      }
> @@ -248,6 +286,9 @@ xf86platformProbe(void)
>          pci = FALSE;
>      }
>  
> +    /* First pass, look for PCI devices.  If we find a suitable
> +     * PCI device that takes priority.
> +     */
>      for (i = 0; i < xf86_num_platform_devices; i++) {
>          char *busid = xf86_get_platform_attrib(i, ODEV_ATTRIB_BUSID);
>  
> @@ -255,6 +296,24 @@ xf86platformProbe(void)
>              platform_find_pci_info(&xf86_platform_devices[i], busid);
>          }
>      }
> +
> +    /* if we found something, we are done: */
> +    if (primaryBus.type != BUS_NONE)
> +        return 0;
> +
> +    /* Second pass, look for real platform devices (ie. in the linux-
> +     * kernel sense of platform device.. something that is not pci)
> +     */
> +    for (i = 0; i < xf86_num_platform_devices; i++) {
> +        char *busid = xf86_get_platform_attrib(i, ODEV_ATTRIB_BUSID);
> +
> +        if (strncmp(busid, "platform:", 9) == 0) {
> +            primaryBus.type = BUS_PLATFORM;
> +            primaryBus.id.plat = &xf86_platform_devices[i];
> +            break;
> +        }
> +    }
> +
>      return 0;
>  }
>  
> @@ -401,10 +460,8 @@ xf86platformProbeDev(DriverPtr drvp)
>                  /* for non-seat0 servers assume first device is the master */
>                  if (ServerIsNotSeat0())
>                      break;
> -                if (xf86_platform_devices[j].pdev) {
> -                    if (xf86IsPrimaryPlatform(&xf86_platform_devices[j]))
> -                        break;
> -                }
> +                if (xf86IsPrimaryPlatform(&xf86_platform_devices[j]))
> +                    break;
>              }
>          }

This seems like an unrelated cleanup which should be in its own patch,
with a commit message explaining why it is safe to remove the
"if (xf86_platform_devices[j].pdev)" check.

With this hunk removed, this patch is:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Regards,

Hans