On Wed, Jan 30, 2019 at 09:04:23PM +0000, Kazlauskas, Nicholas wrote:

On 1/30/19 4:02 PM, Daniel Vetter wrote:

...

On Wed, Jan 30, 2019 at 05:33:00PM +0000, Kazlauskas, Nicholas wrote:

...

On 1/30/19 11:30 AM, Daniel Vetter wrote:

...

It only talks about crtc, brings up intel as an exampel and I think is

nit: Should be "example".

Will fix when applying.

...

...

more misleading than useful really. Plus we have lots of discussion about how your standard kms driver should be initialized/cleaned up, so maybe better to document this when we have a better idea.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Documentation/gpu/drm-kms.rst | 96 ----------------------------------- 1 file changed, 96 deletions(-)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 75c882e09fee..23a3c986ef6d 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -410,102 +410,6 @@ Encoder Functions Reference .. kernel-doc:: drivers/gpu/drm/drm_encoder.c :export:

-KMS Initialization and Cleanup

-A KMS device is abstracted and exposed as a set of planes, CRTCs, -encoders and connectors. KMS drivers must thus create and initialize all -those objects at load time after initializing mode setting.

-CRTCs (:c:type:`struct drm_crtc <drm_crtc>`)

-A CRTC is an abstraction representing a part of the chip that contains a -pointer to a scanout buffer. Therefore, the number of CRTCs available -determines how many independent scanout buffers can be active at any -given time. The CRTC structure contains several fields to support this: -a pointer to some video memory (abstracted as a frame buffer object), a -display mode, and an (x, y) offset into the video memory to support -panning or configurations where one piece of video memory spans multiple -CRTCs.

This is mostly a duplicate of what's already in the CRTC abstraction section but it may be worth carrying over the bit about "the number of CRTCs available determining the number of independent scanout buffers that can be active at any given time".

Oh I didn't even read this. This is just plain wrong and probably predates drm_plane (when it kinda was true-ish, if you'd ignore cursors). Now this would map to number of drm_planes, except

• cursors can still be an exceptions
• yuv multi-planar might or might not be multiple planes per drm_plane.

Aka it's really complicated :-)

This is definitely much better explained in the kms overview section at the top of:

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#overview

Plus everything that's getting linked from there ofc.

...

...

-CRTC Initialization -~~~~~~~~~~~~~~~~~~~

-A KMS device must create and register at least one struct -:c:type:`struct drm_crtc <drm_crtc>` instance. The instance is -allocated and zeroed by the driver, possibly as part of a larger -structure, and registered with a call to :c:func:`drm_crtc_init()` -with a pointer to CRTC functions.

-Cleanup

-The DRM core manages its objects' lifetime. When an object is not needed -anymore the core calls its destroy function, which must clean up and -free every resource allocated for the object. Every -:c:func:`drm_*_init()` call must be matched with a corresponding -:c:func:`drm_*_cleanup()` call to cleanup CRTCs -(:c:func:`drm_crtc_cleanup()`), planes -(:c:func:`drm_plane_cleanup()`), encoders -(:c:func:`drm_encoder_cleanup()`) and connectors -(:c:func:`drm_connector_cleanup()`). Furthermore, connectors that -have been added to sysfs must be removed by a call to -:c:func:`drm_connector_unregister()` before calling -:c:func:`drm_connector_cleanup()`.

-Connectors state change detection must be cleanup up with a call to -:c:func:`drm_kms_helper_poll_fini()`.

-Output discovery and initialization example

-.. code-block:: c

• void intel_crt_init(struct drm_device *dev)
• {

•    struct drm_connector *connector;
  

•    struct intel_output *intel_output;
  

•    intel_output = kzalloc(sizeof(struct intel_output), GFP_KERNEL);
  

•    if (!intel_output)
  

•        return;
  

•    connector = &intel_output->base;
  

•    drm_connector_init(dev, &intel_output->base,
  

•               &intel_crt_connector_funcs, DRM_MODE_CONNECTOR_VGA);
  

•    drm_encoder_init(dev, &intel_output->enc, &intel_crt_enc_funcs,
  

•             DRM_MODE_ENCODER_DAC);
  

•    drm_connector_attach_encoder(&intel_output->base,
  

•                      &intel_output->enc);
  

•    /* Set up the DDC bus. */
  

•    intel_output->ddc_bus = intel_i2c_create(dev, GPIOA, "CRTDDC_A");
  

•    if (!intel_output->ddc_bus) {
  

•        dev_printk(KERN_ERR, &dev->pdev->dev, "DDC bus registration "
  

•               "failed.\n");
  

•        return;
  

•    }
  

•    intel_output->type = INTEL_OUTPUT_ANALOG;
  

•    connector->interlace_allowed = 0;
  

•    connector->doublescan_allowed = 0;
  

•    drm_encoder_helper_add(&intel_output->enc, &intel_crt_helper_funcs);
  

•    drm_connector_helper_add(connector, &intel_crt_connector_helper_funcs);
  

•    drm_connector_register(connector);
  

• }

-In the example above (taken from the i915 driver), a CRTC, connector and -encoder combination is created. A device-specific i2c bus is also -created for fetching EDID data and performing monitor detection. Once -the process is complete, the new connector is registered with sysfs to -make its properties available to applications.

• KMS Locking

Everything else looks fine to drop.

The bits about initialization/cleanup/subclassing/connectors can all be found in the Atomic Mode Setting section or one of the abstraction sections.

With the one thing you wanted to keep also wrong - ok with this patch?

Thanks for taking a look at this. -Daniel

Sounds good.

Reviewed-by: Nicholas Kazlauskas nicholas.kazlauskas@amd.com

Thanks for reviewing, patch merged to drm-misc-next. -Daniel