Re: [PATCH 00/21] OMAPDSS: DISPC changes for writeback pipeline
On Fri, 2012-09-14 at 15:54 +0530, Archit Taneja wrote: > On Friday 14 September 2012 02:16 PM, Tomi Valkeinen wrote: > > On Fri, 2012-09-14 at 11:27 +0300, Tomi Valkeinen wrote: > >> On Thu, 2012-09-13 at 17:44 +0530, Archit Taneja wrote: > > > >>> This series prepares the low level DISPC driver(dispc.c) to configure > >>> writeback > >>> registers. The aim is to reuse most of the code as most of its registers > >>> are > >>> like overlay or manager registers, and are configured in the same way in > >>> most > >>> cases. The first few patches rename dispc_ovl_* functions to dispc_plane_* > >> > >> I'm not sure if the renaming causes more confusion than clarity... It > >> kinda creates a mishmash of ovl/plane names, and the term "plane" > >> doesn't really sound like it's a base for both overlays and wb. Could we > >> consider the wb as a special case, and keep the ovl name for most of the > >> things and have "wb" used for wb specific things? > > > > And while WB is a combination of overlays and ovl managers, do you think > > it'd be difficult to consider WB as a special, extended overlay? So just > > call it an overlay, and consider it as an overlay with special features, > > at least inside dispc.c. We probably need to have it as a totally > > different entity from user's point of view (i.e. the list of overlays > > wouldn't return WB, etc). > > Yes, we could do that within dispc.c, we would still need some manager > like functions which set GO or ENABLE. But apart from that it should be > okay. Yep, I was going through the WB registers, and to me it looks like 99% of them are like overlay regs. Then there are a few bits like GO which are special. > I think for dispc.c in general, for future, it might be a good idea to > represent each piece of HW(like scalar or color converter, or a timing Scal_e_r! ;) > FSM) as a little function/module, and construct > overlay/writeback/manager out of those, it might be cleaner. However, > this may be an overkill, and not needed much if there aren't any new > blocks comprising of these little blocks. I agree. In the minimum we should try to somehow group functions related to certain block, perhaps with name prefixes etc. I think it'll also help understanding the code. We probably currently have functions that touch multiple different blocks. Those funcs should be split to handle only one of the blocks. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 00/21] OMAPDSS: DISPC changes for writeback pipeline
On Fri, 2012-09-14 at 15:43 +0530, Archit Taneja wrote: > On Friday 14 September 2012 01:57 PM, Tomi Valkeinen wrote: > I initially kept all of this the same, but I changed my mind at some > point, not totally sure why. Even if we stick to the dispc_ovl_* names, > we would still need to create q common function which dispc_ovl_setup() > and dispc_wb_setup() could call. I called this dispc_plane_setup(), and > then it felt weird to call everything else ovl specifuic, hence renamed > all of them to dispc_plane_*. > > Could you suggest a better name than dispc_plane_setup? Well... dispc_ovl_setup_common? The function is also quite big, with huge number of arguments. Makes me wonder if we could split it up to some sensible parts. Would it be possible to have functions to setup, say, input related parameters (base-address, pix format, etc.), output related parameters (ovl position, ...). Well, it could just make it more confusing, as some things are shared between input and output, like scaling related things. But just an idea. > > > > >> functions. The next few patches change how overlay caps are used within the > >> dispc functions, this helps reusing more functions between overlays and > > > > I dislike this a bit, I think dispc driver should know what HW it has, > > you shouldn't need to pass caps to it. So I'd prefer the dispc driver to > > to have this information in dispc_features. I believe all OVL_CAPS > > should be there, and then exported to other drivers via some means. I > > guess this means could for now be just initializing ovl->caps with data > > from dispc.c. > > Currently, we pass the plane id to these low level functions, it > extracts out the ovl struct usingthe plane id, and checks the ovl caps. > > What I'm doing now is just passing the caps directly to these low level > functions. So that I don't need to have complicated checks in every > function to extract caps between overlays or writeback. Yep, I see. It's ok. My main dislike is the use of omap_dss_get_overlay() in dispc.c. I'd like dispc.c to be self-contained, so what I mean is that instead of initializing the caps in dss_features.c, and calling the above function in dispc.c, we should have a dispc.c internal table for dispc's HW, which would contain the caps and other necessary information. But that's not really related to this series. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 00/21] OMAPDSS: DISPC changes for writeback pipeline
On Friday 14 September 2012 02:16 PM, Tomi Valkeinen wrote: On Fri, 2012-09-14 at 11:27 +0300, Tomi Valkeinen wrote: On Thu, 2012-09-13 at 17:44 +0530, Archit Taneja wrote: This series prepares the low level DISPC driver(dispc.c) to configure writeback registers. The aim is to reuse most of the code as most of its registers are like overlay or manager registers, and are configured in the same way in most cases. The first few patches rename dispc_ovl_* functions to dispc_plane_* I'm not sure if the renaming causes more confusion than clarity... It kinda creates a mishmash of ovl/plane names, and the term "plane" doesn't really sound like it's a base for both overlays and wb. Could we consider the wb as a special case, and keep the ovl name for most of the things and have "wb" used for wb specific things? And while WB is a combination of overlays and ovl managers, do you think it'd be difficult to consider WB as a special, extended overlay? So just call it an overlay, and consider it as an overlay with special features, at least inside dispc.c. We probably need to have it as a totally different entity from user's point of view (i.e. the list of overlays wouldn't return WB, etc). Yes, we could do that within dispc.c, we would still need some manager like functions which set GO or ENABLE. But apart from that it should be okay. I think for dispc.c in general, for future, it might be a good idea to represent each piece of HW(like scalar or color converter, or a timing FSM) as a little function/module, and construct overlay/writeback/manager out of those, it might be cleaner. However, this may be an overkill, and not needed much if there aren't any new blocks comprising of these little blocks. 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 00/21] OMAPDSS: DISPC changes for writeback pipeline
On Friday 14 September 2012 01:57 PM, Tomi Valkeinen wrote: On Thu, 2012-09-13 at 17:44 +0530, Archit Taneja wrote: DSS HW on OMAP4 onwards supports a new pipeline called writeback. Unlike other pipelines(called overlays in OMAPDSS), writeback takes pixel data from an overlay output or a overlay manager output and writes it back into a specified address in memory. writeback pipeline allows us to take benefit of the hardware processing available inside the DISPC like color space conversion, rescaling, compositing etc and do either a) perform memory-to-memory transfer with data processing, b) capture a displayed frame. The former is known as memory to memory mode of the writeback pipeline, and the latter is known as capture mode. More details about writeback can be found in the Display Subsystem section of the OMAP4/5 TRMs. witeback has properties of both overlays and overlay managers. It is like an overlay as it has programmable base addresses and contains blocks like scalar, You consistently use the term "scalar" in the patches, but I believe the correct term is "scaler". Yes, my bad, I'll fix this. color conversion unit, truncation unit, DISPC DMA FIFO. It is like a manager as enabling it immediately starts transfer to the memory, and it has a GO bit to use a new writeback configuration. This series prepares the low level DISPC driver(dispc.c) to configure writeback registers. The aim is to reuse most of the code as most of its registers are like overlay or manager registers, and are configured in the same way in most cases. The first few patches rename dispc_ovl_* functions to dispc_plane_* I'm not sure if the renaming causes more confusion than clarity... It kinda creates a mishmash of ovl/plane names, and the term "plane" doesn't really sound like it's a base for both overlays and wb. Could we consider the wb as a special case, and keep the ovl name for most of the things and have "wb" used for wb specific things? I initially kept all of this the same, but I changed my mind at some point, not totally sure why. Even if we stick to the dispc_ovl_* names, we would still need to create q common function which dispc_ovl_setup() and dispc_wb_setup() could call. I called this dispc_plane_setup(), and then it felt weird to call everything else ovl specifuic, hence renamed all of them to dispc_plane_*. Could you suggest a better name than dispc_plane_setup? functions. The next few patches change how overlay caps are used within the dispc functions, this helps reusing more functions between overlays and I dislike this a bit, I think dispc driver should know what HW it has, you shouldn't need to pass caps to it. So I'd prefer the dispc driver to to have this information in dispc_features. I believe all OVL_CAPS should be there, and then exported to other drivers via some means. I guess this means could for now be just initializing ovl->caps with data from dispc.c. Currently, we pass the plane id to these low level functions, it extracts out the ovl struct usingthe plane id, and checks the ovl caps. What I'm doing now is just passing the caps directly to these low level functions. So that I don't need to have complicated checks in every function to extract caps between overlays or writeback. Archit Tomi -- 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 00/21] OMAPDSS: DISPC changes for writeback pipeline
On Fri, 2012-09-14 at 11:27 +0300, Tomi Valkeinen wrote: > On Thu, 2012-09-13 at 17:44 +0530, Archit Taneja wrote: > > This series prepares the low level DISPC driver(dispc.c) to configure > > writeback > > registers. The aim is to reuse most of the code as most of its registers are > > like overlay or manager registers, and are configured in the same way in > > most > > cases. The first few patches rename dispc_ovl_* functions to dispc_plane_* > > I'm not sure if the renaming causes more confusion than clarity... It > kinda creates a mishmash of ovl/plane names, and the term "plane" > doesn't really sound like it's a base for both overlays and wb. Could we > consider the wb as a special case, and keep the ovl name for most of the > things and have "wb" used for wb specific things? And while WB is a combination of overlays and ovl managers, do you think it'd be difficult to consider WB as a special, extended overlay? So just call it an overlay, and consider it as an overlay with special features, at least inside dispc.c. We probably need to have it as a totally different entity from user's point of view (i.e. the list of overlays wouldn't return WB, etc). Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 00/21] OMAPDSS: DISPC changes for writeback pipeline
On Thu, 2012-09-13 at 17:44 +0530, Archit Taneja wrote: > DSS HW on OMAP4 onwards supports a new pipeline called writeback. Unlike other > pipelines(called overlays in OMAPDSS), writeback takes pixel data from an > overlay output or a overlay manager output and writes it back into a specified > address in memory. > > writeback pipeline allows us to take benefit of the hardware processing > available inside the DISPC like color space conversion, rescaling, compositing > etc and do either a) perform memory-to-memory transfer with data processing, > b) capture a displayed frame. The former is known as memory to memory mode of > the writeback pipeline, and the latter is known as capture mode. More details > about writeback can be found in the Display Subsystem section of the OMAP4/5 > TRMs. > > witeback has properties of both overlays and overlay managers. It is like an > overlay as it has programmable base addresses and contains blocks like scalar, You consistently use the term "scalar" in the patches, but I believe the correct term is "scaler". > color conversion unit, truncation unit, DISPC DMA FIFO. It is like a manager > as > enabling it immediately starts transfer to the memory, and it has a GO bit to > use > a new writeback configuration. > > This series prepares the low level DISPC driver(dispc.c) to configure > writeback > registers. The aim is to reuse most of the code as most of its registers are > like overlay or manager registers, and are configured in the same way in most > cases. The first few patches rename dispc_ovl_* functions to dispc_plane_* I'm not sure if the renaming causes more confusion than clarity... It kinda creates a mishmash of ovl/plane names, and the term "plane" doesn't really sound like it's a base for both overlays and wb. Could we consider the wb as a special case, and keep the ovl name for most of the things and have "wb" used for wb specific things? > functions. The next few patches change how overlay caps are used within the > dispc functions, this helps reusing more functions between overlays and I dislike this a bit, I think dispc driver should know what HW it has, you shouldn't need to pass caps to it. So I'd prefer the dispc driver to to have this information in dispc_features. I believe all OVL_CAPS should be there, and then exported to other drivers via some means. I guess this means could for now be just initializing ovl->caps with data from dispc.c. Tomi signature.asc Description: This is a digitally signed message part
[PATCH 00/21] OMAPDSS: DISPC changes for writeback pipeline
DSS HW on OMAP4 onwards supports a new pipeline called writeback. Unlike other pipelines(called overlays in OMAPDSS), writeback takes pixel data from an overlay output or a overlay manager output and writes it back into a specified address in memory. writeback pipeline allows us to take benefit of the hardware processing available inside the DISPC like color space conversion, rescaling, compositing etc and do either a) perform memory-to-memory transfer with data processing, b) capture a displayed frame. The former is known as memory to memory mode of the writeback pipeline, and the latter is known as capture mode. More details about writeback can be found in the Display Subsystem section of the OMAP4/5 TRMs. witeback has properties of both overlays and overlay managers. It is like an overlay as it has programmable base addresses and contains blocks like scalar, color conversion unit, truncation unit, DISPC DMA FIFO. It is like a manager as enabling it immediately starts transfer to the memory, and it has a GO bit to use a new writeback configuration. This series prepares the low level DISPC driver(dispc.c) to configure writeback registers. The aim is to reuse most of the code as most of its registers are like overlay or manager registers, and are configured in the same way in most cases. The first few patches rename dispc_ovl_* functions to dispc_plane_* functions. The next few patches change how overlay caps are used within the dispc functions, this helps reusing more functions between overlays and writeback. The patches in the end add writeback register offsets, and make changes in the code where writeback behaves differently than The changes are made only keeping writeback mem to mem support in mind. There would be a few changes required when capture mode is added, but those are minimal. Reference branch: git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git 1-writeback-dispc Archit Taneja (21): OMAPDSS: DISPC: Constify omap_overlay_info in dispc_ovl_setup() OMAPDSS: DISPC: Rename scalar related functions from dispc_ovl_* to dispc_plane_* OMAPDSS: DISPC: Rename fifo/burst related functions from dispc_ovl_* to dispc_plane_* OMAPDSS: DISPC: Rename misc functions from dispc_ovl_* to dispc_plane_* OMAPDSS: DISPC: Simplify function names for setting pipeline input and output sizes OMAPDSS: DISPC: Pass overlay caps as a parameter to dispc plane functions OMAPDSS: OVERLAY: Add position and replication as overlay caps OMAPDSS: DISPC: Make dispc_ovl_setup call dispc_plane_setup OMAPDSS: DISPC: Calculate scaling limits in a more generic way OMAPDSS: DISPC: Allow both upscaling and downscaling of chroma OMAPDSS: DISPC: Add writeback register offsets and dss features structs OMAPDSS: DISPC: Configure input and output sizes for writeback OMAPDSS: DISPC: Pass dummy scalar output rates for writeback pipeline OMAPDSS: DISPC: Downscale chroma if plane is writeback OMAPDSS: DISPC: Don't set chroma resampling bit for writeback OMAPDSS: DISPC: Add function to set channel in for writeback OMAPDSS: DISPC: Configure overlay-like parameters in dispc_wb_setup OMAPDSS: DISPC: Configure writeback specific parameters in dispc_wb_setup() OMAPDSS: DISPC: Configure writeback FIFOs OMAPDSS: DISPC: Add manager like functions for writeback OMAPDSS: DISPC: Configure color conversion coefficients for writeback drivers/video/omap2/dss/apply.c|4 +- drivers/video/omap2/dss/dispc.c| 659 +--- drivers/video/omap2/dss/dispc.h| 35 +- drivers/video/omap2/dss/dispc_coefs.c |2 +- drivers/video/omap2/dss/dss.h | 26 +- drivers/video/omap2/dss/dss_features.c | 57 ++- drivers/video/omap2/dss/dss_features.h |1 + include/video/omapdss.h| 15 + 8 files changed, 564 insertions(+), 235 deletions(-) -- 1.7.9.5 -- 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
