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

[Sean Paul <seanpaul@chromium.org>]

On Wed, Mar 21, 2018 at 04:05:31PM +0530, skolluku@codeaurora.org wrote:
> On 2018-03-20 20:47, Sean Paul wrote:
> > 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
> > 
> 
> 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)

Yeah, if you can get the version from a register and avoid a new dt binding,
that's even better.

> 
> 
> 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.

Hmm, ok.

Sean

> 
> Thanks,
> Sravanthi
> 
> > > Please clarify.
> > > 
> > > Thanks,
> > > Sravanthi

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