Re: [PATCH 02/10] drm: xlnx: Add xlnx crtc of Xilinx DRM KMS

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jan 11, 2018 at 02:04:47AM +0000, Hyun Kwon wrote:
> Hi Daniel,
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Tuesday, January 09, 2018 1:38 AM
> > To: Hyun Kwon <hyunk@xilinx.com>
> > Cc: dri-devel@lists.freedesktop.org; devicetree@vger.kernel.org; Michal
> > Simek <michal.simek@xilinx.com>
> > Subject: Re: [PATCH 02/10] drm: xlnx: Add xlnx crtc of Xilinx DRM KMS
> > 
> > On Thu, Jan 04, 2018 at 06:05:51PM -0800, Hyun Kwon wrote:
> > > xlnx_crtc is a part of Xilinx DRM KMS and a layer that
> > > provides some interface between the Xilinx DRM KMS and
> > > crtc drivers.
> > >
> > > Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
> > 
> > Personal style, but I don't see much value in these small helpers.
> > Splitting them from the main driver at least makes reading the patches a
> > bit harder. But no strong opinion, just a bikeshed, feel free to ignore
> > :-)
> 
> I also don't, but some reviewers prefer this way. I'll leave it this way for now. :-)

Patch splitting is good imo, but I don't like free-standing new code. Imo
all functions you're adding need to have a caller (or at least be part of
a function table), so that it's clear how they're used. Without the users
being known it's not possible to review the code, rendering the splitting
pointless.

But splitting itself isn't a bad idea, just not splitting so much that the
individual patches dont' make sense anymore :-)
-Daniel
> 
> Thanks,
> -hyun
> 
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/xlnx/xlnx_crtc.c | 194
> > +++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/xlnx/xlnx_crtc.h |  70 ++++++++++++++
> > >  2 files changed, 264 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/xlnx/xlnx_crtc.c
> > >  create mode 100644 drivers/gpu/drm/xlnx/xlnx_crtc.h
> > >
> > > diff --git a/drivers/gpu/drm/xlnx/xlnx_crtc.c
> > b/drivers/gpu/drm/xlnx/xlnx_crtc.c
> > > new file mode 100644
> > > index 0000000..57ee939
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xlnx/xlnx_crtc.c
> > > @@ -0,0 +1,194 @@
> > > +/*
> > > + * Xilinx DRM crtc driver
> > > + *
> > > + *  Copyright (C) 2017 - 2018 Xilinx, Inc.
> > > + *
> > > + *  Author: Hyun Woo Kwon <hyun.kwon@xilinx.com>
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0
> > > + */
> > > +
> > > +#include <drm/drmP.h>
> > > +
> > > +#include <linux/list.h>
> > > +
> > > +#include "xlnx_crtc.h"
> > > +
> > > +/*
> > > + * Overview
> > > + * --------
> > > + *
> > > + * The Xilinx CRTC layer is to enable the custom interface to CRTC drivers.
> > > + * The interface is used by Xilinx DRM driver where it needs CRTC
> > > + * functionailty. CRTC drivers should attach the desired callbacks
> > > + * to struct xlnx_crtc and register the xlnx_crtc with correcsponding
> > > + * drm_device. It's highly recommended CRTC drivers register all
> > callbacks
> > > + * even though many of them are optional.
> > > + * The CRTC helper simply walks through the registered CRTC device,
> > > + * and call the callbacks.
> > > + */
> > > +
> > > +/**
> > > + * struct xlnx_crtc_helper - Xilinx CRTC helper
> > > + * @xlnx_crtcs: list of Xilinx CRTC devices
> > > + * @lock: lock to protect @xlnx_crtcs
> > > + * @drm: back pointer to DRM core
> > > + */
> > > +struct xlnx_crtc_helper {
> > > +	struct list_head xlnx_crtcs;
> > > +	struct mutex lock; /* lock for @xlnx_crtcs */
> > > +	struct drm_device *drm;
> > > +};
> > > +
> > > +#define XLNX_CRTC_MAX_HEIGHT_WIDTH	UINT_MAX
> > > +
> > > +int xlnx_crtc_helper_enable_vblank(struct xlnx_crtc_helper *helper,
> > > +				   unsigned int crtc_id)
> > > +{
> > > +	struct xlnx_crtc *crtc;
> > > +
> > > +	list_for_each_entry(crtc, &helper->xlnx_crtcs, list)
> > > +		if (drm_crtc_index(&crtc->crtc) == crtc_id)
> > > +			if (crtc->enable_vblank)
> > > +				return crtc->enable_vblank(crtc);
> > > +	return -ENODEV;
> > > +}
> > > +
> > > +void xlnx_crtc_helper_disable_vblank(struct xlnx_crtc_helper *helper,
> > > +				     unsigned int crtc_id)
> > > +{
> > > +	struct xlnx_crtc *crtc;
> > > +
> > > +	list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> > > +		if (drm_crtc_index(&crtc->crtc) == crtc_id) {
> > > +			if (crtc->disable_vblank)
> > > +				crtc->disable_vblank(crtc);
> > > +			return;
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +unsigned int xlnx_crtc_helper_get_align(struct xlnx_crtc_helper *helper)
> > > +{
> > > +	struct xlnx_crtc *crtc;
> > > +	unsigned int align = 1, tmp;
> > > +
> > > +	list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> > > +		if (crtc->get_align) {
> > > +			tmp = crtc->get_align(crtc);
> > > +			align = ALIGN(align, tmp);
> > > +		}
> > > +	}
> > > +
> > > +	return align;
> > > +}
> > > +
> > > +u64 xlnx_crtc_helper_get_dma_mask(struct xlnx_crtc_helper *helper)
> > > +{
> > > +	struct xlnx_crtc *crtc;
> > > +	u64 mask = DMA_BIT_MASK(sizeof(dma_addr_t) * 8), tmp;
> > > +
> > > +	list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> > > +		if (crtc->get_dma_mask) {
> > > +			tmp = crtc->get_dma_mask(crtc);
> > > +			mask = min(mask, tmp);
> > > +		}
> > > +	}
> > > +
> > > +	return mask;
> > > +}
> > > +
> > > +int xlnx_crtc_helper_get_max_width(struct xlnx_crtc_helper *helper)
> > > +{
> > > +	struct xlnx_crtc *crtc;
> > > +	unsigned int width = XLNX_CRTC_MAX_HEIGHT_WIDTH, tmp;
> > > +
> > > +	list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> > > +		if (crtc->get_max_width) {
> > > +			tmp = crtc->get_max_width(crtc);
> > > +			width = min(width, tmp);
> > > +		}
> > > +	}
> > > +
> > > +	return width;
> > > +}
> > > +
> > > +int xlnx_crtc_helper_get_max_height(struct xlnx_crtc_helper *helper)
> > > +{
> > > +	struct xlnx_crtc *crtc;
> > > +	unsigned int height = XLNX_CRTC_MAX_HEIGHT_WIDTH, tmp;
> > > +
> > > +	list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> > > +		if (crtc->get_max_height) {
> > > +			tmp = crtc->get_max_height(crtc);
> > > +			height = min(height, tmp);
> > > +		}
> > > +	}
> > > +
> > > +	return height;
> > > +}
> > > +
> > > +uint32_t xlnx_crtc_helper_get_format(struct xlnx_crtc_helper *helper)
> > > +{
> > > +	struct xlnx_crtc *crtc;
> > > +	u32 format = 0, tmp;
> > > +
> > > +	list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> > > +		if (crtc->get_format) {
> > > +			tmp = crtc->get_format(crtc);
> > > +			if (format && format != tmp)
> > > +				return 0;
> > > +			format = tmp;
> > > +		}
> > > +	}
> > > +
> > > +	return format;
> > > +}
> > > +
> > > +struct xlnx_crtc_helper *xlnx_crtc_helper_init(struct drm_device *drm)
> > > +{
> > > +	struct xlnx_crtc_helper *helper;
> > > +
> > > +	helper = devm_kzalloc(drm->dev, sizeof(*helper), GFP_KERNEL);
> > > +	if (!helper)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	INIT_LIST_HEAD(&helper->xlnx_crtcs);
> > > +	mutex_init(&helper->lock);
> > > +	helper->drm = drm;
> > > +
> > > +	return helper;
> > > +}
> > > +
> > > +void xlnx_crtc_helper_fini(struct drm_device *drm,
> > > +			   struct xlnx_crtc_helper *helper)
> > > +{
> > > +	if (WARN_ON(helper->drm != drm))
> > > +		return;
> > > +
> > > +	if (WARN_ON(!list_empty(&helper->xlnx_crtcs)))
> > > +		return;
> > > +
> > > +	mutex_destroy(&helper->lock);
> > > +	devm_kfree(drm->dev, helper);
> > > +}
> > > +
> > > +void xlnx_crtc_register(struct drm_device *drm, struct xlnx_crtc *crtc)
> > > +{
> > > +	struct xlnx_crtc_helper *helper = xlnx_get_crtc_helper(drm);
> > > +
> > > +	mutex_lock(&helper->lock);
> > > +	list_add_tail(&crtc->list, &helper->xlnx_crtcs);
> > > +	mutex_unlock(&helper->lock);
> > > +}
> > > +EXPORT_SYMBOL_GPL(xlnx_crtc_register);
> > > +
> > > +void xlnx_crtc_unregister(struct drm_device *drm, struct xlnx_crtc *crtc)
> > > +{
> > > +	struct xlnx_crtc_helper *helper = xlnx_get_crtc_helper(drm);
> > > +
> > > +	mutex_lock(&helper->lock);
> > > +	list_del(&crtc->list);
> > > +	mutex_unlock(&helper->lock);
> > > +}
> > > +EXPORT_SYMBOL_GPL(xlnx_crtc_unregister);
> > > diff --git a/drivers/gpu/drm/xlnx/xlnx_crtc.h
> > b/drivers/gpu/drm/xlnx/xlnx_crtc.h
> > > new file mode 100644
> > > index 0000000..db7404e
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xlnx/xlnx_crtc.h
> > > @@ -0,0 +1,70 @@
> > > +/*
> > > + * Xilinx DRM crtc header
> > > + *
> > > + *  Copyright (C) 2017 - 2018 Xilinx, Inc.
> > > + *
> > > + *  Author: Hyun Woo Kwon <hyun.kwon@xilinx.com>
> > > + *
> > > + * SPDX-License-Identifier: GPL-2.0
> > > + */
> > > +
> > > +#ifndef _XLNX_CRTC_H_
> > > +#define _XLNX_CRTC_H_
> > > +
> > > +/**
> > > + * struct xlnx_crtc - Xilinx CRTC device
> > > + * @crtc: DRM CRTC device
> > > + * @list: list node for Xilinx CRTC device list
> > > + * @enable_vblank: Enable vblank
> > > + * @disable_vblank: Disable vblank
> > > + * @get_align: Get the alignment requirement of CRTC device
> > > + * @get_dma_mask: Get the dma mask of CRTC device
> > > + * @get_max_width: Get the maximum supported width
> > > + * @get_max_height: Get the maximum supported height
> > > + * @get_format: Get the current format of CRTC device
> > > + */
> > > +struct xlnx_crtc {
> > > +	struct drm_crtc crtc;
> > > +	struct list_head list;
> > > +	int (*enable_vblank)(struct xlnx_crtc *crtc);
> > > +	void (*disable_vblank)(struct xlnx_crtc *crtc);
> > > +	unsigned int (*get_align)(struct xlnx_crtc *crtc);
> > > +	u64 (*get_dma_mask)(struct xlnx_crtc *crtc);
> > > +	int (*get_max_width)(struct xlnx_crtc *crtc);
> > > +	int (*get_max_height)(struct xlnx_crtc *crtc);
> > > +	uint32_t (*get_format)(struct xlnx_crtc *crtc);
> > > +};
> > > +
> > > +/*
> > > + * Helper functions: used within Xlnx DRM
> > > + */
> > > +
> > > +struct xlnx_crtc_helper;
> > > +
> > > +int xlnx_crtc_helper_enable_vblank(struct xlnx_crtc_helper *helper,
> > > +				   unsigned int crtc_id);
> > > +void xlnx_crtc_helper_disable_vblank(struct xlnx_crtc_helper *helper,
> > > +				     unsigned int crtc_id);
> > > +unsigned int xlnx_crtc_helper_get_align(struct xlnx_crtc_helper *helper);
> > > +u64 xlnx_crtc_helper_get_dma_mask(struct xlnx_crtc_helper *helper);
> > > +int xlnx_crtc_helper_get_max_width(struct xlnx_crtc_helper *helper);
> > > +int xlnx_crtc_helper_get_max_height(struct xlnx_crtc_helper *helper);
> > > +uint32_t xlnx_crtc_helper_get_format(struct xlnx_crtc_helper *helper);
> > > +
> > > +struct xlnx_crtc_helper *xlnx_crtc_helper_init(struct drm_device *drm);
> > > +void xlnx_crtc_helper_fini(struct drm_device *drm,
> > > +			   struct xlnx_crtc_helper *helper);
> > > +
> > > +/*
> > > + * CRTC registration: used by other sub-driver modules
> > > + */
> > > +
> > > +static inline struct xlnx_crtc *to_xlnx_crtc(struct drm_crtc *crtc)
> > > +{
> > > +	return container_of(crtc, struct xlnx_crtc, crtc);
> > > +}
> > > +
> > > +void xlnx_crtc_register(struct drm_device *drm, struct xlnx_crtc *crtc);
> > > +void xlnx_crtc_unregister(struct drm_device *drm, struct xlnx_crtc
> > *crtc);
> > > +
> > > +#endif /* _XLNX_CRTC_H_ */
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
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