Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
So it's just more code to copy-paste later. Probably without anyone ever checking if the code guarded by this makes sense for the new platform at all. It seems to me like a bad work- around for bad development practice.
[Subrata] yes, its a bad W/A for early platform agree thats the reason its default n.
I wasn't referring to a workaround for a hardware problem but a potential workaround for problems that arise from your software development practice here at coreboot. The code guarded here was copied from cannonlake/ without checking if it applies to ICL. And when it starts making trouble, you disable it instead of trying to understand what the code does. Still not checking if it applies to ICL at all. I have a hunch, that you can simply remove the code instead.
If I understand you correctly, the display engine of your early samples works (you can see output on a display) but the graphics acceleration doesn't? But the code guarded here has absolutely nothing to do with graphics acceleration. Pushing this weird workaround with wrong explanations makes it very hard to work together in a community, I can't agree to it. But if you have to, go on.
[Subrata] as i have explained several time, you can bring display either by using sw framebuffer only mode or using hardware acceleration mode. Not using this config we would like to guard any transaction that goes to GT register which is not available on specific parts.
In this notion, coreboot always uses sw framebuffer. I don't see how this is related to hardware acceleration. Keeping to tell so without giving any reference, won't help. You might have learned by now, that you cannot convince me by repeating what was already said. You need arguments to reach me :)
Please don't get me wrong, I don't doubt that it was necessary to disable this code. I just think you are telling the wrong reasons.
https://review.coreboot.org/#/c/33189/2/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/2/src/soc/intel/icelake/graphics.c@50 PS2, Line 50: graphics_gtt_write(DDI_BUF_CTL_A, ddi_buf_ctl);
This registers are available in ICL EDS vol 1 and only thing that we like to skip if touching these […]
I couldn't find a register description their, maybe my version is outdated. Which version are you looking at?
However, I noticed that the EDS doesn't talk about the DP bifurcation (that this is about according to the code comment above) anymore. Prior EDS for CNL, CFL etc. did. So I really think this code here is obsolete anyway.