| Message ID | 1515117959-18068-3-git-send-email-hyun.kwon@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 :-) -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
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. :-) 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
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
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_ */
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> --- 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