On 2018-03-20 20:47, Sean Paul wrote:
On Tue, Mar 20, 2018 at 07:13:38PM +0530, skolluku@xxxxxxxxxxxxxx
wrote:
On 2018-03-19 19:29, Sean Paul wrote:
> On Wed, Mar 14, 2018 at 11:21:38AM +0530, Sravanthi Kollukuduru wrote:
> > This change adds the hardware catalog information in driver source
> > for SDM845. This removes the current logic of dt based parsing
> > of target catalog information.
> >
> > Signed-off-by: Sravanthi Kollukuduru <skolluku@xxxxxxxxxxxxxx>
<snip />
> > +{
> > + /* Layer capability */
> > + static const struct dpu_sspp_sub_blks vig_sblk_0 = {
> > + .maxlinewidth = 2560,
> > + .pixel_ram_size = 50 * 1024,
> > + .maxdwnscale = 4,
> > + .maxupscale = 20,
> > + .maxhdeciexp = DECIMATION_40X_MAX_H,
> > + .maxvdeciexp = DECIMATION_40X_MAX_V,
> > + .smart_dma_priority = 5,
> > + .src_blk = {.name = "sspp_src_0", .id = DPU_SSPP_SRC,
> > + .base = 0x00, .len = 0x150,},
> > + .scaler_blk = {.name = "sspp_scaler0",
> > + .id = DPU_SSPP_SCALER_QSEED3,
> > + .base = 0xa00, .len = 0xa0,},
> > + .csc_blk = {.name = "sspp_csc0", .id = DPU_SSPP_CSC_10BIT,
> > + .base = 0x1a00, .len = 0x100,},
> > + .format_list = plane_formats_yuv,
> > + .virt_format_list = plane_formats,
> > + };
>
> Instead of locating all of these parameters in one file, these should be
> located in their respective driver file. It also seems like you could
> separate
> out the common stuff such as line width, ram size, scaling, format, etc
> parameters from the pipeline setup.
>
> The same comments apply to the other blocks. Move things into the
> drivers,
> use compatibility string to determine the version, and then associate
> the common
> parameters with of_device_id.data.
>
> Sean
>
> <snip />
Thanks Sean for the feedback.
The idea behind this approach is to maintain a one point access for
all the
target specific information, analogous to the current dpu dtsi file.
This also ensures easy maintenance for different hardware versions, as
all
it
takes is to add another file instead of updating across individual sub
block
files.
I am not convinced this is what we should optimize for. This file is
basically
unreadable, and it's abstracting relevant details away from the block
code. There
are also a TON of duplicated parameters/values which is error-prone.
Lastly,
this is not the type of file that you want to copy/paste multiple
times, it would
be much better to simply add the new structs to the block drivers
where applicable.
Also, i'm not quite clear on how compatibility strings is applicable
to sub
blocks.
Consider the following example from rockchip:
https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/for-next/drivers/gpu/drm/rockchip/rockchip_vop_reg.c#L538
Each time the vop is changed, it gets a new compatible string in the
dt bindings.
This compatible string is tied to a parameters that describe the
features of
that version of vop. This data is tied to the driver data during probe
and used
whe needed throughout the driver.
So all of your catalog data should be broken up into structs specific
to the
various sub-blocks of the dpu driver and associated with compatible
strings.
When a new chip comes out with different parameters, a new struct
should be
defined along with a new compatible string.
Make sense?
Sean
Yes Sean, thanks for sharing the rockchip_vop reference.
Based on the discussions so far, there are two main points to be
addressed:
1. Associate catalog information with hardware versions using compatible
strings
2. Create sub block structures that various hardware versions can
reuse.
The intent of Point 1. is present in the current implementation.
The hardware version is read from register to extract the relevant
catalog information.
Hence, we don't plan to define new DT compatible strings for this
purpose.
(Upstream reference for similar implementation :
https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/for-next/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c#L556)
Point 2. however, is a valid concern and needs to be thoroughly looked
into.
The challenge here is to assess the code impact if we plan to modify the
present
catalog structures (for instance, create a new common structure).
Will get back to you on this after internal review.
Thanks,
Sravanthi
Please clarify.
Thanks,
Sravanthi
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
