Re: [PATCH 1/9] drm: Extract drm_encoder.[hc]

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Aug 25, 2016 at 05:53:51PM +0530, Archit Taneja wrote:
> 
> Hi,
> 
> On 08/18/2016 02:25 AM, Daniel Vetter wrote:
> > Same treatment as before. Only hiccup is drm_crtc_mask, which
> > unfortunately can't be resolved until drm_crtc.h is less of a monster.
> > Untangle the header loop with a forward delcaration for that static
> 
> s/delcaration/declaration
> 
> > inline.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >   Documentation/gpu/drm-kms.rst       |   9 ++
> >   drivers/gpu/drm/Makefile            |   3 +-
> >   drivers/gpu/drm/drm_crtc.c          | 193 -------------------------------
> >   drivers/gpu/drm/drm_crtc_internal.h |  10 +-
> >   drivers/gpu/drm/drm_encoder.c       | 220 ++++++++++++++++++++++++++++++++++++
> >   include/drm/drm_crtc.h              | 134 +---------------------
> >   include/drm/drm_encoder.h           | 167 +++++++++++++++++++++++++++
> >   7 files changed, 407 insertions(+), 329 deletions(-)
> >   create mode 100644 drivers/gpu/drm/drm_encoder.c
> >   create mode 100644 include/drm/drm_encoder.h
> > 
> 
> <snip>
> 
> > +
> > +/**
> > + * drm_encoder_init - Init a preallocated encoder
> > + * @dev: drm device
> > + * @encoder: the encoder to init
> > + * @funcs: callbacks for this encoder
> > + * @encoder_type: user visible type of the encoder
> > + * @name: printf style format string for the encoder name, or NULL for default name
> > + *
> > + * Initialises a preallocated encoder. Encoder should be
> > + * subclassed as part of driver encoder objects.
> > + *
> > + * Returns:
> > + * Zero on success, error code on failure.
> > + */
> > +int drm_encoder_init(struct drm_device *dev,
> > +		      struct drm_encoder *encoder,
> > +		      const struct drm_encoder_funcs *funcs,
> > +		      int encoder_type, const char *name, ...)
> 
> Alignment with the open parentheses is needed here.
> 
> > +{
> > +	int ret;
> > +
> > +	drm_modeset_lock_all(dev);
> > +
> > +	ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
> > +	if (ret)
> > +		goto out_unlock;
> > +
> > +	encoder->dev = dev;
> > +	encoder->encoder_type = encoder_type;
> > +	encoder->funcs = funcs;
> > +	if (name) {
> > +		va_list ap;
> > +
> > +		va_start(ap, name);
> > +		encoder->name = kvasprintf(GFP_KERNEL, name, ap);
> > +		va_end(ap);
> > +	} else {
> > +		encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
> > +					  drm_encoder_enum_list[encoder_type].name,
> > +					  encoder->base.id);
> > +	}
> > +	if (!encoder->name) {
> > +		ret = -ENOMEM;
> > +		goto out_put;
> > +	}
> > +
> > +	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
> > +	encoder->index = dev->mode_config.num_encoder++;
> > +
> > +out_put:
> > +	if (ret)
> > +		drm_mode_object_unregister(dev, &encoder->base);
> > +
> > +out_unlock:
> > +	drm_modeset_unlock_all(dev);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_encoder_init);
> > +
> > +/**
> > + * drm_encoder_cleanup - cleans up an initialised encoder
> > + * @encoder: encoder to cleanup
> > + *
> > + * Cleans up the encoder but doesn't free the object.
> > + */
> > +void drm_encoder_cleanup(struct drm_encoder *encoder)
> > +{
> > +	struct drm_device *dev = encoder->dev;
> > +
> > +	/* Note that the encoder_list is considered to be static; should we
> > +	 * remove the drm_encoder at runtime we would have to decrement all
> > +	 * the indices on the drm_encoder after us in the encoder_list.
> > +	 */
> > +
> > +	drm_modeset_lock_all(dev);
> > +	drm_mode_object_unregister(dev, &encoder->base);
> > +	kfree(encoder->name);
> > +	list_del(&encoder->head);
> > +	dev->mode_config.num_encoder--;
> > +	drm_modeset_unlock_all(dev);
> > +
> > +	memset(encoder, 0, sizeof(*encoder));
> > +}
> > +EXPORT_SYMBOL(drm_encoder_cleanup);
> > +
> > +static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
> > +{
> > +	struct drm_connector *connector;
> > +	struct drm_device *dev = encoder->dev;
> > +	bool uses_atomic = false;
> > +
> > +	/* For atomic drivers only state objects are synchronously updated and
> > +	 * protected by modeset locks, so check those first. */
> > +	drm_for_each_connector(connector, dev) {
> > +		if (!connector->state)
> > +			continue;
> > +
> > +		uses_atomic = true;
> > +
> > +		if (connector->state->best_encoder != encoder)
> > +			continue;
> > +
> > +		return connector->state->crtc;
> > +	}
> > +
> > +	/* Don't return stale data (e.g. pending async disable). */
> > +	if (uses_atomic)
> > +		return NULL;
> > +
> > +	return encoder->crtc;
> > +}
> > +
> > +/**
> > + * drm_mode_getencoder - get encoder configuration
> > + * @dev: drm device for the ioctl
> > + * @data: data pointer for the ioctl
> > + * @file_priv: drm file for the ioctl call
> > + *
> > + * Construct a encoder configuration structure to return to the user.
> > + *
> > + * Called by the user via ioctl.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_mode_getencoder(struct drm_device *dev, void *data,
> > +			struct drm_file *file_priv)
> > +{
> > +	struct drm_mode_get_encoder *enc_resp = data;
> > +	struct drm_encoder *encoder;
> > +	struct drm_crtc *crtc;
> > +
> > +	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > +		return -EINVAL;
> > +
> > +	encoder = drm_encoder_find(dev, enc_resp->encoder_id);
> > +	if (!encoder)
> > +		return -ENOENT;
> > +
> > +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > +	crtc = drm_encoder_get_crtc(encoder);
> > +	if (crtc)
> > +		enc_resp->crtc_id = crtc->base.id;
> > +	else
> > +		enc_resp->crtc_id = 0;
> > +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > +
> > +	enc_resp->encoder_type = encoder->encoder_type;
> > +	enc_resp->encoder_id = encoder->base.id;
> > +	enc_resp->possible_crtcs = encoder->possible_crtcs;
> > +	enc_resp->possible_clones = encoder->possible_clones;
> > +
> > +	return 0;
> > +}
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 3fa0275e509f..61d81fb3c8fc 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -40,6 +40,7 @@
> >   #include <drm/drm_framebuffer.h>
> >   #include <drm/drm_modes.h>
> >   #include <drm/drm_connector.h>
> > +#include <drm/drm_encoder.h>
> > 
> >   struct drm_device;
> >   struct drm_mode_set;
> > @@ -662,97 +663,6 @@ struct drm_crtc {
> >   };
> > 
> >   /**
> > - * struct drm_encoder_funcs - encoder controls
> > - *
> > - * Encoders sit between CRTCs and connectors.
> > - */
> > -struct drm_encoder_funcs {
> > -	/**
> > -	 * @reset:
> > -	 *
> > -	 * Reset encoder hardware and software state to off. This function isn't
> > -	 * called by the core directly, only through drm_mode_config_reset().
> > -	 * It's not a helper hook only for historical reasons.
> > -	 */
> > -	void (*reset)(struct drm_encoder *encoder);
> > -
> > -	/**
> > -	 * @destroy:
> > -	 *
> > -	 * Clean up encoder resources. This is only called at driver unload time
> > -	 * through drm_mode_config_cleanup() since an encoder cannot be
> > -	 * hotplugged in DRM.
> > -	 */
> > -	void (*destroy)(struct drm_encoder *encoder);
> > -
> > -	/**
> > -	 * @late_register:
> > -	 *
> > -	 * This optional hook can be used to register additional userspace
> > -	 * interfaces attached to the encoder like debugfs interfaces.
> > -	 * It is called late in the driver load sequence from drm_dev_register().
> > -	 * Everything added from this callback should be unregistered in
> > -	 * the early_unregister callback.
> > -	 *
> > -	 * Returns:
> > -	 *
> > -	 * 0 on success, or a negative error code on failure.
> > -	 */
> > -	int (*late_register)(struct drm_encoder *encoder);
> > -
> > -	/**
> > -	 * @early_unregister:
> > -	 *
> > -	 * This optional hook should be used to unregister the additional
> > -	 * userspace interfaces attached to the encoder from
> > -	 * late_unregister(). It is called from drm_dev_unregister(),
> > -	 * early in the driver unload sequence to disable userspace access
> > -	 * before data structures are torndown.
> > -	 */
> > -	void (*early_unregister)(struct drm_encoder *encoder);
> > -};
> > -
> > -/**
> > - * struct drm_encoder - central DRM encoder structure
> > - * @dev: parent DRM device
> > - * @head: list management
> > - * @base: base KMS object
> > - * @name: human readable name, can be overwritten by the driver
> > - * @encoder_type: one of the DRM_MODE_ENCODER_<foo> types in drm_mode.h
> > - * @possible_crtcs: bitmask of potential CRTC bindings
> > - * @possible_clones: bitmask of potential sibling encoders for cloning
> > - * @crtc: currently bound CRTC
> > - * @bridge: bridge associated to the encoder
> > - * @funcs: control functions
> > - * @helper_private: mid-layer private data
> > - *
> > - * CRTCs drive pixels to encoders, which convert them into signals
> > - * appropriate for a given connector or set of connectors.
> > - */
> > -struct drm_encoder {
> > -	struct drm_device *dev;
> > -	struct list_head head;
> > -
> > -	struct drm_mode_object base;
> > -	char *name;
> > -	int encoder_type;
> > -
> > -	/**
> > -	 * @index: Position inside the mode_config.list, can be used as an array
> > -	 * index. It is invariant over the lifetime of the encoder.
> > -	 */
> > -	unsigned index;
> > -
> > -	uint32_t possible_crtcs;
> > -	uint32_t possible_clones;
> > -
> > -	struct drm_crtc *crtc;
> > -	struct drm_bridge *bridge;
> > -	const struct drm_encoder_funcs *funcs;
> > -	const struct drm_encoder_helper_funcs *helper_private;
> > -};
> > -
> > -/**
> >    * struct drm_plane_state - mutable plane state
> >    * @plane: backpointer to the plane
> >    * @crtc: currently bound CRTC, NULL if disabled
> > @@ -2091,7 +2001,6 @@ struct drm_mode_config {
> >   		for_each_if ((encoder_mask) & (1 << drm_encoder_index(encoder)))
> > 
> >   #define obj_to_crtc(x) container_of(x, struct drm_crtc, base)
> > -#define obj_to_encoder(x) container_of(x, struct drm_encoder, base)
> >   #define obj_to_mode(x) container_of(x, struct drm_display_mode, base)
> >   #define obj_to_fb(x) container_of(x, struct drm_framebuffer, base)
> >   #define obj_to_property(x) container_of(x, struct drm_property, base)
> > @@ -2136,37 +2045,6 @@ static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc)
> >   	return 1 << drm_crtc_index(crtc);
> >   }
> > 
> > -extern __printf(5, 6)
> > -int drm_encoder_init(struct drm_device *dev,
> > -		     struct drm_encoder *encoder,
> > -		     const struct drm_encoder_funcs *funcs,
> > -		     int encoder_type, const char *name, ...);
> > -
> > -/**
> > - * drm_encoder_index - find the index of a registered encoder
> > - * @encoder: encoder to find index for
> > - *
> > - * Given a registered encoder, return the index of that encoder within a DRM
> > - * device's list of encoders.
> > - */
> > -static inline unsigned int drm_encoder_index(struct drm_encoder *encoder)
> > -{
> > -	return encoder->index;
> > -}
> > -
> > -/**
> > - * drm_encoder_crtc_ok - can a given crtc drive a given encoder?
> > - * @encoder: encoder to test
> > - * @crtc: crtc to test
> > - *
> > - * Return false if @encoder can't be driven by @crtc, true otherwise.
> > - */
> > -static inline bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
> > -				       struct drm_crtc *crtc)
> > -{
> > -	return !!(encoder->possible_crtcs & drm_crtc_mask(crtc));
> > -}
> > -
> >   extern __printf(8, 9)
> >   int drm_universal_plane_init(struct drm_device *dev,
> >   			     struct drm_plane *plane,
> > @@ -2202,8 +2080,6 @@ extern void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> >   extern int drm_crtc_force_disable(struct drm_crtc *crtc);
> >   extern int drm_crtc_force_disable_all(struct drm_device *dev);
> > 
> > -extern void drm_encoder_cleanup(struct drm_encoder *encoder);
> > -
> >   extern void drm_mode_config_init(struct drm_device *dev);
> >   extern void drm_mode_config_reset(struct drm_device *dev);
> >   extern void drm_mode_config_cleanup(struct drm_device *dev);
> > @@ -2315,14 +2191,6 @@ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
> >   	return mo ? obj_to_crtc(mo) : NULL;
> >   }
> > 
> > -static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
> > -	uint32_t id)
> > -{
> > -	struct drm_mode_object *mo;
> > -	mo = drm_mode_object_find(dev, id, DRM_MODE_OBJECT_ENCODER);
> > -	return mo ? obj_to_encoder(mo) : NULL;
> > -}
> > -
> >   static inline struct drm_property *drm_property_find(struct drm_device *dev,
> >   		uint32_t id)
> >   {
> > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > new file mode 100644
> > index 000000000000..2712fd1a686b
> > --- /dev/null
> > +++ b/include/drm/drm_encoder.h
> > @@ -0,0 +1,167 @@
> > +/*
> > + * Copyright (c) 2016 Intel Corporation
> > + *
> > + * Permission to use, copy, modify, distribute, and sell this software and its
> > + * documentation for any purpose is hereby granted without fee, provided that
> > + * the above copyright notice appear in all copies and that both that copyright
> > + * notice and this permission notice appear in supporting documentation, and
> > + * that the name of the copyright holders not be used in advertising or
> > + * publicity pertaining to distribution of the software without specific,
> > + * written prior permission.  The copyright holders make no representations
> > + * about the suitability of this software for any purpose.  It is provided "as
> > + * is" without express or implied warranty.
> > + *
> > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > + * OF THIS SOFTWARE.
> > + */
> > +
> > +#ifndef __DRM_ENCODER_H__
> > +#define __DRM_ENCODER_H__
> > +
> > +#include <linux/list.h>
> > +#include <linux/ctype.h>
> > +#include <drm/drm_modeset.h>
> > +
> > +/**
> > + * struct drm_encoder_funcs - encoder controls
> > + *
> > + * Encoders sit between CRTCs and connectors.
> > + */
> > +struct drm_encoder_funcs {
> > +	/**
> > +	 * @reset:
> > +	 *
> > +	 * Reset encoder hardware and software state to off. This function isn't
> > +	 * called by the core directly, only through drm_mode_config_reset().
> > +	 * It's not a helper hook only for historical reasons.
> > +	 */
> > +	void (*reset)(struct drm_encoder *encoder);
> > +
> > +	/**
> > +	 * @destroy:
> > +	 *
> > +	 * Clean up encoder resources. This is only called at driver unload time
> > +	 * through drm_mode_config_cleanup() since an encoder cannot be
> > +	 * hotplugged in DRM.
> > +	 */
> > +	void (*destroy)(struct drm_encoder *encoder);
> > +
> > +	/**
> > +	 * @late_register:
> > +	 *
> > +	 * This optional hook can be used to register additional userspace
> > +	 * interfaces attached to the encoder like debugfs interfaces.
> > +	 * It is called late in the driver load sequence from drm_dev_register().
> > +	 * Everything added from this callback should be unregistered in
> > +	 * the early_unregister callback.
> > +	 *
> > +	 * Returns:
> > +	 *
> > +	 * 0 on success, or a negative error code on failure.
> > +	 */
> > +	int (*late_register)(struct drm_encoder *encoder);
> > +
> > +	/**
> > +	 * @early_unregister:
> > +	 *
> > +	 * This optional hook should be used to unregister the additional
> > +	 * userspace interfaces attached to the encoder from
> > +	 * late_unregister(). It is called from drm_dev_unregister(),
> > +	 * early in the driver unload sequence to disable userspace access
> > +	 * before data structures are torndown.
> > +	 */
> > +	void (*early_unregister)(struct drm_encoder *encoder);
> > +};
> > +
> > +/**
> > + * struct drm_encoder - central DRM encoder structure
> > + * @dev: parent DRM device
> > + * @head: list management
> > + * @base: base KMS object
> > + * @name: human readable name, can be overwritten by the driver
> > + * @encoder_type: one of the DRM_MODE_ENCODER_<foo> types in drm_mode.h
> > + * @possible_crtcs: bitmask of potential CRTC bindings
> > + * @possible_clones: bitmask of potential sibling encoders for cloning
> > + * @crtc: currently bound CRTC
> > + * @bridge: bridge associated to the encoder
> > + * @funcs: control functions
> > + * @helper_private: mid-layer private data
> > + *
> > + * CRTCs drive pixels to encoders, which convert them into signals
> > + * appropriate for a given connector or set of connectors.
> > + */
> > +struct drm_encoder {
> > +	struct drm_device *dev;
> > +	struct list_head head;
> > +
> > +	struct drm_mode_object base;
> > +	char *name;
> > +	int encoder_type;
> > +
> > +	/**
> > +	 * @index: Position inside the mode_config.list, can be used as an array
> > +	 * index. It is invariant over the lifetime of the encoder.
> > +	 */
> > +	unsigned index;
> > +
> > +	uint32_t possible_crtcs;
> > +	uint32_t possible_clones;
> > +
> > +	struct drm_crtc *crtc;
> > +	struct drm_bridge *bridge;
> > +	const struct drm_encoder_funcs *funcs;
> > +	const struct drm_encoder_helper_funcs *helper_private;
> > +};
> > +
> > +#define obj_to_encoder(x) container_of(x, struct drm_encoder, base)
> > +
> > +__printf(5, 6)
> > +int drm_encoder_init(struct drm_device *dev,
> > +		     struct drm_encoder *encoder,
> > +		     const struct drm_encoder_funcs *funcs,
> > +		     int encoder_type, const char *name, ...);
> > +
> > +/**
> > + * drm_encoder_index - find the index of a registered encoder
> > + * @encoder: encoder to find index for
> > + *
> > + * Given a registered encoder, return the index of that encoder within a DRM
> > + * device's list of encoders.
> > + */
> > +static inline unsigned int drm_encoder_index(struct drm_encoder *encoder)
> > +{
> > +	return encoder->index;
> > +}
> > +
> > +/* FIXME: We have an include file mess still, drm_crtc.h needs untangling. */
> > +static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc);
> > +
> > +/**
> > + * drm_encoder_crtc_ok - can a given crtc drive a given encoder?
> > + * @encoder: encoder to test
> > + * @crtc: crtc to test
> > + *
> > + * Return false if @encoder can't be driven by @crtc, true otherwise.
> > + */
> > +static inline bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
> > +				       struct drm_crtc *crtc)
> > +{
> > +	return !!(encoder->possible_crtcs & drm_crtc_mask(crtc));
> > +}
> > +
> > +static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
> > +	uint32_t id)
> 
> and here.
> 
> > +{
> > +	struct drm_mode_object *mo;
> > +	mo = drm_mode_object_find(dev, id, DRM_MODE_OBJECT_ENCODER);
> 
> checkpatch --strict asks for a blank line above too. Otherwise:
> 
> Reviewed-by: Archit Taneja <architt@codeaurora.org>

I intentionally don't reformat/touch code when moving it, at all. Ok if I
just fix the typo in the commit message?
-Daniel

> 
> Thanks,
> Archit
> 
> > +	return mo ? obj_to_encoder(mo) : NULL;
> > +}
> > +
> > +void drm_encoder_cleanup(struct drm_encoder *encoder);
> > +
> > +#endif
> > 
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel