Re: [DPU PATCH 2/2] drm/msm: Add hardware catalog file for SDM845

[Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]

On Tue, Mar 20, 2018 at 07:13:38PM +0530, skolluku@codeaurora.org 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@codeaurora.org>

<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

> Please clarify.
> 
> Thanks,
> Sravanthi

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno