Hi all, Do you think the patch is "acceptable" or should I change it somehow? Any opinion is welcomed : ) Many thanks, Philippe :-) On 11/24/2017 02:54 PM, Philippe CORNU wrote: > Hi Peter, > > On 11/13/2017 11:40 AM, Philippe CORNU wrote: >> Hi Peter, >> >> On 11/12/2017 01:31 PM, Peter Rosin wrote: >>> On 2017-11-10 17:12, Philippe CORNU wrote: >>>> Hi Peter, >>>> >>>> On 11/07/2017 05:34 PM, Peter Rosin wrote: >>>>> On 2017-11-07 16:53, Philippe CORNU wrote: >>>>>> + Peter >>>>>> >>>>>> Hi Peter, >>>>>> >>>>>> CLUT support on STM32 has been removed thanks to your clean up patch >>>>> >>>>> Support is a bit strong for what I thought was a dead function, or >>>>> are you saying that it used to work before my series? Really sorry >>>>> if that is the case! >>>> >>>> As I wrote in the previous related thread >>>> (https://lists.freedesktop.org/archives/dri-devel/2017-June/145070.html), >>>> >>>> STM32 chipsets supports 8-bit CLUT mode but this driver version does >>>> not >>>> support it "yet"... >>>> >>>> So, no worry regarding your clean up, I gave you an "acked-by" for >>>> that : ) >>> >>> Ok, good. Thanks for clearing that up! >>> >>>>> >>>>> Anyway, the function I removed seemed to indicate that the hardware >>>>> could handle a separate clut for each layer, but your new version >>>>> does not. Why is that? >>>> >>>> Yes I confirm the clut support is available for each layer... but I >>>> thought the gamma_lut was only at the crtc level, not at layer level... >>>> Maybe I am wrong. >>>> Moreover, small test applications I used play only with clut at crtc >>>> level... >>>> >>>> Anyway, could you please help me to "find" a per-layer clut >>>> implementation because when I read "crtc->state->gamma_lut->data" it >>>> looks like gamma_lut is per crtc, not per plane...? or maybe I have to >>>> add extra properties for that... >>> >>> I wasn't clear enough. Yes, there is to my knowledge only one clut, >>> not one per plane. What I noticed was that the function I removed >>> seemed to touch clut registers for multiple layers, but your new >>> function appears to only touch registers for one layer. So, I >>> wondered if the "one and only" clut needed to be copied to the >>> registers for the other layers, or if the old dead code was simply >>> confused. Clearer? >>> >> >> The old code was a generic helper function (ie. for all layers) but >> used only for the 1st layer. So, we could say that "old dead code was >> simply confused" :-) >> >> When I put back the clut support in this patch, I decided to update >> only the 1st layer (because there is no API for handling it on other >> layers). I also decided to not re-use the former generic helper >> function as the update loop is pretty small. >> >> This patch offers the clut mode feature for fbdev (only one plane in >> fbdev) and for drm (single plane for many use cases, 2nd plane being >> used mostly for video...) >> >> If tomorrow the API offers clut support per plane, the update loop >> will be moved to the plane update function, means the generic helper >> function will not be require anymore too. > > From the explanations above, do you think the patch is "acceptable" or > should I change it somehow? > What is your opinion? > Many thanks, > Philippe :-) > >> >> Many thanks >> Philippe :) >> >>> Cheers. >>> Peter >>> _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
