<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
tt
{mso-style-priority:99;
font-family:"Courier New";}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body bgcolor="white" lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span lang="EN-IN" style="color:windowtext"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-IN" style="color:windowtext"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><a name="_____replyseparator"></a><b><span style="color:windowtext">From:</span></b><span style="color:windowtext"> Sharma, Swati2
<br>
<b>Sent:</b> Thursday, August 29, 2019 2:00 AM<br>
<b>To:</b> Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org<br>
<b>Cc:</b> Nikula, Jani <jani.nikula@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>; Manna, Animesh <animesh.manna@intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com<br>
<b>Subject:</b> Re: [v8][PATCH 05/10] drm/i915/display: Extract i9xx_read_luts()<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"><tt><span style="font-size:10.0pt">On 28-Aug-19 9:25 PM, Shankar, Uma wrote:</span></tt><o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>-----Original Message-----<o:p></o:p></pre>
<pre>From: Sharma, Swati2<o:p></o:p></pre>
<pre>Sent: Monday, August 26, 2019 11:56 AM<o:p></o:p></pre>
<pre>To: <a href="mailto:intel-gfx@lists.freedesktop.org">intel-gfx@lists.freedesktop.org</a><o:p></o:p></pre>
<pre>Cc: Nikula, Jani <a href="mailto:jani.nikula@intel.com"><jani.nikula@intel.com></a>; Sharma, Shashank<o:p></o:p></pre>
<pre><a href="mailto:shashank.sharma@intel.com"><shashank.sharma@intel.com></a>; Manna, Animesh <a href="mailto:animesh.manna@intel.com"><animesh.manna@intel.com></a>;<o:p></o:p></pre>
<pre>Nautiyal, Ankit K <a href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a>; <a href="mailto:daniel.vetter@ffwll.ch">daniel.vetter@ffwll.ch</a>;<o:p></o:p></pre>
<pre><a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a>; Shankar, Uma <a href="mailto:uma.shankar@intel.com"><uma.shankar@intel.com></a>; Sharma,<o:p></o:p></pre>
<pre>Swati2 <a href="mailto:swati2.sharma@intel.com"><swati2.sharma@intel.com></a><o:p></o:p></pre>
<pre>Subject: [v8][PATCH 05/10] drm/i915/display: Extract i9xx_read_luts()<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>For the legacy gamma, have hw read out to create hw blob of gamma lut values.<o:p></o:p></pre>
</blockquote>
<pre><o:p> </o:p></pre>
<pre>Would be better if we define platforms for which this is applicable (I mean what all is<o:p></o:p></pre>
<pre>considered legacy here)<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>Also, add function intel_color_lut_pack to convert hw value with given bit_precision<o:p></o:p></pre>
</blockquote>
<pre><o:p> </o:p></pre>
<pre>Wrap this up within 75 characters.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>to lut property val.<o:p></o:p></pre>
</blockquote>
<pre><o:p> </o:p></pre>
<pre>Keep the version history, don't drop that.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>Signed-off-by: Swati Sharma <a href="mailto:swati2.sharma@intel.com"><swati2.sharma@intel.com></a><o:p></o:p></pre>
<pre>---<o:p></o:p></pre>
<pre>drivers/gpu/drm/i915/display/intel_color.c | 51 ++++++++++++++++++++++++++++++<o:p></o:p></pre>
<pre>drivers/gpu/drm/i915/i915_reg.h | 3 ++<o:p></o:p></pre>
<pre>2 files changed, 54 insertions(+)<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>diff --git a/drivers/gpu/drm/i915/display/intel_color.c<o:p></o:p></pre>
<pre>b/drivers/gpu/drm/i915/display/intel_color.c<o:p></o:p></pre>
<pre>index 27727a1..45e0ee8 100644<o:p></o:p></pre>
<pre>--- a/drivers/gpu/drm/i915/display/intel_color.c<o:p></o:p></pre>
<pre>+++ b/drivers/gpu/drm/i915/display/intel_color.c<o:p></o:p></pre>
<pre>@@ -1521,6 +1521,56 @@ bool intel_color_lut_equal(struct drm_property_blob<o:p></o:p></pre>
<pre>*blob1,<o:p></o:p></pre>
<pre> return true;<o:p></o:p></pre>
<pre>}<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>+/* convert hw value with given bit_precision to lut property val */<o:p></o:p></pre>
<pre>+static u32 intel_color_lut_pack(u32 val, u32 bit_precision) {<o:p></o:p></pre>
<pre>+ u32 max = 0xffff >> (16 - bit_precision);<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ val = clamp_val(val, 0, max);<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ if (bit_precision < 16)<o:p></o:p></pre>
<pre>+ val <<= 16 - bit_precision;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ return val;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+static struct drm_property_blob *<o:p></o:p></pre>
<pre>+i9xx_read_lut_8(struct intel_crtc_state *crtc_state) {<o:p></o:p></pre>
</blockquote>
<pre><o:p> </o:p></pre>
<pre>Would be good to add some comments describing the rationale of this<o:p></o:p></pre>
<pre>function. Why 8 etc.<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal"><tt><span style="font-size:10.0pt">Func is written similar to load luts for i9xx.Therefore didn't explain 8.</span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>Do I need to add comment for all the functions/platform? Won't commit message
</tt><br>
<tt>sufficient enough?</tt></span><tt><span style="font-size:10.0pt;color:windowtext"><o:p></o:p></span></tt></p>
<p class="MsoNormal"><span style="color:windowtext"><o:p> </o:p></span></p>
<p class="MsoNormal" style="margin-left:4.8pt"><span style="color:windowtext">Commit message will not be visible while browsing the code. Would be good to add comments explaining the rationale.<o:p></o:p></span></p>
<pre><o:p> </o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>+ struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);<o:p></o:p></pre>
<pre>+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);<o:p></o:p></pre>
<pre>+ enum pipe pipe = crtc->pipe;<o:p></o:p></pre>
<pre>+ struct drm_property_blob *blob;<o:p></o:p></pre>
<pre>+ struct drm_color_lut *blob_data;<o:p></o:p></pre>
<pre>+ u32 i, val;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ blob = drm_property_create_blob(&dev_priv->drm,<o:p></o:p></pre>
<pre>+ sizeof(struct drm_color_lut) * 256,<o:p></o:p></pre>
</blockquote>
<pre><o:p> </o:p></pre>
<pre>Have a macro for 256. Explain why 256, add comments.<o:p></o:p></pre>
</div>
<p class="MsoNormal" style="margin-left:4.8pt"><tt><span style="font-size:10.0pt">This is similar to load luts, since nothing new i added so didn't give
</span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>explanation. I can re-use LEGACY_LUT_LENGTH for this, since wanted these</tt><br>
<tt>functions to be similar to load_luts, therefore kept same.</tt><br>
<br>
</span><span style="color:windowtext"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext">Ok, we still can use the macro.<o:p></o:p></span></p>
<pre><o:p> </o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>+ NULL);<o:p></o:p></pre>
<pre>+ if (IS_ERR(blob))<o:p></o:p></pre>
<pre>+ return NULL;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ blob_data = blob->data;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ for (i = 0; i < 256; i++) {<o:p></o:p></pre>
</blockquote>
<pre><o:p> </o:p></pre>
<pre>Add the macro for 256.<o:p></o:p></pre>
<p class="MsoNormal"><tt><span style="font-size:10.0pt">Macro already there LEGACY_LUT_LENGTH. Should i reuse that?</span></tt><o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">Yes you can reuse it.<o:p></o:p></span></pre>
<pre><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext"><o:p> </o:p></span></pre>
<pre><o:p> </o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>+ if (HAS_GMCH(dev_priv))<o:p></o:p></pre>
<pre>+ val = I915_READ(PALETTE(pipe, i));<o:p></o:p></pre>
<pre>+ else<o:p></o:p></pre>
<pre>+ val = I915_READ(LGC_PALETTE(pipe, i));<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ blob_data[i].red =<o:p></o:p></pre>
<pre>intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_RED_MASK, val), 8);<o:p></o:p></pre>
<pre>+ blob_data[i].green =<o:p></o:p></pre>
<pre>intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_GREEN_MASK, val), 8);<o:p></o:p></pre>
<pre>+ blob_data[i].blue =<o:p></o:p></pre>
<pre>intel_color_lut_pack(REG_FIELD_GET(LGC_PALETTE_BLUE_MASK, val), 8);<o:p></o:p></pre>
<pre>+ }<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ return blob;<o:p></o:p></pre>
<pre>+}<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+void i9xx_read_luts(struct intel_crtc_state *crtc_state) {<o:p></o:p></pre>
<pre>+ crtc_state->base.gamma_lut = i9xx_read_lut_8(crtc_state); }<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>void intel_color_init(struct intel_crtc *crtc) {<o:p></o:p></pre>
<pre> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); @@ -1541,6<o:p></o:p></pre>
<pre>+1591,7 @@ void intel_color_init(struct intel_crtc *crtc)<o:p></o:p></pre>
<pre> dev_priv->display.color_check = i9xx_color_check;<o:p></o:p></pre>
<pre> dev_priv->display.color_commit = i9xx_color_commit;<o:p></o:p></pre>
<pre> dev_priv->display.load_luts = i9xx_load_luts;<o:p></o:p></pre>
<pre>+ dev_priv->display.read_luts = i9xx_read_luts;<o:p></o:p></pre>
<pre> }<o:p></o:p></pre>
<pre> } else {<o:p></o:p></pre>
<pre> if (INTEL_GEN(dev_priv) >= 11)<o:p></o:p></pre>
<pre>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index<o:p></o:p></pre>
<pre>a092b34..b687faa 100644<o:p></o:p></pre>
<pre>--- a/drivers/gpu/drm/i915/i915_reg.h<o:p></o:p></pre>
<pre>+++ b/drivers/gpu/drm/i915/i915_reg.h<o:p></o:p></pre>
<pre>@@ -7192,6 +7192,9 @@ enum {<o:p></o:p></pre>
<pre>/* legacy palette */<o:p></o:p></pre>
<pre>#define _LGC_PALETTE_A 0x4a000<o:p></o:p></pre>
<pre>#define _LGC_PALETTE_B 0x4a800<o:p></o:p></pre>
<pre>+#define LGC_PALETTE_RED_MASK REG_GENMASK(23, 16)<o:p></o:p></pre>
<pre>+#define LGC_PALETTE_GREEN_MASK REG_GENMASK(15, 8)<o:p></o:p></pre>
<pre>+#define LGC_PALETTE_BLUE_MASK REG_GENMASK(7, 0)<o:p></o:p></pre>
<pre>#define LGC_PALETTE(pipe, i) _MMIO(_PIPE(pipe, _LGC_PALETTE_A,<o:p></o:p></pre>
<pre>_LGC_PALETTE_B) + (i) * 4)<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>/* ilk/snb precision palette */<o:p></o:p></pre>
<pre>--<o:p></o:p></pre>
<pre>1.9.1<o:p></o:p></pre>
</blockquote>
<pre><o:p> </o:p></pre>
</blockquote>
<p class="MsoNormal"><br>
<br>
<br>
<o:p></o:p></p>
<pre>-- <o:p></o:p></pre>
<pre>~Swati Sharma<o:p></o:p></pre>
</div>
</body>
</html>