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

[skolluku at codeaurora.org]

On 2018-03-20 20:47, Sean Paul wrote:
> On Tue, Mar 20, 2018 at 07:13:38PM +0530, skolluku at 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 at 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)


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