Re: [PATCH] drm/omap: use omapdss low level API
On 2012-11-16 14:19, Greg KH wrote: > On Thu, Nov 15, 2012 at 06:00:58PM -0600, Rob Clark wrote: >> This patch changes the omapdrm KMS to bypass the omapdss "compat" >> layer and use the core omapdss API directly. This solves some layering >> issues that would cause unpin confusion vs GO bit status, because we >> would not know whether a particular pageflip or overlay update has hit >> the screen or not. Now instead we explicitly manage the GO bits in >> dispc and handle the vblank/framedone interrupts ourself so that we >> always know which buffers are being scanned out at any given time, and >> so on. >> >> As an added bonus, we no longer leave the last overlay buffer pinned >> when the display is disabled, and have been able to add the previously >> missing vblank event handling. >> >> Signed-off-by: Rob Clark >> --- >> Note: this patch applies on top of staging-next plus the "OMAPDSS: >> create compat layer" patch series: >> >> http://comments.gmane.org/gmane.linux.ports.arm.omap/89435 > > Hm, I can't take that patch set in staging-next, so how should this go > in? I wonder, could the omapdrm part go in for -rc2? It's "only" a driver in staging. I think that would be the easiest way. Perhaps it'd be even possible to split the patch into two, of which the first half would not depend on omapdss, but would prepare omapdrm for the change, thus making the patch for -rc2 smaller. Otherwise it's more tricky... Either wait for 3.9, or get omapdss, omapfb and omapdrm merged via the same tree. But which that would be? omapdss and omapfb usually go through fbdev tree. I don't know what its maintainer thinks of merging drm driver. And if the omapdrm depends on new drm changes, I guess fbdev is out of the question. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] drm/omap: use omapdss low level API
On Fri, Nov 16, 2012 at 12:44 AM, Archit Taneja wrote:
> On Friday 16 November 2012 05:30 AM, Rob Clark wrote:
>>
>> +static void omap_crtc_set_lcd_config(struct omap_overlay_manager *mgr,
>> + const struct dss_lcd_mgr_config *config)
>> +{
>> + struct omap_crtc *omap_crtc = container_of(mgr, struct omap_crtc,
>> mgr);
>> + DBG("%s", omap_crtc->name);
>> + dispc_mgr_set_lcd_config(omap_crtc->channel, config);
>
>
> Maybe you should move this dispc write also to omap_crtc_pre_apply, the same
> way it's done for timings. We'll also be confident about having the clocks
> required if this is called in pre_apply.
>
That was the first thing I had tried, although the order doesn't
really work out well and lcd config ends up getting set on the n+1'th
apply. Currently this is only called indirectly from
omap_encoder_set_enabled() -> dssdrv->enable() which always happens on
apply worker. In fact no dispc or dssdev fxn that changes state is
called outside of apply worker. (Only things like detect and reading
edid are happening outside of apply worker.)
When we get to the point of having more sophisticated panel drivers,
these callbacks from the panel are probably going to need some sort of
if (is_in_apply_worker()) sort of check.
>> +static void apply_worker(struct work_struct *work)
>> +{
>> + struct omap_crtc *omap_crtc =
>> + container_of(work, struct omap_crtc, apply_work);
>> + struct drm_crtc *crtc = &omap_crtc->base;
>> + struct drm_device *dev = crtc->dev;
>> + struct omap_drm_apply *apply, *n;
>> + bool need_apply;
>> +
>> + /*
>> +* Synchronize everything on mode_config.mutex, to keep
>> +* the callbacks and list modification all serialized
>> +* with respect to modesetting ioctls from userspace.
>> +*/
>> + mutex_lock(&dev->mode_config.mutex);
>> + dispc_runtime_get();
>> +
>> + /*
>> +* If we are still pending a previous update, wait.. when the
>> +* pending update completes, we get kicked again.
>> +*/
>> + if (omap_crtc->apply_irq.registered)
>> + goto out;
>> +
>> + /* finish up previous apply's: */
>> + list_for_each_entry_safe(apply, n,
>> + &omap_crtc->pending_applies, pending_node) {
>> + apply->post_apply(apply);
>> + list_del(&apply->pending_node);
>> + }
>> +
>> + need_apply = !list_empty(&omap_crtc->queued_applies);
>> +
>> + /* then handle the next round of of queued apply's: */
>> + list_for_each_entry_safe(apply, n,
>> + &omap_crtc->queued_applies, queued_node) {
>> + apply->pre_apply(apply);
>> + list_del(&apply->queued_node);
>> + apply->queued = false;
>> + list_add_tail(&apply->pending_node,
>> + &omap_crtc->pending_applies);
>> + }
>> +
>> + if (need_apply) {
>> + enum omap_channel channel = omap_crtc->channel;
>> +
>> + DBG("%s: GO", omap_crtc->name);
>> +
>> + if (dispc_mgr_is_enabled(channel)) {
>> + omap_irq_register(dev, &omap_crtc->apply_irq);
>> + dispc_mgr_go(channel);
>> + } else if (!dispc_mgr_go_busy(channel)) {
>
>
> I'm not clear about this. Why would GO be set in the first place if the
> manager isn't enabled? This could be replaced with a simple 'else'
>
Yeah, that extra if should be redundant
>
>> +static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
>> +{
>> + struct omap_crtc *omap_crtc =
>> + container_of(apply, struct omap_crtc, apply);
>> + struct drm_crtc *crtc = &omap_crtc->base;
>> + struct drm_encoder *encoder = NULL;
>> +
>> + DBG("%s: enabled=%d, full=%d", omap_crtc->name,
>> + omap_crtc->enabled, omap_crtc->full_update);
>> +
>> + if (omap_crtc->full_update) {
>
>
> If I get it right, full_update refers to manager properties that previously
> used to propogate downstream from the output driver to dispc, i.e, things
> like timings and so on. and the ones which aren't full_update are upstream
> properties like transparency keys, default_color and so on?
>
Well, it basically means power or timings have changed. So it means
closer to "full modeset" vs "pageflip".
But this function does deal with one mismatch between DRM and DSS.. in
DRM, everything gets setup in crtc -> downstream order, whereas
omapdss to accommodate more sophisticated panels does things in the
reverse order. So the crtc here propagates timing/power state change
to the encoder (which may turn into mgr op callbacks), rather than
relying on the encoder-helper fxn ptrs called from KMS.
And in fact other DRM drivers will need the same thing eventually if
they are to support the common panel/display framework. So I think
e
Re: [PATCH] drm/omap: use omapdss low level API
On Thu, Nov 15, 2012 at 06:00:58PM -0600, Rob Clark wrote: > This patch changes the omapdrm KMS to bypass the omapdss "compat" > layer and use the core omapdss API directly. This solves some layering > issues that would cause unpin confusion vs GO bit status, because we > would not know whether a particular pageflip or overlay update has hit > the screen or not. Now instead we explicitly manage the GO bits in > dispc and handle the vblank/framedone interrupts ourself so that we > always know which buffers are being scanned out at any given time, and > so on. > > As an added bonus, we no longer leave the last overlay buffer pinned > when the display is disabled, and have been able to add the previously > missing vblank event handling. > > Signed-off-by: Rob Clark > --- > Note: this patch applies on top of staging-next plus the "OMAPDSS: > create compat layer" patch series: > > http://comments.gmane.org/gmane.linux.ports.arm.omap/89435 Hm, I can't take that patch set in staging-next, so how should this go in? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm/omap: use omapdss low level API
On Friday 16 November 2012 12:14 PM, Archit Taneja wrote:
On Friday 16 November 2012 05:30 AM, Rob Clark wrote:
if (!obj)
@@ -565,14 +344,6 @@ static int dev_load(struct drm_device *dev,
unsigned long flags)
dev->dev_private = priv;
-ret = omapdss_compat_init();
-if (ret) {
-dev_err(dev->dev, "coult not init omapdss\n");
-dev->dev_private = NULL;
-kfree(priv);
-return ret;
-}
-
How can omapdss_compat_init/ininit already exist in the driver, and
removed in this patch?
Had a look at Tomi's series, please ignore this one.
Archit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drm/omap: use omapdss low level API
On Friday 16 November 2012 05:30 AM, Rob Clark wrote:
This patch changes the omapdrm KMS to bypass the omapdss "compat"
layer and use the core omapdss API directly. This solves some layering
issues that would cause unpin confusion vs GO bit status, because we
would not know whether a particular pageflip or overlay update has hit
the screen or not. Now instead we explicitly manage the GO bits in
dispc and handle the vblank/framedone interrupts ourself so that we
always know which buffers are being scanned out at any given time, and
so on.
As an added bonus, we no longer leave the last overlay buffer pinned
when the display is disabled, and have been able to add the previously
missing vblank event handling.
Signed-off-by: Rob Clark
---
Note: this patch applies on top of staging-next plus the "OMAPDSS:
create compat layer" patch series:
http://comments.gmane.org/gmane.linux.ports.arm.omap/89435
drivers/staging/omapdrm/Makefile | 1 +
drivers/staging/omapdrm/TODO | 3 -
drivers/staging/omapdrm/omap_connector.c | 111 +---
drivers/staging/omapdrm/omap_crtc.c | 472 +--
drivers/staging/omapdrm/omap_drv.c | 439 +---
drivers/staging/omapdrm/omap_drv.h | 140 +++--
drivers/staging/omapdrm/omap_encoder.c | 132 -
drivers/staging/omapdrm/omap_irq.c | 322 +
drivers/staging/omapdrm/omap_plane.c | 452 +++--
9 files changed, 1207 insertions(+), 865 deletions(-)
create mode 100644 drivers/staging/omapdrm/omap_irq.c
diff --git a/drivers/staging/omapdrm/Makefile b/drivers/staging/omapdrm/Makefile
index 1ca0e00..d85e058 100644
--- a/drivers/staging/omapdrm/Makefile
+++ b/drivers/staging/omapdrm/Makefile
@@ -5,6 +5,7 @@
ccflags-y := -Iinclude/drm -Werror
omapdrm-y := omap_drv.o \
+ omap_irq.o \
omap_debugfs.o \
omap_crtc.o \
omap_plane.o \
diff --git a/drivers/staging/omapdrm/TODO b/drivers/staging/omapdrm/TODO
index 938c788..abeeb00 100644
--- a/drivers/staging/omapdrm/TODO
+++ b/drivers/staging/omapdrm/TODO
@@ -17,9 +17,6 @@ TODO
. Revisit GEM sync object infrastructure.. TTM has some framework for this
already. Possibly this could be refactored out and made more common?
There should be some way to do this with less wheel-reinvention.
-. Review DSS vs KMS mismatches. The omap_dss_device is sort of part encoder,
- part connector. Which results in a bit of duct tape to fwd calls from
- encoder to connector. Possibly this could be done a bit better.
. Solve PM sequencing on resume. DMM/TILER must be reloaded before any
access is made from any component in the system. Which means on suspend
CRTC's should be disabled, and on resume the LUT should be reprogrammed
diff --git a/drivers/staging/omapdrm/omap_connector.c
b/drivers/staging/omapdrm/omap_connector.c
index 91edb3f..4cc9ee7 100644
--- a/drivers/staging/omapdrm/omap_connector.c
+++ b/drivers/staging/omapdrm/omap_connector.c
@@ -31,9 +31,10 @@
struct omap_connector {
struct drm_connector base;
struct omap_dss_device *dssdev;
+ struct drm_encoder *encoder;
};
-static inline void copy_timings_omap_to_drm(struct drm_display_mode *mode,
+void copy_timings_omap_to_drm(struct drm_display_mode *mode,
struct omap_video_timings *timings)
{
mode->clock = timings->pixel_clock;
@@ -64,7 +65,7 @@ static inline void copy_timings_omap_to_drm(struct
drm_display_mode *mode,
mode->flags |= DRM_MODE_FLAG_NVSYNC;
}
-static inline void copy_timings_drm_to_omap(struct omap_video_timings *timings,
+void copy_timings_drm_to_omap(struct omap_video_timings *timings,
struct drm_display_mode *mode)
{
timings->pixel_clock = mode->clock;
@@ -96,48 +97,7 @@ static inline void copy_timings_drm_to_omap(struct
omap_video_timings *timings,
timings->sync_pclk_edge = OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES;
}
-static void omap_connector_dpms(struct drm_connector *connector, int mode)
-{
- struct omap_connector *omap_connector = to_omap_connector(connector);
- struct omap_dss_device *dssdev = omap_connector->dssdev;
- int old_dpms;
-
- DBG("%s: %d", dssdev->name, mode);
-
- old_dpms = connector->dpms;
-
- /* from off to on, do from crtc to connector */
- if (mode < old_dpms)
- drm_helper_connector_dpms(connector, mode);
-
- if (mode == DRM_MODE_DPMS_ON) {
- /* store resume info for suspended displays */
- switch (dssdev->state) {
- case OMAP_DSS_DISPLAY_SUSPENDED:
- dssdev->activate_after_resume = true;
- break;
- case OMAP_DSS_DISPLAY_DISABLED: {
- int ret = dssdev->driver->enable(dssdev);
- if (ret) {
- DBG("%s: failed to enab
