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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux