On Tue, May 23, 2023 at 04:02:46PM +0300, Jani Nikula wrote:
> On Mon, 22 May 2023, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote:
> > For platforms with GMD_ID support (i.e., everything MTL and beyond),
> > identification of the display IP present should be based on the contents
> > of the GMD_ID register rather than a PCI devid match.
> >
> > Note that since GMD_ID readout requires access to the PCI BAR, a slight
> > change to the driver init sequence is needed --- pci_enable_device() is
> > now called before i915_driver_create().
> >
> > v2:
> > - Fix use of uninitialized i915 pointer in error path if
> > pci_enable_device() fails before the i915 device is created. (lkp)
> > - Use drm_device parameter to intel_display_device_probe. This goes
> > against i915 conventions, but since the primary goal here is to make
> > it easy to call this function from other drivers (like Xe) and since
> > we don't need anything from the i915 structure, this seems like an
> > exception where drm_device is a more natural fit.
> >
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > .../drm/i915/display/intel_display_device.c | 64 +++++++++++++++++--
> > .../drm/i915/display/intel_display_device.h | 5 +-
> > drivers/gpu/drm/i915/i915_driver.c | 17 +++--
> > drivers/gpu/drm/i915/intel_device_info.c | 12 ++--
> > 4 files changed, 81 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> > index 3c5941c8788d..6605487c3890 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> > @@ -6,7 +6,10 @@
> > #include <drm/i915_pciids.h>
> > #include <drm/drm_color_mgmt.h>
> > #include <linux/mod_devicetable.h>
> > +#include <linux/pci.h>
> >
> > +#include "i915_drv.h"
> > +#include "i915_reg.h"
> > #include "intel_display_device.h"
> > #include "intel_display_power.h"
> > #include "intel_display_reg_defs.h"
> > @@ -692,18 +695,69 @@ static const struct pci_device_id intel_display_ids[] = {
> > INTEL_RPLP_IDS(&xe_lpd_display),
> > INTEL_DG2_IDS(&xe_hpd_display),
> >
> > - /* FIXME: Replace this with a GMD_ID lookup */
> > - INTEL_MTL_IDS(&xe_lpdp_display),
> > + /*
> > + * Do not add any GMD_ID-based platforms to this list. They will
> > + * be probed automatically based on the IP version reported by
> > + * the hardware.
> > + */
> > };
> >
> > +struct {
> > + u16 ver;
> > + u16 rel;
> > + const struct intel_display_device_info *display;
> > +} gmdid_display_map[] = {
> > + { 14, 0, &xe_lpdp_display },
> > +};
> > +
> > +static const struct intel_display_device_info *
> > +probe_gmdid_display(struct drm_device *drm, u16 *ver, u16 *rel, u16 *step)
>
> Please always prefer struct drm_i915_private * over struct drm_device *.
>
> > +{
> > + struct pci_dev *pdev = to_pci_dev(drm->dev);
> > + void __iomem *addr;
> > + u32 val;
> > + int i;
> > +
> > + addr = pci_iomap_range(pdev, 0, i915_mmio_reg_offset(GMD_ID_DISPLAY), sizeof(u32));
> > + if (!addr) {
> > + drm_err(drm, "Cannot map MMIO BAR to read display GMD_ID\n");
> > + return &no_display;
> > + }
> > +
> > + val = ioread32(addr);
> > + pci_iounmap(pdev, addr);
> > +
> > + if (val == 0)
> > + /* Platform doesn't have display */
> > + return &no_display;
> > +
> > + *ver = REG_FIELD_GET(GMD_ID_ARCH_MASK, val);
> > + *rel = REG_FIELD_GET(GMD_ID_RELEASE_MASK, val);
> > + *step = REG_FIELD_GET(GMD_ID_STEP, val);
> > +
> > + for (i = 0; i < ARRAY_SIZE(gmdid_display_map); i++)
> > + if (*ver == gmdid_display_map[i].ver &&
> > + *rel == gmdid_display_map[i].rel)
> > + return gmdid_display_map[i].display;
> > +
> > + drm_err(drm, "Unrecognized display IP version %d.%02d; disabling display.\n",
> > + *ver, *rel);
> > + return &no_display;
> > +}
> > +
> > const struct intel_display_device_info *
> > -intel_display_device_probe(u16 pci_devid)
> > +intel_display_device_probe(struct drm_device *drm, bool has_gmdid,
> > + u16 *gmdid_ver, u16 *gmdid_rel, u16 *gmdid_step)
>
> Ditto.
I was a bit torn on this one. Although we usually use drm_i915_private
in i915 code, in this case it seems like it adds a needless dependency
on i915 types and leads to more complications when calling this from
non-i915 code (like Xe). What we really need here is just 'struct
pci_dev' to obtain the device ID and map the BAR, but drm_device
lets us also use the drm_err() calls.
But for now I can just put this back to drm_i915_private; we can revisit
this later once we have more driver restructuring to move away from
drm_i915_private in a more global manner.
Matt
>
> > {
> > + struct pci_dev *pdev = to_pci_dev(drm->dev);
> > int i;
> >
> > + if (has_gmdid)
> > + return probe_gmdid_display(drm, gmdid_ver, gmdid_rel, gmdid_step);
> > +
> > for (i = 0; i < ARRAY_SIZE(intel_display_ids); i++) {
> > - if (intel_display_ids[i].device == pci_devid)
> > - return (struct intel_display_device_info *)intel_display_ids[i].driver_data;
> > + if (intel_display_ids[i].device == pdev->device)
> > + return (const struct intel_display_device_info *)intel_display_ids[i].driver_data;
> > }
> >
> > return &no_display;
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> > index 1f7d08b3ad6b..2a14943313ad 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > @@ -80,7 +80,10 @@ struct intel_display_device_info {
> > } color;
> > };
> >
> > +struct drm_device;
> > +
>
> Please keep forward declarations near the top of the file, right after
> includes.
>
> > const struct intel_display_device_info *
> > -intel_display_device_probe(u16 pci_devid);
> > +intel_display_device_probe(struct drm_device *drm, bool has_gmdid,
> > + u16 *ver, u16 *rel, u16 *step);
> >
> > #endif
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index 522733a89946..37532e55327d 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -754,13 +754,17 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > struct drm_i915_private *i915;
> > int ret;
> >
> > - i915 = i915_driver_create(pdev, ent);
> > - if (IS_ERR(i915))
> > - return PTR_ERR(i915);
> > -
> > ret = pci_enable_device(pdev);
> > - if (ret)
> > - goto out_fini;
> > + if (ret) {
> > + pr_err("Failed to enable graphics device: %pe\n", ERR_PTR(ret));
> > + return ret;
> > + }
> > +
> > + i915 = i915_driver_create(pdev, ent);
> > + if (IS_ERR(i915)) {
> > + ret = PTR_ERR(i915);
> > + goto out_pci_disable;
> > + }
> >
> > ret = i915_driver_early_probe(i915);
> > if (ret < 0)
> > @@ -843,7 +847,6 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > i915_driver_late_release(i915);
> > out_pci_disable:
> > pci_disable_device(pdev);
> > -out_fini:
> > i915_probe_error(i915, "Device initialization failed (%d)\n", ret);
> > return ret;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> > index e1507ae59f2d..85105639d55d 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -345,7 +345,6 @@ static void ip_ver_read(struct drm_i915_private *i915, u32 offset, struct intel_
> > static void intel_ipver_early_init(struct drm_i915_private *i915)
> > {
> > struct intel_runtime_info *runtime = RUNTIME_INFO(i915);
> > - struct intel_display_runtime_info *display_runtime = DISPLAY_RUNTIME_INFO(i915);
> >
> > if (!HAS_GMD_ID(i915)) {
> > drm_WARN_ON(&i915->drm, RUNTIME_INFO(i915)->graphics.ip.ver > 12);
> > @@ -366,8 +365,6 @@ static void intel_ipver_early_init(struct drm_i915_private *i915)
> > RUNTIME_INFO(i915)->graphics.ip.ver = 12;
> > RUNTIME_INFO(i915)->graphics.ip.rel = 70;
> > }
> > - ip_ver_read(i915, i915_mmio_reg_offset(GMD_ID_DISPLAY),
> > - (struct intel_ip_version *)&display_runtime->ip);
> > ip_ver_read(i915, i915_mmio_reg_offset(GMD_ID_MEDIA),
> > &runtime->media.ip);
> > }
> > @@ -574,6 +571,7 @@ void intel_device_info_driver_create(struct drm_i915_private *i915,
> > {
> > struct intel_device_info *info;
> > struct intel_runtime_info *runtime;
> > + u16 ver, rel, step;
> >
> > /* Setup the write-once "constant" device info */
> > info = mkwrite_device_info(i915);
> > @@ -584,8 +582,14 @@ void intel_device_info_driver_create(struct drm_i915_private *i915,
> > memcpy(runtime, &INTEL_INFO(i915)->__runtime, sizeof(*runtime));
> >
> > /* Probe display support */
> > - info->display = intel_display_device_probe(device_id);
> > + info->display = intel_display_device_probe(&i915->drm, info->has_gmd_id,
> > + &ver, &rel, &step);
> > *DISPLAY_RUNTIME_INFO(i915) = DISPLAY_INFO(i915)->__runtime_defaults;
> > + if (info->has_gmd_id) {
> > + DISPLAY_RUNTIME_INFO(i915)->ip.ver = ver;
> > + DISPLAY_RUNTIME_INFO(i915)->ip.rel = rel;
> > + DISPLAY_RUNTIME_INFO(i915)->ip.step = step;
> > + }
>
> The division of initialization responsibilities between here and
> intel_display_device_probe() is perhaps a bit odd?
>
> Nothing that can't be fixed later though, I guess.
>
> BR,
> Jani.
>
> >
> > runtime->device_id = device_id;
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
