Re: [PATCH 16/17] drm/mgag200: Convert to simple KMS helper

Hi Thomas.

Introducing drm_simple_display_pipe to hold some of the data, and then a later patch to change over would have helped the readability I think.

Anyway - deleted unused code belongs to another patch. Other than tthose details I did not spot anything, but then I did not read this patch as carefully as some of the others.

With the removed code migrated to another aptch, and the missing code explained the patch is: Acked-by: Sam Ravnborg sam@ravnborg.org

On Wed, Apr 29, 2020 at 04:32:37PM +0200, Thomas Zimmermann wrote:

The mgag200 supports a single pipeline with only a primary plane. It can be converted to simple KMS helpers. This also adds support for atomic modesetting. Wayland compositors, which use pageflip ioctls, can now be used with mgag200.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +- drivers/gpu/drm/mgag200/mgag200_drv.h | 4 +- drivers/gpu/drm/mgag200/mgag200_mode.c | 396 +++++++++++-------------- 3 files changed, 171 insertions(+), 231 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 3298b7ef18b03..b1272165621ed 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -140,7 +140,7 @@ int mgag200_driver_dumb_create(struct drm_file *file, }

static struct drm_driver driver = {

• .driver_features = DRIVER_GEM | DRIVER_MODESET,

• .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, .fops = &mgag200_driver_fops, .name = DRIVER_NAME, .desc = DRIVER_DESC,

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index b10da90e0f35a..2e407508714c8 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -19,6 +19,7 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_gem.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_simple_kms_helper.h>

#include "mgag200_reg.h"

@@ -107,6 +108,7 @@

#define to_mga_crtc(x) container_of(x, struct mga_crtc, base) #define to_mga_connector(x) container_of(x, struct mga_connector, base) +#define to_mga_device(x) (dev->dev_private)

Upclassing please.

struct mga_crtc { struct drm_crtc base; @@ -176,7 +178,7 @@ struct mga_device { u32 unique_rev_id;

struct mga_connector connector;

• struct drm_encoder encoder;

• struct drm_simple_display_pipe display_pipe;

};

static inline enum mga_type diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 884fc668a6dae..d9b4055e38982 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -11,10 +11,13 @@ #include <linux/delay.h> #include <linux/pci.h>

+#include <drm/drm_atomic_helper.h> +#include <drm/drm_atomic_state_helper.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_plane_helper.h> +#include <drm/drm_print.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h>

@@ -30,13 +33,18 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; struct mga_device *mdev = dev->dev_private;

• struct drm_framebuffer *fb = crtc->primary->fb;

• struct drm_framebuffer *fb; u16 *r_ptr, *g_ptr, *b_ptr; int i;

  if (!crtc->enabled) return;

• if (!mdev->display_pipe.plane.state)

• return;
  

• fb = mdev->display_pipe.plane.state->fb;

• r_ptr = crtc->gamma_store; g_ptr = r_ptr + crtc->gamma_size; b_ptr = g_ptr + crtc->gamma_size;

@@ -845,56 +853,6 @@ static void mgag200_set_startadd(struct mga_device *mdev, WREG_ECRT(0x00, crtcext0); }

-static int mga_crtc_do_set_base(struct mga_device *mdev,

• 		const struct drm_framebuffer *fb,
  

• 		const struct drm_framebuffer *old_fb)
  

-{

• struct drm_gem_vram_object *gbo;
• int ret;
• s64 gpu_addr;
• if (old_fb) {

• gbo = drm_gem_vram_of_gem(old_fb->obj[0]);
  

• drm_gem_vram_unpin(gbo);
  

• }
• gbo = drm_gem_vram_of_gem(fb->obj[0]);
• ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
• if (ret)

• return ret;
  

• gpu_addr = drm_gem_vram_offset(gbo);
• if (gpu_addr < 0) {

• ret = (int)gpu_addr;
  

• goto err_drm_gem_vram_unpin;
  

• }
• mgag200_set_startadd(mdev, (unsigned long)gpu_addr);
• return 0;

-err_drm_gem_vram_unpin:

• drm_gem_vram_unpin(gbo);
• return ret;

-}

-static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,

• 		  struct drm_framebuffer *old_fb)
  

-{

• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = dev->dev_private;
• struct drm_framebuffer *fb = crtc->primary->fb;
• unsigned int count;
• while (RREG8(0x1fda) & 0x08) { }
• while (!(RREG8(0x1fda) & 0x08)) { }
• count = RREG8(MGAREG_VCOUNT) + 2;
• while (RREG8(MGAREG_VCOUNT) < count) { }

I think we discussed the above lines before. I cannot find then in the converted code.

• return mga_crtc_do_set_base(mdev, fb, old_fb);

-}

static void mgag200_set_pci_regs(struct mga_device *mdev) { uint32_t option = 0, option2 = 0; @@ -1291,93 +1249,6 @@ static void mgag200_g200ev_set_hiprilvl(struct mga_device *mdev) WREG_ECRT(0x06, 0x00); }

-static int mga_crtc_mode_set(struct drm_crtc *crtc,

• 		struct drm_display_mode *mode,
  

• 		struct drm_display_mode *adjusted_mode,
  

• 		int x, int y, struct drm_framebuffer *old_fb)
  

-{

• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = dev->dev_private;
• const struct drm_framebuffer *fb = crtc->primary->fb;
• mgag200_init_regs(mdev);
• mgag200_set_format_regs(mdev, fb);
• mga_crtc_do_set_base(mdev, fb, old_fb);
• mgag200_set_offset(mdev, fb);
• mgag200_set_mode_regs(mdev, mode);
• if (mdev->type == G200_ER)

• mgag200_g200er_reset_tagfifo(mdev);
  

• if (IS_G200_SE(mdev))

• mgag200_g200se_set_hiprilvl(mdev, mode, fb);
  

• else if (mdev->type == G200_EV)

• mgag200_g200ev_set_hiprilvl(mdev);
  

• return 0;

-}

This part

-#if 0 /* code from mjg to attempt D3 on crtc dpms off - revisit later */ -static int mga_suspend(struct drm_crtc *crtc) -{

• struct mga_crtc *mga_crtc = to_mga_crtc(crtc);
• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = dev->dev_private;
• struct pci_dev *pdev = dev->pdev;
• int option;
• if (mdev->suspended)

• return 0;
  

• WREG_SEQ(1, 0x20);
• WREG_ECRT(1, 0x30);
• /* Disable the pixel clock */
• WREG_DAC(0x1a, 0x05);
• /* Power down the DAC */
• WREG_DAC(0x1e, 0x18);
• /* Power down the pixel PLL */
• WREG_DAC(0x1a, 0x0d);
• /* Disable PLLs and clocks */
• pci_read_config_dword(pdev, PCI_MGA_OPTION, &option);
• option &= ~(0x1F8024);
• pci_write_config_dword(pdev, PCI_MGA_OPTION, option);
• pci_set_power_state(pdev, PCI_D3hot);
• pci_disable_device(pdev);
• mdev->suspended = true;
• return 0;

-}

-static int mga_resume(struct drm_crtc *crtc) -{

• struct mga_crtc *mga_crtc = to_mga_crtc(crtc);
• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = dev->dev_private;
• struct pci_dev *pdev = dev->pdev;
• int option;
• if (!mdev->suspended)

• return 0;
  

• pci_set_power_state(pdev, PCI_D0);
• pci_enable_device(pdev);
• /* Disable sysclk */
• pci_read_config_dword(pdev, PCI_MGA_OPTION, &option);
• option &= ~(0x4);
• pci_write_config_dword(pdev, PCI_MGA_OPTION, option);
• mdev->suspended = false;
• return 0;

-}

-#endif

ending here - does not belong in this patch.

static void mga_crtc_dpms(struct drm_crtc *crtc, int mode) { struct drm_device *dev = crtc->dev; @@ -1470,7 +1341,6 @@ static void mga_crtc_commit(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; struct mga_device *mdev = dev->dev_private;

• const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; u8 tmp;

  if (mdev->type == G200_WB || mdev->type == G200_EW3)

@@ -1489,78 +1359,7 @@ static void mga_crtc_commit(struct drm_crtc *crtc) WREG_SEQ(0x1, tmp); WREG_SEQ(0, 3); }

• crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON);

-}

-/*

• • The core can pass us a set of gamma values to program. We actually only
• • use this for 8-bit mode so can't perform smooth fades on deeper modes,
• • but it's a requirement that we provide the function
• */

-static int mga_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,

• 	      u16 *blue, uint32_t size,
  

• 	      struct drm_modeset_acquire_ctx *ctx)
  

-{

• mga_crtc_load_lut(crtc);
• return 0;

-}

-/* Simple cleanup function */ -static void mga_crtc_destroy(struct drm_crtc *crtc) -{

• struct mga_crtc *mga_crtc = to_mga_crtc(crtc);
• drm_crtc_cleanup(crtc);
• kfree(mga_crtc);

-}

-static void mga_crtc_disable(struct drm_crtc *crtc) -{

• DRM_DEBUG_KMS("\n");
• mga_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
• if (crtc->primary->fb) {

• struct drm_framebuffer *fb = crtc->primary->fb;
  

• struct drm_gem_vram_object *gbo =
  

• 	drm_gem_vram_of_gem(fb->obj[0]);
  

• drm_gem_vram_unpin(gbo);
  

• }
• crtc->primary->fb = NULL;

-}

-/* These provide the minimum set of functions required to handle a CRTC */ -static const struct drm_crtc_funcs mga_crtc_funcs = {

• .gamma_set = mga_crtc_gamma_set,
• .set_config = drm_crtc_helper_set_config,
• .destroy = mga_crtc_destroy,

-};

-static const struct drm_crtc_helper_funcs mga_helper_funcs = {

• .disable = mga_crtc_disable,
• .dpms = mga_crtc_dpms,
• .mode_set = mga_crtc_mode_set,
• .mode_set_base = mga_crtc_mode_set_base,
• .prepare = mga_crtc_prepare,
• .commit = mga_crtc_commit,

-};

-/* CRTC setup */ -static void mga_crtc_init(struct mga_device *mdev) -{

• struct mga_crtc *mga_crtc;
• mga_crtc = kzalloc(sizeof(struct mga_crtc) +

• 	      (MGAG200FB_CONN_LIMIT * sizeof(struct drm_connector *)),
  

• 	      GFP_KERNEL);
  

• if (mga_crtc == NULL)

• return;
  

• drm_crtc_init(mdev->dev, &mga_crtc->base, &mga_crtc_funcs);
• drm_mode_crtc_set_gamma_size(&mga_crtc->base, MGAG200_LUT_SIZE);
• drm_crtc_helper_add(&mga_crtc->base, &mga_helper_funcs);

• mga_crtc_dpms(crtc, DRM_MODE_DPMS_ON);

}

/* @@ -1694,14 +1493,16 @@ static void mga_connector_destroy(struct drm_connector *connector) }

static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {

• .get_modes = mga_vga_get_modes,

• .get_modes = mga_vga_get_modes, .mode_valid = mga_vga_mode_valid,

};

static const struct drm_connector_funcs mga_vga_connector_funcs = {

• .dpms = drm_helper_connector_dpms,
• .fill_modes = drm_helper_probe_single_connector_modes,
• .destroy = mga_connector_destroy,

• .reset = drm_atomic_helper_connector_reset,
• .fill_modes = drm_helper_probe_single_connector_modes,
• .destroy = mga_connector_destroy,
• .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,

};

static int mgag200_vga_connector_init(struct mga_device *mdev) @@ -1733,8 +1534,138 @@ static int mgag200_vga_connector_init(struct mga_device *mdev) return ret; }

+/*

• • Simple Display Pipe
• */

+static enum drm_mode_status +mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,

• 		       const struct drm_display_mode *mode)
  

+{

• return MODE_OK;

+}

+static void +mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,

• 		   struct drm_crtc_state *crtc_state,
  

• 		   struct drm_plane_state *plane_state)
  

+{

• struct drm_crtc *crtc = &pipe->crtc;
• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = to_mga_device(dev);
• struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
• struct drm_framebuffer *fb = plane_state->fb;
• struct drm_gem_vram_object *gbo;
• s64 gpu_addr;
• gbo = drm_gem_vram_of_gem(fb->obj[0]);
• gpu_addr = drm_gem_vram_offset(gbo);
• if (drm_WARN_ON_ONCE(dev, gpu_addr < 0))

• return; /* BUG: BO should have been pinned to VRAM. */
  

• mga_crtc_prepare(crtc);
• mgag200_set_format_regs(mdev, fb);
• mgag200_set_mode_regs(mdev, adjusted_mode);
• if (mdev->type == G200_ER)

• mgag200_g200er_reset_tagfifo(mdev);
  

• if (IS_G200_SE(mdev))

• mgag200_g200se_set_hiprilvl(mdev, adjusted_mode, fb);
  

• else if (mdev->type == G200_EV)

• mgag200_g200ev_set_hiprilvl(mdev);
  

• mga_crtc_commit(crtc);

+}

+static void +mgag200_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe) +{

• struct drm_crtc *crtc = &pipe->crtc;
• mga_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);

+}

+static int +mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,

• 		  struct drm_plane_state *plane_state,
  

• 		  struct drm_crtc_state *crtc_state)
  

+{

• struct drm_plane *plane = plane_state->plane;
• struct drm_framebuffer *new_fb = plane_state->fb;
• struct drm_framebuffer *fb = NULL;
• if (!new_fb)

• return 0;
  

• if (plane->state)

• fb = plane->state->fb;
  

• if (!fb || (fb->format != new_fb->format))

• crtc_state->mode_changed = true; /* update PLL settings */
  

• return 0;

+}

+static void +mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,

• 		   struct drm_plane_state *old_state)
  

+{

• struct drm_plane *plane = &pipe->plane;
• struct drm_device *dev = plane->dev;
• struct mga_device *mdev = to_mga_device(dev);
• struct drm_plane_state *state = plane->state;
• struct drm_framebuffer *fb = state->fb;
• struct drm_gem_vram_object *gbo;
• s64 gpu_addr;
• if (!fb)

• return;
  

• gbo = drm_gem_vram_of_gem(fb->obj[0]);
• gpu_addr = drm_gem_vram_offset(gbo);
• if (drm_WARN_ON_ONCE(dev, gpu_addr < 0))

• return; /* BUG: BO should have been pinned to VRAM. */
  

• mgag200_set_format_regs(mdev, fb);
• mgag200_set_startadd(mdev, (unsigned long)gpu_addr);
• mgag200_set_offset(mdev, fb);

+}

+static const struct drm_simple_display_pipe_funcs +mgag200_simple_display_pipe_funcs = {

• .mode_valid = mgag200_simple_display_pipe_mode_valid,
• .enable = mgag200_simple_display_pipe_enable,
• .disable = mgag200_simple_display_pipe_disable,
• .check = mgag200_simple_display_pipe_check,
• .update = mgag200_simple_display_pipe_update,
• .prepare_fb = drm_gem_vram_simple_display_pipe_prepare_fb,
• .cleanup_fb = drm_gem_vram_simple_display_pipe_cleanup_fb,

+};

+static const uint32_t mgag200_simple_display_pipe_formats[] = {

• DRM_FORMAT_XRGB8888,
• DRM_FORMAT_RGB565,
• DRM_FORMAT_RGB888,

+};

+static const uint64_t mgag200_simple_display_pipe_modifiers[] = {

• DRM_FORMAT_MOD_LINEAR,
• DRM_FORMAT_MOD_INVALID

+};

+/*

• • Mode config
• */

static const struct drm_mode_config_funcs mgag200_mode_config_funcs = {

• .fb_create = drm_gem_fb_create

• .fb_create = drm_gem_fb_create,
• .mode_valid = drm_vram_helper_mode_valid,
• .atomic_check = drm_atomic_helper_check,
• .atomic_commit = drm_atomic_helper_commit,

};

static unsigned int mgag200_preferred_depth(struct mga_device *mdev) @@ -1748,10 +1679,13 @@ static unsigned int mgag200_preferred_depth(struct mga_device *mdev) int mgag200_modeset_init(struct mga_device *mdev) { struct drm_device *dev = mdev->dev;

• struct drm_encoder *encoder = &mdev->encoder; struct drm_connector *connector = &mdev->connector.base;

• struct drm_simple_display_pipe *pipe = &mdev->display_pipe;

• size_t format_count = ARRAY_SIZE(mgag200_simple_display_pipe_formats); int ret;

• mgag200_init_regs(mdev);

• ret = drmm_mode_config_init(dev); if (ret) { drm_err(dev, "drmm_mode_config_init() failed, error %d\n",

@@ -1769,26 +1703,30 @@ int mgag200_modeset_init(struct mga_device *mdev)

dev->mode_config.funcs = &mgag200_mode_config_funcs;

• mga_crtc_init(mdev);
• ret = drm_simple_encoder_init(mdev->dev, encoder,

• 		      DRM_MODE_ENCODER_DAC);
  

• ret = mgag200_vga_connector_init(mdev); if (ret) {

• drm_err(mdev->dev,
  

• 	"drm_simple_encoder_init() failed, error %d\n",
  

• drm_err(dev, "mga_vga_connector_init() failed, error %d\n",
  ret);
  

  return ret; }

• encoder->possible_crtcs = 0x1;

• ret = mgag200_vga_connector_init(mdev);

• ret = drm_simple_display_pipe_init(dev, pipe,

• 			   &mgag200_simple_display_pipe_funcs,
  

• 			   mgag200_simple_display_pipe_formats,
  

• 			   format_count,
  

• 			   mgag200_simple_display_pipe_modifiers,
  

• 			   connector);
  

  if (ret) {

• drm_err(mdev->dev,
  

• 	"mga_vga_connector_init() failed, error %d\n", ret);
  

• return -1;
  

• drm_err(dev,
  

• 	"drm_simple_display_pipe_init() failed, error %d\n",
  

• 	ret);
  

• return ret;
  

  }

• drm_connector_attach_encoder(connector, encoder);

• /* FIXME: legacy gamma tables; convert to CRTC state */

• drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE);

• drm_mode_config_reset(dev);

  return 0;

}

2.26.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel