On 2017-11-24 14:54, 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? Oops, sorry about forgetting to respond. Since I don't know how the hw behaves with respect to the clut registers for the other layers, I'm just not qualified to say if you should feed the one-and-only clut to all layers or if it is sufficient to only feed it to the first plane. I don't even know what the expected outcome is if you use clut modes on other planes? Since there is no API for setting up plane-specific cluts, the easy out seems to be to just reuse the one-and-only clut for all planes. Otherwise, there is no way at all to change the clut of the other planes. No? Other than that, when I added clut-support for atmel-hlcdc, I updated the hw registers as part of drm_plane_helper_funcs.atomic_update (as requested by Boris Brezillon) while you do it in drm_crtc_helper_funcs.atomic_flush. I don't know which is the better place to do it, but since the hw presumably(?) do have clut registers for each plane (as indicated by the code I removed), the plane-helper .atomic_update seems like a better fit (at least from here). And maybe it doesn't matter, but what do I know? For the record, I know very little about the subsystem. I'm probably wrong in oh so many ways... Cheers, Peter _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
