Re: [PATCH 2/5] OMAPFB: simplify locking
On Fri, Dec 07, 2012 at 03:42:10PM +0200, Tomi Valkeinen wrote: > On 2012-12-07 14:53, Ville Syrjälä wrote: > > On Fri, Dec 07, 2012 at 01:55:06PM +0200, Tomi Valkeinen wrote: > >> Kernel lock verification code has lately detected possible circular > >> locking in omapfb. The exact problem is unclear, but omapfb's current > >> locking seems to be overly complex. > >> > >> This patch simplifies the locking in the following ways: > >> > >> - Remove explicit omapfb mem region locking. I couldn't figure out the > >> need for this, as long as we take care to take omapfb lock. > > > > I suppose the idea with that was that you wouldn't need the global > > omapfb lock, and also it was an rwsem so it allowed parallel access to > > the mem regions, unless the region size was being changed, in which > > case it took the write lock. I can't really remember what the reason > > for using an rwsem was, but I suppose there was one at the time. > > Right. Yes, I have no recollection either of the possible reason for it > =). Did we have multiple concurrerent users for the fbs? It still sounds > like a useless optimization, as all the region locks were only held for > a short time, as far as I saw. > > It could also be that we're missing something from the mainline kernel, > which we had in the Nokia kernel, and which would explain the need for > region locks. Yeah, perhaps there was some use for it at the time. Possibly the zero copy video playback benefited from it in the beginning. But IIRC almost everything was handled via the X server in the end, so I don't think there's any compelling reason for keeping the rwsem. -- Ville Syrjälä Intel OTC -- 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 2/5] OMAPFB: simplify locking
On Fri, Dec 07, 2012 at 01:55:06PM +0200, Tomi Valkeinen wrote: > Kernel lock verification code has lately detected possible circular > locking in omapfb. The exact problem is unclear, but omapfb's current > locking seems to be overly complex. > > This patch simplifies the locking in the following ways: > > - Remove explicit omapfb mem region locking. I couldn't figure out the > need for this, as long as we take care to take omapfb lock. I suppose the idea with that was that you wouldn't need the global omapfb lock, and also it was an rwsem so it allowed parallel access to the mem regions, unless the region size was being changed, in which case it took the write lock. I can't really remember what the reason for using an rwsem was, but I suppose there was one at the time. I think the only correctness issue with your patch is that you're opening up a race between omapfb_mmap and omapfb_setup_mem/store_size. -- Ville Syrjälä Intel OTC -- 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] ARM: add get_user() support for 8 byte types
On Mon, Nov 19, 2012 at 02:48:06PM +, Russell King - ARM Linux wrote: > On Mon, Nov 19, 2012 at 04:32:36PM +0200, Ville Syrjälä wrote: > > On Thu, Nov 15, 2012 at 02:39:41PM +, Arnd Bergmann wrote: > > > On Thursday 15 November 2012, Rob Clark wrote: > > > > > I still haven't heard a conclusive argument why we need to use > > > > > get_user() > > > > > rather than copy_from_user() in the DRM code. Is this about a fast > > > > > path > > > > > where you want to shave off a few cycles for each call, or does this > > > > > simplify the code structure, or something else? > > > > > > > > well, it is mostly because it seemed like a good idea to first try to > > > > solve the root issue, rather than having to fix things up in each > > > > driver when someone from x86-world introduces a 64b get_user().. > > > > > > As pointed out by hpa earlier, x86-32 doesn't have a 64b get_user > > > either. I don't think we have a lot of drivers that are used only > > > on 64-bit x86 and on 32-bit ARM but not on 32-bit x86. > > > > Ouch. I didn't realize that x86-32 doesn't have it. All the systems > > where I've run the new code are 64bit so I never noticed the problem. > > > > I see there was a patch [1] posted a long time ago to implement 64bit > > get_user() on x86-32. I wonder what happened to it? > > > > [1] https://lkml.org/lkml/2004/4/20/96 > > Wonderful lkml.org after four "Negotiating SSL connection..." messages > gives me under elinks... > what a wonderful site... please choose another LKML archive, preferably > one which works. Thanks. This one look like the same thing: http://article.gmane.org/gmane.linux.kernel/198823 -- Ville Syrjälä Intel OTC -- 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] ARM: add get_user() support for 8 byte types
On Thu, Nov 15, 2012 at 02:39:41PM +, Arnd Bergmann wrote: > On Thursday 15 November 2012, Rob Clark wrote: > > > I still haven't heard a conclusive argument why we need to use get_user() > > > rather than copy_from_user() in the DRM code. Is this about a fast path > > > where you want to shave off a few cycles for each call, or does this > > > simplify the code structure, or something else? > > > > well, it is mostly because it seemed like a good idea to first try to > > solve the root issue, rather than having to fix things up in each > > driver when someone from x86-world introduces a 64b get_user().. > > As pointed out by hpa earlier, x86-32 doesn't have a 64b get_user > either. I don't think we have a lot of drivers that are used only > on 64-bit x86 and on 32-bit ARM but not on 32-bit x86. Ouch. I didn't realize that x86-32 doesn't have it. All the systems where I've run the new code are 64bit so I never noticed the problem. I see there was a patch [1] posted a long time ago to implement 64bit get_user() on x86-32. I wonder what happened to it? [1] https://lkml.org/lkml/2004/4/20/96 -- Ville Syrjälä Intel OTC -- 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 1/2] drm: support for rotated scanout
On Tue, Sep 04, 2012 at 11:35:56AM -0500, Rob Clark wrote: > From: Rob Clark > > For drivers that can support rotated scanout, the extra parameter > checking in drm-core, while nice, tends to get confused. To solve > this drivers can set the crtc or plane invert_dimensions field so > that the dimension checking takes into account the rotation that > the driver is performing. > > v1: original > v2: remove invert_dimensions from plane, at Ville's suggestion. > Userspace can give rotated src coordinates, so invert_dimensions > is not required for planes. > > Signed-off-by: Rob Clark Sorry for the delay Rob, I somehow missed this. In any case, the patch looks OK to me. Reviewed-by: Ville Syrjälä -- Ville Syrjälä Intel OTC -- 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 2/2] Expose the OMAP Z-Order property through DRM
On Wed, Aug 15, 2012 at 03:18:02PM -0500, Rob Clark wrote:
> From: Andre Renaud
>
> Added support for zorder changes through DRM plane properties
>
> Signed-off-by: Andre Renaud
> Signed-off-by: Rob Clark
> ---
> drivers/staging/omapdrm/omap_drv.h |1 +
> drivers/staging/omapdrm/omap_plane.c | 19 +++
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/staging/omapdrm/omap_drv.h
> b/drivers/staging/omapdrm/omap_drv.h
> index b103d28..9dc72d1 100644
> --- a/drivers/staging/omapdrm/omap_drv.h
> +++ b/drivers/staging/omapdrm/omap_drv.h
> @@ -62,6 +62,7 @@ struct omap_drm_private {
>
> /* properties: */
> struct drm_property *rotation_prop;
> + struct drm_property *zorder_prop;
> };
>
> /* this should probably be in drm-core to standardize amongst drivers */
> diff --git a/drivers/staging/omapdrm/omap_plane.c
> b/drivers/staging/omapdrm/omap_plane.c
> index 6931d06..4bde639 100644
> --- a/drivers/staging/omapdrm/omap_plane.c
> +++ b/drivers/staging/omapdrm/omap_plane.c
> @@ -433,6 +433,15 @@ void omap_plane_install_properties(struct drm_plane
> *plane,
> priv->rotation_prop = prop;
> }
> drm_object_attach_property(obj, prop, 0);
> +
> +prop = priv->zorder_prop;
> +if (!prop) {
> + prop = drm_property_create_range(dev, 0, "zorder", 0, 3);
> + if (prop == NULL)
> + return;
> + priv->zorder_prop = prop;
> + }
> + drm_object_attach_property(obj, prop, 0);
> }
>
> int omap_plane_set_property(struct drm_plane *plane,
> @@ -452,6 +461,16 @@ int omap_plane_set_property(struct drm_plane *plane,
> ret = omap_plane_dpms(plane, DRM_MODE_DPMS_ON);
> else
> ret = 0;
> + } else if (property == priv->zorder_prop) {
> + struct omap_overlay *ovl = omap_plane->ovl;
> +
> + DBG("%s: zorder: %d", ovl->name, (uint32_t)val);
> + omap_plane->info.zorder = val;
What would happen when there's a conflicting assignment between two
planes?
I tried to think of a decent way to do this stuff, but some hardware
can have rather complicated stacking order limitations. One idea I
came up with was to have an enum prop on the crtc, where the individual
enum value names would somehow describe the whole stacking order within
the crtc. That way user space couldn't even try to use an unsupported
configuration. The downside is that user space would need to parse
those strings if it wants to do some automagic stacking order changes,
which means the string format would need some though.
--
Ville Syrjälä
Intel OTC
--
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: refcnt drm_framebuffer
On Tue, Jul 31, 2012 at 02:28:29PM -0500, Rob Clark wrote: > On Tue, Jul 31, 2012 at 12:47 PM, Chris Wilson > wrote: > > On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark wrote: > >> On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson > >> wrote: > >> > On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark > >> > wrote: > >> >> From: Rob Clark > >> >> > >> >> This simplifies drm fb lifetime, and if the crtc/plane needs to hold > >> >> a ref to the fb when disabling a pipe until the next vblank, this > >> >> avoids the need to make disabling an overlay synchronous. This is a > >> >> problem that shows up when userspace is using a drm plane to > >> >> implement a hw cursor.. making overlay disable synchronous causes > >> >> a performance problem when x11 is rapidly enabling/disabling the > >> >> hw cursor. But not making it synchronous opens up a race condition > >> >> for crashing if userspace turns around and immediately deletes the > >> >> fb. Refcnt'ing the fb makes it possible to solve this problem. > >> > > >> > Presumably you have a follow-on patch putting the new refcnt to use so > >> > that we can judge whether you truly need refcnting on the fb itself in > >> > addition to the refcnted object and the various hw bookkeeping that > >> > needs to be performed? > >> > >> Yes, I do.. although it is a bit experimental at this point, so not > >> really ready to be submitted as anything other than an RFC.. it is > >> part of omapdrm kms re-write to use dispc directly rather than go thru > >> omapdss. (With omapdss we don't hit this issue because disabling > >> overlays is forced to be synchronous.. so instead we have the > >> performance problem I mentioned.) > >> > >> I *could* just rely on the GEM refcnt, but that gets messier when you > >> take into account multi-planar formats. I suppose I also could have > >> my own internal refcnt'd object to hold the set of GEM objects > >> associated w/ the fb, but, well, that seems a bit silly. And > >> refcnt'ing the fb had been mentioned previously as a good thing to do > >> (I think it was danvet?) > > > > Sure, there are a few places in the code that have caused ordering > > issues in the past due to lack of refcnting the fb... But since you > > haven't fixed up those cases, I'm looking for justification for adding > > that extra bit of complexity. Adding a new interface and no users is > > just asking for trouble. > > hmm, I did realize that drm_plane cleanup only happens as a result of > drm_framebuffer_cleanup().. which doesn't work too well if the driver > is holding a ref to the fb :-/ > > so I guess at a minimum I need to fix plane cleanup to be part of > drm_fb_helper_restore_fbdev_mode() Your patch would still significantly change the behavior of drm_mode_rmfb(). Currently it disables all planes and crtcs which currently use the fb, and it removes the fb id from the idr so that no new users of the fb can appear afterwards. Not that I really like the current behaviour of drm_mode_rmfb(), but it's been like that always, so changing it doesn't seem acceptable. -- Ville Syrjälä Intel OTC -- 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] staging: drm/omap: add rotation properties
On Fri, Jun 29, 2012 at 07:17:23AM -0500, Rob Clark wrote: > On Fri, Jun 29, 2012 at 5:46 AM, Tomi Valkeinen wrote: > > On Wed, 2012-06-27 at 09:06 -0500, Rob Clark wrote: > >> From: Rob Clark > >> > >> Use tiled buffers for rotated/reflected scanout, with CRTC and plane > >> properties as the interface for userspace to configure rotation. > >> > >> Signed-off-by: Rob Clark > > > >> +/* this should probably be in drm-core to standardize amongst drivers */ > >> +#define DRM_ROTATE_0 0 > >> +#define DRM_ROTATE_90 1 > >> +#define DRM_ROTATE_180 2 > >> +#define DRM_ROTATE_270 3 > >> +#define DRM_REFLECT_X 4 > >> +#define DRM_REFLECT_Y 5 > > > > Are both reflect X and Y needed? You can get all the possible > > orientations with just one of the reflects. > > > > And I think the word "mirror" represents nicely what the reflect does, > > i.e. if you look at the mirror, the image you see is flipped > > horizontally. > > fwiw these values are aligned with what is used in userspace xrandr.. > an earlier version of this patch used just 3 bits, which where aligned > with what the omap hw uses and can describe all possible combinations > of mirroring and isomorphic rotation (x-invert, y-invert, and > xy-flip). But the advantage of the xrandr approach is you can more > easily leave out bits for rotation/mirroring modes that your hw does > not support.. for example if some hw supports only certain rotations > or does not support mirror/reflect. When I hear "mirror" I always think of it as the happening after rotation, but that's not how xrandr does things. I suppose "reflection" has more or less the same connotation for me, but since it's already used by xrandr, I know I have to do the necessary mental gymnastics. If you think about it in terms of matrix multiplication, xrandr does things like this: x' = Rot * Ref * x, whereas for me the more natural order (for whatever reason) would be x' = Ref * Rot * x, where x is the source coordinates, and x' the destination coordinates. IIRC in dss mirroring was specified in the post-rotation way, and the direction of the rotation was also opposite to xrandr (cw in dss, ccw in xrandr). So FWIW, my vote goes to "reflect" if we want to match xrandr semantics. Also I agree on the number of bits, six is better than three since it allows you to describe the hw capabilities. -- Ville Syrjälä Intel OTC -- 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 2/2] OMAPDSS: DISPC: Handle synclost errors in OMAP3
On Thu, Mar 15, 2012 at 05:18:52PM +0530, Chandrabhanu Mahapatra wrote:
> In OMAP3 DISPC video overlays suffer from some undocumented horizontal
> position
> and timing related limitations leading to SYNCLOST errors. Whenever the image
> window is moved towards the right of the screen SYNCLOST errors become
> frequent. Checks have been implemented to see that DISPC driver rejects
> configuration exceeding above limitations.
>
> This code was successfully tested on OMAP3. This code is written based on code
> written by Ville Syrjälä in Linux OMAP kernel. Ville
> Syrjälä had added checks for video overlay
> horizontal
> timing and DISPC horizontal blanking length limitations.
>
> Signed-off-by: Chandrabhanu Mahapatra
> ---
> drivers/video/omap2/dss/dispc.c | 67
> +--
> 1 files changed, 64 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index 5a1963e..ebfa613 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -1622,6 +1622,58 @@ static void calc_dma_rotation_offset(u8 rotation, bool
> mirror,
> }
> }
>
> +static int check_horiz_timing(enum omap_channel channel, u16 pos_x,
> + u16 width, u16 height, u16 out_width, u16 out_height)
> +{
> + int DS = DIV_ROUND_UP(height, out_height);
> + struct omap_dss_device *dssdev = dispc_mgr_get_device(channel);
> + struct omap_video_timings t = dssdev->panel.timings;
> + int pcd = REG_GET(DISPC_DIVISORo(channel), 7, 0);
pcd doesn't exist for TV out, which is the reason why you see
'lclk/pclk' ratio used instead in the harmattan kernel.
Another thing which still seems to be wrong in the upstream code is the
use of fclk in the scaling checks. It should be checking lclk instead.
> + unsigned long nonactive, val, blank;
> + static const u8 limits[3] = { 8, 10, 20 };
> + int i;
> +
> + nonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;
> +
> + /*
> + * Atleast DS-2 lines must have already been fetched
> + * before the display active video period starts.
> + */
> + val = (nonactive - pos_x) * pcd;
> + DSSDBG("(nonactive - pos_x) * pcd = %lu,"
> + " max(0, DS - 2) * width = %d\n",
> + val, max(0, DS - 2) * width);
> + if (val < max(0, DS - 2) * width)
> + return -EINVAL;
> +
> + /*
> + * Only one line can be fetched during the overlay active
> + * period, the rest have to be fetched during the inactive
> + * period.
> + */
> + val = nonactive * pcd;
> + DSSDBG("nonactive * pcd = %lu, max(0, DS - 1) * width = %d\n",
> + val, max(0, DS - 1) * width);
> + if (val < max(0, DS - 1) * width)
> + return -EINVAL;
> +
> + /*
> + * Atleast Ceil(DS) lines should have been loaded during
> + * PPL (screen width) + blanking period.
> + */
> + i = 0;
> + if (out_height < height)
> + i++;
> + if (out_width < width)
> + i++;
> + blank = (t.hbp + t.hsw + t.hfp) * pcd;
> + DSSDBG("blanking period + ppl = %lu (limit = %u)\n", blank, limits[i]);
> + if (blank <= limits[i])
> + return -EINVAL;
The comment and code here are totally out of sync. IIRC the hblank
length check came directly from the TRM. I have no idea if it's correct
for more recent OMAPs, or if it was just copy pasted from the
old TRM when the TRM got adjusted for more recent chips. That is why I
stuck it into a separate function in harmattan.
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
--
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 1/2] OMAPDSS: DISPC: Enable predecimation
On Thu, Mar 15, 2012 at 05:18:28PM +0530, Chandrabhanu Mahapatra wrote: > In OMAP3 and OMAP4, the DISPC Scaler can downscale an image up to 4 times, and > up to 2 times in OMAP2. However, with predecimation, the image can be reduced > to 16 times by fetching only the necessary pixels in memory. Then this > predecimated image can be downscaled further by the DISPC scaler. Now, where does that number 16 come from? IIRC the hardware can skip basically any number of pixels/rows. I certainly didn't add any such limit to the code in the harmattan kernel, and distinctly remember being able to downscale the N9/N950 UI even down to 1 pixel size :) > Based on the downscaling required, a prior calculation of predecimation values > for width and height of an image is done. Since, Predecimation reduces quality > of an image higher priorty is given to DISPC Scaler for downscaling. > > This code was successfully tested on OMAP2, OMAP3 and OMAP4. Horizontal and > vertical predecimation worked fine except for some synclost errors due to > undocumented errata in OMAP3 which are fixed later and skewed images were seen > on OMAP2 and OMAP3 during horizontal predecimation which will be addressed in > the future patches. All the rotation offset calculations still look suspiciously different to what is in the harmattan kernel. I remember that the original code was quite broken, and I fixed a lot of things when I was implementing pre-decimation and some rotation stuff for the N9/N950. Too bad I never managed to push that stuff upstream... -- Ville Syrjälä syrj...@sci.fi http://www.sci.fi/~syrjala/ -- 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: OMAP:DSS: possible bug in WAITFOR_VSYNC ioctl
On Fri, Nov 26, 2010 at 05:38:11PM +0530, ext Hiremath, Vaibhav wrote: > > > -Original Message- > > From: Hiremath, Vaibhav > > Sent: Friday, November 26, 2010 5:34 PM > > To: 'Måns Rullgård'; linux-omap@vger.kernel.org > > Subject: RE: OMAP:DSS: possible bug in WAITFOR_VSYNC ioctl > > > > > -Original Message- > > > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- > > > ow...@vger.kernel.org] On Behalf Of Måns Rullgård > > > Sent: Friday, November 26, 2010 2:09 PM > > > To: linux-omap@vger.kernel.org > > > Subject: Re: OMAP:DSS: possible bug in WAITFOR_VSYNC ioctl > > > > > > "Hiremath, Vaibhav" writes: > > > > > > >> -Original Message- > > > >> From: Ville Syrjälä [mailto:ville.syrj...@nokia.com] > > > >> Sent: Wednesday, November 24, 2010 10:01 PM > > > >> To: Hiremath, Vaibhav > > > >> Cc: Tomi Valkeinen; linux-omap@vger.kernel.org > > > >> Subject: Re: OMAP:DSS: possible bug in WAITFOR_VSYNC ioctl > > > >> > > > >> On Wed, Nov 24, 2010 at 03:39:44PM +0530, ext Hiremath, Vaibhav > > wrote: > > > >> > > > > >> > > -Original Message- > > > >> > > From: Tomi Valkeinen [mailto:tomi.valkei...@nokia.com] > > > >> > > Sent: Wednesday, November 24, 2010 2:28 PM > > > >> > > To: Hiremath, Vaibhav > > > >> > > Cc: linux-omap@vger.kernel.org > > > >> > > Subject: Re: OMAP:DSS: possible bug in WAITFOR_VSYNC ioctl > > > >> > > > > > >> > > On Tue, 2010-11-23 at 23:46 +0530, ext Hiremath, Vaibhav wrote: > > > >> > > > Hi, > > > > > > > >> > > > > > > > > As far as WAITFORGO is concerned, I think GO bit concept is > > > > something OMAP notion/term and doesn't make sense to standardize > > > > it. Atleast I am not aware of any other architecture having GO bit. > > > > > > Naming is minor detail. Feel free to suggest a better one. > > > > > [Hiremath, Vaibhav] If I fail to convince on this, then I think the only > > left option is to make WAITFORGO ioctl generic. And put a disclaimer on > > WAITFORVSYNC, it must not be used in panning use-case. > > > > > [Hiremath, Vaibhav] Also let me bring another point here, > > If I understand correctly most of the application libraries (DirectFB, X, > etc..) does use FBIO_WAITFORVSYNC to synchronize with HW, and manage ping > pong mechanism. DirectFB uses it also for waiting for vsync. > With this finding, in case of OMAP3 we have to use OMAPFB_WAITFORGO (breaking > standard applications). Applications using the standard fbdev API won't work with manual update displays anyway. You need omapfb specific code to handle it so having another small difference is not a big deal. In DirectFB the that's trivial since there's already a simple omap gfxdriver where you could override the default flip functionality with WAITFORGO based stuff. Or, as I said, you could add another standard ioctl and fix up userspace to use it where appropriate and if the kernel driver supports it. -- Ville Syrjälä -- 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: OMAP:DSS: possible bug in WAITFOR_VSYNC ioctl
On Wed, Nov 24, 2010 at 03:39:44PM +0530, ext Hiremath, Vaibhav wrote: > > > -Original Message- > > From: Tomi Valkeinen [mailto:tomi.valkei...@nokia.com] > > Sent: Wednesday, November 24, 2010 2:28 PM > > To: Hiremath, Vaibhav > > Cc: linux-omap@vger.kernel.org > > Subject: Re: OMAP:DSS: possible bug in WAITFOR_VSYNC ioctl > > > > On Tue, 2010-11-23 at 23:46 +0530, ext Hiremath, Vaibhav wrote: > > > Hi, > > > > > > While supporting one of customer I came across one interesting issue and > > finding with WAITFORVSYNC ioctl - > > > > > > Problem Statement - > > > --- > > > With WAITFORVSYNC lots of tearing artifacts are visible on LCD output, > > but WAITFORGO works fine without any issues. > > > > > > Debugging/Findings - > > > > > > > > > Technically both, WAITFORVSYNC and WAITFORGO wait on VSYNC interrupt - > > but there is slight difference in their implementation/processing. > > > > No, that's not quite right. > > > > WAITFORVSYNC waits for the next vsync. > > > [Hiremath, Vaibhav] Yes, certainly. > > > WAITFORGO waits until the the config changes for the particular overlay > > have been applied to the HW, which may take multiple vsyncs if there are > > already pending config changes. Or, if there are no changes to be > > applied, it doesn't wait at all. > > > [Hiremath, Vaibhav] Yes, completely agreed. But in the panning use-case where > if we assume there will be always config change when you enter WAIFORGO ioctl > it waits for next VSYNC, and as you mentioned it may wait for multiple vsyncs > to make sure that config change gets applied to HW. > > > > WAITFORGO ioctl ensures that dirty & shadow_dirty flags (software flag) > > are cleared, making sure that hardware state and software state always > > stays in sync. It makes 4 attempts to do so - inside loop it checks for > > dirty flags and call wait_for_vsync API. In ideal usecase scenario it > > should come out in single iteration. > > > > > Suggestions/Recommendation - > > > -- > > > > > > From User application point of view, user won't care about driver > > internal implementation. Application believes that WAITFORVSYNC will only > > return after displaying (DMAing) previous buffer and now with addition to > > FBIO_WAITFORVSYNC standard ioctl interface this is very well expected from > > user application as a standard behavior. > > > > > > I would recommend having WAITFORGO like implementation under > > WAITFORVSYNC, merging WAITFORGO with WAITFORVSYNC, and killing WAITFORGO > > (since we have FBIO_WAITFORVSYNC standard ioctl). > > > Also WAITFORGO ioctl is OMAP specific custom ioctl. > > > > I have to say that I'm not quite sure what WAITFORVSYNC should do. > [Hiremath, Vaibhav] Me neither. > > > The > > name implies that it should wait for the next vsync, which is what it > > does for omapfb. > > > > > Changing it to WAITFORGO would alter the behaviour. Sometimes it would > > not wait at all, sometimes it could wait for multiple vsyncs. > [Hiremath, Vaibhav] I am quite not sure, whether it makes sense from > application point of view to wait for vsync if config is not changed. What > could be the use-case for such requirement, where application won't change > anything but still would like to wait on vsync? > > And as far as wait on multiple vsync is concerned, it does make sense to coat > "WAITFORVSYNC ioctl makes sure that your change got applied to HW". > > I am not aware of other architectures, but I believe this is something OMAP > specific stuff. And for other platforms, WAITFORVSYNC means changes applied > to HW (why does apps care about whether it is vsync or anything else?). WAITFORVSYNC waits for the next vsync (or actually vblank in many cases). That's it. I don't see any point in trying to shoehorn other functionality into it. If you want to standardise WAITFORGO type of stuff then just add another standard ioctl for it. -- Ville Syrjälä -- 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] OMAP: DSS2: OMAPFB: Fix null pointer check
On Fri, Nov 12, 2010 at 12:47:15PM +0100, ext Samreen wrote: > A null pointer check added. > > Signed-off-by: Samreen > --- > drivers/video/omap2/omapfb/omapfb-main.c |2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/video/omap2/omapfb/omapfb-main.c > b/drivers/video/omap2/omapfb/omapfb-main.c > index 6a704f1..55bed89 100644 > --- a/drivers/video/omap2/omapfb/omapfb-main.c > +++ b/drivers/video/omap2/omapfb/omapfb-main.c > @@ -2133,6 +2133,8 @@ static int omapfb_parse_def_modes(struct omapfb2_device > *fbdev) > int r = 0; > > str = kmalloc(strlen(def_mode) + 1, GFP_KERNEL); > + if (str == NULL) > + return -EINVAL; > strcpy(str, def_mode); While you're at it you could change the code to use kstrdup(). -- Ville Syrjälä -- 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] OMAP:DSS:Add support for Additional color modes supported by OMAP4
On Wed, Aug 11, 2010 at 11:22:44AM +0200, ext K, Mythri P wrote:
>
> Hi Tomi,
> Can you please comment on the below patch . This is to add new color modes
> supported by OMAP4.
>
> Thanks and regards,
> Mythri.
> -Original Message-
> From: K, Mythri P
> Sent: Thursday, August 05, 2010 11:24 AM
> To: linux-omap@vger.kernel.org
> Cc: tomi.valkei...@nokia.com; Semwal, Sumit; K, Mythri P
> Subject: [PATCH] OMAP:DSS:Add support for Additional color modes supported by
> OMAP4
>
> From: Sumit semwal
>
> This patch adds support for new color modes that are supported by the
> video/graphics pipeline of OMAP4
>
> Signed-off-by: Mythri P K
> ---
> arch/arm/plat-omap/include/plat/display.h | 16 -
> drivers/video/omap2/dss/dispc.c | 53
> ++---
> drivers/video/omap2/dss/overlay.c |6 ++-
> 3 files changed, 59 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/display.h
> b/arch/arm/plat-omap/include/plat/display.h
> index 7a6eedd..ebf1020 100644
> --- a/arch/arm/plat-omap/include/plat/display.h
> +++ b/arch/arm/plat-omap/include/plat/display.h
> @@ -89,6 +89,12 @@ enum omap_color_mode {
> OMAP_DSS_COLOR_ARGB32 = 1 << 11, /* ARGB32 */
> OMAP_DSS_COLOR_RGBA32 = 1 << 12, /* RGBA32 */
> OMAP_DSS_COLOR_RGBX32 = 1 << 13, /* RGBx32 */
> + OMAP_DSS_COLOR_NV12 = 1 << 14, /* NV12 format: YUV 4:2:0 */
> + OMAP_DSS_COLOR_RGBA12 = 1 << 15, /* RGBA12 - */
> + OMAP_DSS_COLOR_XRGB12 = 1 << 16, /* xRGB12, 16-bit container */
Isn't this the same as RGB12U?
--
Ville Syrjälä
--
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] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC
On Tue, Jul 06, 2010 at 01:26:28PM +0200, ext Hiremath, Vaibhav wrote:
>
> > -Original Message-
> > From: Ville Syrjälä [mailto:ville.syrj...@nokia.com]
> > Sent: Tuesday, July 06, 2010 3:36 PM
> > To: Hiremath, Vaibhav
> > Cc: Grazvydas Ignotas; linux-fb...@vger.kernel.org; linux-
> > o...@vger.kernel.org; Valkeinen Tomi (Nokia-MS/Helsinki)
> > Subject: Re: [PATCH] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC
> >
> > On Tue, Jul 06, 2010 at 08:08:14AM +0200, ext Hiremath, Vaibhav wrote:
> > > > @@ -648,6 +649,17 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int
> > cmd,
> > > > unsigned long arg)
> > > > r = -EFAULT;
> > > > break;
> > > >
> > > > + case FBIO_WAITFORVSYNC:
> > > > + if (get_user(p.crt, (__u32 __user *)arg)) {
> > > > + r = -EFAULT;
> > > > + break;
> > > > + }
> > > > + if (p.crt != 0) {
> > > > + r = -ENODEV;
> > > > + break;
> > > > + }
> > > > + /* FALLTHROUGH */
> > > > +
> > > > case OMAPFB_WAITFORVSYNC:
> > > [Hiremath, Vaibhav] I don't see any reason why we should still keep old
> > custom IOCTL support here.
> >
> > It can already be used so it should not be removed.
> >
> [Hiremath, Vaibhav] I am not in favor of this, if we have standard interface
> then we should encourage people to use it. Don't you think we will have
> different interface for OMAP and different for non-omap device.
What anyone thinks that apps should do doesn't matter. Removing the
ioctl will break the ABI and that is not an acceptable thing to do in
kernel development usually.
--
Ville Syrjälä
--
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] OMAP: DSS2: OMAPFB: add support for FBIO_WAITFORVSYNC
On Tue, Jul 06, 2010 at 08:08:14AM +0200, ext Hiremath, Vaibhav wrote:
> > @@ -648,6 +649,17 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd,
> > unsigned long arg)
> > r = -EFAULT;
> > break;
> >
> > + case FBIO_WAITFORVSYNC:
> > + if (get_user(p.crt, (__u32 __user *)arg)) {
> > + r = -EFAULT;
> > + break;
> > + }
> > + if (p.crt != 0) {
> > + r = -ENODEV;
> > + break;
> > + }
> > + /* FALLTHROUGH */
> > +
> > case OMAPFB_WAITFORVSYNC:
> [Hiremath, Vaibhav] I don't see any reason why we should still keep old
> custom IOCTL support here.
It can already be used so it should not be removed.
--
Ville Syrjälä
--
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: OMAP3 display patches
On Tue, Jun 29, 2010 at 02:20:36PM +0200, ext Nagarajan, Rajkumar wrote: > > Tomi, > > Would you please check if there are no further comments and can you please > pull the following patches in? > > https://patchwork.kernel.org/patch/106498/ This one seems to have two separate changes in the same patch. Needs splitting. > https://patchwork.kernel.org/patch/106670/ I think this one needs a (cpu_is_omap3630() && id == OMAP_DSS_VIDEO2) check in store() so that the user gets a proper error if he tries to write an unsupported value. > https://patchwork.kernel.org/patch/107547/ You never addressed any of my comments so asking Tomi to pull this seems rather premature. -- Ville Syrjälä -- 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] OMAP: DSS2: DMA optimization using scaler line buffers
e > BUG(); > > + /* for vdma */ > + /* TODO: VDMA support for RGB16 mode */ > + if (cpu_is_omap3630()) > + if (color_mode == OMAP_DSS_COLOR_YUV2) > + if ((rotation == 1) || (rotation == 3)) > + pixel_size_exp = 2; > + where did UYVY go? Also what's this supposed to do? pixel_size_exp will be to 2 for YUV anyway. > vrfb_width = ALIGN(width * bytespp, VRFB_PAGE_WIDTH) / bytespp; > vrfb_height = ALIGN(height, VRFB_PAGE_HEIGHT); > > @@ -211,7 +219,9 @@ void omap_vrfb_setup(struct vrfb *vrfb, unsigned long > paddr, > vrfb->xoffset = vrfb_width - width; > vrfb->yoffset = vrfb_height - height; > vrfb->bytespp = bytespp; > - vrfb->yuv_mode = yuv_mode; > + if (color_mode == OMAP_DSS_COLOR_YUV2 || > + color_mode == OMAP_DSS_COLOR_UYVY) > + vrfb->yuv_mode = true; > } > EXPORT_SYMBOL(omap_vrfb_setup); > > -- > 1.5.4.3 > -- > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Ville Syrjälä -- 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 1/4] fbdev: add a MIPI DSI header
On Wed, May 19, 2010 at 05:00:58PM +0200, ext Paul Mundt wrote: > On Wed, May 19, 2010 at 05:27:32PM +0300, Ville Syrj?l? wrote: > > On Wed, May 19, 2010 at 10:21:48AM +0200, Valkeinen Tomi (Nokia-D/Helsinki) > > wrote: > > > I think a simple solution would be to just use defines, and have > > > functions that take the command as u8. That's what the OMAP DSI driver > > > does. If you have better ideas, please share =). > > > > I find enums easier on the eye than defines. Less irrelevant junk on > > each line. There's no reason you can't pass enum values as u8. But in > > that case giving the enum a name doesn't really make sense. > > > enums are cleaner for these cases, but you also have the case where the > enum type itself is variable size depending on the ABI being used. If > the type in question isn't being packed in to a user-visible data > structure then this will never matter, but it does help to be a bit > careful here regardless. Many people were bitten by this in the ARM > OABI -> EABI conversion, while other architectures generally managed to > get it right from the onset. Yeah using the enum type in ABI is a bad idea since actual type is implementation defined. But if you don't give the enum an identifier there's no way to even accidentally use it as a type. -- Ville Syrjälä -- 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 1/4] fbdev: add a MIPI DSI header
On Wed, May 19, 2010 at 10:21:48AM +0200, Valkeinen Tomi (Nokia-D/Helsinki)
wrote:
> Hi,
>
> On Wed, 2010-05-19 at 10:08 +0200, ext Guennadi Liakhovetski wrote:
> > Hi Tomi
> >
> > On Wed, 19 May 2010, Tomi Valkeinen wrote:
> > > > + MIPI_DSI_DCS_SHORT_WRITE= 5,
> > >
> > > Please use the same number format for all enums. So this should be 0x05.
> >
> > Where does this requirement come from? Is it in CodingStyle?;)
>
> I don't know, but mixing different formats looks ugly to me =).
One case where it would perhaps make sense if you define some fields
with shifts and masks. Specifying shifts as decimal and masks as hex
is clearer IMO. But in this case I agree it just looks ugly and only
serves to confuse the reader as he tries to figure out if there's
some magic meaning to the different formats.
>
> > > > + MIPI_DSI_DCS_SHORT_WRITE_PARAM = 0x15,
> > > > + MIPI_DSI_COLOR_MODE_OFF = 2,
> > > > + MIPI_DSI_COLOR_MODE_ON = 0x12,
> > > > + MIPI_DSI_SHUTDOWN_PERIPHERAL= 0x22,
> > > > + MIPI_DSI_TURN_ON_PERIPHERAL = 0x32,
> > > > + MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM= 3,
> > > > + MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM= 0x13,
> > > > + MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM= 0x23,
> > > > + MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM = 4,
> > > > + MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM = 0x14,
> > > > + MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM = 0x24,
> > > > + MIPI_DSI_DCS_LONG_WRITE = 0x39,
> > > > + MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE = 0x37,
> > > > + MIPI_DSI_NULL_PACKET= 9,
> > > > + MIPI_DSI_BLANKING_PACKET= 0x19,
> > > > + MIPI_DSI_GENERIC_LONG_WRITE = 0x29,
> > > > + MIPI_DSI_LOOSELY_PACKED_PIXEL_STREAM_YCBCR20= 0xc,
> > > > + MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR24= 0x1c,
> > > > + MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR16= 0x2c,
> > > > + MIPI_DSI_PACKED_PIXEL_STREAM_30 = 0xd,
> > > > + MIPI_DSI_PACKED_PIXEL_STREAM_36 = 0x1d,
> > > > + MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR12= 0x3d,
> > > > + MIPI_DSI_PACKED_PIXEL_STREAM_16 = 0xe,
> > > > + MIPI_DSI_PACKED_PIXEL_STREAM_18 = 0x1e,
> > > > + MIPI_DSI_PIXEL_STREAM_3BYTE_18 = 0x2e,
> > > > + MIPI_DSI_PACKED_PIXEL_STREAM_24 = 0x3e,
> > > > +};
> > > > +
> > > > +enum mipi_dcs_cmd {
> > >
> > > While the MIPI specs define a certain set of commands, the real panels
> > > usually implement also their own custom commands. That's why I don't
> > > think enum is a good choice here.
> >
> > Yes, that's a valid point, I'll have to think about it more.
>
> I think a simple solution would be to just use defines, and have
> functions that take the command as u8. That's what the OMAP DSI driver
> does. If you have better ideas, please share =).
I find enums easier on the eye than defines. Less irrelevant junk on
each line. There's no reason you can't pass enum values as u8. But in
that case giving the enum a name doesn't really make sense.
--
Ville Syrjälä
--
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] OMAP: DSS2: GFX FIFO UNDERFLOW issue fixed
On Tue, May 04, 2010 at 03:23:10PM +0200, ext Hiremath, Vaibhav wrote: > > > -Original Message- > > From: Tomi Valkeinen [mailto:tomi.valkei...@nokia.com] > > Sent: Wednesday, April 21, 2010 4:40 PM > > To: Hiremath, Vaibhav > > Cc: linux-omap@vger.kernel.org > > Subject: RE: [PATCH] OMAP: DSS2: GFX FIFO UNDERFLOW issue fixed > > > > On Wed, 2010-04-21 at 12:45 +0200, ext Hiremath, Vaibhav wrote: > > > > -Original Message- > > > > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- > > > > ow...@vger.kernel.org] On Behalf Of Hiremath, Vaibhav > > > > Sent: Friday, April 16, 2010 4:28 PM > > > > To: Tomi Valkeinen > > > > Cc: linux-omap@vger.kernel.org > > > > Subject: RE: [PATCH] OMAP: DSS2: GFX FIFO UNDERFLOW issue fixed > > > > > > > > > > > > > -Original Message- > > > > > From: Tomi Valkeinen [mailto:tomi.valkei...@nokia.com] > > > > > Sent: Friday, April 16, 2010 3:08 PM > > > > > To: Hiremath, Vaibhav > > > > > Cc: linux-omap@vger.kernel.org > > > > > Subject: Re: [PATCH] OMAP: DSS2: GFX FIFO UNDERFLOW issue fixed > > > > > > > > > > On Mon, 2010-03-22 at 14:09 +0100, ext hvaib...@ti.com wrote: > > > > > > From: Vaibhav Hiremath > > > > > > > > > > > > In case of 720P with 90/270 degree rotation, the system reports > > > > > > GFX_FIFO_UNDERFLOW error which usually happens if DSS DMA is not > > able to > > > > > fill > > > > > > the FIFO as per requirement. > > > > > > > > > > > > In TRM (section 11.2.6.1.3), where is has been clearly mentioned > > that, > > > > > > > > > > > > "To improve the performance on 90 degree rotation, split the data > > access > > > > > on > > > > > > write side and not read side." > > > > > > > > > > > > That means, read should always happen on 0 degree and write should > > go to > > > > > > respective rotation view. > > > > > > > > > > > > > > > > With this patch my db test app (from > > > > > git://gitorious.org/linux-omap-dss2/omapfb-tests.git) shows a lot of > > > > > tearing when rotation != 0. I tested this on 3430SDP using the LCD. > > > [Hiremath, Vaibhav] Tomi, > > > > > > Yes, just now I tried your db application and I am also seeing tearing > > effect. All other apps (including the one which I used for testing) works > > fine for other rotation angles. > > > > > > Let me debug further before commenting anything on this, I just thought of > > updating you on this. > > > > "db" app uses double buffering for updating the display, with maximum > > update rate. There's also "pan" app, that does a bit similar thing using > > fb offsets. > [Hiremath, Vaibhav] Tomi, > > I found the bug which is causing tearing effect, I tested it here with both, > mine and your applications and for me it is working fine. > > Can you please check at your end? > (Sorry for the attachment) You should really avoid that. I can't quote the patch. What are those CW<->CCW swaps that you do in the patch? Also I think the ioremap stuff is a bit broken since AFAICS it will leave the old angle still mapped when you rotate to another angle. -- Ville Syrjälä -- 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 v3 1/4] DSS2: OMAPFB: Add support for switching memory regions
On Fri, Mar 19, 2010 at 08:46:04AM +0100, Deak Imre (Nokia-D/Helsinki) wrote: > On Thu, Mar 18, 2010 at 04:26:04PM +0100, Syrjala Ville (Nokia-D/Helsinki) > wrote: > > [...] > > > > Just tried it and seems to be mostly OK. We get lockdep checking as a > > > > bonus. It didn't like setup_plane taking the same rwsem twice so I > > > > added a check to see if the old and new regions are the same and just > > > > lock once in that case. I thought rwsem was supposed to be OK with > > > > read recursion but perhaps I was mitaken, or perhaps it's just lockdep > > > > that's misbehaving. > > > > > > Ah ok, so it's not so obvious change. Nested read locks could really lead > > > to a deadlock I think. A read lock will block if there is a write waiter > > > in the queue to avoid write starvation.. > > > > Yes but I think in out case it should be fine because if we hit this: > > > > t thread 1 thread 2 > > | > > | down_read(0) > > | down_write(1) > > v down_read(1) > > > > then thread 2 will eventually do a up_write() without taking any > > other region rwsem, and thread 1 can then continue. > > Yes and things will work fine with the extra ordering you added. But > lockdep was right in that without the ordering you can get - the not > too likely - scenario: > > t thread 1thread 2 thread 3 thread 4 > | down_read(0) > | down_write(0) > |down_read(1) > | down_write(1) > | down_read(1) > vdown_read(0) Right. I didn't actually consider the case with so many threads. It's good that lockdep was smarter than me :) -- Ville Syrjälä -- 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 v3 1/4] DSS2: OMAPFB: Add support for switching memory regions
On Thu, Mar 18, 2010 at 09:52:39AM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> On Wed, Mar 17, 2010 at 09:14:25PM +0100, Syrjala Ville (Nokia-D/Helsinki)
> wrote:
> > On Wed, Mar 17, 2010 at 06:34:07PM +0100, Deak Imre (Nokia-D/Helsinki)
> > wrote:
> > > Hi,
> > >
> > > couple of minor comments inlined.
> > >
> > > On Fri, Mar 05, 2010 at 02:26:19PM +0100, Syrjala Ville
> > > (Nokia-D/Helsinki) wrote:
> > > [...]
> > > > @@ -115,30 +184,57 @@ static int omapfb_setup_mem(struct fb_info *fbi,
> > > > struct omapfb_mem_info *mi)
> > > > struct omapfb_info *ofbi = FB2OFB(fbi);
> > > > struct omapfb2_device *fbdev = ofbi->fbdev;
> > > > struct omapfb2_mem_region *rg;
> > > > - int r, i;
> > > > + int r = 0;
> > > > size_t size;
> > > > + int i;
> > > >
> > > > if (mi->type > OMAPFB_MEMTYPE_MAX)
> > > > return -EINVAL;
> > > >
> > > > size = PAGE_ALIGN(mi->size);
> > > >
> > > > - rg = &ofbi->region;
> > > > + rg = ofbi->region;
> > > >
> > > > - for (i = 0; i < ofbi->num_overlays; i++) {
> > > > - if (ofbi->overlays[i]->info.enabled)
> > > > - return -EBUSY;
> > > > + /* FIXME probably should be a rwsem ... */
> > > > + mutex_lock(&rg->mtx);
> > > > + while (rg->ref) {
> > > > + mutex_unlock(&rg->mtx);
> > > > + schedule();
> > > > + mutex_lock(&rg->mtx);
> > > > + }
> > >
> > > Yes, rwsem would mean no unnecessary scheduling and also make things
> > > clearer.
> >
> > Just tried it and seems to be mostly OK. We get lockdep checking as a
> > bonus. It didn't like setup_plane taking the same rwsem twice so I
> > added a check to see if the old and new regions are the same and just
> > lock once in that case. I thought rwsem was supposed to be OK with
> > read recursion but perhaps I was mitaken, or perhaps it's just lockdep
> > that's misbehaving.
>
> Ah ok, so it's not so obvious change. Nested read locks could really lead
> to a deadlock I think. A read lock will block if there is a write waiter
> in the queue to avoid write starvation..
Yes but I think in out case it should be fine because if we hit this:
t thread 1 thread 2
|
| down_read(0)
| down_write(1)
v down_read(1)
then thread 2 will eventually do a up_write() without taking any
other region rwsem, and thread 1 can then continue.
The other locks we have to worry about are the fb_info mutex which
should always be taken before any region lock, and mmap_sem which in
mmap() is taken before the region lock. I'm hoping there are no
mmap_sem users lurking in the driver that already hold the region
rwsem.
--
Ville Syrjälä
--
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 v3 1/4] DSS2: OMAPFB: Add support for switching memory regions
On Wed, Mar 17, 2010 at 06:34:07PM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> Hi,
>
> couple of minor comments inlined.
>
> On Fri, Mar 05, 2010 at 02:26:19PM +0100, Syrjala Ville (Nokia-D/Helsinki)
> wrote:
> > From: Ville Syrjälä
> >
> > Separate the memory region from the framebuffer device a little bit.
> > It's now possible to select the memory region used by the framebuffer
> > device using the new mem_idx parameter of omapfb_plane_info. If the
> > mem_idx is specified it will be interpreted as an index into the
> > memory regions array, if it's not specified the framebuffer's index is
> > used instead. So by default each framebuffer keeps using it's own
> > memory region which preserves backwards compatibility.
> >
> > This allows cloning the same memory region to several overlays and yet
> > each overlay can be controlled independently since they can be
> > associated with separate framebuffer devices.
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > Changes since v2:
> > * Removed the use_count and rely on just counting all active overlays. A
> > bit racy
> > but no chance of getting stuck in a state where memory allocation can't
> > be changed.
> > * s/source_idx/mem_idx as that seems to be a little more self explanatory
> > [...]
> >
> > drivers/video/omap2/omapfb/omapfb-ioctl.c | 163 -
> > drivers/video/omap2/omapfb/omapfb-main.c | 184
> > +++--
> > drivers/video/omap2/omapfb/omapfb-sysfs.c | 60 --
> > drivers/video/omap2/omapfb/omapfb.h | 38 ++-
> > include/linux/omapfb.h|5 +-
> > 5 files changed, 339 insertions(+), 111 deletions(-)
> >
> > diff --git a/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > b/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > index 1ffa760..cd00bdc 100644
> > --- a/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > +++ b/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > [...]
> > static int omapfb_query_plane(struct fb_info *fbi, struct
> > omapfb_plane_info *pi)
> > {
> > struct omapfb_info *ofbi = FB2OFB(fbi);
> > + struct omap_overlay *ovl = ofbi->overlays[0];
> > + struct omap_overlay_info *ovli = &ovl->info;
> >
> > if (ofbi->num_overlays != 1) {
> > memset(pi, 0, sizeof(*pi));
> > } else {
> > - struct omap_overlay_info *ovli;
> > - struct omap_overlay *ovl;
> > -
> > - ovl = ofbi->overlays[0];
> > - ovli = &ovl->info;
> > -
>
> Is this really necessary?
Nope. Leftovers from some earlier stuff I think.
>
> > pi->pos_x = ovli->pos_x;
> > pi->pos_y = ovli->pos_y;
> > pi->enabled = ovli->enabled;
> > pi->channel_out = 0; /* xxx */
> > pi->mirror = 0;
> > + pi->mem_idx = get_mem_idx(ofbi);
> > pi->out_width = ovli->out_width;
> > pi->out_height = ovli->out_height;
> > }
> > @@ -115,30 +184,57 @@ static int omapfb_setup_mem(struct fb_info *fbi,
> > struct omapfb_mem_info *mi)
> > struct omapfb_info *ofbi = FB2OFB(fbi);
> > struct omapfb2_device *fbdev = ofbi->fbdev;
> > struct omapfb2_mem_region *rg;
> > - int r, i;
> > + int r = 0;
> > size_t size;
> > + int i;
> >
> > if (mi->type > OMAPFB_MEMTYPE_MAX)
> > return -EINVAL;
> >
> > size = PAGE_ALIGN(mi->size);
> >
> > - rg = &ofbi->region;
> > + rg = ofbi->region;
> >
> > - for (i = 0; i < ofbi->num_overlays; i++) {
> > - if (ofbi->overlays[i]->info.enabled)
> > - return -EBUSY;
> > + /* FIXME probably should be a rwsem ... */
> > + mutex_lock(&rg->mtx);
> > + while (rg->ref) {
> > + mutex_unlock(&rg->mtx);
> > + schedule();
> > + mutex_lock(&rg->mtx);
> > + }
>
> Yes, rwsem would mean no unnecessary scheduling and also make things
> clearer.
Just tried it and seems to be mostly OK. We get lockdep checking as a
bonus. It didn't like setup_plane taking the same rwsem twice so I
added a check to see if the old and new regions are the same a
Re: [PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory regions
On Wed, Mar 17, 2010 at 02:50:45PM +0100, Valkeinen Tomi (Nokia-D/Helsinki)
wrote:
> Hi,
>
> On Fri, 2010-03-05 at 14:26 +0100, Syrjala Ville (Nokia-D/Helsinki)
> wrote:
> > From: Ville Syrjälä
> >
> > Separate the memory region from the framebuffer device a little bit.
> > It's now possible to select the memory region used by the framebuffer
> > device using the new mem_idx parameter of omapfb_plane_info. If the
> > mem_idx is specified it will be interpreted as an index into the
> > memory regions array, if it's not specified the framebuffer's index is
> > used instead. So by default each framebuffer keeps using it's own
> > memory region which preserves backwards compatibility.
> >
> > This allows cloning the same memory region to several overlays and yet
> > each overlay can be controlled independently since they can be
> > associated with separate framebuffer devices.
>
> Couple of comments inline. Mostly about making the patch a bit simpler.
>
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > Changes since v2:
> > * Removed the use_count and rely on just counting all active overlays. A
> > bit racy
> > but no chance of getting stuck in a state where memory allocation can't
> > be changed.
> > * s/source_idx/mem_idx as that seems to be a little more self explanatory
> >
> > drivers/video/omap2/omapfb/omapfb-ioctl.c | 163 -
> > drivers/video/omap2/omapfb/omapfb-main.c | 184
> > +++--
> > drivers/video/omap2/omapfb/omapfb-sysfs.c | 60 --
> > drivers/video/omap2/omapfb/omapfb.h | 38 ++-
> > include/linux/omapfb.h|5 +-
> > 5 files changed, 339 insertions(+), 111 deletions(-)
>
>
>
> > +static void omapfb_calc_addr(const struct omapfb_info *ofbi,
> > +const struct fb_var_screeninfo *var,
> > +const struct fb_fix_screeninfo *fix,
> > +int rotation, u32 *paddr, void __iomem **vaddr)
> > +{
> > + u32 data_start_p;
> > + void __iomem *data_start_v;
> > + int offset;
> > +
> > + if (ofbi->rotation_type == OMAP_DSS_ROT_VRFB) {
> > + data_start_p = omapfb_get_region_rot_paddr(ofbi, rotation);
> > + data_start_v = NULL;
> > + } else {
> > + data_start_p = omapfb_get_region_paddr(ofbi);
> > + data_start_v = omapfb_get_region_vaddr(ofbi);
> > + }
> > +
> > + if (ofbi->rotation_type == OMAP_DSS_ROT_VRFB)
> > + offset = calc_rotation_offset_vrfb(var, fix, rotation);
> > + else
> > + offset = calc_rotation_offset_dma(var, fix, rotation);
> > +
> > + data_start_p += offset;
> > + data_start_v += offset;
> > +
> > + if (offset)
> > + DBG("offset %d, %d = %d\n",
> > + var->xoffset, var->yoffset, offset);
> > +
> > + DBG("paddr %x, vaddr %p\n", data_start_p, data_start_v);
> > +
> > + *paddr = data_start_p;
> > + *vaddr = data_start_v;
> > +}
> >
> > /* setup overlay according to the fb */
> > -static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay
> > *ovl,
> > +int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
> > u16 posx, u16 posy, u16 outw, u16 outh)
> > {
> > int r = 0;
> > @@ -831,9 +863,8 @@ static int omapfb_setup_overlay(struct fb_info *fbi,
> > struct omap_overlay *ovl,
> > struct fb_var_screeninfo *var = &fbi->var;
> > struct fb_fix_screeninfo *fix = &fbi->fix;
> > enum omap_color_mode mode = 0;
> > - int offset;
> > - u32 data_start_p;
> > - void __iomem *data_start_v;
> > + u32 data_start_p = 0;
> > + void __iomem *data_start_v = NULL;
> > struct omap_overlay_info info;
> > int xres, yres;
> > int screen_width;
> > @@ -860,28 +891,9 @@ static int omapfb_setup_overlay(struct fb_info *fbi,
> > struct omap_overlay *ovl,
> > yres = var->yres;
> > }
> >
> > -
> > - if (ofbi->rotation_type == OMAP_DSS_ROT_VRFB) {
> > - data_start_p = omapfb_get_region_rot_paddr(ofbi, rotation);
> > - data_start_v = NULL;
> > - } else {
> > -
Re: [PATCH v2] DSS2: OMAPFB: Add support for switching memory regions
On Tue, Mar 02, 2010 at 07:36:59PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote: > From: Ville Syrjälä > > Separate the memory region from the framebuffer device a little bit. > It's now possible to select the memory region used by the framebuffer > device using the new source_idx parameter of omapfb_plane_info. If the > source_idx is specified it will be interpreted as an index into the > memory regions array, if it's not specified the framebuffer's index is > used instead. So by default each framebuffer keeps using it's own > memory region which preserves backwards compatibility. > > This allows cloning the same memory region to several overlays and yet > each overlay can be controlled independently since they can be > associated with separate framebuffer devices. > > Signed-off-by: Ville Syrjälä Actaully scrap this one. The use_count thing makes it's somewhat too easy to get stuck in a state where you can't change the memory size anymore and going in via sysfs in an effort to fix it doesn't work. I think I'll just go back to checking all the overlays and expand it to loop over all the fb devices too. The check won't be entirely accurate since the fb_infos can't be locked as that could easily lead to ABBA deadlock with the fb_info lock and the region mutex, but I suppose it's better than not being able to free/allocate memory anymore. -- Ville Syrjälä -- 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 2/2] OMAP: DSS2: OMAPFB: implement OMAPFB_GET_DISPLAY_INFO
On Thu, Feb 04, 2010 at 05:31:26PM +0200, Tomi Valkeinen wrote:
> Previously the only place to get the size of the display was from the
> DSS's sysfs interface, making, for example, configuring overlays and doing
> updates on manual displays more difficult.
>
> Signed-off-by: Tomi Valkeinen
> ---
> drivers/video/omap2/omapfb/omapfb-ioctl.c | 18 ++
> include/linux/omapfb.h|7 +++
> 2 files changed, 25 insertions(+), 0 deletions(-)
>
> @@ -216,6 +217,12 @@ struct omapfb_tearsync_info {
> __u16 reserved2;
> };
>
> +struct omapfb_display_info {
> + __u16 width;
> + __u16 height;
How about adding the physical display size here as well? I suppose
mm is the standard unit for such things but for small displays more
accuracy might be nice.
--
Ville Syrjälä
syrj...@sci.fi
http://www.sci.fi/~syrjala/
--
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
